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:

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

        Gliffy Diagrams

          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: