Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32203

Course completion data object misses records

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Aaron.

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

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

            Michael: I've filled in the missing details

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

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

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

            Thanks for working on this.

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

            Show
            salvetore Michael de Raadt added a comment - Thanks for working on this. I'll see if we can get this peer reviewed soon.
            Hide
            poltawski 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
            poltawski 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
            sry_not4sale Aaron Barnes added a comment -

            Hi Dan,

            Fixed issue - please note change in repository url.

            Thanks,
            Aaron

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

            Updated branch with a fix for php syntax error.

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

            Thanks Aaron - looks good

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

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

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

            Hi Dan,

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

            Cheers,
            Aaron

            Show
            sry_not4sale Aaron Barnes added a comment - Hi Dan, Sure is - I'll rebase it and cherry-pick to other branches now. Cheers, Aaron
            Hide
            stronk7 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
            stronk7 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
            sry_not4sale 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
            sry_not4sale 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            danmarsden 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
            danmarsden 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
            sry_not4sale Aaron Barnes added a comment -

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

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

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

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

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

            https://gist.github.com/2591679

            Show
            sry_not4sale Aaron Barnes added a comment - Overview of changes between MDL-32203 c and MDL-32203 b: https://gist.github.com/2591679
            Hide
            poltawski 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
            poltawski 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
            poltawski Dan Poltawski added a comment -

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

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

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

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

            Great, thanks Dan!

            Show
            sry_not4sale Aaron Barnes added a comment - Great, thanks Dan!
            Hide
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            danmarsden 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
            danmarsden 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
            samhemelryk Sam Hemelryk added a comment -

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

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

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

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

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

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

            Done

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

            Thanks Aaron!

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

            Thanks Aaron,

            No error/notice occurred while testing course completion.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Aaron, No error/notice occurred while testing course completion.
            Hide
            stronk7 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
            stronk7 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:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Jul/12