Moodle
  1. Moodle
  2. MDL-32705

No way to clear all checkboxes in backup schema settings

    Details

    • Testing Instructions:
      Hide

      For each of Backup, Restore, and Import, in the course settings and in other contexts.

      1. Work all the way through the process, and verify it is not broken.

      2. On the Schema page of the wizard, verify that the appropriate Select all/none links appear, and that they work.

      Show
      For each of Backup, Restore, and Import, in the course settings and in other contexts. 1. Work all the way through the process, and verify it is not broken. 2. On the Schema page of the wizard, verify that the appropriate Select all/none links appear, and that they work.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      39660

      Description

      In Moodle 1.9, when performing a backup of a large course, it was possible to clear all the checkboxes by clicking the "Include all/none" box. This made it easy to create a backup containing just a few resources.

      In Moodle 2.2, the equivalent form is "Schema settings" in backup, and this does not have a facility to clear the checkboxes. This means they have to be cleared one by one, a tedious process when the course is large.

      Restoring the include all/none feature would be an improvement.

      Steps to reproduce
      ==================
      1. Find an existing course containing many resources and activities.
      2. Click Backup
      3. On Backup settings form, select "Include activities" and "Include enrolled users".
      4. On Schema settings form, try to include only the announcements forum and its user data in the backup. It can only be done by manually unchecking every single other box.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          A similar issue has been reported for importing, and import/backup/restore are related.

          I've put it on our backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. A similar issue has been reported for importing, and import/backup/restore are related. I've put it on our backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch, please add a patch label so we will spot it.
          Hide
          Tim Hunt added a comment -

          I am working on a hacky JavaScript solution: https://github.com/timhunt/moodle/compare/master...MDL-32705

          I don't actually understand why this is not working, but when I try it, I get TypeError: M.core_backup_select is undefined in the firebug console, even though I can see that it is loading the 'moodle-backup-selectall'. Actually, it loads that module, and 'moodle-backup-confirmcancel', twice, which is interesting.

          Eloy, would a patch like this be accepted into core, assuming I can get this working? Obviously, I am aiming at the same UI for backup/restore and import schema pages.

          Show
          Tim Hunt added a comment - I am working on a hacky JavaScript solution: https://github.com/timhunt/moodle/compare/master...MDL-32705 I don't actually understand why this is not working, but when I try it, I get TypeError: M.core_backup_select is undefined in the firebug console, even though I can see that it is loading the 'moodle-backup-selectall'. Actually, it loads that module, and 'moodle-backup-confirmcancel', twice, which is interesting. Eloy, would a patch like this be accepted into core, assuming I can get this working? Obviously, I am aiming at the same UI for backup/restore and import schema pages.
          Hide
          Tim Hunt added a comment -

          I nearly have a working patch for this. Note that,

          1. the fix will rely on MDL-34707.
          2. with my fix, when you do select all / none, it does not update the disableIf state of all the checkboxes until you click on one of them. This is because of the way lib/form/form.js is written, and there is nothing I can do about that. I am hoping that Sam Hemelryk might be able to help with this, because he wrote form.js.
          3. there is nothing I can do about Restore step 4. Schema. Suddenly, for no readily apparent reason, we have Yes/No select menus instead of check-boxes. WTF! I declare fixing that a separate issue.

          I think where I have got to represents a big step forwards in usability for admins. I think we should integrate this, and if anyone else wants to work on 2. and 3. they can create separate issues for them.

          Show
          Tim Hunt added a comment - I nearly have a working patch for this. Note that, the fix will rely on MDL-34707 . with my fix, when you do select all / none, it does not update the disableIf state of all the checkboxes until you click on one of them. This is because of the way lib/form/form.js is written, and there is nothing I can do about that. I am hoping that Sam Hemelryk might be able to help with this, because he wrote form.js. there is nothing I can do about Restore step 4. Schema. Suddenly, for no readily apparent reason, we have Yes/No select menus instead of check-boxes. WTF! I declare fixing that a separate issue. I think where I have got to represents a big step forwards in usability for admins. I think we should integrate this, and if anyone else wants to work on 2. and 3. they can create separate issues for them.
          Hide
          Tim Hunt added a comment -

          If this is OK, I will cherry-pick to stable branches.

          Show
          Tim Hunt added a comment - If this is OK, I will cherry-pick to stable branches.
          Hide
          Tim Hunt added a comment -

          To INTEGRATORS, MDL-34707 needs to be integrated first. This is on the same branch. MDL-34727 follows this.

          Show
          Tim Hunt added a comment - To INTEGRATORS, MDL-34707 needs to be integrated first. This is on the same branch. MDL-34727 follows this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Wow, really surprised we have select menus for user information fields (MDL-34727)!

          About this... I like those global (all/none) links a lot, but I think we need to make them to proceed with disabling properly. Else you can end with sections disabled and activities within them enabled, that lead to no-backup happening.

          More yet, we should, at the same time, make the uncheck of the sections to cause also the uncheck of the activities, not only disabling them but unchecking.

          That way, both the global all/none links and the section checkboxes will look and behave consistent. Always unchecking and disabling all their children.

          So I'd give this one more round... perhaps Sam can imagine how to achieve it. I'm far from understanding all that JS.

          Reopening this and chained MDL-34727 too. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Wow, really surprised we have select menus for user information fields ( MDL-34727 )! About this... I like those global (all/none) links a lot, but I think we need to make them to proceed with disabling properly. Else you can end with sections disabled and activities within them enabled, that lead to no-backup happening. More yet, we should, at the same time, make the uncheck of the sections to cause also the uncheck of the activities, not only disabling them but unchecking. That way, both the global all/none links and the section checkboxes will look and behave consistent. Always unchecking and disabling all their children. So I'd give this one more round... perhaps Sam can imagine how to achieve it. I'm far from understanding all that JS. Reopening this and chained MDL-34727 too. Ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Tim Hunt added a comment -

          OK, will look again at the disabledIf thing.

          I disagree about the section options. I think there is a fundamental difference between

          • the setting for a section, which is a real backup setting that controls what is backed-up / restored - with dependencies, and
          • the short-cut links that just automate doing select all/none.

          Therefore, it is quite reasonable to have different UI for them.

          Show
          Tim Hunt added a comment - OK, will look again at the disabledIf thing. I disagree about the section options. I think there is a fundamental difference between the setting for a section, which is a real backup setting that controls what is backed-up / restored - with dependencies, and the short-cut links that just automate doing select all/none. Therefore, it is quite reasonable to have different UI for them.
          Hide
          Tim Hunt added a comment -

          OK, branch amended to use the new hook provided by MDL-34728. Peer review please, then I will cherry-pick.

          Show
          Tim Hunt added a comment - OK, branch amended to use the new hook provided by MDL-34728 . Peer review please, then I will cherry-pick.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integration note: Due to the decision @ MDL-34727, this will go to master and 23_STABLE only.

          Show
          Eloy Lafuente (stronk7) added a comment - Integration note: Due to the decision @ MDL-34727 , this will go to master and 23_STABLE only.
          Hide
          Tim Hunt added a comment -

          Submitting for integration now. Remember, MDL-34728 needs to be integrated first.

          Show
          Tim Hunt added a comment - Submitting for integration now. Remember, MDL-34728 needs to be integrated first.
          Hide
          Sam Hemelryk added a comment -

          Hi Tim,

          Sending this back this week sorry.
          Pretty minor but I noticed that this code is relying on #coursesettings. A quick test shows that this change leads to a JS error on the first page of a backup (the backup settings page).
          Just a case checking in JS that firstsection is not null before using it.

          Also just noting that the 23 branch looks like it needs updating. My intention was just to cherry-pick the master changes but seeing as this is going back could you please update that as well.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tim, Sending this back this week sorry. Pretty minor but I noticed that this code is relying on #coursesettings. A quick test shows that this change leads to a JS error on the first page of a backup (the backup settings page). Just a case checking in JS that firstsection is not null before using it. Also just noting that the 23 branch looks like it needs updating. My intention was just to cherry-pick the master changes but seeing as this is going back could you please update that as well. Cheers Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Tim Hunt added a comment -

          OK, will fix for next week.

          Show
          Tim Hunt added a comment - OK, will fix for next week.
          Hide
          Tim Hunt added a comment -

          Fixed up now. Thanks for catching this.

          Show
          Tim Hunt added a comment - Fixed up now. Thanks for catching this.
          Hide
          Dan Poltawski added a comment -

          Integrated to 2.3 and master.

          Thanks!

          Show
          Dan Poltawski added a comment - Integrated to 2.3 and master. Thanks!
          Hide
          Jason Fowler added a comment -

          Works fine

          Show
          Jason Fowler added a comment - Works fine
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

            People

            • Votes:
              13 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: