Moodle
  1. Moodle
  2. MDL-19142

3rd party Assignment types are not backed up

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8, 1.8.1, 1.8.2, 1.8.3, 1.8.4, 1.8.5, 1.8.6, 1.8.7, 1.8.8, 1.9, 1.9.1, 1.9.2, 1.9.3, 1.9.4
    • Fix Version/s: 1.9.6, 2.0
    • Component/s: Assignment (2.2)
    • Labels:
      None
    • Database:
      Any
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      31709

      Description

      I have written a new assignment type (uploadpdf) which creates extra database tables which are used by both the assignments and by the submissions. There does not seem to be any way that I can 'hook in' to the backup process and get this extra data backed up at the same time as the assignment is backed up.

      I am not sure whether the best place for a fix would be in 'backup/backuplib.php' (a search for all the plugins for each module and then a check for 'backuplib' in the 'mod/MODULENAME/type/PLUGINNAME' folder), or a fix in 'mod/assignment/backuplib.php' to look for backuplib.php there.

      To clarify, I would like to be able to create a file:
      moodle/mod/assignment/type/uploadpdf/backuplib.php
      and then backup some extra data there, whilst leaving the main assignment backup code (in moodle/mod/assignment/backuplib.php) to do the backup of the 'mdl_assignment' and 'mdl_assignment_submission', as usual.

      The current result is that the file mentioned above would be ignored and no backup of the extra data would take place.

      Obviously similar hooks would be needed for restoring modules.

      I am prepared to look at creating a patch for this, if I can be given a bit of guidance on it (would it be practical to fix moodle 1.8/1.9 or would this only be included in 2.0 and above?)

      1. corechanges.diff
        7 kB
        Penny Leach
      2. newversion.diff
        7 kB
        Penny Leach
      3. uploadpdfchanges.diff
        2 kB
        Penny Leach

        Issue Links

          Activity

          Hide
          Penny Leach added a comment -

          Hi David,

          I have to add backup/restore support to your uploadpdf subtype for a client. I'm going to patch the main assignment backuplib.php and restorelib.php to do this, I'll attach the patch when it's done.

          I can't see your module in contrib - is there any chance we can get it in there? If so, I can just commit directly there, that would be great.

          Cheers,
          Penny

          Show
          Penny Leach added a comment - Hi David, I have to add backup/restore support to your uploadpdf subtype for a client. I'm going to patch the main assignment backuplib.php and restorelib.php to do this, I'll attach the patch when it's done. I can't see your module in contrib - is there any chance we can get it in there? If so, I can just commit directly there, that would be great. Cheers, Penny
          Hide
          Martin Dougiamas added a comment -

          http://cvs.moodle.org/contrib/plugins/mod/assignment/type/uploadpdf/

          At a guess I think it would work like this:

          If those files exist for $assignment->assignmenttype then include it and call some function in there like assignment_backup_one_mod_uploadpdf ($bf,$preferences,$assignment) and in there produce XML for the assignment_uploadpdf_comment and assignment_uploadpdf tables
          (or restore from)

          Those two tables are keyed to assignment id and assignment submissions id

          I don't think you can backup/restore the template tables so easily, but perhaps that's not a problem

          No problem if you put the subtype hook stuff in stable, it's probably just a few lines

          Show
          Martin Dougiamas added a comment - http://cvs.moodle.org/contrib/plugins/mod/assignment/type/uploadpdf/ At a guess I think it would work like this: If those files exist for $assignment->assignmenttype then include it and call some function in there like assignment_backup_one_mod_uploadpdf ($bf,$preferences,$assignment) and in there produce XML for the assignment_uploadpdf_comment and assignment_uploadpdf tables (or restore from) Those two tables are keyed to assignment id and assignment submissions id I don't think you can backup/restore the template tables so easily, but perhaps that's not a problem No problem if you put the subtype hook stuff in stable, it's probably just a few lines
          Hide
          Penny Leach added a comment -

          Yeah, sorry, for some reason I didn't see that, I generally only have the 1.9 contrib checkout up to date

          Anyway I'll come up with a patch and get Eloy to review it for the core stuff, and figure something out with David for the template tables. I guess used-templates can be backed up in each backup, and restore can check if one exists already and skip it if it's the same.

          Show
          Penny Leach added a comment - Yeah, sorry, for some reason I didn't see that, I generally only have the 1.9 contrib checkout up to date Anyway I'll come up with a patch and get Eloy to review it for the core stuff, and figure something out with David for the template tables. I guess used-templates can be backed up in each backup, and restore can check if one exists already and skip it if it's the same.
          Hide
          Davo Smith added a comment -

          I had started to think about implementing the backup, but hadn't actually got as far as to write any code (family/holidays/lesson preparation all got in the way).

          Very happy to help clarify how any of the bits of uploadpdf fit together and discuss anything to do with the backup format, although further discussions specific to uploadpdf (rather than general backup of assignment types), should go here: CONTRIB-1268

          Show
          Davo Smith added a comment - I had started to think about implementing the backup, but hadn't actually got as far as to write any code (family/holidays/lesson preparation all got in the way). Very happy to help clarify how any of the bits of uploadpdf fit together and discuss anything to do with the backup format, although further discussions specific to uploadpdf (rather than general backup of assignment types), should go here: CONTRIB-1268
          Hide
          Penny Leach added a comment -

          Changes required to the assignment module to support backup/restore in subtypes.

          Show
          Penny Leach added a comment - Changes required to the assignment module to support backup/restore in subtypes.
          Hide
          Penny Leach added a comment -

          Small changes to the uploadpdf subtype to support being able to have custom extra backup, as well as method stubs.

          Show
          Penny Leach added a comment - Small changes to the uploadpdf subtype to support being able to have custom extra backup, as well as method stubs.
          Hide
          Penny Leach added a comment -

          Eloy, can you please review the assignment changes, since it affects backup? (Don't worry about the subtype specific patch)

          Show
          Penny Leach added a comment - Eloy, can you please review the assignment changes, since it affects backup? (Don't worry about the subtype specific patch)
          Hide
          Penny Leach added a comment -

          David, can you please review the 4 new hooks and confirm it will cover everything?

          Show
          Penny Leach added a comment - David, can you please review the 4 new hooks and confirm it will cover everything?
          Hide
          Davo Smith added a comment -

          I think those 4 hooks cover everything - but that is only from a short look at them.

          Show
          Davo Smith added a comment - I think those 4 hooks cover everything - but that is only from a short look at them.
          Hide
          Penny Leach added a comment -

          dev chat with eloy:

          13:32 < stronk7> so, penny, you've implemented it by allowing two hooks: one at module
          level and another at submission level?
          13:32 < penny> yes,
          13:32 < penny> times two
          13:33 < stronk7> aha. well, apart from some print_object() and some control of the
          returned assignment_ensure_backup_subtype() I think it's safe
          13:35 < penny> control ?
          13:36 < stronk7> yes, does that function always return (by reference) one valid assignment object (note I don't know about that "atypeobj" condition).
          13:37 < penny> it doesn't return
          13:37 < penny> oh
          13:37 < penny> yes
          13:38 < penny> it takes it by ref and modifies it
          13:38 < moodlebot_> penny: Does it really?
          13:38 * penny glares at moodlebot
          13:38 < penny> eloy atypeobj is just where it stores the assignment object (the real one,
          not the database one)
          13:40 < stronk7> aha. oki
          13:40 < stronk7> also... just saw....
          13:40 < stronk7> that you are playing with "course_modules" table....
          13:41 < stronk7> and I think that table is adjusted AFTER restoring all modules, so not
          sure if info there is reliable at that stage.
          13:41 < stronk7> restore_check_instances()
          13:42 < penny> don't really have a choice
          13:42 < penny> i thougth about that
          13:42 < penny> it's between when
          13:42 < penny> a. the course_module record is created,
          13:42 < penny> and b. the instance is set
          13:42 < penny> which is why i set instance on the record i get back in php
          13:43 < stronk7> yup
          13:43 < stronk7> bloody constructor
          13:44 < penny> ?
          13:48 < stronk7> just thinking if you need that $cm really there? I mean... it's a "faked"
          $cm, because DB really haven't that instance yet.
          13:49 < stronk7> what if assignment internals do anything with that info. Just that, a
          "theoretical" question.
          13:50 < stronk7> Anyway... if that doesn't break anything in core subtypes.... +1 here,
          please merge it to head to have it present there too.
          13:54 < penny> thanks eloy
          13:54 < penny> they don't do anything, only the restore* methods are called
          13:55 < penny> i agree it seems fragile,
          13:55 < penny> but i couldn't see a better way

          So I'll commit this later today or tomorrow.

          Show
          Penny Leach added a comment - dev chat with eloy: 13:32 < stronk7> so, penny, you've implemented it by allowing two hooks: one at module level and another at submission level? 13:32 < penny> yes, 13:32 < penny> times two 13:33 < stronk7> aha. well, apart from some print_object() and some control of the returned assignment_ensure_backup_subtype() I think it's safe 13:35 < penny> control ? 13:36 < stronk7> yes, does that function always return (by reference) one valid assignment object (note I don't know about that "atypeobj" condition). 13:37 < penny> it doesn't return 13:37 < penny> oh 13:37 < penny> yes 13:38 < penny> it takes it by ref and modifies it 13:38 < moodlebot_> penny: Does it really? 13:38 * penny glares at moodlebot 13:38 < penny> eloy atypeobj is just where it stores the assignment object (the real one, not the database one) 13:40 < stronk7> aha. oki 13:40 < stronk7> also... just saw.... 13:40 < stronk7> that you are playing with "course_modules" table.... 13:41 < stronk7> and I think that table is adjusted AFTER restoring all modules, so not sure if info there is reliable at that stage. 13:41 < stronk7> restore_check_instances() 13:42 < penny> don't really have a choice 13:42 < penny> i thougth about that 13:42 < penny> it's between when 13:42 < penny> a. the course_module record is created, 13:42 < penny> and b. the instance is set 13:42 < penny> which is why i set instance on the record i get back in php 13:43 < stronk7> yup 13:43 < stronk7> bloody constructor 13:44 < penny> ? 13:48 < stronk7> just thinking if you need that $cm really there? I mean... it's a "faked" $cm, because DB really haven't that instance yet. 13:49 < stronk7> what if assignment internals do anything with that info. Just that, a "theoretical" question. 13:50 < stronk7> Anyway... if that doesn't break anything in core subtypes.... +1 here, please merge it to head to have it present there too. 13:54 < penny> thanks eloy 13:54 < penny> they don't do anything, only the restore* methods are called 13:55 < penny> i agree it seems fragile, 13:55 < penny> but i couldn't see a better way So I'll commit this later today or tomorrow.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          As commented in HQ chat:

          • I hate assignment subtypes, not sure how/where they will end in 2.0.
          • I like chocolate
          • The patch looks correct (but some remaining print_object() and minor doubts about the "faked" $cm.

          So:

          • if it doesn't break anything in core plugins (plz test!) => +1 to commit.
          • And plz, merge the patch also to HEAD to have it present there, no matter restore isn't working there.

          Tons of thanks, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - As commented in HQ chat: I hate assignment subtypes, not sure how/where they will end in 2.0. I like chocolate The patch looks correct (but some remaining print_object() and minor doubts about the "faked" $cm. So: if it doesn't break anything in core plugins (plz test!) => +1 to commit. And plz, merge the patch also to HEAD to have it present there, no matter restore isn't working there. Tons of thanks, ciao
          Hide
          Penny Leach added a comment -

          i committed this and then almost immediately backed it out once i figured the course_module stuff wasn't going to work. backup_getid wants the id of the old coursemodule, not the id of the old module instance. i'll come up with another patch, we'll have to use the subtype classes statically I think, and pass $assignment to them.

          Show
          Penny Leach added a comment - i committed this and then almost immediately backed it out once i figured the course_module stuff wasn't going to work. backup_getid wants the id of the old coursemodule, not the id of the old module instance. i'll come up with another patch, we'll have to use the subtype classes statically I think, and pass $assignment to them.
          Hide
          Penny Leach added a comment -

          new version, using the methods statically.

          although i am following convention here in the assignment module & subtypes, i don't approve of using a constructor with 'staticonly' as an id argument. why aren't we using proper static calls?! surely php4 has static?

          anyway, eloy please re-review.

          Show
          Penny Leach added a comment - new version, using the methods statically. although i am following convention here in the assignment module & subtypes, i don't approve of using a constructor with 'staticonly' as an id argument. why aren't we using proper static calls?! surely php4 has static? anyway, eloy please re-review.
          Hide
          Penny Leach added a comment -

          Yay, fixed in stable & head

          static keyword for methods not supported in php4 so i left it out but they're in HEAD.

          Show
          Penny Leach added a comment - Yay, fixed in stable & head static keyword for methods not supported in php4 so i left it out but they're in HEAD.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: