Moodle
  1. Moodle
  2. MDL-28949

Duplicated course completion entries in database not handled correctly

    Details

    • Testing Instructions:
      Hide

      To test the upgrade code you must create duplicate records in the tables this commit cleans up (course_completions, course_completion_aggr_method, course_completion_crit_compl).

      These are the indexes that an upgrade should create, or a fresh install should include:

      $table = new xmldb_table('course_completions');
      $index = new xmldb_index('useridcourse', XMLDB_INDEX_UNIQUE, array('userid', 'course'));
      
      $table = new xmldb_table('course_completion_crit_compl');
      $index = new xmldb_index('useridcoursecriteraid', XMLDB_INDEX_UNIQUE, array('userid', 'course', 'criteriaid'));
      
      $table = new xmldb_table('course_completion_aggr_methd');
      $index = new xmldb_index('coursecriteriatype', XMLDB_INDEX_UNIQUE, array('course', 'criteriatype'));
      

      So creating multiple records that share identical values in these columns, but differing (or indentical, it doesn't matter) data in other columns should be merged into single records allowing the indexes to be created.

      An example of duplicate data I created during testing:

      // Add some test records
      $obj = new stdClass();
      $obj->course = 2;
      $obj->userid = 2;
      $obj->timecompleted = NULL;
      $obj->timestarted = 100;
      $obj->timeenrolled = 100;
      $DB->insert_record('course_completions', $obj);
      
      $obj->timestarted = 105;
      $DB->insert_record('course_completions', $obj);
      
      $obj->timecompleted = 105;
      $DB->insert_record('course_completions', $obj);
      
      $obj->timestarted = 0;
      $DB->insert_record('course_completions', $obj);
      
      $obj->userid = 3;
      $DB->insert_record('course_completions', $obj);
      
      $obj->timecompleted = 102;
      $DB->insert_record('course_completions', $obj);
      
      
      // Add some test records
      $obj = new stdClass();
      $obj->course = 2;
      $obj->userid = 2;
      $obj->criteriaid = 12;
      $obj->timecompleted = NULL;
      $DB->insert_record('course_completion_crit_compl', $obj);
      
      $obj->timecompleted = 12;
      $DB->insert_record('course_completion_crit_compl', $obj);
      
      $obj->timecompleted = 18;
      $DB->insert_record('course_completion_crit_compl', $obj);
      
      $obj->timecompleted = 11;
      $DB->insert_record('course_completion_crit_compl', $obj);
      
      $obj->userid = 3;
      $DB->insert_record('course_completion_crit_compl', $obj);
      
      $obj->timecompleted = 21;
      $DB->insert_record('course_completion_crit_compl', $obj);
      
      // Add some test records
      $obj = new stdClass();
      $obj->course = 2;
      $obj->criteriatype = 12;
      $obj->method = 1;
      $DB->insert_record('course_completion_aggr_methd', $obj);
      
      $obj->method = 2;
      $DB->insert_record('course_completion_aggr_methd', $obj);
      

      The logic behind merging duplicates means that it takes the lowest integer value for the unique columns, but will take an integer over null.

      To test that these changes do not affect the use of course completion:

      • Enable course completion in site advanced settings
      • Create a new course, enabling completion
      • In the "completion" menu in course settings, enable some criteria
      • Add the completion status block to course
      • Enrol users in the course
      • Have them complete some of the criteria (cron run may be required to have their "completion" register in system)
      • View their progress in the completion status block (as the learner), or in the completion course report as a teacher/siteadmin

      No coding or critical errors should occur.

      Show
      To test the upgrade code you must create duplicate records in the tables this commit cleans up (course_completions, course_completion_aggr_method, course_completion_crit_compl). These are the indexes that an upgrade should create, or a fresh install should include: $table = new xmldb_table('course_completions'); $index = new xmldb_index('useridcourse', XMLDB_INDEX_UNIQUE, array('userid', 'course')); $table = new xmldb_table('course_completion_crit_compl'); $index = new xmldb_index('useridcoursecriteraid', XMLDB_INDEX_UNIQUE, array('userid', 'course', 'criteriaid')); $table = new xmldb_table('course_completion_aggr_methd'); $index = new xmldb_index('coursecriteriatype', XMLDB_INDEX_UNIQUE, array('course', 'criteriatype')); So creating multiple records that share identical values in these columns, but differing (or indentical, it doesn't matter) data in other columns should be merged into single records allowing the indexes to be created. An example of duplicate data I created during testing: // Add some test records $obj = new stdClass(); $obj->course = 2; $obj->userid = 2; $obj->timecompleted = NULL; $obj->timestarted = 100; $obj->timeenrolled = 100; $DB->insert_record('course_completions', $obj); $obj->timestarted = 105; $DB->insert_record('course_completions', $obj); $obj->timecompleted = 105; $DB->insert_record('course_completions', $obj); $obj->timestarted = 0; $DB->insert_record('course_completions', $obj); $obj->userid = 3; $DB->insert_record('course_completions', $obj); $obj->timecompleted = 102; $DB->insert_record('course_completions', $obj); // Add some test records $obj = new stdClass(); $obj->course = 2; $obj->userid = 2; $obj->criteriaid = 12; $obj->timecompleted = NULL; $DB->insert_record('course_completion_crit_compl', $obj); $obj->timecompleted = 12; $DB->insert_record('course_completion_crit_compl', $obj); $obj->timecompleted = 18; $DB->insert_record('course_completion_crit_compl', $obj); $obj->timecompleted = 11; $DB->insert_record('course_completion_crit_compl', $obj); $obj->userid = 3; $DB->insert_record('course_completion_crit_compl', $obj); $obj->timecompleted = 21; $DB->insert_record('course_completion_crit_compl', $obj); // Add some test records $obj = new stdClass(); $obj->course = 2; $obj->criteriatype = 12; $obj->method = 1; $DB->insert_record('course_completion_aggr_methd', $obj); $obj->method = 2; $DB->insert_record('course_completion_aggr_methd', $obj); The logic behind merging duplicates means that it takes the lowest integer value for the unique columns, but will take an integer over null. To test that these changes do not affect the use of course completion: Enable course completion in site advanced settings Create a new course, enabling completion In the "completion" menu in course settings, enable some criteria Add the completion status block to course Enrol users in the course Have them complete some of the criteria (cron run may be required to have their "completion" register in system) View their progress in the completion status block (as the learner), or in the completion course report as a teacher/siteadmin No coding or critical errors should occur.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      18496

      Description

      This bug has been turned into a more generic bug than it once was (as the original report is actually caused by multiple issues). This issue and MDL-32203 fix the majority of issues.

      The problem is due to the bug MDL-32203 and the fact that the completion code in cron needs to run at the same time the site is being used a few of the course completion tables can get filled with duplicate records.

      This breaks completion in multiple places due to it's assuming there will be no duplicates.

      Adding unique indexes to the columns will prevent the duplicates being created by other race conditions. This bug will also remove some unused database fields/tables that confuse developers, and most importantly clean up any existing duplicate records (which needs to happen before the indexes can be created).

      OLD DESCRIPTION:

      This is nearly the same problem as in MDL-28021 - somehow there are duplicate entires for course completion landing in the table {$CFG->prefix}_course_completion_crit_compl.

      This causes a 'more than one record in fetch' error for some users in courses with a completion block, effectively blocking them from using moodle.

      I'm not sure of the origin of these duplicates yet, I suggest for a start we delete duplicates and put a unique index on the database; this prevents the access loss issue and should throw an error from the responsible location when an insert is made.

      I don't yet have a way to reproduce this issue but it is confirmed in about half a dozen courses and affects about 200 users in the site...

      Backtrace:
      [error] [client 10.229.5.75] Default exception handler: Found more than one record in fetch() ! Debug:

      • line 429 of /lib/setuplib.php: moodle_exception thrown
      • line 130 of /lib/completion/data_object.php: call to print_error()
      • line 119 of /lib/completion/completion_criteria_completion.php: call to data_object::fetch_helper()
      • line 65 of /lib/completion/data_object.php: call to completion_criteria_completion::fetch()
      • line 273 of /lib/completionlib.php: call to data_object->__construct()
      • line 58 of /blocks/completionstatus/block_completionstatus.php: call to completion_info->get_completions()
      • line 280 of /blocks/moodleblock.class.php: call to block_completionstatus->get_content()

        Issue Links

          Activity

          Hide
          Aaron Barnes added a comment -

          Hi Tony,

          Could you be more specific regarding which table the duplicates are in?

          The reason I ask is that the cron causes some issues when it runs at the same moment someone is performing certain actions - and these can only be fixed with a unique index.

          There is also another issue with backup and restore creating duplicate records as discussed in MDL-28180

          Thanks,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Tony, Could you be more specific regarding which table the duplicates are in? The reason I ask is that the cron causes some issues when it runs at the same moment someone is performing certain actions - and these can only be fixed with a unique index. There is also another issue with backup and restore creating duplicate records as discussed in MDL-28180 Thanks, Aaron
          Hide
          Tony Levi added a comment - - edited

          Hi,

          The table is {$CFG->prefix}_course_completion_crit_compl (edit: updated description to include this)
          Indeed, unique index is what I'm looking at doing, similar to activity completions needed.
          (Really, this should /always/ be done in such cases because then we find the bad inserts before it gets used much)

          I'm probably going to do something like:

          DELETE FROM mdl_course_completion_crit_compl
          WHERE id IN (
          SELECT DISTINCT cc1.id
          FROM mdl_course_completion_crit_compl cc1
          JOIN mdl_course_completion_crit_compl cc2
          ON cc2.criteriaid = cc1.criteriaid
          --AND cc1.course = cc2.course
          AND cc2.userid = cc1.userid
          AND cc2.id > cc1.id)

          CREATE UNIQUE INDEX mdl_course_complcritcompl_uix ON mdl_course_completion_crit_compl (userid, criteriaid);

          Criteria should already be specific to a course so this doesn't go over a (uid, crid, cid) tuple.
          This will obviously throw an error at insertion time, but at least that gives hope of patching the bad code.

          I guess the next steps for me will be to wait for the error to happen and/or trace back any insert_record('{$CFG->prefix}_course_completion_crit_compl' ...

          Show
          Tony Levi added a comment - - edited Hi, The table is {$CFG->prefix}_course_completion_crit_compl (edit: updated description to include this) Indeed, unique index is what I'm looking at doing, similar to activity completions needed. (Really, this should /always/ be done in such cases because then we find the bad inserts before it gets used much) I'm probably going to do something like: DELETE FROM mdl_course_completion_crit_compl WHERE id IN ( SELECT DISTINCT cc1.id FROM mdl_course_completion_crit_compl cc1 JOIN mdl_course_completion_crit_compl cc2 ON cc2.criteriaid = cc1.criteriaid --AND cc1.course = cc2.course AND cc2.userid = cc1.userid AND cc2.id > cc1.id) CREATE UNIQUE INDEX mdl_course_complcritcompl_uix ON mdl_course_completion_crit_compl (userid, criteriaid); Criteria should already be specific to a course so this doesn't go over a (uid, crid, cid) tuple. This will obviously throw an error at insertion time, but at least that gives hope of patching the bad code. I guess the next steps for me will be to wait for the error to happen and/or trace back any insert_record('{$CFG->prefix}_course_completion_crit_compl' ...
          Hide
          Tony Levi added a comment -

          Adding backtrace.

          Show
          Tony Levi added a comment - Adding backtrace.
          Hide
          Michael de Raadt added a comment -

          I've just placed this in our backlog, but please don't let that stop anythin that's in the works.

          Show
          Michael de Raadt added a comment - I've just placed this in our backlog, but please don't let that stop anythin that's in the works.
          Hide
          Aaron Barnes added a comment -

          Hi Tony,

          What about this SQL-wise:
          DELETE FROM mdl_course_completion_crit_compl
          WHERE id NOT IN
          (
          SELECT MAX(id)
          FROM mdl_course_completion_crit_compl
          GROUP BY course, userid, criteriaid
          )

          I have some patches for the code that will prevent this reoccuring, I'll link to the bug from here.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Tony, What about this SQL-wise: DELETE FROM mdl_course_completion_crit_compl WHERE id NOT IN ( SELECT MAX(id) FROM mdl_course_completion_crit_compl GROUP BY course, userid, criteriaid ) I have some patches for the code that will prevent this reoccuring, I'll link to the bug from here. Cheers, Aaron
          Hide
          Tony Levi added a comment -

          Hi,

          That looks a little quicker, I was just copying from the activity completion issue upgrade code.
          Really looking forward to those patches, then I can resolve this for good for the impacted users.

          Cheers, Tony

          Show
          Tony Levi added a comment - Hi, That looks a little quicker, I was just copying from the activity completion issue upgrade code. Really looking forward to those patches, then I can resolve this for good for the impacted users. Cheers, Tony
          Hide
          Tony Levi added a comment -

          Hi Aaron, have you put those patches up anywhere or a link?

          Show
          Tony Levi added a comment - Hi Aaron, have you put those patches up anywhere or a link?
          Hide
          Aaron Barnes added a comment -

          Hi Tony,

          I'm going to try get these up today or tommorrow!

          Thanks,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Tony, I'm going to try get these up today or tommorrow! Thanks, Aaron
          Hide
          Evan Irving-Pease added a comment -

          Hi Aaron,

          In addition to duplicates in `course_completion_crit_compl` I've also found duplicates in `course_completions`.

          Any progress with getting this fixed?

          Thanks,
          Evan.

          Show
          Evan Irving-Pease added a comment - Hi Aaron, In addition to duplicates in `course_completion_crit_compl` I've also found duplicates in `course_completions`. Any progress with getting this fixed? Thanks, Evan.
          Hide
          Aaron Barnes added a comment -

          Hi Evan,

          https://github.com/totara/moodle/commit/cdddef3e88d4b1e66de62451631fd2b80845d15d

          I have pushed a rather large patch to github. Part of this patch is the unique index stuff we have talked about...

          The code you need to be looking at is:
          lib/completion/*
          course/completion.php
          lib/db/upgrade.php

          It might need some tweaking to work, I have yet to complete my testing and review. I'd love copies of any changes you make though!

          Hope that helps, and I will keep pushing my progress to that branch until it is ready for prime time.

          Thanks,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Evan, https://github.com/totara/moodle/commit/cdddef3e88d4b1e66de62451631fd2b80845d15d I have pushed a rather large patch to github. Part of this patch is the unique index stuff we have talked about... The code you need to be looking at is: lib/completion/* course/completion.php lib/db/upgrade.php It might need some tweaking to work, I have yet to complete my testing and review. I'd love copies of any changes you make though! Hope that helps, and I will keep pushing my progress to that branch until it is ready for prime time. Thanks, Aaron
          Hide
          Evan Irving-Pease added a comment -

          Hi Aaron,

          As I was initially unable to find the cause for these duplicate records I setup a few simple unique constraints with the idea that the stack trace in the error log would help diagnose the problem when next the records came to be duplicated.

          ALTER TABLE `mdl_course_completions` ADD UNIQUE `uniquepairing` (`userid`, `course`);
          ALTER TABLE `mdl_course_completion_aggr_methd` ADD UNIQUE `uniquepairing` (`course`, `criteriatype`);
          ALTER TABLE `mdl_course_completion_crit_compl` ADD UNIQUE `uniquepairing` (`userid`, `course`, `criteriaid`);

          I can't speak for others who are experiencing this issue, however I have found the source of my own duplicate records. When the cron executes multiple times simultaneously the query in completion_criteria_activity::cron() returns the same result set for all runs of the cron, which in turn attempts to create multiple completion records for each person.

          Here's a copy of the stack trace I was getting...

          [Tue Oct 11 15:55:04 2011] [error] [client xx.xx.xx.xx] Default exception handler: Error writing to database Debug: Duplicate entry '1964-47-94' for key 'uniquepairing'
          INSERT INTO mdl_course_completion_crit_compl (userid,course,criteriaid,gradefinal,deleted,unenroled,timecompleted) VALUES(?,?,?,?,?,?,?)
          [array (
            0 => '1964',
            1 => '47',
            2 => '94',
            3 => NULL,
            4 => NULL,
            5 => NULL,
            6 => 1318308904,
          )]
          * line 396 of /lib/dml/moodle_database.php: dml_write_exception thrown
          * line 878 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
          * line 920 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw()
          * line 263 of /lib/completion/data_object.php: call to mysqli_native_moodle_database->insert_record()
          * line 155 of /lib/completion/completion_criteria_completion.php: call to data_object->insert()
          * line 234 of /lib/completion/completion_criteria_activity.php: call to completion_criteria_completion->mark_complete()
          * line 223 of /lib/completion/cron.php: call to completion_criteria_activity->cron()
          * line 43 of /lib/completion/cron.php: call to completion_cron_criteria()
          * line 177 of /lib/cronlib.php: call to completion_cron()
          * line 79 of /admin/cron.php: call to cron_run()
          
          Show
          Evan Irving-Pease added a comment - Hi Aaron, As I was initially unable to find the cause for these duplicate records I setup a few simple unique constraints with the idea that the stack trace in the error log would help diagnose the problem when next the records came to be duplicated. ALTER TABLE `mdl_course_completions` ADD UNIQUE `uniquepairing` (`userid`, `course`); ALTER TABLE `mdl_course_completion_aggr_methd` ADD UNIQUE `uniquepairing` (`course`, `criteriatype`); ALTER TABLE `mdl_course_completion_crit_compl` ADD UNIQUE `uniquepairing` (`userid`, `course`, `criteriaid`); I can't speak for others who are experiencing this issue, however I have found the source of my own duplicate records. When the cron executes multiple times simultaneously the query in completion_criteria_activity::cron() returns the same result set for all runs of the cron, which in turn attempts to create multiple completion records for each person. Here's a copy of the stack trace I was getting... [Tue Oct 11 15:55:04 2011] [error] [client xx.xx.xx.xx] Default exception handler: Error writing to database Debug: Duplicate entry '1964-47-94' for key 'uniquepairing' INSERT INTO mdl_course_completion_crit_compl (userid,course,criteriaid,gradefinal,deleted,unenroled,timecompleted) VALUES(?,?,?,?,?,?,?) [array ( 0 => '1964', 1 => '47', 2 => '94', 3 => NULL, 4 => NULL, 5 => NULL, 6 => 1318308904, )] * line 396 of /lib/dml/moodle_database.php: dml_write_exception thrown * line 878 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() * line 920 of /lib/dml/mysqli_native_moodle_database.php: call to mysqli_native_moodle_database->insert_record_raw() * line 263 of /lib/completion/data_object.php: call to mysqli_native_moodle_database->insert_record() * line 155 of /lib/completion/completion_criteria_completion.php: call to data_object->insert() * line 234 of /lib/completion/completion_criteria_activity.php: call to completion_criteria_completion->mark_complete() * line 223 of /lib/completion/cron.php: call to completion_criteria_activity->cron() * line 43 of /lib/completion/cron.php: call to completion_cron_criteria() * line 177 of /lib/cronlib.php: call to completion_cron() * line 79 of /admin/cron.php: call to cron_run()
          Hide
          kyle Egan added a comment -

          I'm also getting this issue (Moodle 2.0) not a very nice one. A small work around I have used that might help others in the short term is to remove the completion block from the course, this will allow the user to continue using the course at least.

          Has anyone seen this issue occur with Moodle 2.2, I'm just thinking the easiest thing to do maybe upgrade, then remove the duplicates.

          Show
          kyle Egan added a comment - I'm also getting this issue (Moodle 2.0) not a very nice one. A small work around I have used that might help others in the short term is to remove the completion block from the course, this will allow the user to continue using the course at least. Has anyone seen this issue occur with Moodle 2.2, I'm just thinking the easiest thing to do maybe upgrade, then remove the duplicates.
          Hide
          Tony Levi added a comment -

          I think nothing has changed that would prevent this in 2.2.

          Show
          Tony Levi added a comment - I think nothing has changed that would prevent this in 2.2.
          Hide
          Chris Follin added a comment -

          Hi, Aaron.

          We have run into this bug with clients. What happened with your patch from October? Is it ready for prime time?

          Show
          Chris Follin added a comment - Hi, Aaron. We have run into this bug with clients. What happened with your patch from October? Is it ready for prime time?
          Hide
          Tim Lock added a comment -

          Hi Aaron,

          How is your patch progressing?

          Regards,
          Tim

          Show
          Tim Lock added a comment - Hi Aaron, How is your patch progressing? Regards, Tim
          Hide
          Martin Dougiamas added a comment -

          This seems important and not something we want to leave to 2.4.

          My +1 for this in 2.3 if it can land soon and get tested properly in the QA testing this week. But I'll leave it up to integrators to decide.

          Show
          Martin Dougiamas added a comment - This seems important and not something we want to leave to 2.4. My +1 for this in 2.3 if it can land soon and get tested properly in the QA testing this week. But I'll leave it up to integrators to decide.
          Hide
          Michael de Raadt added a comment -

          Hi, Andrew.

          Can you please peer review this issue. The version values might need to be updated.

          Show
          Michael de Raadt added a comment - Hi, Andrew. Can you please peer review this issue. The version values might need to be updated.
          Hide
          Aaron Barnes added a comment -

          Andrew, if I can speed anything up or you need any questions answered I am available in developer chat!

          Thanks heaps,
          Aaron

          Show
          Aaron Barnes added a comment - Andrew, if I can speed anything up or you need any questions answered I am available in developer chat! Thanks heaps, Aaron
          Hide
          Andrew Davis added a comment -

          Hi. course_completion_remove_duplicates() should be in upgradelib.php rather than upgrade.php The main upgrade function goes in upgrade.php but all supporting functions go in upgradelib.php. Its more obvious how it works if you have a look at the version in 2.2 stable.

          Are you able to provide the tester with any guidance on how to go about inserting duplicate rows? I'm guessing this will come down to hand writing SQL inserts but if you can provide any additional information I'm sure that would be greatly appreciated by whoever tests this.

          Also, update the testing instructions to explicitly state that the tester needs to check that those indexes were created correctly. Currently its a bit unclear what you want done.

          Show
          Andrew Davis added a comment - Hi. course_completion_remove_duplicates() should be in upgradelib.php rather than upgrade.php The main upgrade function goes in upgrade.php but all supporting functions go in upgradelib.php. Its more obvious how it works if you have a look at the version in 2.2 stable. Are you able to provide the tester with any guidance on how to go about inserting duplicate rows? I'm guessing this will come down to hand writing SQL inserts but if you can provide any additional information I'm sure that would be greatly appreciated by whoever tests this. Also, update the testing instructions to explicitly state that the tester needs to check that those indexes were created correctly. Currently its a bit unclear what you want done.
          Hide
          Aaron Barnes added a comment -

          Hi Andrew,

          Thanks. OK, moving code now. Will make documentation fixes this afternoon and update this bug when complete.

          Thanks,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Andrew, Thanks. OK, moving code now. Will make documentation fixes this afternoon and update this bug when complete. Thanks, Aaron
          Hide
          Andrew Davis added a comment -

          How many duplicate records are we expecting to find? Would it be worth collecting their IDs then doing a single bulk delete rather than deleting them one by one? Maybe the difference will be trivial but something to consider.

          Show
          Andrew Davis added a comment - How many duplicate records are we expecting to find? Would it be worth collecting their IDs then doing a single bulk delete rather than deleting them one by one? Maybe the difference will be trivial but something to consider.
          Hide
          Aaron Barnes added a comment -

          OK fixed code is up

          Show
          Aaron Barnes added a comment - OK fixed code is up
          Hide
          Aaron Barnes added a comment -

          Hi Andrew,

          Generally not very much as once a single duplicate appears in a course then completion generally stop working for that course.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Andrew, Generally not very much as once a single duplicate appears in a course then completion generally stop working for that course. Cheers, Aaron
          Hide
          Dan Poltawski added a comment -

          Hi Everyone,

          Submitting this for integration to get it on our list so not to be missed (feel free to continue discussions meanwhile).

          Show
          Dan Poltawski added a comment - Hi Everyone, Submitting this for integration to get it on our list so not to be missed (feel free to continue discussions meanwhile).
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I have to shoot off for a bank meeting I havn't had time to finish my review sorry (got distracted by every manner of thing today).
          However I did note a couple of things that I think need to happen before this gets integrated:

          1. Don't drop the course_completion_notification table as part of this issue, do that as a separate issue so that both this and that are easier to review and safer to integrate. I think backup/restore still references that table?
          2. Split the main upgrade code into several blocks. One for each significant operation so that should something go wrong when upgrade is rerun we don't do any more than we need to. Will also help isolate any issues if people experience them and a general best practice idea for upgrade code.
          3. Rebase of the current master please (easy one aye!)

          Will pick this up again first thing tomorrow. At a glance rest of the code looks fine however.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I have to shoot off for a bank meeting I havn't had time to finish my review sorry (got distracted by every manner of thing today). However I did note a couple of things that I think need to happen before this gets integrated: Don't drop the course_completion_notification table as part of this issue, do that as a separate issue so that both this and that are easier to review and safer to integrate. I think backup/restore still references that table? Split the main upgrade code into several blocks. One for each significant operation so that should something go wrong when upgrade is rerun we don't do any more than we need to. Will also help isolate any issues if people experience them and a general best practice idea for upgrade code. Rebase of the current master please (easy one aye!) Will pick this up again first thing tomorrow. At a glance rest of the code looks fine however. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Sorry about the late reply just realised I must not have actually clicked the add comment button here.
          Everything else looks fine with these changes, however I do think those three points I raised before need to be addressed.
          Is there any chance you can still get to this Aaron?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Sorry about the late reply just realised I must not have actually clicked the add comment button here. Everything else looks fine with these changes, however I do think those three points I raised before need to be addressed. Is there any chance you can still get to this Aaron? Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Reopening this so points can be addressed.

          Show
          Sam Hemelryk added a comment - Reopening this so points can be addressed.
          Hide
          Aaron Barnes added a comment -

          Hi Sam,

          Will have a look at this now

          Thanks,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Sam, Will have a look at this now Thanks, Aaron
          Hide
          Sam Hemelryk added a comment -

          Thanks for cleaning that up Aaron. This has been integrated now.
          Presently I've only integrated this on master however I will create an issue about deciding whether this should be backported to stable branches to fix any issues people may be encountering there.

          Have you created an issue to remove the course_completion_notification table yet? If not could you please.
          There's no rush on that of course but it would be nice to have an issue so that it is not forgotten.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for cleaning that up Aaron. This has been integrated now. Presently I've only integrated this on master however I will create an issue about deciding whether this should be backported to stable branches to fix any issues people may be encountering there. Have you created an issue to remove the course_completion_notification table yet? If not could you please. There's no rush on that of course but it would be nice to have an issue so that it is not forgotten. Cheers Sam
          Hide
          Aaron Barnes added a comment -

          Hi Sam,

          Testing a patch for MDL-32119 that will cover the other stuff - do you recon we could get it into 2.3 too?

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Sam, Testing a patch for MDL-32119 that will cover the other stuff - do you recon we could get it into 2.3 too? Cheers, Aaron
          Hide
          Sam Hemelryk added a comment -

          Hi Aaron,

          This has been integrated to master which will become 2.3 at the next main release in less than a month. So yip is already in 2.3.

          I've created MDL-33394 for the stable branches (2.1 and 2.2) having looked at this and had a bit of a think about it I think that it probably should be backported.
          Given the potential risk involved with the upgrade code to remove duplicates I think perhaps we should look to backport it in a couple of weeks just to flush out any issues that may exist while this is on master only.

          What are your thoughts?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Aaron, This has been integrated to master which will become 2.3 at the next main release in less than a month. So yip is already in 2.3. I've created MDL-33394 for the stable branches (2.1 and 2.2) having looked at this and had a bit of a think about it I think that it probably should be backported. Given the potential risk involved with the upgrade code to remove duplicates I think perhaps we should look to backport it in a couple of weeks just to flush out any issues that may exist while this is on master only. What are your thoughts? Cheers Sam
          Hide
          Ankit Agarwal added a comment -

          this is working as expected. Encountered the cron issue (MDL-33398) while working on this.

          Passing as the bug is not related to this patch.
          Thanks

          Show
          Ankit Agarwal added a comment - this is working as expected. Encountered the cron issue ( MDL-33398 ) while working on this. Passing as the bug is not related to this patch. Thanks
          Hide
          Kyle Egan added a comment -

          Hi I applied the patch and got the following error when running the Cron.Any thoughts?

          ...
          Starting processing the event queue...
          done.
          Starting the completion cron...
          Marking users as started
          Running completion_criteria_date->cron()
          Running completion_criteria_activity->cron()
          !!! Coding error detected, it must be fixed by a programmer: data_object params should be in the form of an array, not an object !!!
          !! Stack trace: * line 68 of \lib\completion\data_object.php: coding_exception thrown

          • line 233 of \lib\completion\completion_criteria_activity.php: call to data_object->__construct()
          • line 223 of \lib\completion\cron.php: call to completion_criteria_activity->cron()
          • line 43 of \lib\completion\cron.php: call to completion_cron_criteria()
          • line 351 of \lib\cronlib.php: call to completion_cron()
          • line 85 of \admin\cron.php: call to cron_run()
            !!
          Show
          Kyle Egan added a comment - Hi I applied the patch and got the following error when running the Cron.Any thoughts? ... Starting processing the event queue... done. Starting the completion cron... Marking users as started Running completion_criteria_date->cron() Running completion_criteria_activity->cron() !!! Coding error detected, it must be fixed by a programmer: data_object params should be in the form of an array, not an object !!! !! Stack trace: * line 68 of \lib\completion\data_object.php: coding_exception thrown line 233 of \lib\completion\completion_criteria_activity.php: call to data_object->__construct() line 223 of \lib\completion\cron.php: call to completion_criteria_activity->cron() line 43 of \lib\completion\cron.php: call to completion_cron_criteria() line 351 of \lib\cronlib.php: call to completion_cron() line 85 of \admin\cron.php: call to cron_run() !!
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          Your work has made into the latest Moodle release!

          You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

          Show
          Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!
          Hide
          Aaron Barnes added a comment -

          Hi Kyle,

          There is a prerequisite patch that need to be applied first:

          • MDL-32203 - Course completion data object misses records

          And a postrequisite? patch that needs to be applied after this one:

          • MDL-33398 - Cron fails when course completion is enabled

          These three patches should fix any critical issues with course completion you might have - if not, please feel free to send me an email

          Cheers and good luck!
          Aaron

          Show
          Aaron Barnes added a comment - Hi Kyle, There is a prerequisite patch that need to be applied first: MDL-32203 - Course completion data object misses records And a postrequisite? patch that needs to be applied after this one: MDL-33398 - Cron fails when course completion is enabled These three patches should fix any critical issues with course completion you might have - if not, please feel free to send me an email Cheers and good luck! Aaron

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: