Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-19142

3rd party Assignment types are not backed up

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

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

        Gliffy Diagrams

        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
            mjollnir 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
            mjollnir 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
            dougiamas 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
            dougiamas 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
            mjollnir 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
            mjollnir 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
            davosmith 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
            davosmith 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
            mjollnir Penny Leach added a comment -

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

            Show
            mjollnir Penny Leach added a comment - Changes required to the assignment module to support backup/restore in subtypes.
            Hide
            mjollnir 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
            mjollnir 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
            mjollnir 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
            mjollnir 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
            mjollnir Penny Leach added a comment -

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

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

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

            Show
            davosmith Davo Smith added a comment - I think those 4 hooks cover everything - but that is only from a short look at them.
            Hide
            mjollnir 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
            mjollnir 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
            stronk7 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
            stronk7 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
            mjollnir 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
            mjollnir 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
            mjollnir 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
            mjollnir 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
            mjollnir 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
            mjollnir 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:
                  Fix Release Date:
                  21/Oct/09