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:

      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.

        Gliffy Diagrams

          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: