Moodle
  1. Moodle
  2. MDL-33952

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

    Details

    • Testing Instructions:
      Hide
      1. Before this patch:
        1. Enable both mod_assign and mod_assignment
        2. Create lots of instance of mod_assignment (all the different types).
        3. Add some data (submission/grades) to the assignment (the more the better)
        4. Create a backup of the course with all the assignments
      1. Install this patch
        1. Upgrade your site
        2. Verify that the admin received a notice about assignments that need upgrading
        3. Verify that accessing any of the assignments gives an upgrade required message
        4. Copy the urls of the existing assignments
        5. Verify that you can upgrade the assignments using the upgrade tool
        6. Verify you can access the assignments at their previous urls
        7. Verify that the grade book and calendar link to the correct assignment instance
        8. Restore the backup file into a new course
        9. Access the course and verify that all assignments are visible and have been upgraded to mod_assign as part of the restore
        10. Verify that you cannot create new instances of the old assignment module
        11. Verify that you cannot do anything with the old assignments but upgrade, backup or delete them (or indent/outdent).
      Show
      Before this patch: Enable both mod_assign and mod_assignment Create lots of instance of mod_assignment (all the different types). Add some data (submission/grades) to the assignment (the more the better) Create a backup of the course with all the assignments Install this patch Upgrade your site Verify that the admin received a notice about assignments that need upgrading Verify that accessing any of the assignments gives an upgrade required message Copy the urls of the existing assignments Verify that you can upgrade the assignments using the upgrade tool Verify you can access the assignments at their previous urls Verify that the grade book and calendar link to the correct assignment instance Restore the backup file into a new course Access the course and verify that all assignments are visible and have been upgraded to mod_assign as part of the restore Verify that you cannot create new instances of the old assignment module Verify that you cannot do anything with the old assignments but upgrade, backup or delete them (or indent/outdent).
    • Workaround:
      1. Enable mod_assignment
      2. Restore an assignment backup
      3. Use the upgrade tool to upgrade the assignment
      4. Hide mod_assignment
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33952-master

      Description

      If you have a site with Assignment (2.2) disabled/hidden, when somebody restores a backup with a Assignment (2.2) activity, it will show up in the restore process, saying it will be restored, etc, but when the restore is finished, it won't be available.

      If the site administrator re-enables Assignment (2.2) site-wide, it will be there, so it is restored, but to the user, it just completely disappeared with no warning.

      In my opinion, there should be a site setting that causes Assignment (2.2) activities to be automatically upgraded on restore - or at least add a warning to the user that the assignment won't be available.

      It boils down to - if a site wants to completely upgrade to the new Assignment, they run the upgrader, disable the old Assignment (2.2), the behaviour to the end-user becomes inconsistant.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Poltawski added a comment -

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

            Show
            Dan Poltawski added a comment - I'm surprised this is the first time this has come, it surely stops us from dropping Assignment 2.2 until this is implemented?
            Hide
            Damyon Wiese added a comment -

            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

            Show
            Damyon Wiese added a comment - 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
            Hide
            Ankit Agarwal added a comment -

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

            Show
            Ankit Agarwal added a comment - Shouldn't we have a config to let user decide if he wants the assignments to be upgraded or not during restore?
            Hide
            Damyon Wiese added a comment -

            Should the default be yes or no?

            Show
            Damyon Wiese added a comment - Should the default be yes or no?
            Hide
            Ankit Agarwal added a comment -

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

            Show
            Ankit Agarwal added a comment - Hi Damyon, I have added Eloy as watcher to this issue. He might be the best person to answer that. Thanks
            Hide
            Eric Merrill added a comment -

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

            Show
            Eric Merrill added a comment - 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).
            Hide
            Damyon Wiese added a comment -

            I'll update the patch to add a setting.

            Show
            Damyon Wiese added a comment - I'll update the patch to add a setting.
            Hide
            Damyon Wiese added a comment -

            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.

            Show
            Damyon Wiese added a comment - 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.
            Hide
            Frédéric Massart added a comment -

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

            Show
            Frédéric Massart added a comment - Hi Damyon. Patch looks good! Feel free to submit for integration whenever you like.
            Hide
            Damyon Wiese added a comment -

            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

            Show
            Damyon Wiese added a comment - 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
            Hide
            Martin Dougiamas added a comment -

            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.

            Show
            Martin Dougiamas added a comment - 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.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            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

            Show
            Eloy Lafuente (stronk7) added a comment - 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
            Hide
            David Mudrak added a comment -

            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).
            Show
            David Mudrak added a comment - 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).
            Hide
            Damyon Wiese added a comment -

            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

            Show
            Damyon Wiese added a comment - 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
            Hide
            Eloy Lafuente (stronk7) added a comment -

            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

            Show
            Eloy Lafuente (stronk7) added a comment - 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
            Hide
            Sam Hemelryk added a comment -

            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

            Show
            Sam Hemelryk added a comment - 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
            Hide
            Damyon Wiese added a comment -

            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

            Show
            Damyon Wiese added a comment - 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
            Hide
            Dan Poltawski added a comment -

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

            Show
            Dan Poltawski added a comment - Reopening this and taking out of integration whilst way forward is being decided.
            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
            David Mudrak added a comment -

            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.

            Show
            David Mudrak added a comment - 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.
            Hide
            Mary Cooch added a comment -

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

            Show
            Mary Cooch added a comment - I have documented this here (section 4) http://docs.moodle.org/23/en/Upgrade_tool Would someone please check I have understood it correctly
            Hide
            Eric Merrill added a comment -

            Looks good to me.

            Show
            Eric Merrill added a comment - Looks good to me.
            Hide
            Damyon Wiese added a comment -

            Excellent idea - thanks David!

            Show
            Damyon Wiese added a comment - Excellent idea - thanks David!
            Hide
            Jonathan Champ added a comment -

            So is this targeting 2.4 now?

            Show
            Jonathan Champ added a comment - So is this targeting 2.4 now?
            Hide
            Damyon Wiese added a comment -

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

            Show
            Damyon Wiese added a comment - No - it hasn't been looked at for 2.4 but will be a priority for 2.5.
            Hide
            Tim Lock added a comment -

            Hows it looking for 2.5?

            Show
            Tim Lock added a comment - Hows it looking for 2.5?
            Hide
            Damyon Wiese added a comment -

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

            Show
            Damyon Wiese added a comment - 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).
            Hide
            Kris Stokking added a comment -

            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.

            Show
            Kris Stokking added a comment - 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.
            Hide
            Dan Marsden added a comment -

            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.

            Show
            Dan Marsden added a comment - 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.
            Hide
            Helen Foster added a comment -

            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.

            Show
            Helen Foster added a comment - 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.
            Hide
            Damyon Wiese added a comment -

            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.

            Show
            Damyon Wiese added a comment - 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.
            Hide
            Damyon Wiese added a comment -

            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.

            Show
            Damyon Wiese added a comment - 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.
            Hide
            David Mudrak added a comment -

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

            Show
            David Mudrak added a comment - 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...
            Hide
            Damyon Wiese added a comment -

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

            Show
            Damyon Wiese added a comment - Sorry - I'm a compulsive squasher (especially when I can't remember writing the first commits).
            Hide
            Damyon Wiese added a comment -

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

            Show
            Damyon Wiese added a comment - Maybe instead of nasty FEATURE_PLUGIN_DEPRECATED - I can work-around the broken features in the stub. Testing...
            Hide
            Damyon Wiese added a comment -

            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.

            Show
            Damyon Wiese added a comment - 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.
            Hide
            Petr Skoda added a comment -

            looks ok to me, +3, yay!

            Show
            Petr Skoda added a comment - looks ok to me, +3, yay!
            Hide
            Damyon Wiese added a comment -

            Rebased, squashed.

            Show
            Damyon Wiese added a comment - Rebased, squashed.
            Hide
            Martin Dougiamas added a comment -

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

            Show
            Martin Dougiamas added a comment - +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.
            Hide
            Dan Poltawski added a comment -

            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

            Show
            Dan Poltawski added a comment - Hi Damyon, This is breaking phpunit badly in various places. I should also ask if you've run behat against it... 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() assignment_delete_instance() has lots of var_dump(_ LINE _); mod/assignment/db/upgradelib.php has incorrect phpdocs talking about googledocs. What is the need for the course/renderer.php change? 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. I would prefer spacing between the operators in teh sql queruies reopening, cheers Dan
            Hide
            Damyon Wiese added a comment -

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

            Show
            Damyon Wiese added a comment - Bah - sorry - I'll get this cleaned up for next week.
            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
            Damyon Wiese added a comment -

            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.

            Show
            Damyon Wiese added a comment - 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.
            Hide
            Damyon Wiese added a comment -

            Behat:

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

            Show
            Damyon Wiese added a comment - Behat: 258 scenarios (258 passed) 9010 steps (9010 passed)
            Hide
            Dan Poltawski added a comment -

            thanks Damyon, looks good and integrated to master.

            Show
            Dan Poltawski added a comment - thanks Damyon, looks good and integrated to master.
            Hide
            David Monllaó added a comment -

            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

            Show
            David Monllaó added a comment - 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
            Hide
            Dan Poltawski added a comment -

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

            Show
            Dan Poltawski added a comment - Failing the test. Strange that Damyons run didn't pick it up
            Hide
            Damyon Wiese added a comment -

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

            Show
            Damyon Wiese added a comment - Hmm. I ran it in phantom - so maybe it was _switch_windows or _only_local. Will fix.
            Hide
            Damyon Wiese added a comment -

            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

            Show
            Damyon Wiese added a comment - 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
            Hide
            Damyon Wiese added a comment -

            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.

            Show
            Damyon Wiese added a comment - 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.
            Hide
            Dan Poltawski added a comment -

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

            Show
            Dan Poltawski added a comment - I agree and i've integrated this. Back to waiting for testing.
            Hide
            Andrew Nicols added a comment -

            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.

            Show
            Andrew Nicols added a comment - 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.
            Hide
            Andrew Nicols added a comment -

            Also listed in the calendar prior to upgrade

            Show
            Andrew Nicols added a comment - Also listed in the calendar prior to upgrade
            Hide
            Damyon Wiese added a comment -

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

            Show
            Damyon Wiese added a comment - 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).
            Hide
            Damyon Wiese added a comment -

            I have pushed a branch with this change here:

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

            Show
            Damyon Wiese added a comment - I have pushed a branch with this change here: git pull git://github.com/damyon/moodle.git MDL-33952 -master-fix2
            Hide
            Dan Poltawski added a comment -

            thanks Damyon, i've integrated that now

            Show
            Dan Poltawski added a comment - thanks Damyon, i've integrated that now
            Hide
            Adrian Greeve added a comment -

            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.

            Show
            Adrian Greeve added a comment - 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.
            Hide
            Andrew Nicols added a comment -

            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.

            Show
            Andrew Nicols added a comment - 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.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Well done is better than well said.

            Closing, big thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Well done is better than well said. Closing, big thanks!
            Hide
            Mary Cooch added a comment -

            I am just documenting this and can't seem to find the old mod/assignment in the plugins database - has it been added yet?

            Show
            Mary Cooch added a comment - I am just documenting this and can't seem to find the old mod/assignment in the plugins database - has it been added yet?
            Hide
            Aparup Banerjee added a comment -

            Damyon Wiese has just now submitted it Mary. Thanks for the ping!
            Its awaiting approval at https://moodle.org/plugins/view.php?plugin=mod_assignment
            It'll be approved once David Mudrak or Anthony Borrow get to it

            Show
            Aparup Banerjee added a comment - Damyon Wiese has just now submitted it Mary. Thanks for the ping! Its awaiting approval at https://moodle.org/plugins/view.php?plugin=mod_assignment It'll be approved once David Mudrak or Anthony Borrow get to it
            Hide
            Aparup Banerjee added a comment -

            actually its now not there anymore, removed... due to a legacy issues with AMOS/backup/restore still using 'mod_assignment'
            Its here now: http://github.com/moodlehq/moodle-mod_assignment

            Show
            Aparup Banerjee added a comment - actually its now not there anymore, removed... due to a legacy issues with AMOS/backup/restore still using 'mod_assignment' Its here now: http://github.com/moodlehq/moodle-mod_assignment
            Hide
            Mary Cooch added a comment -

            Thanks - I'm removing docs_required label as it's documented in http://docs.moodle.org/dev/Moodle_2.7_release_notes and http://docs.moodle.org/27/en/Upgrading_to_Moodle_2.7 (thanks Damyon Wiese and also http://docs.moodle.org/27/en/Assignment_upgrade_tool (if someone could just check that thanks)

            Show
            Mary Cooch added a comment - Thanks - I'm removing docs_required label as it's documented in http://docs.moodle.org/dev/Moodle_2.7_release_notes and http://docs.moodle.org/27/en/Upgrading_to_Moodle_2.7 (thanks Damyon Wiese and also http://docs.moodle.org/27/en/Assignment_upgrade_tool (if someone could just check that thanks)

              People

              • Votes:
                34 Vote for this issue
                Watchers:
                39 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: