Moodle
  1. Moodle
  2. MDL-33367

move_section function can lead to data corruption

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Test 1:

      1. run phpunit course/tests/courselib_test.php

      Test 2:

      1. Turn AJAX off for course
      2. Move course up and down and make sure you can do it.

      Test 3:

      1. Turn AJAX on for course
      2. Drag and move course up and down and make sure you can do it.
      Show
      Test 1: run phpunit course/tests/courselib_test.php Test 2: Turn AJAX off for course Move course up and down and make sure you can do it. Test 3: Turn AJAX on for course Drag and move course up and down and make sure you can do it.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-33367
    • Rank:
      41234

      Description

      move_section function is not using transaction and can lead to data corruption.
      Also, it should use move_section_to, as it do similar work.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Is the move_section() function used any more? Should it be deprecated?

          Show
          Michael de Raadt added a comment - Is the move_section() function used any more? Should it be deprecated?
          Hide
          Rajesh Taneja added a comment -

          It can be deprecated in future, probably in 2.5 or so.

          Show
          Rajesh Taneja added a comment - It can be deprecated in future, probably in 2.5 or so.
          Hide
          Rajesh Taneja added a comment -

          Added deprecated message for move_section and removed it's usage.

          Show
          Rajesh Taneja added a comment - Added deprecated message for move_section and removed it's usage.
          Hide
          Sam Marshall added a comment -

          Brief look, seems good - albeit I don't understand the change in reorder_section.

          Show
          Sam Marshall added a comment - Brief look, seems good - albeit I don't understand the change in reorder_section.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the review Sam,

          reorder_section api was not working properly for sections moving up. We were adding 1 to make it work.

          So either we can live with it, or modify it to work as expected. I didn't find any other use-case for move_section_to or reorder_section, so preferred to rectify it and updated the test case.

          Show
          Rajesh Taneja added a comment - Thanks for the review Sam, reorder_section api was not working properly for sections moving up. We were adding 1 to make it work. So either we can live with it, or modify it to work as expected. I didn't find any other use-case for move_section_to or reorder_section, so preferred to rectify it and updated the test case.
          Hide
          Rajesh Taneja added a comment -

          Submitting it for integration.

          Show
          Rajesh Taneja added a comment - Submitting it for integration.
          Hide
          Michael de Raadt added a comment -

          In the code it says the function will be removed in 2.4 (in the comment and deprecation message). This should be 2.5.

          Show
          Michael de Raadt added a comment - In the code it says the function will be removed in 2.4 (in the comment and deprecation message). This should be 2.5.
          Hide
          Rajesh Taneja added a comment -

          Thanks Michael,

          It is now changed to 2.5.

          Show
          Rajesh Taneja added a comment - Thanks Michael, It is now changed to 2.5.
          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
          Rajesh Taneja added a comment -

          Branch re-based.

          Show
          Rajesh Taneja added a comment - Branch re-based.
          Hide
          Sam Hemelryk added a comment -

          This has been integrated now thanks Raj.

          Show
          Sam Hemelryk added a comment - This has been integrated now thanks Raj.
          Hide
          Michael de Raadt added a comment -

          Test result: Success

          I tested this in 2.3 on Firefox. I assume by turning AJAX on and off you meant the setting at the site level. I also assume by moving a course up and down you meant moving a section up and down. I tried this in three ways:

          • AJAX off at site level
          • JS disabled in browser
          • AJAX and JS enabled

          I found an issue with weekly section highlighting which I have reported and linked. I also noted that dates were not updating correctly in weekly format, but this error was intermittent and I could not reproduce it when I wanted to.

          The PHPUnit test passed without any problems.

          Show
          Michael de Raadt added a comment - Test result: Success I tested this in 2.3 on Firefox. I assume by turning AJAX on and off you meant the setting at the site level. I also assume by moving a course up and down you meant moving a section up and down. I tried this in three ways: AJAX off at site level JS disabled in browser AJAX and JS enabled I found an issue with weekly section highlighting which I have reported and linked. I also noted that dates were not updating correctly in weekly format, but this error was intermittent and I could not reproduce it when I wanted to. The PHPUnit test passed without any problems.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: