Moodle
  1. Moodle
  2. MDL-38543

Links for selecting all/none checkboxes lost from backup/import pages

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Backup, Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      This test requires a site with at least two courses, one with activities/resources.

      1. Log in as admin/teacher
      2. Navigate to one of the courses
      3. Click Import
      4. Select another course with activities/resources
      5. Click continue
      6. Click next on the initial settings page
      7. Verify that the all/none links appear above the course sections
      Show
      This test requires a site with at least two courses, one with activities/resources. Log in as admin/teacher Navigate to one of the courses Click Import Select another course with activities/resources Click continue Click next on the initial settings page Verify that the all/none links appear above the course sections
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-mdl-38543
    • Rank:
      48557

      Description

      Recent form simplifications have introduced changes to fieldset identifiers which has broken (possibly among other things) the all/none links on backup/import pages. The fieldset identifiers are now modified with a preceeding 'id_' being added to fieldset ids (why?). The JavaScript that attempts to identify of the all/none links should be added relies on the fieldset identifier.

      Replication steps:
      This requires a site with at least two courses, one with activities resources

      1. Log in as admin/teacher
      2. Navigate to one of the courses
      3. Click Import
      4. Select another course with activities/resources
      5. Click continue
      6. Click next on the initial settings page
      7. Check above the course sections

      Expected outcome: You should see all/none links

      Actual outcome: The all/none links are missing

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          This functionality was introduced in MDL-32705 and has been broken by changes introduced by MDL-30637.

          Show
          Michael de Raadt added a comment - This functionality was introduced in MDL-32705 and has been broken by changes introduced by MDL-30637 .
          Hide
          Michael de Raadt added a comment -

          I've added a quick fix.

          It's unfortunate that the insertion of the all/none links is bound to fieldset identifier. A better fix might decouple this dependency. I'm not even sure why we are using YUI to insert these links anyway. Wouldn't it be better to insert them from PHP?

          Show
          Michael de Raadt added a comment - I've added a quick fix. It's unfortunate that the insertion of the all/none links is bound to fieldset identifier. A better fix might decouple this dependency. I'm not even sure why we are using YUI to insert these links anyway. Wouldn't it be better to insert them from PHP?
          Hide
          Michael de Raadt added a comment -

          Hi, Tim.

          As the original author of the all/none links code, I thought you might like to peer review this issue.

          Show
          Michael de Raadt added a comment - Hi, Tim. As the original author of the all/none links code, I thought you might like to peer review this issue.
          Hide
          Tim Hunt added a comment -

          +1 from me. Nice simple patch.

          We insert the links using JS because this is progressive enhancement. These links are useless if JavaScript is disabled.

          Show
          Tim Hunt added a comment - +1 from me. Nice simple patch. We insert the links using JS because this is progressive enhancement. These links are useless if JavaScript is disabled.
          Hide
          Tim Hunt added a comment -

          As usual, I did the peer review before remembering to click the buttons.

          Show
          Tim Hunt added a comment - As usual, I did the peer review before remembering to click the buttons.
          Hide
          Michael de Raadt added a comment -

          Thanks for the peer review, Tim. It's nice to go through the Dev process myself when I have the chance, even on such a small issue.

          I thought of a possible improvement using the .jsenabled class (whoever came up with that is brilliant), but while we have a working system, suggesting an improvement would only add a task to our backlog that would never be fixed. So I think we should continue with this.

          Show
          Michael de Raadt added a comment - Thanks for the peer review, Tim. It's nice to go through the Dev process myself when I have the chance, even on such a small issue. I thought of a possible improvement using the .jsenabled class (whoever came up with that is brilliant), but while we have a working system, suggesting an improvement would only add a task to our backlog that would never be fixed. So I think we should continue with this.
          Hide
          Damyon Wiese 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.

          Thanks!

          Show
          Damyon Wiese 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. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And tested, I got the all/none there working.

          Show
          Eloy Lafuente (stronk7) added a comment - And tested, I got the all/none there working.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

            People

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

              Dates

              • Created:
                Updated:
                Resolved: