Moodle

move_section function can lead to data corruption

Details

  • Type: Bug Bug
  • Status: Closed 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 from Repository:
  • Pull Master Branch:
    wip-mdl-33367
  • Pull Master Diff URL:

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

has a non-specific relationship to

Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-33379 Remove deprecated move_section in course/lib.php

  • Critical - Crashes server, loss of data, severe memory leak
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
has been marked as being related by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-33347 Sections can't be moved when ajax is turned off

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
testing discovered

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-33546 Current week not updating after section drag-and-drop

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Rajesh Taneja added a comment -

Submitting it for integration.

Show
Rajesh Taneja added a comment - Submitting it for integration.
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
Rajesh Taneja added a comment -

Branch re-based.

Show
Rajesh Taneja added a comment - Branch re-based.
Hide
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: