Moodle
  1. Moodle
  2. MDL-44141

Completion system updates data during restore

    Details

    • Testing Instructions:
      Hide

      Specifically test this in all the branches (25, 26 and master), please.

      0. (Completion must be enabled in Moodle features admin option.)
      1. Create a new course, with completion enabled.
      2. Enrol a test account on the course.
      3. Add a quiz, set to be marked complete when user gets a grade.
      4. Add one true/false question to the quiz.
      5. Log in as the test user.
      6. Complete the quiz.
      7. Back as admin user, look at the completion report for the course and note the time that the user completed it.
      8. Do a backup of course using default settings.
      9. Restore to new course using default settings.
      10. Look at completion report.

      EXPECTED: The time against the user should be the same as noted in step 7.
      BEFORE FIX: The time is set to the time of the restore.

      Show
      Specifically test this in all the branches (25, 26 and master), please. 0. (Completion must be enabled in Moodle features admin option.) 1. Create a new course, with completion enabled. 2. Enrol a test account on the course. 3. Add a quiz, set to be marked complete when user gets a grade. 4. Add one true/false question to the quiz. 5. Log in as the test user. 6. Complete the quiz. 7. Back as admin user, look at the completion report for the course and note the time that the user completed it. 8. Do a backup of course using default settings. 9. Restore to new course using default settings. 10. Look at completion report. EXPECTED: The time against the user should be the same as noted in step 7. BEFORE FIX: The time is set to the time of the restore.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.6 Branch:
      MDL-44141-m26
    • Pull Master Branch:
      MDL-44141-master

      Description

      After a course restore, the activity completion time is set to the restore time. This way all the original activity completion time are lost.

      How to reproduce this issue: with completion tracking enabled

      • Create a course
      • Add a quiz activity
      • Set activity completion: Completion tracking -> "Show activity as complete when conditions are met" and check "Student must receive a grade to complete this activity".
      • Add this activity in course Course completion.
      • Enrol some user to this course
      • Complete the activity with an user.
      • Check in Reports -> Activity completion that the activity is completed and remember the Completed time.
      • Backup this course.
      • Restore the course.
      • In the new course check in Reports -> Activity completion: the completion time is changes and is now set to the restore time

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Mark Nelson added a comment -

            Thanks for reporting this Federico. I am putting this on the BACKEND backlog. If you would like to work on this yourself please feel free and I will happily review it and push it for integration. Thanks!

            Show
            Mark Nelson added a comment - Thanks for reporting this Federico. I am putting this on the BACKEND backlog. If you would like to work on this yourself please feel free and I will happily review it and push it for integration. Thanks!
            Hide
            Federico added a comment -

            I have found the problem: when an activity is restored, in step "restore_activity_grades_structure_step" the grade is restored and the completion record is created with current time.
            When the completion is restored the following code generate the issue (backup/moodle2/restore_stepslib.php line 3173)

            if ($existing) {
            	// Update it to these new values, but only if the time is newer
            	if ($existing->timemodified < $data->timemodified) {
            		$data->id = $existing->id;
            		$DB->update_record('course_modules_completion', $data);
            	}
            }
            

            because $existing->timemodified are newer than the restored completion.
            If I remove the control I solve the problem , but is that really correct?
            Why that control is there?

            Show
            Federico added a comment - I have found the problem: when an activity is restored, in step "restore_activity_grades_structure_step" the grade is restored and the completion record is created with current time. When the completion is restored the following code generate the issue (backup/moodle2/restore_stepslib.php line 3173) if ($existing) { // Update it to these new values, but only if the time is newer if ($existing->timemodified < $data->timemodified) { $data->id = $existing->id; $DB->update_record('course_modules_completion', $data); } } because $existing->timemodified are newer than the restored completion. If I remove the control I solve the problem , but is that really correct? Why that control is there?
            Hide
            Mark Nelson added a comment -

            Hey Federico, I believe Sam Marshall may have a better understanding as to why this is needed. Sam, are you able to provide an answer? Thanks mate.

            Show
            Mark Nelson added a comment - Hey Federico, I believe Sam Marshall may have a better understanding as to why this is needed. Sam, are you able to provide an answer? Thanks mate.
            Hide
            Sam Marshall added a comment -

            Hi,

            The code shown is, I think, supposed to cope with the situation where you restore something on top of an existing course. I'm not sure how it would happen in practice.

            IMO the actual bug here is that when you are restoring grades this should NOT trigger completion calculations. The completion system should not operate during restore of grades (because it's going to restore that data anyway). There is code that is supposed to detect this but I guess it is not working... I'll take a look.

            Show
            Sam Marshall added a comment - Hi, The code shown is, I think, supposed to cope with the situation where you restore something on top of an existing course. I'm not sure how it would happen in practice. IMO the actual bug here is that when you are restoring grades this should NOT trigger completion calculations. The completion system should not operate during restore of grades (because it's going to restore that data anyway). There is code that is supposed to detect this but I guess it is not working... I'll take a look.
            Hide
            Sam Marshall added a comment -

            Confirmed this is breaking. Added test script.

            Show
            Sam Marshall added a comment - Confirmed this is breaking. Added test script.
            Hide
            Sam Marshall added a comment -

            Updating code to point to my fix of the underlying problem, will submit for review in a bit.

            Note: This change should slightly improve performance when restoring gradebook grades, for sites with the completion system enabled.

            Show
            Sam Marshall added a comment - Updating code to point to my fix of the underlying problem, will submit for review in a bit. Note: This change should slightly improve performance when restoring gradebook grades, for sites with the completion system enabled.
            Hide
            Sam Marshall added a comment -

            Submitting for review.

            I think this change fixes the underlying problem, which is that the code in gradebook which informs the completion system about grade changes was incorrectly running in restore. There is logic to check if a restore is in progress but this was badly done and probably has not worked for ages.

            It needed a proper way to test if a restore is in progress, which I have now added.

            I also added a unit test for the new restore_controller::is_executing static method I added.

            Show
            Sam Marshall added a comment - Submitting for review. I think this change fixes the underlying problem, which is that the code in gradebook which informs the completion system about grade changes was incorrectly running in restore. There is logic to check if a restore is in progress but this was badly done and probably has not worked for ages. It needed a proper way to test if a restore is in progress, which I have now added. I also added a unit test for the new restore_controller::is_executing static method I added.
            Hide
            Sam Marshall added a comment -

            Forgot to say - the commits for master and 2.6 are identical, the one for 2.5 is slightly different due to resolving conflict.

            Show
            Sam Marshall added a comment - Forgot to say - the commits for master and 2.6 are identical, the one for 2.5 is slightly different due to resolving conflict.
            Show
            CiBoT added a comment - Results for MDL-44141 Remote repository: https://github.com/sammarshallou/moodle.git Remote branch MDL-44141 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1778 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1778/artifact/work/smurf.html Remote branch MDL-44141 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1779 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1779/artifact/work/smurf.html Remote branch MDL-44141 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1780 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1780/artifact/work/smurf.html
            Hide
            Sam Marshall added a comment -

            note: CiBoT result indicates no errors (the missing documentation for that function is allowed, because it's a function that implements a base class).

            Show
            Sam Marshall added a comment - note: CiBoT result indicates no errors (the missing documentation for that function is allowed, because it's a function that implements a base class).
            Hide
            Sam Marshall added a comment -

            I have updated all branches with a minor fix to the code (it previously kept the 'executing' marker if an exception was thrown, which caused problems in unit tests). Have also rebased to latest branches.

            Show
            Sam Marshall added a comment - I have updated all branches with a minor fix to the code (it previously kept the 'executing' marker if an exception was thrown, which caused problems in unit tests). Have also rebased to latest branches.
            Show
            CiBoT added a comment - Results for MDL-44141 Remote repository: https://github.com/sammarshallou/moodle.git Remote branch MDL-44141 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1912 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1912/artifact/work/smurf.html Remote branch MDL-44141 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1913 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1913/artifact/work/smurf.html Remote branch MDL-44141 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1914 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1914/artifact/work/smurf.html
            Hide
            Dan Poltawski added a comment -

            Thanks Sam. Looks OK to me. I am not 100% certain on the 'restore in progress' approach, but I can't say anything against it.

            Show
            Dan Poltawski added a comment - Thanks Sam. Looks OK to me. I am not 100% certain on the 'restore in progress' approach, but I can't say anything against it.
            Hide
            Sam Marshall added a comment -

            Thanks Dan.

            I agree the approach still seems a bit sketchy but at least it is less sketchy than the previous code it replaces, even when that worked!

            I do think what it actually does is clearly the right thing - i.e. when restoring grades, we shouldn't be updating user completion data that is based on those grades, because we are going to restore the completion data too so it's a waste of time quite apart from meaning it makes the data slightly wrong. So it's just about how to communicate that information between the different areas of code.

            Show
            Sam Marshall added a comment - Thanks Dan. I agree the approach still seems a bit sketchy but at least it is less sketchy than the previous code it replaces, even when that worked! I do think what it actually does is clearly the right thing - i.e. when restoring grades, we shouldn't be updating user completion data that is based on those grades, because we are going to restore the completion data too so it's a waste of time quite apart from meaning it makes the data slightly wrong. So it's just about how to communicate that information between the different areas of code.
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Oh, my the old detection was really fu..gly and unreliable. Your new hack is way better, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Oh, my the old detection was really fu..gly and unreliable. Your new hack is way better, thanks!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Uh, oh... Sam, for sure the 25_STABLE unit tests are not going to work. There was no progress there afaik... uhm and 26_STABLE does not have \core\progress\base but core_backup_display_progress... trying...

            Show
            Eloy Lafuente (stronk7) added a comment - Uh, oh... Sam, for sure the 25_STABLE unit tests are not going to work. There was no progress there afaik... uhm and 26_STABLE does not have \core\progress\base but core_backup_display_progress... trying...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Oh, yes. Confirmed. Both 25 and 26 unit tests are broken.

            So, while I think the solution is perfect, the tests need some love.

            Perhaps you can extend/mockup restore_controller itself and override the load_plan() itself, by loading an alternative restore_plan able to look for $this->controller->is_executing() being true....

            Something like that of we agree about to get rid of unit tests in 25 & 26. NP if human testing verifies it works.

            Show
            Eloy Lafuente (stronk7) added a comment - Oh, yes. Confirmed. Both 25 and 26 unit tests are broken. So, while I think the solution is perfect, the tests need some love. Perhaps you can extend/mockup restore_controller itself and override the load_plan() itself, by loading an alternative restore_plan able to look for $this->controller->is_executing() being true.... Something like that of we agree about to get rid of unit tests in 25 & 26. NP if human testing verifies it works.
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Sam Marshall added a comment -

            lol, I am an idiot, sorry.

            I'll do something about this tomorrow and resubmit for next week.

            Show
            Sam Marshall added a comment - lol, I am an idiot, sorry. I'll do something about this tomorrow and resubmit for next week.
            Hide
            Sam Marshall added a comment -

            Resubmitting - apologies for screwup!

            I rebased all the branches and:

            1. MOODLE_25_STABLE - deleted unit test
            2. MOODLE_26_STABLE - changed unit test to use non-namespaced name (and actually ran it - passes now)

            Hopefully this compromise is OK! I didn't really want to rewrite the unit test for 2.5 (although it is possible as suggested).

            Show
            Sam Marshall added a comment - Resubmitting - apologies for screwup! I rebased all the branches and: 1. MOODLE_25_STABLE - deleted unit test 2. MOODLE_26_STABLE - changed unit test to use non-namespaced name (and actually ran it - passes now) Hopefully this compromise is OK! I didn't really want to rewrite the unit test for 2.5 (although it is possible as suggested).
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Perfect for me, Sam, thanks! (I've amended the instructions to enforce testing in all branches)

            Show
            Eloy Lafuente (stronk7) added a comment - Perfect for me, Sam, thanks! (I've amended the instructions to enforce testing in all branches)
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Marina Glancy added a comment -

            I can see that this issue correlates with MDL-44106, we need to decide which way is better to determine if we are inside the restore process

            Show
            Marina Glancy added a comment - I can see that this issue correlates with MDL-44106 , we need to decide which way is better to determine if we are inside the restore process
            Hide
            Petr Skoda added a comment -

            Hi, I think you could do class_exists('restore_controller', false) instead of the unconditional include to lower memory use by limiting unnecessary include of the whole restore framework, right?

            I would do the same trick in MDL-44106...

            Show
            Petr Skoda added a comment - Hi, I think you could do class_exists('restore_controller', false) instead of the unconditional include to lower memory use by limiting unnecessary include of the whole restore framework, right? I would do the same trick in MDL-44106 ...
            Hide
            Marina Glancy added a comment -

            this is a very good idea. If class restore_controller is not loaded in memory this automatically means that we can not be inside the restore process.
            Sam, can you update your commits please?

            Show
            Marina Glancy added a comment - this is a very good idea. If class restore_controller is not loaded in memory this automatically means that we can not be inside the restore process. Sam, can you update your commits please?
            Hide
            Sam Marshall added a comment -

            Good idea. Updating commits now.

            Show
            Sam Marshall added a comment - Good idea. Updating commits now.
            Hide
            Sam Marshall added a comment -

            OK, all 3 commits are now updated, and I reran unit tests (for the two branches that have unit tests). Also the 3 commits are now based on latest branches - it didn't make any difference, just thought I might as well since I was updating commits.

            By the way I did realise this sucked slightly while I was coding it (just didn't think of Petr's solution). I think the underlying problem is that backup/restore should be changed to use autoloading so it doesn't have to load a bazillion classes when you just want one of them but I don't think now is the time for that change.

            Show
            Sam Marshall added a comment - OK, all 3 commits are now updated, and I reran unit tests (for the two branches that have unit tests). Also the 3 commits are now based on latest branches - it didn't make any difference, just thought I might as well since I was updating commits. By the way I did realise this sucked slightly while I was coding it (just didn't think of Petr's solution). I think the underlying problem is that backup/restore should be changed to use autoloading so it doesn't have to load a bazillion classes when you just want one of them but I don't think now is the time for that change.
            Hide
            Marina Glancy added a comment -

            Thanks Sam, integrated in 2.5, 2.6 and master. Very useful function

            Show
            Marina Glancy added a comment - Thanks Sam, integrated in 2.5, 2.6 and master. Very useful function
            Hide
            Damyon Wiese added a comment -

            Not sure if it's caused by this (I didn't look at the patch). I am seeing this on master when editing a quiz.

            Fatal error: Call to a member function get_name() on a non-object in /home/damyonw/Documents/Moodle/integration/master/moodle/question/editlib.php on line 1047
            Call Stack
            #	Time	Memory	Function	Location
            1	0.0019	237752	{main}( )	../edit.php:0
            2	0.0805	5954000	quiz_question_bank_view->__construct( )	../edit.php:152
            3	0.0805	5954504	question_bank_view->__construct( )	../editlib.php:1110
            4	0.0823	6552816	question_bank_view->init_columns( )	../editlib.php:934
            

            Show
            Damyon Wiese added a comment - Not sure if it's caused by this (I didn't look at the patch). I am seeing this on master when editing a quiz. Fatal error: Call to a member function get_name() on a non-object in /home/damyonw/Documents/Moodle/integration/master/moodle/question/editlib.php on line 1047 Call Stack # Time Memory Function Location 1 0.0019 237752 {main}( ) ../edit.php:0 2 0.0805 5954000 quiz_question_bank_view->__construct( ) ../edit.php:152 3 0.0805 5954504 question_bank_view->__construct( ) ../editlib.php:1110 4 0.0823 6552816 question_bank_view->init_columns( ) ../editlib.php:934
            Hide
            Marina Glancy added a comment -

            this line was added in MDL-40457, it's already failed, I'll copy your comment there Damyon. This issue will need to be retested when MDL-40457 is corrected

            Show
            Marina Glancy added a comment - this line was added in MDL-40457 , it's already failed, I'll copy your comment there Damyon. This issue will need to be retested when MDL-40457 is corrected
            Hide
            Damyon Wiese added a comment -

            26 and 25 are passing - I'll retest master when this is reset.

            Show
            Damyon Wiese added a comment - 26 and 25 are passing - I'll retest master when this is reset.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Sending this back to testing, MDL-40457 has been vanished from history (--force your clones) so master should be testable here.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Sending this back to testing, MDL-40457 has been vanished from history (--force your clones) so master should be testable here. Ciao
            Hide
            Damyon Wiese added a comment -

            Master is fine too now.

            Thanks Sam - passing this issue.

            Show
            Damyon Wiese added a comment - Master is fine too now. Thanks Sam - passing this issue.
            Hide
            Marina Glancy added a comment -

            Thanks for your awesome code, it is now part of Moodle. Don't forget to submit another issue next week (and peer review two).

            Show
            Marina Glancy added a comment - Thanks for your awesome code, it is now part of Moodle. Don't forget to submit another issue next week (and peer review two).

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: