Moodle
  1. Moodle
  2. MDL-31914

Deleting an activity/resource with course completion enabled doesn't remove it's entry in course_completion_criteria

    Details

    • Testing Instructions:
      Hide
      • Enable 'Completion Tracking' in the Site Administration -> Advanced Features page
      • In a course, choose Edit Settings, and under 'Student Progress' change Completion tracking to Enabled and save changed
      • Create a new activity - I created a page for simplicity
        • set Completion tracking to 'Show activity as complete when conditions are met'
        • tick the 'Require view' checkbox
        • save changes
      • Create a second new activity - I created a page for simplicity
        • set Completion tracking to 'Show activity as complete when conditions are met'
        • tick the 'Require view' checkbox
        • save changes
      • Open Settings -> Course Administration -> Completion Tracking
        • Under 'Activites Completed' tick the checkboxes for the new activities you created
        • save changes
      • Open Reports -> Course Completion
        • Confirm that there are no errors
        • You may wish to keep this page open
      • Back in the course, remove one of the activities
      • Refresh the Course Completion report
        • Confirm that there are no errors

      The previous result was that errors would be displayed in the course completion report.
      You can also verify that the database entry has been removed from the course_completion_criteria table

      Show
      Enable 'Completion Tracking' in the Site Administration -> Advanced Features page In a course, choose Edit Settings, and under 'Student Progress' change Completion tracking to Enabled and save changed Create a new activity - I created a page for simplicity set Completion tracking to 'Show activity as complete when conditions are met' tick the 'Require view' checkbox save changes Create a second new activity - I created a page for simplicity set Completion tracking to 'Show activity as complete when conditions are met' tick the 'Require view' checkbox save changes Open Settings -> Course Administration -> Completion Tracking Under 'Activites Completed' tick the checkboxes for the new activities you created save changes Open Reports -> Course Completion Confirm that there are no errors You may wish to keep this page open Back in the course, remove one of the activities Refresh the Course Completion report Confirm that there are no errors The previous result was that errors would be displayed in the course completion report. You can also verify that the database entry has been removed from the course_completion_criteria table
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      master_MDL-31914
    • Rank:
      38563

      Description

      With course completion enabled and one or more activities tracked under course completion, the criteria for course completion are stored in the course_completion_criteria table.
      If an activity is removed, then its entry in this table is never removed.

      This doesn't cause an issue on most pages, but on the course completion report, this throws a number of errors and causes broken rendering of the course completion page.

      I suggest that this fix:

      1. remove completion criteria when removing an activity/resource;
      2. tidies up existing 'broken' data; and
      3. improves checking on the report.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          This commit will need separate patches for all branches owing to the database change.

          I've not addressed the third point I raised. It should be possible but the report could do with some refactoring first - for starters MDL-31918 would help with this.

          Show
          Andrew Nicols added a comment - This commit will need separate patches for all branches owing to the database change. I've not addressed the third point I raised. It should be possible but the report could do with some refactoring first - for starters MDL-31918 would help with this.
          Hide
          Andrew Nicols added a comment -

          I'll provide patches for all branches after the peer review stage.

          Show
          Andrew Nicols added a comment - I'll provide patches for all branches after the peer review stage.
          Hide
          Dan Poltawski added a comment -

          Looks good for integration.

          You put a comment in the upgrade script saying 'Delete each of those rows', maybe nice to add a comment saying what you are doing (deleting orphaned rows).

          [Just checking that you have checked that this table entries isn't references anywhere else also which needs cleaning up..]

          Show
          Dan Poltawski added a comment - Looks good for integration. You put a comment in the upgrade script saying 'Delete each of those rows', maybe nice to add a comment saying what you are doing (deleting orphaned rows). [Just checking that you have checked that this table entries isn't references anywhere else also which needs cleaning up..]
          Hide
          Andrew Nicols added a comment -

          This cherry-picks cleanly to:

          • master
          • MOODLE_22_STABLE
          • MOODLE_21_STABLE

          I've updated the comment in the upgrade as suggested.
          I've also checked that no other tables reference this one - I can't see any which do.

          Show
          Andrew Nicols added a comment - This cherry-picks cleanly to: master MOODLE_22_STABLE MOODLE_21_STABLE I've updated the comment in the upgrade as suggested. I've also checked that no other tables reference this one - I can't see any which do.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some hours ago...

          the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some hours ago... the main moodle.git repository has been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Andrew,

          I'm reopening this because it relays on delete_records_list() for potentially BIG lists of ids. And some DBs have a small limit to the length of the SQL, for sure breaking in this case (say, for example, oracle if there are more than 1000 ids to delete).

          I would try something like:

          • For mysql:
          DELETE cc
            FROM mdl_course_completion_criteria cc
            LEFT JOIN mdl_course_modules cm ON cm.id = cc.moduleinstance
           WHERE cm.id IS NULL
          
          • For the rest:
          DELETE FROM mdl_course_completion_criteria
          WHERE NOT EXISTS (
              SELECT 'x'
              FROM mdl_course_modules
              WHERE mdl_course_modules.id = mdl_course_completion_criteria.moduleinstance)
          

          Note that:

          1) the queries above are untested.
          2) I dont use table aliases in the 2nd alternative because it sounds to me that some DB does not allow them on tables being deleted/updated (postgres perhaps).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Andrew, I'm reopening this because it relays on delete_records_list() for potentially BIG lists of ids. And some DBs have a small limit to the length of the SQL, for sure breaking in this case (say, for example, oracle if there are more than 1000 ids to delete). I would try something like: For mysql: DELETE cc FROM mdl_course_completion_criteria cc LEFT JOIN mdl_course_modules cm ON cm.id = cc.moduleinstance WHERE cm.id IS NULL For the rest: DELETE FROM mdl_course_completion_criteria WHERE NOT EXISTS ( SELECT 'x' FROM mdl_course_modules WHERE mdl_course_modules.id = mdl_course_completion_criteria.moduleinstance) Note that: 1) the queries above are untested. 2) I dont use table aliases in the 2nd alternative because it sounds to me that some DB does not allow them on tables being deleted/updated (postgres perhaps). Ciao
          Hide
          Michael de Raadt added a comment -

          I've added Jason as tester as he is assiged to test the related issue.

          Show
          Michael de Raadt added a comment - I've added Jason as tester as he is assiged to test the related issue.
          Hide
          Jason Fowler added a comment -

          That's pretty pointless as the related issue's testing is covered off by this one, but they aren't integrated at the same time, any one can take the testing on this one when they want to, after it is integrated

          Show
          Jason Fowler added a comment - That's pretty pointless as the related issue's testing is covered off by this one, but they aren't integrated at the same time, any one can take the testing on this one when they want to, after it is integrated
          Hide
          Andrew Nicols added a comment -

          Eloy: I've updated the patch to reflect your suggestions.
          In the absence of a delete_records_sql function or similar, I've used execute.

          Show
          Andrew Nicols added a comment - Eloy: I've updated the patch to reflect your suggestions. In the absence of a delete_records_sql function or similar, I've used execute.
          Hide
          Andrew Nicols added a comment -

          Ahem... updated the patch super-quick like with a comment in the code!

          Show
          Andrew Nicols added a comment - Ahem... updated the patch super-quick like with a comment in the code!
          Hide
          Andrew Nicols added a comment -

          I've rebased against master for the DB changes.

          Show
          Andrew Nicols added a comment - I've rebased against master for the DB changes.
          Hide
          Andrew Nicols added a comment -

          I've made the DB changes Eloy requested but wouldn't mind a second eye before putting it up for IR

          Show
          Andrew Nicols added a comment - I've made the DB changes Eloy requested but wouldn't mind a second eye before putting it up for IR
          Hide
          Andrew Nicols added a comment -

          Hi Dan,

          Any chance you could peer review this again so we can try and get this included for integration this week?

          Show
          Andrew Nicols added a comment - Hi Dan, Any chance you could peer review this again so we can try and get this included for integration this week?
          Hide
          Dan Poltawski added a comment -

          Looks OK here - please confirm you have tested it on a few dbs at least.

          thanks

          Show
          Dan Poltawski added a comment - Looks OK here - please confirm you have tested it on a few dbs at least. thanks
          Hide
          Andrew Nicols added a comment -

          I've tested on MySQL and Postgres but not on others.

          Show
          Andrew Nicols added a comment - I've tested on MySQL and Postgres but not on others.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the records, I've run the upgrade (master) against the 4 RDBMS and the DELETE worked ok.

          Show
          Eloy Lafuente (stronk7) added a comment - For the records, I've run the upgrade (master) against the 4 RDBMS and the DELETE worked ok.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (21, 22 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (21, 22 & master), thanks!
          Hide
          Jason Fowler added a comment -

          Got this error in 2.2:

          completion/err_nocriteria

          More information about this error

          Stack trace:
          line 463 of /lib/setuplib.php: moodle_exception thrown
          line 100 of /report/completion/index.php: call to print_error()

          Show
          Jason Fowler added a comment - Got this error in 2.2: completion/err_nocriteria More information about this error Stack trace: line 463 of /lib/setuplib.php: moodle_exception thrown line 100 of /report/completion/index.php: call to print_error()
          Hide
          Jason Fowler added a comment -

          And in 2.3

          Show
          Jason Fowler added a comment - And in 2.3
          Hide
          Andrew Nicols added a comment -

          Apologies - this was a problem in the test instructions.
          The course completion report is only available if at least one activity has completion tracking enabled and in my testing I always did have a couple of activities with completion tracking.

          I've clarified the test instructions to add a second activity and only remove one of them. I've also removed the course completion settings as these aren't required for this testing.

          Show
          Andrew Nicols added a comment - Apologies - this was a problem in the test instructions. The course completion report is only available if at least one activity has completion tracking enabled and in my testing I always did have a couple of activities with completion tracking. I've clarified the test instructions to add a second activity and only remove one of them. I've also removed the course completion settings as these aren't required for this testing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (reset to waiting for testing, thanks Andrew and Jason)

          Show
          Eloy Lafuente (stronk7) added a comment - (reset to waiting for testing, thanks Andrew and Jason)
          Hide
          Andrew Nicols added a comment -

          Ahem - just getting course and activity completion confused.

          The issue was the test instructions and for the same reason - you need a minimum of one to be able to view the report.

          Show
          Andrew Nicols added a comment - Ahem - just getting course and activity completion confused. The issue was the test instructions and for the same reason - you need a minimum of one to be able to view the report.
          Hide
          Aaron Barnes added a comment -

          Just upgraded to the lastest MOODLE_22_STABLE and have noticed this patch deletes criteria it shouldn't be during upgrade! It deletes any course_completion_criteria records without a related course module, however there are different types of criteria - many of which that do not set a moduleinstance id!

          Show
          Aaron Barnes added a comment - Just upgraded to the lastest MOODLE_22_STABLE and have noticed this patch deletes criteria it shouldn't be during upgrade! It deletes any course_completion_criteria records without a related course module, however there are different types of criteria - many of which that do not set a moduleinstance id!
          Hide
          Jason Fowler added a comment -

          that's not possible, as this patch is not actually in MOODLE_22_STABLE yet, it is only in integration...

          Andrew: I will test this after SCRUM today.

          Show
          Jason Fowler added a comment - that's not possible, as this patch is not actually in MOODLE_22_STABLE yet, it is only in integration... Andrew: I will test this after SCRUM today.
          Hide
          Aaron Barnes added a comment -

          Jason: sorry, I am using the wrong remote!

          Also, this patch will break completion as the reaggregate field needs to be updated for the relative course_completions records.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Jason: sorry, I am using the wrong remote! Also, this patch will break completion as the reaggregate field needs to be updated for the relative course_completions records. Cheers, Aaron
          Hide
          Dan Poltawski added a comment -

          Hi Aaron,

          Thanks for the report - I am failing this and intend to revert it before it makes its way into the weeklies

          Show
          Dan Poltawski added a comment - Hi Aaron, Thanks for the report - I am failing this and intend to revert it before it makes its way into the weeklies
          Hide
          Aaron Barnes added a comment -

          To expand:

          The course_completion_criteria table contains criteria definitions of multiple types (all found in lib/completion/completion_criteria_*.php). One of these types is "activity" which contains a reference to a course module record. The criteria type is recorded in the criteriatype column of the table, which is the value of the COMPLETION_CRITERIA_TYPE_ACTIVITY constant.

          Also, the possibility exists that if a deleted activities related criteria is removed, a user will have enough "criteria" to now complete the course - however this will not happen due to the cron not reaggregating the criteria for the course. This can be triggered by updating the course_completions records for the course in question with a reaggregate value of the current timestamp.

          Personally, I think some more thought needs to go into whether this is the correct behaviour. Normally when editing a course's criteria all previous completion data for the course is required to be deleted as it seems somewhat unfair and/or inconsistent that some user's may have been required to do more to complete the course than later users.

          Hope this helps demystifies things!

          Show
          Aaron Barnes added a comment - To expand: The course_completion_criteria table contains criteria definitions of multiple types (all found in lib/completion/completion_criteria_*.php). One of these types is "activity" which contains a reference to a course module record. The criteria type is recorded in the criteriatype column of the table, which is the value of the COMPLETION_CRITERIA_TYPE_ACTIVITY constant. Also, the possibility exists that if a deleted activities related criteria is removed, a user will have enough "criteria" to now complete the course - however this will not happen due to the cron not reaggregating the criteria for the course. This can be triggered by updating the course_completions records for the course in question with a reaggregate value of the current timestamp. Personally, I think some more thought needs to go into whether this is the correct behaviour. Normally when editing a course's criteria all previous completion data for the course is required to be deleted as it seems somewhat unfair and/or inconsistent that some user's may have been required to do more to complete the course than later users. Hope this helps demystifies things!
          Hide
          Jason Fowler added a comment -

          Okay, I'll hold off until Andrew comments on this and decides what should be done

          Show
          Jason Fowler added a comment - Okay, I'll hold off until Andrew comments on this and decides what should be done
          Hide
          Andrew Nicols added a comment -

          Thanks for clarifying this Aaron - I hadn't clocked onto it.

          I think that the solution to the overdeletion should just be a case of adding a clause to the SQL on criteriatype = COMPLETION_CRITERIA_TYPE_ACTIVITY, which would additionally require the inclusion of $CFG->libdir . /completion/completion_criteria.php.

          I'm a little confused as to what you're suggesting with the re-aggregation - should we re-aggregate as part of the upgrade, or leave it to cron, or ..?

          Jason: I suggest that we continue with the revert in case there's anything else that I've missed.

          Sorry all for wasted time on integration and testing etc

          Show
          Andrew Nicols added a comment - Thanks for clarifying this Aaron - I hadn't clocked onto it. I think that the solution to the overdeletion should just be a case of adding a clause to the SQL on criteriatype = COMPLETION_CRITERIA_TYPE_ACTIVITY, which would additionally require the inclusion of $CFG->libdir . /completion/completion_criteria.php. I'm a little confused as to what you're suggesting with the re-aggregation - should we re-aggregate as part of the upgrade, or leave it to cron, or ..? Jason: I suggest that we continue with the revert in case there's anything else that I've missed. Sorry all for wasted time on integration and testing etc
          Hide
          Eloy Lafuente (stronk7) added a comment -

          So we need to know what to do. This is my initial list (numbered):

          For sure:

          1) Perform the same upgrade cleanup restricting to criteriatype == COMPLETION_CRITERIA_TYPE_ACTIVITY
          2) Perform the same deletion in delete_course_module(), perhaps also restricting to criteriatype == COMPLETION_CRITERIA_TYPE_ACTIVITY for extra safety.

          Probably:

          3) Add sort of "recalculate/sync" completion in delete_course_module()

          Perhaps (separate issue):

          4) Review how completion behaves when the course is modified by adding new criterias, hidding parts... whatever. Do we always recalculate/sync ? Is it consistent ? Can somebody already completed become, suddenly, incomplete?

          Feel free to advance / add / comment... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - So we need to know what to do. This is my initial list (numbered): For sure: 1) Perform the same upgrade cleanup restricting to criteriatype == COMPLETION_CRITERIA_TYPE_ACTIVITY 2) Perform the same deletion in delete_course_module(), perhaps also restricting to criteriatype == COMPLETION_CRITERIA_TYPE_ACTIVITY for extra safety. Probably: 3) Add sort of "recalculate/sync" completion in delete_course_module() Perhaps (separate issue): 4) Review how completion behaves when the course is modified by adding new criterias, hidding parts... whatever. Do we always recalculate/sync ? Is it consistent ? Can somebody already completed become, suddenly, incomplete? Feel free to advance / add / comment... ciao
          Hide
          Dan Poltawski added a comment -

          This has been reverted now, reopening

          Show
          Dan Poltawski added a comment - This has been reverted now, reopening
          Hide
          Aaron Barnes added a comment - - edited

          I think we need to discuss how completion behaves first tbh.

          Current behaviour for the only other situation that modifies criteria - the completion settings course page - is:

          • if there is no user data (e.g. course_completions or course_completion_crit_compl) for the course, can modify criteria freely
          • if there is user data, it must be truncated before criteria can be modified.

          This idea behind this is that course completion (once achieved) is immutable, unless the requirements change in which case the completion data for the course is reset so that all completions are "equal".

          Hope this makes sense! Do you think this is the correct logic?

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - - edited I think we need to discuss how completion behaves first tbh. Current behaviour for the only other situation that modifies criteria - the completion settings course page - is: if there is no user data (e.g. course_completions or course_completion_crit_compl) for the course, can modify criteria freely if there is user data, it must be truncated before criteria can be modified. This idea behind this is that course completion (once achieved) is immutable, unless the requirements change in which case the completion data for the course is reset so that all completions are "equal". Hope this makes sense! Do you think this is the correct logic? Cheers, Aaron
          Hide
          Dan Marsden added a comment -

          Hi Aaron/Eloy/Andrew,

          It sounds like there is general agreement on Eloys recommendation for points 1/2 which can be resolved very easily/quickly and prevent some invalid data - can we implement this first and discuss the re-sync/recalculate stuff on a separate tracker issue? - it would be good to improve the stability of the completion code asap.

          Show
          Dan Marsden added a comment - Hi Aaron/Eloy/Andrew, It sounds like there is general agreement on Eloys recommendation for points 1/2 which can be resolved very easily/quickly and prevent some invalid data - can we implement this first and discuss the re-sync/recalculate stuff on a separate tracker issue? - it would be good to improve the stability of the completion code asap.
          Hide
          Aaron Barnes added a comment -

          Hi Dan,

          Sure, for now points 1 and 2 could be implemented

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Dan, Sure, for now points 1 and 2 could be implemented Cheers, Aaron
          Hide
          Dan Marsden added a comment -

          submitting Andrews changes with a small mod to check for only criteria_type_activity. - thanks.

          Show
          Dan Marsden added a comment - submitting Andrews changes with a small mod to check for only criteria_type_activity. - thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho, could you plz create that/those new issues for the pending points? Just to avoid missing them. I'll be happy to integrate this immediately after that.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, could you plz create that/those new issues for the pending points? Just to avoid missing them. I'll be happy to integrate this immediately after that. TIA and ciao
          Hide
          Dan Marsden added a comment -

          done - MDL-32599 thanks.

          Show
          Dan Marsden added a comment - done - MDL-32599 thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated by cherry-pick + conflict (master) & version fixing (21, 22). Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated by cherry-pick + conflict (master) & version fixing (21, 22). Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Aha, adding one more commit to all branches, thanks to Andrew Nicols for spotting it:

          -  AND cc.criteriatype = ".COMPLETION_CRITERIA_TYPE_ACTIVITY;
          +  AND {course_completion_criteria}.criteriatype = ".COMPLETION_CRITERIA_TYPE_ACTIVITY;
          

          (Some DB - PG I think - does not allow to specify aliases in DELETE statements)

          Adding and running under the big four.

          Show
          Eloy Lafuente (stronk7) added a comment - Aha, adding one more commit to all branches, thanks to Andrew Nicols for spotting it: - AND cc.criteriatype = ".COMPLETION_CRITERIA_TYPE_ACTIVITY; + AND {course_completion_criteria}.criteriatype = ".COMPLETION_CRITERIA_TYPE_ACTIVITY; (Some DB - PG I think - does not allow to specify aliases in DELETE statements) Adding and running under the big four.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Upgrade checked to work ok under the big four DBs.

          Show
          Eloy Lafuente (stronk7) added a comment - Upgrade checked to work ok under the big four DBs.
          Hide
          Jason Fowler added a comment -

          All working now - awesome

          Show
          Jason Fowler added a comment - All working now - awesome
          Hide
          Petr Škoda added a comment -

          Please do not use any completion constants in core upgrade code - the problem is that theoretically the constants might be removed in the future or they might change.

          Show
          Petr Škoda added a comment - Please do not use any completion constants in core upgrade code - the problem is that theoretically the constants might be removed in the future or they might change.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been near becoming rejected, because it's not the best code you are able to produce.

          But, luckily, at the end, it has landed and has been spread to all repos out there.

          Many thanks and, don't forget it, keep improving your skills, you can!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: