In the interest of ensuring stability, we need to add some tests.

Specific things to test:

  • That enabling the module doesn't break the site :-)
  • That the various admin pages load as intended.
  • That the admin pages work as intended.
  • That backups can be executed.
  • That the permissions appropriately limit the functionality.

The first two would be a huge start - make sure the site still works and that the admin pages load, that would be a great base to start from.

CommentFileSizeAuthor
#13 backup_migrate-n2912460-13.patch13.67 KBdamienmckenna
#13 backup_migrate-n2912460-13.interdiff.txt2.92 KBdamienmckenna
#11 backup_migrate-n2912460-11.patch11.47 KBdamienmckenna
#9 backup_migrate-n2912460-9.patch8.75 KBdamienmckenna
#4 basic-tests-2912460.patch8.04 KBdinu r.

Comments

DamienMcKenna created an issue. See original summary.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Priority: Normal » Major

This needs to be high on the priority list.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue summary: View changes
dinu r.’s picture

dinu r. commented
StatusFileSize
new basic-tests-2912460.patch8.04 KB

Attached patch with basic functional testing

alex andrascu’s picture

alex andrascu commented
Status: Active » Needs review
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Needs work

This is a great start, thank you!

This needs a little polish to make the code match the d.o coding standards, primarily the comments need to wrap at 80 characters and the foreach() statement needs a space after the word "foreach".

  • Dinu Rodnitchi authored 5e80b23 on 8.x-4.x 
    Issue #2912460 by Dinu Rodnitchi: Add tests for all basic functionality...
alex andrascu’s picture

alex andrascu commented
Status: Needs work » Needs review

Hi Damien,

I believe I've done the above changes. Can you please review and maybe RTBC. I've already pushed it and will enable the auto testing for the dev branch.

Al.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
StatusFileSize
new backup_migrate-n2912460-9.patch8.75 KB

Some refinements for the permissions tests, though it needs finishing off because some of the permissions serve multiple purposes.

Status: Needs review » Needs work

The last submitted patch, 9: backup_migrate-n2912460-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new backup_migrate-n2912460-11.patch11.47 KB

There were a few minor bugs related to this:

  • The permission check in backup_migrate_backup_migrate_service_object_alter() for 'access backup files' looked for the wrong permission.
  • The "delete backup files" permission didn't actually work as intended, and it really didn't make sense on its own so I've removed it.

Status: Needs review » Needs work

The last submitted patch, 11: backup_migrate-n2912460-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new backup_migrate-n2912460-13.interdiff.txt2.92 KB
new backup_migrate-n2912460-13.patch13.67 KB

So the 'access backup and migrate' permission wasn't actually being used either.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

So what does everyone think of removing the "delete backup files" permission?

ikit-claw’s picture

ikit-claw commented

So what does everyone think of removing the "delete backup files" permission?

Is it currently being used or could it be put to good use?

I am a little unsure if it is a good idea or not.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

I looked through the code again and the "delete" permission isn't actually used.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Fixed

Committed.

  • DamienMcKenna committed b16ff90 on 8.x-4.x 
    Issue #2912460 by DamienMcKenna: Removed two unused permissions, fixed...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.