Moodle
  1. Moodle
  2. MDL-32203

Course completion data object misses records

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.8, 2.1, 2.2
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Course completion
    • Labels:
    • Testing Instructions:
      Hide

      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 will 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 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 will 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_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
    • Rank:
      38962

      Description

      The class data_object is the base of all completion classes. It was originally subclasses from grade_object as it was assumed this was a good piece of code to base a similar solution on ( how wrong we were! ).

      It has be come apparent that the constructor is a bit too specific with it's where clauses, and if it is used to fetch a record it will include all data passed to it in the where clause. This causes issues as sometimes it will fail to locate a record in the database, and instead of updating will insert a duplicate record causing issues later on. This seems to occur frequently due to issues with concurrency, especially since a lot of work is passed off to the cron and a lot of calls to the constructor include unneccessary data like timemodified.

      Attached is the code to fix this issue, which is the cause of a majority of the duplicate record issues in course completion. It has been tried and tested in it's 1.9 form in Totara for many months now.

      The next step is to add unique indexes to relevant tables, but I understand that has to happen in master as is a database change while I am hoping this commit can make it's way into earlier versions.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Aaron.

          It looks like you've got something planned. I'll leave this alone, then.

          Show
          Michael de Raadt added a comment - Hi, Aaron. It looks like you've got something planned. I'll leave this alone, then.
          Hide
          Aaron Barnes added a comment -

          Michael: I've filled in the missing details

          Show
          Aaron Barnes added a comment - Michael: I've filled in the missing details
          Hide
          Aaron Barnes added a comment -

          I'll need to port this fix to 2.0, 2.1 and 2.2 before integration.

          Show
          Aaron Barnes added a comment - I'll need to port this fix to 2.0, 2.1 and 2.2 before integration.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this.

          I'll see if we can get this peer reviewed soon.

          Show
          Michael de Raadt added a comment - Thanks for working on this. I'll see if we can get this peer reviewed soon.
          Hide
          Dan Poltawski added a comment -

          Hi Aaron,

          Sorry for the delay with peer review.

          I note that the completion stuff is already quite far away from the coding guidelines, but with regard to a new variable addded $agg_data - we do not permit underscores in variable names: http://docs.moodle.org/dev/Coding_style#Variables

          thanks,

          Dan

          Show
          Dan Poltawski added a comment - Hi Aaron, Sorry for the delay with peer review. I note that the completion stuff is already quite far away from the coding guidelines, but with regard to a new variable addded $agg_data - we do not permit underscores in variable names: http://docs.moodle.org/dev/Coding_style#Variables thanks, Dan
          Hide
          Aaron Barnes added a comment -

          Hi Dan,

          Fixed issue - please note change in repository url.

          Thanks,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Dan, Fixed issue - please note change in repository url. Thanks, Aaron
          Hide
          Aaron Barnes added a comment -

          Updated branch with a fix for php syntax error.

          Show
          Aaron Barnes added a comment - Updated branch with a fix for php syntax error.
          Hide
          Dan Poltawski added a comment -

          Thanks Aaron - looks good

          Show
          Dan Poltawski added a comment - Thanks Aaron - looks good
          Hide
          Dan Marsden added a comment -

          Hi Aaron - is this ready for integration? - it appears to have passed peer review from Dan

          Show
          Dan Marsden added a comment - Hi Aaron - is this ready for integration? - it appears to have passed peer review from Dan
          Hide
          Aaron Barnes added a comment -

          Hi Dan,

          Sure is - I'll rebase it and cherry-pick to other branches now.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Dan, Sure is - I'll rebase it and cherry-pick to other branches now. Cheers, Aaron
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Aaron,

          one tiny detail... it seems that in data_object constructor you are accepting both arrays and objects to be passed. Also, you've converted a lot of instantations to use arrays ($aggdata) in your patch.

          So.. the question is... should we continue supporting objects for any reason or can we drop support for them and accept only arrays. In fact, or I'm wrong or it seems that your __construct() is only able to work properly with arrays an objects are not really supported (I cannot find any cast there).

          Halting this open for some hours.... fyc, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Aaron, one tiny detail... it seems that in data_object constructor you are accepting both arrays and objects to be passed. Also, you've converted a lot of instantations to use arrays ($aggdata) in your patch. So.. the question is... should we continue supporting objects for any reason or can we drop support for them and accept only arrays. In fact, or I'm wrong or it seems that your __construct() is only able to work properly with arrays an objects are not really supported (I cannot find any cast there). Halting this open for some hours.... fyc, ciao
          Hide
          Aaron Barnes added a comment -

          Hi Eloy,

          Yeah, no real need for the object code as using objects could potentially throw notices, and not work as expected. Would you like me to modify the code before it goes into core?

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Eloy, Yeah, no real need for the object code as using objects could potentially throw notices, and not work as expected. Would you like me to modify the code before it goes into core? Cheers, Aaron
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (apologizes for the previous automated message. it was not intended for this issue).

          Yup, if you can amend that code, I'll be happy to integrate this next week, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - (apologizes for the previous automated message. it was not intended for this issue). Yup, if you can amend that code, I'll be happy to integrate this next week, thanks!
          Hide
          Dan Marsden added a comment -

          Ping? - Aaron, any chance of getting this sorted before next weeks integration? - would be good to finally get this in!

          Show
          Dan Marsden added a comment - Ping? - Aaron, any chance of getting this sorted before next weeks integration? - would be good to finally get this in!
          Hide
          Aaron Barnes added a comment -

          Agreed, I'll have a look at it this afternoon

          Show
          Aaron Barnes added a comment - Agreed, I'll have a look at it this afternoon
          Hide
          Aaron Barnes added a comment -

          Done, ready for peer review! (note new branches)

          Show
          Aaron Barnes added a comment - Done, ready for peer review! (note new branches)
          Hide
          Aaron Barnes added a comment -

          Overview of changes between MDL-32203c and MDL-32203b:

          https://gist.github.com/2591679

          Show
          Aaron Barnes added a comment - Overview of changes between MDL-32203 c and MDL-32203 b: https://gist.github.com/2591679
          Hide
          Dan Poltawski added a comment -

          Hi,

          Just removing myself as peer reviewer on this as i've got a really full plate at the moment so wont be albe to do it quickly. Dan: if you are able to do it please go ahead!

          Show
          Dan Poltawski added a comment - Hi, Just removing myself as peer reviewer on this as i've got a really full plate at the moment so wont be albe to do it quickly. Dan: if you are able to do it please go ahead!
          Hide
          Dan Poltawski added a comment -

          Well, looks like nobody was able to get to it before me :/

          Show
          Dan Poltawski added a comment - Well, looks like nobody was able to get to it before me :/
          Hide
          Dan Poltawski added a comment -

          Looks OK to me, thanks i'm submitting this for integration

          Show
          Dan Poltawski added a comment - Looks OK to me, thanks i'm submitting this for integration
          Hide
          Aaron Barnes added a comment -

          Great, thanks Dan!

          Show
          Aaron Barnes added a comment - Great, thanks Dan!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just 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 - The main moodle.git repository has just 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 -

          The main moodle.git repository has just 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 - The main moodle.git repository has just 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
          Dan Marsden added a comment -

          ping - integrators - this has been passed over on 2 integrations and it looks like it isn't included this week either - passed over 3 times? - can we please make sure this gets included?

          Show
          Dan Marsden added a comment - ping - integrators - this has been passed over on 2 integrations and it looks like it isn't included this week either - passed over 3 times? - can we please make sure this gets included?
          Hide
          Sam Hemelryk added a comment -

          Stealing this from Eloy now looks like there are several issues now waiting on this.

          Show
          Sam Hemelryk added a comment - Stealing this from Eloy now looks like there are several issues now waiting on this.
          Hide
          Sam Hemelryk added a comment -

          Ok guys, things look really good to me and as such this has been integrated now!

          Show
          Sam Hemelryk added a comment - Ok guys, things look really good to me and as such this has been integrated now!
          Hide
          Sam Hemelryk added a comment -

          Oh sorry can someone please add testing instructions to this issue ASAP.

          Show
          Sam Hemelryk added a comment - Oh sorry can someone please add testing instructions to this issue ASAP.
          Hide
          Aaron Barnes added a comment -

          Done

          Show
          Aaron Barnes added a comment - Done
          Hide
          Sam Hemelryk added a comment -

          Thanks Aaron!

          Show
          Sam Hemelryk added a comment - Thanks Aaron!
          Hide
          Rajesh Taneja added a comment -

          Thanks Aaron,

          No error/notice occurred while testing course completion.

          Show
          Rajesh Taneja added a comment - Thanks Aaron, No error/notice occurred while testing course completion.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: