Moodle
  1. Moodle
  2. MDL-32353

Backup checkboxes clickable if permission no and default no

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2.2, 2.3
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Backup
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      Default moodle install. Add a course and a user. Put that user in as a Teacher (editing teacher) (user w/o the moodle/backup:userinfo or moodle/backup:anonymise capabilities, but with other backup capabilities).

      As that user, go to the first backup page, "Include enrolled users" is properly Xed out, but "Anonymize user information" is not, even thought the user doesn't have that capability.

      Now go into (as an admin) Settings->Course->Backup->General Backup Defaults, and change backup_general_users to No.
      As the user, go back into the backup first page. Notice that "Include enrolled users" is now unchecked and clickable (and if you click it all the children that should shouldn't have are clickable too). If you click it and continue you will get an exception.

      Should not be clickable.

      Show
      Default moodle install. Add a course and a user. Put that user in as a Teacher (editing teacher) (user w/o the moodle/backup:userinfo or moodle/backup:anonymise capabilities, but with other backup capabilities). As that user, go to the first backup page, "Include enrolled users" is properly Xed out, but "Anonymize user information" is not, even thought the user doesn't have that capability. Now go into (as an admin) Settings->Course->Backup->General Backup Defaults, and change backup_general_users to No. As the user, go back into the backup first page. Notice that "Include enrolled users" is now unchecked and clickable (and if you click it all the children that should shouldn't have are clickable too). If you click it and continue you will get an exception. Should not be clickable.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      This is true for both "Include enrolled users" and "Anonymize user information" on the Initial settings step of backup.

      Default moodle install. Add a course and a user. Put that user in as a Teacher (editing teacher) (user w/o the moodle/backup:userinfo or moodle/backup:anonymise capabilities, but with other backup capabilities).

      As that user, go to the first backup page, "Include enrolled users" is properly Xed out, but "Anonymize user information" is not, even thought the user doesn't have that capability.

      Now go into (as an admin) Settings->Course->Backup->General Backup Defaults, and change backup_general_users to No.
      As the user, go back into the backup first page. Notice that "Include enrolled users" is now unchecked and clickable (and if you click it all the children that should shouldn't have are clickable too). If you click it and continue you will get an exception (hence why this is not a security flaw).

      Basically:
      Capability Yes and Default Yes = Checked and changeable
      Capability Yes and Default No = Unchecked and changeable
      Capability No and Default Yes = Xed out
      Capability No and Default No = Unchecked and changeable (Should be Xed out)

      The same logic flaw applies to both "Include enrolled users" (moodle/backup:userinfo) and "Anonymize user information" (moodle/backup:anonymise)

      I've already tracked it down and have a proposed patch, which I'll add here as soon as I have a MDL number.

        Gliffy Diagrams

          Activity

          Hide
          Eric Merrill added a comment -

          Added a patch branch that is against master.

          I will backport - I just want feedback since this is a backup/restore item.

          Show
          Eric Merrill added a comment - Added a patch branch that is against master. I will backport - I just want feedback since this is a backup/restore item.
          Hide
          Eric Merrill added a comment -

          This is just the master patch, once it is reviewed, I'll backport and submit for additional branched.

          Show
          Eric Merrill added a comment - This is just the master patch, once it is reviewed, I'll backport and submit for additional branched.
          Hide
          Eric Merrill added a comment -

          Left panel shows backup_general_users set to yes, right shows backup general users set to no. Both have moodle/backup:userinfo and moodle/backup:anonymise not set.

          Show
          Eric Merrill added a comment - Left panel shows backup_general_users set to yes, right shows backup general users set to no. Both have moodle/backup:userinfo and moodle/backup:anonymise not set.
          Hide
          Eric Merrill added a comment -

          Adding MOODLE_22_STABLE and MOODLE_21_STABLE patches.

          Show
          Eric Merrill added a comment - Adding MOODLE_22_STABLE and MOODLE_21_STABLE patches.
          Hide
          Helen Foster added a comment -

          Eric, thanks a lot for your report and patch which can hopefully be reviewed soon. I've voted for it to be fixed, because I know how confusing the backup checkboxes can be when they're incorrectly greyed out.

          Show
          Helen Foster added a comment - Eric, thanks a lot for your report and patch which can hopefully be reviewed soon. I've voted for it to be fixed, because I know how confusing the backup checkboxes can be when they're incorrectly greyed out.
          Hide
          Dan Poltawski added a comment -

          Hi Eric,

          Thanks a lot for this - it makes sense to me. I just have a trivial some comments about the comments:

          If you can fix up these two things I will submit for integration.

          cheers,
          dan

          Show
          Dan Poltawski added a comment - Hi Eric, Thanks a lot for this - it makes sense to me. I just have a trivial some comments about the comments: The coding style says that following a // we should have a space ( http://docs.moodle.org/dev/Coding_style#Inline_comments ) Typo: alread If you can fix up these two things I will submit for integration. cheers, dan
          Hide
          Eric Merrill added a comment -

          Thanks for catching those!

          I noticed I typoed the ticket number in the first commit, so I rebuilt the branches with a single new commit that fixes the comment errors you found.

          If you could submit for integration, that would be great!

          Thanks!

          Show
          Eric Merrill added a comment - Thanks for catching those! I noticed I typoed the ticket number in the first commit, so I rebuilt the branches with a single new commit that fixes the comment errors you found. If you could submit for integration, that would be great! Thanks!
          Hide
          Dan Poltawski added a comment -

          Thanks, submitting for integration

          Show
          Dan Poltawski added a comment - Thanks, submitting for integration
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eric Merrill added a comment -

          All 3 branches rebased.

          Show
          Eric Merrill added a comment - All 3 branches rebased.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, changes look spot on and have been integrated now

          Show
          Sam Hemelryk added a comment - Thanks guys, changes look spot on and have been integrated now
          Hide
          Adrian Greeve added a comment -

          Checked pre-patch to observe the problem and then tested in 2.1, 2.2 and master. This patch fixes the problem.
          Thanks.

          Show
          Adrian Greeve added a comment - Checked pre-patch to observe the problem and then tested in 2.1, 2.2 and master. This patch fixes the problem. Thanks.
          Hide
          Dan Poltawski added a comment -

          Bonza mate!

          Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

          Hooroo

          Show
          Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: