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:
    • Rank:
      39176

      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.

        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: