Moodle
  1. Moodle
  2. MDL-32119

Course Completion deleted column (and notify table)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.5
    • Fix Version/s: 2.4
    • Component/s: Course completion
    • Labels:
    • Testing Instructions:
      Hide

      Two tests required:

      1)
      In a fresh install of a version with this patch applied - make sure course_completion_notify table is not created, and course_completions table does not include a deleted or timenotified field. course_completion_crit_compl should not include a deleted field either. Then follow steps at bottom of Testing Instructions.

      2)
      Install an earlier version of Moodle, then upgrade to a version with this patch applied. After upgrade - check the above is true.

      Second part of testing for both 1 & 2:

      Enable completion in advanced settings.
      Create new course, enable completion.
      Create completion criteria for course (enable self-completion).
      Add self completion block and course completion status block to course.
      Enrol at least one user.
      Run cron.
      Login as enrolled user, self complete course using block.
      Visit details page (via completion status block).
      Run cron.
      As admin view completion report for course.

      No errors should occur.

      Show
      Two tests required: 1) In a fresh install of a version with this patch applied - make sure course_completion_notify table is not created, and course_completions table does not include a deleted or timenotified field. course_completion_crit_compl should not include a deleted field either. Then follow steps at bottom of Testing Instructions. 2) Install an earlier version of Moodle, then upgrade to a version with this patch applied. After upgrade - check the above is true. Second part of testing for both 1 & 2: Enable completion in advanced settings. Create new course, enable completion. Create completion criteria for course (enable self-completion). Add self completion block and course completion status block to course. Enrol at least one user. Run cron. Login as enrolled user, self complete course using block. Visit details page (via completion status block). Run cron. As admin view completion report for course. No errors should occur.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
    • Rank:
      38836

      Description

      A number of bugs have arisen recently with course completion and some of these have highlighted issues with the 'deleted' column in several of the tables:

      • course_completion_crit_compl
      • course_completions

      These tables have deleted columns which were added in 2be4d090c0c0b5d09aa6140e9348fb65d20263d6 as part of MDL-2631.
      These fields are referenced in the class definition of:

      • completion_completion: 'Set to 1 if this record has been deleted'; and
      • completion_criteria_completion: 'Course deleted flag'.

      However they're never actually set anywhere.

      These class's fetch functions also appear to be add a check to only return rows where the deleted column is null.

      I propose that these columns are either:

      • removed and all references to them removed; or
      • utilised and the data_object delete() function overridden by those classes to update the column instead of delete the row.

      My gut says that we should be doing the latter to allow the potential for restoration of inadvertently deleted data but I'm not sure whether this is realistic in practice.

      I don't know enough about the requirements of this side of the code - perhaps someone from Catalyst as the original contributors of the code could shed some light?

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Adding Aaron Barnes as named contributor in the original commit, and Dan Marsden who has been working on MDL-32102 as watchers.

          Show
          Andrew Nicols added a comment - Adding Aaron Barnes as named contributor in the original commit, and Dan Marsden who has been working on MDL-32102 as watchers.
          Hide
          Andrew Nicols added a comment -

          These bugs have both come a cropper with this field recently.

          Show
          Andrew Nicols added a comment - These bugs have both come a cropper with this field recently.
          Hide
          Aaron Barnes added a comment -

          My vote is to remove these all together. I can't see a good reason for adding the functionality to actually make use of the columns.

          Show
          Aaron Barnes added a comment - My vote is to remove these all together. I can't see a good reason for adding the functionality to actually make use of the columns.
          Hide
          Dan Marsden added a comment -

          A few of the clients I've been talking with have been looking for a way to "reset" completion on some form of schedule - eg, the user must complete the course every 12months but still wanting to keep "historical" data for reporting - so it would be nice to keep the deleted column so this can be implemented at some point.

          Show
          Dan Marsden added a comment - A few of the clients I've been talking with have been looking for a way to "reset" completion on some form of schedule - eg, the user must complete the course every 12months but still wanting to keep "historical" data for reporting - so it would be nice to keep the deleted column so this can be implemented at some point.
          Hide
          Aaron Barnes added a comment -

          Hi Dan,

          I've talked to clients about this also, I proposed a version column, or perhaps a seperate "history" table for information.

          What do you think?

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Dan, I've talked to clients about this also, I proposed a version column, or perhaps a seperate "history" table for information. What do you think? Cheers, Aaron
          Hide
          Dan Marsden added a comment -

          I'm not sure what I think about a separate history table - that sounds different to how other moodle tables work currently. But it could mean that admins could be allowed to clear the table out safely if needed.

          My vote to remove the deleted column - if it's not being used there's no point in it being there. If the feature to allow historical data to be kept is implemented it can create it's own new fields/tables as required.

          Show
          Dan Marsden added a comment - I'm not sure what I think about a separate history table - that sounds different to how other moodle tables work currently. But it could mean that admins could be allowed to clear the table out safely if needed. My vote to remove the deleted column - if it's not being used there's no point in it being there. If the feature to allow historical data to be kept is implemented it can create it's own new fields/tables as required.
          Hide
          Andrew Davis added a comment -

          This all look sensible enough. And I agree that any new history keeping functionality should be built new rather than using any half implemented bits left lying around. Submit for integration whenever you're ready.

          Show
          Andrew Davis added a comment - This all look sensible enough. And I agree that any new history keeping functionality should be built new rather than using any half implemented bits left lying around. Submit for integration whenever you're ready.
          Hide
          Dan Poltawski added a comment -

          This change looks quite risky for this real late stage of 2.3.

          What would be the impact if we posponed until master becomes 2.4?

          Show
          Dan Poltawski added a comment - This change looks quite risky for this real late stage of 2.3. What would be the impact if we posponed until master becomes 2.4?
          Hide
          Dan Marsden added a comment -

          afaik only impact of postponing is having messy code/db structure for another release. probably makes sense to delay until 2.3 is branched.

          Show
          Dan Marsden added a comment - afaik only impact of postponing is having messy code/db structure for another release. probably makes sense to delay until 2.3 is branched.
          Hide
          Dan Poltawski added a comment -

          Thanks Dan.

          Show
          Dan Poltawski added a comment - Thanks Dan.
          Hide
          Aaron Barnes added a comment -

          Hi Dan's,

          No risk in waiting.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Dan's, No risk in waiting. Cheers, Aaron
          Hide
          Dan Poltawski added a comment -

          Taking integration_held issues out of current integration (whilst we are keeping master and 23_STABLE in sync).

          Show
          Dan Poltawski added a comment - Taking integration_held issues out of current integration (whilst we are keeping master and 23_STABLE in sync).
          Hide
          Sam Hemelryk added a comment -

          Cool thanks guys this has been integrated now

          Show
          Sam Hemelryk added a comment - Cool thanks guys this has been integrated now
          Hide
          Ankit Agarwal added a comment -

          Hi Guys,
          I did a fresh install with last week's HEAD 2cbdaa77ea1d9467e43c88a910420fbfe41a6588.
          The table Structure mdl_course_completion_notify was present in the DB so was the timemodified column in modules_completion.

          Did I get the test instructions wrong? I was supposed to make a fresh install with last week's release and upgrade to the latest integration if am not wrong?

          Anyways failing this until more feedback is received. Sorry.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Guys, I did a fresh install with last week's HEAD 2cbdaa77ea1d9467e43c88a910420fbfe41a6588. The table Structure mdl_course_completion_notify was present in the DB so was the timemodified column in modules_completion. Did I get the test instructions wrong? I was supposed to make a fresh install with last week's release and upgrade to the latest integration if am not wrong? Anyways failing this until more feedback is received. Sorry. Thanks
          Hide
          Aaron Barnes added a comment -

          Ankit:

          Have updated testing instructions to hopefully be more clear

          Show
          Aaron Barnes added a comment - Ankit: Have updated testing instructions to hopefully be more clear
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Thanks for clearing up the test instructions Aaron.
          Ankit can you please give this another shot first thing so that we can determine if anything more is needed before the weeklies.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Thanks for clearing up the test instructions Aaron. Ankit can you please give this another shot first thing so that we can determine if anything more is needed before the weeklies. Cheers Sam
          Hide
          Dan Poltawski added a comment -

          Reset as requested by Ankit

          Show
          Dan Poltawski added a comment - Reset as requested by Ankit
          Hide
          Ankit Agarwal added a comment -

          works as expected.
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - works as expected. Passing Thanks
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          You've made it into the weekly release!

          Thanks for your contribution - here are some random drummers to keep you inspired for the next week!
          http://www.youtube.com/watch?v=_QhpHUmVCmY

          Show
          Dan Poltawski added a comment - Congratulations! You've made it into the weekly release! Thanks for your contribution - here are some random drummers to keep you inspired for the next week! http://www.youtube.com/watch?v=_QhpHUmVCmY

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: