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
    • Rank:
      42059

      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.

        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 Škoda added a comment -

          looks ok to me, +3, yay!

          Show
          Petr Škoda 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!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: