Moodle
  1. Moodle
  2. MDL-33347

Sections can't be moved when ajax is turned off

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Rank:
      41200

      Description

      When ajax is turned off, it should be possible to move course sections using the up and down arrows in the right of each section box. When a user clicks on one of these arrows, the section does not move.

      Test Pre-requisites:

      This test must be performed using the Google Chrome web browser.
      Disable ajax in Site administration > Appearance > Ajax and Javascript
      Any course in Moodle that has the course format set to Topics.
      Course administration > Course layout is set to "Show all sections in one page".

      Test Steps:
      1. Turn editing on/off with the button in top right and verify you are returned to where you were
      2. Add a resource to a section, then verify you are returned to where you were and that the resource was added tot he section.
      3. Move a resource between sections, then verify that you are returned to where you were and that the resource was moved.
      4. Indent a section, then verify that you are returned to where you were and that the indentation is in the correct position.
      5. Edit a resource, then verify that you are returned to where you were.
      6. Hide a resource, then verify that you are returned to where you were and that the resource is hidden.
      7. Delete a resource, then verify that you are returned to where you were and that the resource is deleted.
      8. Duplicate a resource and verify that you are returned to where you were and that the resource was duplicated.
      9. Move a section and verify that the section was moved.

      Expected Result:
      The section moves position.

      Actual Result:
      The section does not move.

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment - - edited

          I thought of putting rebuild_course_cache in move_section function, but seems it will build cache every-time move_section function is called (in loop). Currently it's not the case, so either way (adding rebuild_course_cache in view.php or from move_section function) will solve the issue.

          I am going with adding rebuild_course_cache after move_section is called in view.php, as this seems safer for now.

          Show
          Rajesh Taneja added a comment - - edited I thought of putting rebuild_course_cache in move_section function, but seems it will build cache every-time move_section function is called (in loop). Currently it's not the case, so either way (adding rebuild_course_cache in view.php or from move_section function) will solve the issue. I am going with adding rebuild_course_cache after move_section is called in view.php, as this seems safer for now.
          Hide
          Rajesh Taneja added a comment -

          Hello Sam,
          Could you please review this code, if you can spare some time for this issue.
          Thanks

          Show
          Rajesh Taneja added a comment - Hello Sam, Could you please review this code, if you can spare some time for this issue. Thanks
          Hide
          Rajesh Taneja added a comment -

          On the other thought, as set_section_visible is calling rebuild_course_cache, I think it's safe to call rebuild_course_cache from move_section.
          Hence, code updated.

          Show
          Rajesh Taneja added a comment - On the other thought, as set_section_visible is calling rebuild_course_cache, I think it's safe to call rebuild_course_cache from move_section. Hence, code updated.
          Hide
          Sam Marshall added a comment -

          Rajesh - in case called in a loop, I would always recommend calling rebuild_course_cache with the second parameter 'clearonly' set. (In fact I'm not quite sure why it has any other option...)

          That way all it does is set the 'modinfo' and 'secinfo' fields to null (which is one db query) rather than rebuilding the whole cache (which might be loads).

          Show
          Sam Marshall added a comment - Rajesh - in case called in a loop, I would always recommend calling rebuild_course_cache with the second parameter 'clearonly' set. (In fact I'm not quite sure why it has any other option...) That way all it does is set the 'modinfo' and 'secinfo' fields to null (which is one db query) rather than rebuilding the whole cache (which might be loads).
          Hide
          Sam Marshall added a comment -

          note: there is no reason ever to call rebuild_course_cache WITHOUT setting that 'clear only' parameter. (Except possibly in the get_fast_modinfo function itself.)

          Show
          Sam Marshall added a comment - note: there is no reason ever to call rebuild_course_cache WITHOUT setting that 'clear only' parameter. (Except possibly in the get_fast_modinfo function itself.)
          Hide
          Rajesh Taneja added a comment -

          Thanks for the review Sam,
          Was not sure about resetting 'clear only' parameter as it was not set in set_section_visible function.

          However, I have updated 'clear only' parameter now. In addition to that, I have added another commit to let move_section use move_section_to function because:

          1. move_section is not using sql transaction and hence not safe.
          2. One code should be used for similar operation.

          Can you please check this again for me.

          Show
          Rajesh Taneja added a comment - Thanks for the review Sam, Was not sure about resetting 'clear only' parameter as it was not set in set_section_visible function. However, I have updated 'clear only' parameter now. In addition to that, I have added another commit to let move_section use move_section_to function because: move_section is not using sql transaction and hence not safe. One code should be used for similar operation. Can you please check this again for me.
          Hide
          Rajesh Taneja added a comment -

          I have created a new issue to resolve move_section issue (MDL-33367).

          Pushing this for integration, as it solves this issue.

          Show
          Rajesh Taneja added a comment - I have created a new issue to resolve move_section issue ( MDL-33367 ). Pushing this for integration, as it solves this issue.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks.

          Show
          Dan Poltawski added a comment - Integrated, thanks.
          Hide
          Adrian Greeve added a comment -

          Tested with an old version of master in Chrome. Topics were not moving. Tested with this patch incorporated and moving topics worked as described.
          Test passed

          Show
          Adrian Greeve added a comment - Tested with an old version of master in Chrome. Topics were not moving. Tested with this patch incorporated and moving topics worked as described. Test passed
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          Your work has made into the latest Moodle release!

          You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

          Show
          Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: