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!

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?

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.

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.

Confirmed this is breaking. Added test script.

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.

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.

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

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

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

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.

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

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.

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.

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

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

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

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...

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.

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

lol, I am an idiot, sorry.

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

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).

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

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

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

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

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...

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?

Good idea. Updating commits now.

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.

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

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

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

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

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

Ciao

Master is fine too now.

Thanks Sam - passing this issue.

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