Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Backup, Course completion
    • Labels:
      None
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      32794

      Description

      Understand and add course completion stuff to backup:

      New tables:

      • course_completion_aggr_methd
      • course_completion_criteria
      • course_completion_crit_compl
      • course_completion_notify
      • course_completions (user level)

      New columns:

      • course->enablecompletion
      • course->completionstartonenrol
      • course->completionnotify
      1. MDL-22254.20100913.patch
        12 kB
        Sam Hemelryk
      2. MDL-22254.20100915.patch
        16 kB
        Sam Hemelryk
      3. MDL-22254.20100916.patch
        16 kB
        Sam Hemelryk

        Activity

        Hide
        Martin Dougiamas added a comment -

        Assigning to Sam temporarily, but Aaron we could really use your help on this if you're available!

        Show
        Martin Dougiamas added a comment - Assigning to Sam temporarily, but Aaron we could really use your help on this if you're available!
        Hide
        Aaron Barnes added a comment -

        Hey Martin/Sam,

        I'm available to answer any questions you might have

        Cheers,
        Aaron

        Show
        Aaron Barnes added a comment - Hey Martin/Sam, I'm available to answer any questions you might have Cheers, Aaron
        Hide
        Martin Dougiamas added a comment -

        Would be good to get this moving soon.

        Show
        Martin Dougiamas added a comment - Would be good to get this moving soon.
        Hide
        Sam Hemelryk added a comment -

        Hi Martin,

        I'll start looking at this first thing tomorrow.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Martin, I'll start looking at this first thing tomorrow. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi Aaron,

        I've got a question that you might be able to answer for me.

        I've begun mapping this out and there is one potentially gray area within the database structure that I need to know about.
        I see that course_completion_criteria and course_completion_crit_compl store a reference to the course they are related to, however it looks as though course_completion_crit_compl always relates to a single course_completion_criteria.
        What I need to know is whether the two always store a reference to the same course, either way it is fine, it's just likely to affect the backup structure for course completion.

        If you don't know don't worry, I am about to head off for the day and will look into it tomorrow myself I havn't heard.

        Other than that one check I believe I have a plan now, and will start putting it alltogether tomorrow.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Aaron, I've got a question that you might be able to answer for me. I've begun mapping this out and there is one potentially gray area within the database structure that I need to know about. I see that course_completion_criteria and course_completion_crit_compl store a reference to the course they are related to, however it looks as though course_completion_crit_compl always relates to a single course_completion_criteria . What I need to know is whether the two always store a reference to the same course, either way it is fine, it's just likely to affect the backup structure for course completion. If you don't know don't worry, I am about to head off for the day and will look into it tomorrow myself I havn't heard. Other than that one check I believe I have a plan now, and will start putting it alltogether tomorrow. Cheers Sam
        Hide
        Aaron Barnes added a comment -

        Hi Sam,

        The reference to the course in course_completion_crit_compl is just an extra reference for making queries easier. It should always map to the same course id as the related course_completion_criteria does.

        Hope that helps,

        Cheers,
        Aaron

        Show
        Aaron Barnes added a comment - Hi Sam, The reference to the course in course_completion_crit_compl is just an extra reference for making queries easier. It should always map to the same course id as the related course_completion_criteria does. Hope that helps, Cheers, Aaron
        Hide
        Aaron Barnes added a comment -

        Also, the notify table is unused and can probably be dropped altogether.

        Show
        Aaron Barnes added a comment - Also, the notify table is unused and can probably be dropped altogether.
        Hide
        Sam Hemelryk added a comment -

        Cool thanks Aaron,
        I've just talked to Martin regarding the notify table and we've decided to include it in the backup in case we end up writing the feature in the future.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Cool thanks Aaron, I've just talked to Martin regarding the notify table and we've decided to include it in the backup in case we end up writing the feature in the future. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi Eloy,

        I've attached a patch of my current progress here and if you have a moment I'd love for you to look it over.
        Currently it is 90% done, there are two issues I am aware of:

        1. For one reason or another it is not restoring course_completion_crit_compl records... maybe to do with the next thing.
        2. I have no clue how to map the course_completion_criteria::moduleinstance field back to the correct moduleid during restore. I saw some method of processing after the fact, and also noted you have a backup ids table, perhaps that is the answer... could you please shed some light on that for me??

        Other than that I believe the backup is fine, and the restore will be two once I resolve the above.

        I'm going to be away Monday and Tuesday so I'll continue on Wednesday.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Eloy, I've attached a patch of my current progress here and if you have a moment I'd love for you to look it over. Currently it is 90% done, there are two issues I am aware of: For one reason or another it is not restoring course_completion_crit_compl records... maybe to do with the next thing. I have no clue how to map the course_completion_criteria::moduleinstance field back to the correct moduleid during restore. I saw some method of processing after the fact, and also noted you have a backup ids table, perhaps that is the answer... could you please shed some light on that for me?? Other than that I believe the backup is fine, and the restore will be two once I resolve the above. I'm going to be away Monday and Tuesday so I'll continue on Wednesday. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi Eloy,

        Any chance I could get you too look at this shortly for me?

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Eloy, Any chance I could get you too look at this shortly for me? Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Yeah, I thought you were out all the week, hence I had this planned for next weekend. Will move it to later today if possible. Thanks for pinging me.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Yeah, I thought you were out all the week, hence I had this planned for next weekend. Will move it to later today if possible. Thanks for pinging me. Ciao
        Hide
        Sam Hemelryk added a comment -

        Hehe no worries, would've loved a week off... but there's only so much time one can spend with visitors before work becomes appealing once again

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hehe no worries, would've loved a week off... but there's only so much time one can spend with visitors before work becomes appealing once again Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi Eloy,

        I've attached a new patch that fixes up most of the remaining issues.

        Presently I think the only thing left to be done that isn't working presently is restoring course completion criteria that relate to a module.
        What do you think is the best way to go around this? I have is the module type and the module instance id which I need to map to the newly restored module instance id.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Eloy, I've attached a new patch that fixes up most of the remaining issues. Presently I think the only thing left to be done that isn't working presently is restoring course completion criteria that relate to a module. What do you think is the best way to go around this? I have is the module type and the module instance id which I need to map to the newly restored module instance id. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Sam, some comments after (finally!) reviewing the code:

        1) The "userscompletion" setting only determines if the user related information must be included in backup and restore. Just that. So, all the rules/criteria/whatever must be included in backup and restore without depending of the setting. So I would take out that condition from the backup/restore tasks. And, instead, use that setting within the backup/restore steps to decide about to backup/restore the tables having user-related information.

        2) In the other side, there is one $CFG setting that should be used to completely disable restoring of ANY course completion information. It is $CFG->enablecompletion. You should check for it in the execute_condition() method of restore_course_completion_structure_step. See, for example, how we are handling restore_userscompletion_structure_step observing that exact setting.

        3) Also, I consider that the course completion is something really similar to the gradebook information, meaning that we only can include modules infomation (criteria) if we know that ALL the activities are being included in backup. And the same happens in restore: We only can restore course information criteria if all the activities are being restored. Else, it can fail horrible (non existing mappings => orphan records...). I'd suggest you to take a look to the execute_condition() of the backup_gradebook_structure_step and restore_gradebook_step classes, they achieve exactly that (detecting that all the activities have been included).

        4) About criteria information related to one module, you can safely get mappings for the 'course_module' itemname. But obviously, that only can be done after all the modules have been restored, so I guess you'll need to move the restore_course_completion_structure_step() from the course task to the final task (once more, like the gradebook). All the mappings are only available there.

        Hope it helps. Finally.. note that I haven't tested it at all, so I rely on you about all the structures being complete and so on. Great work, thanks!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Sam, some comments after (finally!) reviewing the code: 1) The "userscompletion" setting only determines if the user related information must be included in backup and restore. Just that. So, all the rules/criteria/whatever must be included in backup and restore without depending of the setting. So I would take out that condition from the backup/restore tasks. And, instead, use that setting within the backup/restore steps to decide about to backup/restore the tables having user-related information. 2) In the other side, there is one $CFG setting that should be used to completely disable restoring of ANY course completion information. It is $CFG->enablecompletion. You should check for it in the execute_condition() method of restore_course_completion_structure_step. See, for example, how we are handling restore_userscompletion_structure_step observing that exact setting. 3) Also, I consider that the course completion is something really similar to the gradebook information, meaning that we only can include modules infomation (criteria) if we know that ALL the activities are being included in backup. And the same happens in restore: We only can restore course information criteria if all the activities are being restored. Else, it can fail horrible (non existing mappings => orphan records...). I'd suggest you to take a look to the execute_condition() of the backup_gradebook_structure_step and restore_gradebook_step classes, they achieve exactly that (detecting that all the activities have been included). 4) About criteria information related to one module, you can safely get mappings for the 'course_module' itemname. But obviously, that only can be done after all the modules have been restored, so I guess you'll need to move the restore_course_completion_structure_step() from the course task to the final task (once more, like the gradebook). All the mappings are only available there. Hope it helps. Finally.. note that I haven't tested it at all, so I rely on you about all the structures being complete and so on. Great work, thanks! Ciao
        Hide
        Sam Hemelryk added a comment -

        Cool, thanks for the feedback Eloy

        I've just attached the revised patch which makes the following changes:

        1. userscompletion setting is now checked only for the structure that relies on the user being included. This is the course_completions, and course_completion_crit_compl tables.
        2. $CFG->enablecompletion is now checked on restore.
        3. course completion is now only included in backup/restore if ALL activities are included.
        4. The course completion steps have been moved to the final task.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Cool, thanks for the feedback Eloy I've just attached the revised patch which makes the following changes: userscompletion setting is now checked only for the structure that relies on the user being included. This is the course_completions, and course_completion_crit_compl tables. $CFG->enablecompletion is now checked on restore. course completion is now only included in backup/restore if ALL activities are included. The course completion steps have been moved to the final task. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Regarding your latest patch... just two comments:

        1) I see the "userscompletion" setting in action @ backup, but seems that restore is ignoring it completely. We must skip the course_completions, and course_completion_crit_compl paths based on that setting.

        2) I see there are some dates that aren't being rolled on restore. In theory all them must be rolled. I've followed the all approach in the rest of restore code.

        3) Also, when applying apply_date_offset() I've seen that you're doing one is_empty() check, what is the rationale about that? The method is clever enough to skip rolling zeros... do you need the is_empty() for something else? Note we can replace the == check by is_empty() within the function, just to keep restore code clear. Feel free to do so.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Regarding your latest patch... just two comments: 1) I see the "userscompletion" setting in action @ backup, but seems that restore is ignoring it completely. We must skip the course_completions, and course_completion_crit_compl paths based on that setting. 2) I see there are some dates that aren't being rolled on restore. In theory all them must be rolled. I've followed the all approach in the rest of restore code. 3) Also, when applying apply_date_offset() I've seen that you're doing one is_empty() check, what is the rationale about that? The method is clever enough to skip rolling zeros... do you need the is_empty() for something else? Note we can replace the == check by is_empty() within the function, just to keep restore code clear. Feel free to do so. Ciao
        Hide
        Sam Hemelryk added a comment -

        Cool thanks Eloy,

        I've attached the latest patch now that makes the following changes:

        1. userscompletion settings used in restore like backup
        2. All dates are now rolled
        3. Removed the empty call around the apply_date_offset

        In regards to rolling the date offsets, indeed in this case I had only rolled the configuration related dates and had kept the dates used primarily for record keeping as is.
        The empty is simply there for cautionary sake, when developing I kept running into notifications about undefined properties for $data objects and as such opted to wrap optional properties with empty calls (if default==null) or isset for properties with defaults.

        Cheers Sam

        Show
        Sam Hemelryk added a comment - Cool thanks Eloy, I've attached the latest patch now that makes the following changes: userscompletion settings used in restore like backup All dates are now rolled Removed the empty call around the apply_date_offset In regards to rolling the date offsets, indeed in this case I had only rolled the configuration related dates and had kept the dates used primarily for record keeping as is. The empty is simply there for cautionary sake, when developing I kept running into notifications about undefined properties for $data objects and as such opted to wrap optional properties with empty calls (if default==null) or isset for properties with defaults. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Just a note to remember the need to change the way restore works for unmached criterias. We need to skip them completely (and their child records).

        The rest looks perfect, yay! Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Just a note to remember the need to change the way restore works for unmached criterias. We need to skip them completely (and their child records). The rest looks perfect, yay! Ciao
        Hide
        Martin Dougiamas added a comment -

        +1 for checkin asap!

        Show
        Martin Dougiamas added a comment - +1 for checkin asap!
        Hide
        Sam Hemelryk added a comment -

        Awesome thanks Eloy.

        I've changed the handling of criteria that can not be restored as suggested and have now commit the patch after testing again of course

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Awesome thanks Eloy. I've changed the handling of criteria that can not be restored as suggested and have now commit the patch after testing again of course Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        Great Sam!

        I've reviewed last changes and the $skipcriteria thing seems perfect.

        Just one final comment: Shouldn't we be using the $skipcriteria thing also for non-matched module instances?

        Edited: Ah, forget this. Course completion only happens if ALL the activities have been included in backup and in restore, so we never should find non-matched module instances.

        Edited2: In any case... perhaps it is a simple and good thing to do to set $skipcriteria = false if not matching happens (makes it clear IMO).

        For your consideration, thanks!

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - - edited Great Sam! I've reviewed last changes and the $skipcriteria thing seems perfect. Just one final comment: Shouldn't we be using the $skipcriteria thing also for non-matched module instances? Edited: Ah, forget this. Course completion only happens if ALL the activities have been included in backup and in restore, so we never should find non-matched module instances. Edited2: In any case... perhaps it is a simple and good thing to do to set $skipcriteria = false if not matching happens (makes it clear IMO). For your consideration, thanks! Ciao
        Hide
        Sam Hemelryk added a comment -

        Hehe good spotting, fixing now

        Show
        Sam Hemelryk added a comment - Hehe good spotting, fixing now
        Hide
        Sam Hemelryk added a comment -

        Hi Eloy, just commit a fix to ensure use $skipcriteria.

        You are write technically we should never arrive at the situation where that does not work however I think you are right, it'll make the code clearer.

        Cheer
        Sam

        Show
        Sam Hemelryk added a comment - Hi Eloy, just commit a fix to ensure use $skipcriteria. You are write technically we should never arrive at the situation where that does not work however I think you are right, it'll make the code clearer. Cheer Sam

          People

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

            Dates

            • Created:
              Updated:
              Resolved: