Allow mod_assign to restore from mod_assignment backup files and completely remove mod_assignment

I'm surprised this is the first time this has come, it surely stops us from dropping Assignment 2.2 until this is implemented?

The attached patch does the job, but this is a complex change (esp this late for 2.3). If we are including this in 2.3 we should get a backup/restore guru to verify the code.

In particular the upgrade hook is in after_restore(). This needs to run as the very last part of the restore process (after EVERYTHING has been created for the assignment - so the data exists and can be converted during the ugprade). Is this the best place for this hook ?

Also - is there anywhere in Moodle where immediately after a restore - a link is created to the new module directly? - If so this won't work because this module gets deleted as part of the ugprade. (In my testing it looked like it always linked to the course - which is fine).

Regards, Damyon

Shouldn't we have a config to let user decide if he wants the assignments to be upgraded or not during restore?

Should the default be yes or no?

Hi Damyon,
I have added Eloy as watcher to this issue. He might be the best person to answer that.
Thanks

Personally, I would most certainly say it should default to No, given that the site admin has to proactively convert assignments in the first place.

And just to clarify, I think this should be a site setting, not a restore setting (or at least some way for the admin to set it and hide it from the user).

I'll update the patch to add a setting.

I have added an admin setting to the old assignment to enable/disable this functionality.

It will still only attempt an upgrade if the old assignment module is disabled regardless of this setting - this is to prevent errors with the duplicate and import functions.

The admin setting uses the old style which is consistent with the rest of the old assignment module (stores it in the global config instead of the plugin config table)

Default is off.

Hi Damyon. Patch looks good! Feel free to submit for integration whenever you like.

Hi Frederic,

Can someone comment on these 2 issues before I submit this for integration:

In particular the upgrade hook is in after_restore(). This needs to run as the very last part of the restore process (after EVERYTHING has been created for the assignment - so the data exists and can be converted during the ugprade). Is this the best place for this hook ?

Also - is there anywhere in Moodle where immediately after a restore - a link is created to the new module directly? - If so this won't work because this module gets deleted as part of the ugprade. (In my testing it looked like it always linked to the course - which is fine).

Thanks, Damyon

Sorry we've dropped the ball on this, Damyon. 2.3.1 is very soon. I think we'll submit it for integration and hopefully Eloy can resolve it.

I think David will look to this (I'm starting vacation period and after that, I've a pile of integration/releasing tasks to do, so I'll be offering support/review here only).

But, in any case... I've one EXISTENTIAL question and is if it's correct to assume that everybody will want their 2.2 mod_assignments converted on restore automatically to the new mod_assigns. Simple question which answer has important consequences.

Ciao

Thanks for providing the patch Damyon. Just a couple of notes.

• I expect that the assign_upgrade_manager() handles gradebook items, conditional access rules, completion tracking and all these things. If so then yes, after_restore() is the only possible place to call the conversion from. If the conversion was an internal to the module's data only, you could use after_execute()
• I am not aware of any automatic linking present. But if you create new course_module record for the converted instance then the restore process will probably fail to correctly remap the interlinks (eg. if an URL like http://old.site/mod/assignment/view.php?id=42 is present in some text in the course such as a section description - which is pretty common case).
• For the same reason, because you create new course_module records, the Assignment 2.3 gets new context after the conversion. This will probably cause failures of file aliases restore because the aliases are restored only after all after_restore() methods have been executed. So the expected mod_assignment won't be already there by that time.
• Actually I'm wondering why did you decide to use new course_modules during the conversion. In all similar cases (typically when mod_resource was split) we have always preserved cmids (and hence context ids too). I would see the point if you left the legacy mod_assignment instances in the course but as long as you delete them, ...
• Note that using strings like $string['configconvertmodassignonrestore'] is discouraged now. You should use $string['convertmodassignonrestore_desc'] so that these two appear next to each other for translators (and they know the context). If needed, the xxx_desc strings can use Markdown formatting, just FYI.
• Overall, I would consider the proposed patch as a temporary solution for 2.3. The point is that if this was supposed to stay as the patch suggests, then we could never get rid of mod_assignment from the standard distribution. The long-term solution is to modify mod_assign's restore in a way that it supports restore of mod_assignment's structures natively. Eloy believes that the backup&restore framework has all needed included (basically mod_assign's activity task has to be tweaked so that it parses and processes XML data for mod_assignment too).

Hi David and Eloy,

Any static URL's in the course would have to be broken regardless because the module name needs changing (e.g. a link to /mod/assignment/view.php?id=45 would have to be changed to /mod/assign/view.php?id=45 even if we kept the id's the same).

Not sure what to do about the aliases - during the upgrade any files are moved to new file areas and the old ones are deleted. Is there any way to get the aliases to restore before this code runs?

The reason I used new cmids is that it means that I can safely call the delete_instance function for the old assignment module to clean up any data that I don't know about (e.g. for custom subtypes).

I agree about this being a temporary fix - for 2.4 we should be able to restore to an instance of mod_assign from a mod_assignment backup. What happens when 2 activities both have matching rules to restore from the same backup structure? I guess I'll find out soon enough.

So in summary - given that this will introduce new subtle bugs (aliases) I think we should not integrate this patch. We should document the behaviour for 2.3 and look to implement restore directly in the restore activity task for 2.4.

Regards, Damyon

Ho,

for sure we'll need to add some extra stuff to restore infrastructure to be able to specify that one module is in charge of restoring another and other tweaks, but I think the idea of mod_assign being able to restore both mod_assign and mod_assignment backups is the way to go.

Of course, that cannot be made for 23_STABLE, so perhaps it's not a bad idea to delay that until then (6 months). In the mean time... I've one question... if we restore those mod_assignments normally (i.e. as mod_assignments)... is it possible later to migrate them in any way? Running the upgrade process selectively or so?

Note, also, that it needs to be decided when mod_assignment is going to be out from core, because some decisions @ restore modifications will be based on that. For example, to show one setting to decide how to restore each mod_assignment (as mod_assignment or as mod_assign).

So I'm also +1 about not to integrate this patch, it leaves many things open, IMO. For your consideration, anyway.

Ciao

Hi guys,

Any thoughts on Eloy's comments from last week?
It looks like he had a good look at this but there has been no response yet.

Cheers
Sam

We have 2 votes for not integrating this patch and implementing direct restore from backups instead.

So - we should close this issue and create a new one for 2.4 to do just that.

(let me know if you disagree)

Cheers,
Damyon

Reopening this and taking out of integration whilst way forward is being decided.

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

By the way, if you are going to document the suggested workaround, maybe the better approach than disabling the old Assignment (2.2) activity is to keep it enabled after the migration but revoke the capability mod/assignment:addinstance from all users at the site.

I have documented this here (section 4) http://docs.moodle.org/23/en/Upgrade_tool Would someone please check I have understood it correctly

Looks good to me.

Excellent idea - thanks David!

So is this targeting 2.4 now?

No - it hasn't been looked at for 2.4 but will be a priority for 2.5.

Hows it looking for 2.5?

No - sorry no work done on this from me yet (I have a month of integration leave booked around July so will probably work on this + activity grader + bugs).

Damyon, we have code in Joule to automatically kick off the assignment upgrade process after the course restore has completed. It works based on the course_restore event handler, but otherwise it functions very similarly to the proposed patch. In addition, we added a reference table to store the assignment id and cmid for both the old and the new assignment. That not only saved us in a few cases where data was lost in the upgrade process (e.g. Outcomes), it also allows us to automatically redirect users from the old assignment when static links were used. This approach has worked out very well for us. We'd be happy to supply any code if you're interested.

My +1 to remove mod_assignment and add code to allow mod_assign to restore mod_assignments at the same time. We shouldn't need to keep both modules in core after that work is completed.

Just noting that questions about restoring course backups containing an assignment from Moodle 2.2 and older come up quite often in the moodle.org forums e.g. https://moodle.org/mod/forum/discuss.php?d=244723. Thus, this issue has my vote to be fixed.

In the meantime I have documented what needs to be done in http://docs.moodle.org/en/Assignment_upgrade_tool and have mentioned it in FAQ and backup pages etc.

Linking to forum post about this:

https://moodle.org/mod/forum/discuss.php?d=245588

Copy paste from that post:

So what we propose is that for 2.7, mod_assignment will be replaced by a stub that looks for a mapping from old to new assignment and redirects. The restore code and DB tables will remain in this stub. A hook will be added to the restore code to upgrade from mod_assignment to mod_assign immediately after restore. The upgrade code will be modified to record the old and new cmid after each upgrade to allow any old urls to continue working. The full mod_assignment will not be available in the plugindb or in core for 2.7+. Plugin sub-type support will remain in the stub, so that old plugins for mod_assignment, can still restore, and then participate in the automatic upgrade.

Note about this patch. I welcome suggestions about FEATURE_PLUGIN_DEPRECATED. I need a way to prevent things that will be broken from using this stub. Things like e.g. duplicate will not work because the duplicated course module will be automatically upgraded and this breaks the duplicate function because it cannot find the id of the new module (because it is deleted already). There is no mod_form etc - so create/update cannot work. I could spend ages working around all these things, but there will be no benefit in the end - they must upgrade before they can do anything with their assignments.

Wow, all these changes mixed into a single commit. Come one, you must know that it really does not make peer-reviews easier. Could the branch be reconstructed so that it illustrates the overall logic in a sequence of well described commits?

I can understand your needs for FEATURE_PLUGIN_DEPRECATED but I'm not personally impressed by that. Polluting core with hacks like that, which are so easy to be overlooked when adding a new feature, ... Can we use capabilities for that? And have an upgrade step that just prohibits the Authenticated user role from doing them in all known contexts. If the admin is crazy enough be un-prohibit it, their choice...

Sorry - I'm a compulsive squasher (especially when I can't remember writing the first commits).

Maybe instead of nasty FEATURE_PLUGIN_DEPRECATED - I can work-around the broken features in the stub. Testing...

OK - Added a commit that reverts the FEATURE_PLUGIN_DEPRECATED and added workarounds for the other functions that are not supposed to work with the mod_assignment stub.

This branch will be squashed before integration.

looks ok to me, +3, yay!

Rebased, squashed.

+1 to go ahead with this.

Obviously we need to be very sure that Moodle 2.7 mod_assign does EVERYTHING that mod_assignment did. I feel pretty sure this must be the case by now but it would be good to have testers focus on this in MDLQA.

Also, we need to make sure the old mod_assignment goes into moodle.org/plugins in case anyone who needs it for 2.7. Doesn't need to be maintained beyond that though.

Hi Damyon,

1. This is breaking phpunit badly in various places.
2. I should also ask if you've run behat against it...
3. On upgrade, I got the following error (and it looks like both indexes are wrong)

   Debug info: ERROR: column "oldid" does not exist
   CREATE INDEX mdl_assiupgr_old_ix ON mdl_assignment_upgrade (oldid)
   Error code: ddlexecuteerror
   Stack trace:
   line 447 of /lib/dml/moodle_database.php: ddl_change_structure_exception thrown
   line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
   line 637 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
   line 88 of /lib/ddl/database_manager.php: call to pgsql_native_moodle_database->change_database_structure()
   line 77 of /lib/ddl/database_manager.php: call to database_manager->execute_sql()
   line 436 of /lib/ddl/database_manager.php: call to database_manager->execute_sql_arr()
   line 106 of /mod/assignment/db/upgrade.php: call to database_manager->create_table()
   line 670 of /lib/upgradelib.php: call to xmldb_assignment_upgrade()
   line 395 of /lib/upgradelib.php: call to upgrade_plugins_modules()
   line 1580 of /lib/upgradelib.php: call to upgrade_plugins()
   line 427 of /admin/index.php: call to upgrade_noncore()
   

4. assignment_delete_instance() has lots of var_dump(_LINE_);
5. mod/assignment/db/upgradelib.php has incorrect phpdocs talking about googledocs.
6. What is the need for the course/renderer.php change?
7. I don't think you should be setting the copyright attribution on mod/assignment/mod_form.php to you. Looking at the git log it should be to Petr.
8. I would prefer spacing between the operators in teh sql queruies

reopening,

cheers
Dan

Bah - sorry - I'll get this cleaned up for next week.

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

Thanks Dan,

1. Fixed the phpunit fails - they were caused because other parts of moodle were trying to use the generator for mod_assignment which was removed. I put a minimal generator back in - but only for mod_assign upgrade tests. All other uses were changed to use mod_assign instead.
2. Running behat now...
3. Indexes fixed.
4. Removed (oops)
5. Fixed - now you know I was inspired by your code
6. Nothing - it was left over from the FEATURE_PLUGIN_DEPRECATED hack (removed now)
7. Changed it to Petr
8. Changed the spacing

I'll submit this if behat passes.

Behat:

258 scenarios (258 passed)
9010 steps (9010 passed)

thanks Damyon, looks good and integrated to master.

There are 2 behat failures related with this issue: http://nightly01.test.local:8080/view/07.%20Behat%20whole%20suite/job/11.1.-%20Run%20all%20behat%20features%20in%20firefox-linux%20(master)/298/console

A string change is needed in blocks/activity_modules/tests/behat/block_activity_modules.feature

Failing the test. Strange that Damyons run didn't pick it up

Hmm. I ran it in phantom - so maybe it was _switch_windows or _only_local. Will fix.

No - still confused why I didn't see the fail before - but I have added a fix at:

git pull git://github.com/damyon/moodle.git MDL-33952-master-fix1

And a comment on the fix. IMO it is more correct to remove the tests for "Assignment (2.2)" because it is no longer a real activity (just a stub) - than it is to change the string in the test. You cannot create instances of "Assignment (2.2)" through the web interface any more. The data generator remains only to allow testing the upgrade code to mod_assign.

I agree and i've integrated this. Back to waiting for testing.

Although I'm not seeing the assignment listed in the course, I am seeing it listed in the "Upcoming Events" block, even as a student. of course, attempting to view the activity gives the warning message still.

Also listed in the calendar prior to upgrade

Talking about this with Andrew and it does not seem a good idea to hide the instances of the module on upgrade - that creates a job to unhide them later. We should still disable the module because that saves a step that might be missed later (and the old assignment will remain in the activity chooser - but lead to an error page).

I have pushed a branch with this change here:

git pull git://github.com/damyon/moodle.git MDL-33952-master-fix2

thanks Damyon, i've integrated that now

Everything went smoothly.
I tried to create assignments with as many different features enabled as possible and I couldn't break the upgrade. Seems pretty robust (famous last words).
Test passed.

Sorry, just realised that my comments from yesterday didn't save for some reason:

I've been unable to test the e-mail section of this - for some reason this particular e-mail is disappearing in the Google blackhole (my MTA is delivering it to Google, but it doesn't appear to arrive when other e-mails do). It'll be a configuration issue at my end.

Otherwise though there are a couple of outstanding issues:

• as part of the upgrade, mod_assignment is disabled again. This has the effect that it will disappear from courses which could (and will) confuse some users. I've discussed with Damyon and we feel that it's better to keep them around rather than have them entirely disappear. Damyon is going to prepare a patch for this;
• even with mod_assignment disabled, the assignments appear in the calendar and recent actvitiy.

===

I think that the first of these issues has now been addressed, and that the calendar and recent activity block are separate issues which are long standing and should be addressed in a new issue.

Well done is better than well said.

Closing, big thanks!