Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32353

Backup checkboxes clickable if permission no and default no

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            emerrill 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
            emerrill 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
            emerrill Eric Merrill added a comment -

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

            Show
            emerrill Eric Merrill added a comment - This is just the master patch, once it is reviewed, I'll backport and submit for additional branched.
            Hide
            emerrill 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
            emerrill 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
            emerrill Eric Merrill added a comment -

            Adding MOODLE_22_STABLE and MOODLE_21_STABLE patches.

            Show
            emerrill Eric Merrill added a comment - Adding MOODLE_22_STABLE and MOODLE_21_STABLE patches.
            Hide
            tsala 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
            tsala 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
            poltawski 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
            poltawski 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
            emerrill 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
            emerrill 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
            poltawski Dan Poltawski added a comment -

            Thanks, submitting for integration

            Show
            poltawski Dan Poltawski added a comment - Thanks, submitting for integration
            Hide
            poltawski 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
            poltawski 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
            emerrill Eric Merrill added a comment -

            All 3 branches rebased.

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

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

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, changes look spot on and have been integrated now
            Hide
            abgreeve 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
            abgreeve 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
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  14/May/12