Moodle

3rd party Assignment types are not backed up

Details

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

Attachments

  1. File
    corechanges.diff
    09/Aug/09 4:31 PM
    7 kB
    Penny Leach
  2. File
    newversion.diff
    11/Aug/09 11:01 PM
    7 kB
    Penny Leach
  3. File
    uploadpdfchanges.diff
    09/Aug/09 4:33 PM
    2 kB
    Penny Leach

Issue Links

This issue blocks:
CONTRIB-1268 Get backup/restore working with UploadPDF Major Resolved
MDL-20020 assignment subtypes in 1.9 can't provide their title in the mod edit form Minor Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.