Details

    • Testing Instructions:
      Hide

      Test 1

      1. Run phpunit and make sure completion test pass

      Test 2

      1. Create a course and enable "Completion tracking"
      2. Check "Completion tracking begins on enrolment"
      3. Enrol users and run cron
      4. Check timeenrolled field in course_completions table and user_enrolments table have same value in timeenrolled and timecreated.
      Show
      Test 1 Run phpunit and make sure completion test pass Test 2 Create a course and enable "Completion tracking" Check "Completion tracking begins on enrolment" Enrol users and run cron Check timeenrolled field in course_completions table and user_enrolments table have same value in timeenrolled and timecreated.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      42702

      Description

      The logic around setting timeenrolled in course completion has a couple of issues.

      First, the cron uses the enrolments timecreated if it's higher than timeenrolled - this was to try work around the fact enrolment plugins might have a timeenrolled value of 0. Unfortunately this means that if an enrolment is created for the past the timecreated value will be higher and therefore completion will pick up the wrong date.

      Secondly, if the course_completions record is created between the user enrolling in the course and the first cron run, completion_completion::mark_inprogress() does not set the timeenrolled value and will be set at 0.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Hi Aaron,

          The changes look really good thanks.
          Just a couple of things perhaps could be considered within completion_completion::_save

          1. That SQL statement is joining to the user table to check the user is not deleted. It caught my eye because joining on the user table is never a great idea unless required because of the performance impact. It got me wondering is the check that the user has not been deleted necessary there?
            Without looking into it I would have guessed no. If thats the case I really think it worth removing that join.
          2. Within that SQL statement again the fields being returned from ue could be limited to id, timestart, timeend.
          3. Optionally if you really wanted to you could look to move the condition checks into the query, sort by timeenrolled and then limit to a single record.

          Rest looks fine.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Aaron, The changes look really good thanks. Just a couple of things perhaps could be considered within completion_completion::_save That SQL statement is joining to the user table to check the user is not deleted. It caught my eye because joining on the user table is never a great idea unless required because of the performance impact. It got me wondering is the check that the user has not been deleted necessary there? Without looking into it I would have guessed no. If thats the case I really think it worth removing that join. Within that SQL statement again the fields being returned from ue could be limited to id, timestart, timeend. Optionally if you really wanted to you could look to move the condition checks into the query, sort by timeenrolled and then limit to a single record. Rest looks fine. Cheers Sam
          Hide
          Aaron Barnes added a comment -

          Hi Sam,

          Great ideas, I have implemented all of them! This is I why I love code review

          I have also remove the code that also starts users who have a specific role in the course (rather than an enrollment) as this differs from the newer behavior of other parts of activity/course completion.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Sam, Great ideas, I have implemented all of them! This is I why I love code review I have also remove the code that also starts users who have a specific role in the course (rather than an enrollment) as this differs from the newer behavior of other parts of activity/course completion. Cheers, Aaron
          Hide
          Sam Hemelryk added a comment -

          Changes to _save look great thanks Aaron.

          I have a couple of questions regarding completion_cron_mark_started.
          The changes here lead it now mark users who are deleted and users who have roles other than those specified as grade book roles to end up with completion records in the db.
          Can you quickly just tell me about these two changes?

          Assuming everything is OK there feel free to put this up for integration review (don't forget to add testing instructions).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Changes to _save look great thanks Aaron. I have a couple of questions regarding completion_cron_mark_started. The changes here lead it now mark users who are deleted and users who have roles other than those specified as grade book roles to end up with completion records in the db. Can you quickly just tell me about these two changes? Assuming everything is OK there feel free to put this up for integration review (don't forget to add testing instructions). Cheers Sam
          Hide
          Aaron Barnes added a comment -

          Hi Sam,

          No longer checking if a user is deleted is fine as in the delete_user() code the user is unenrolled from all courses - so this SQL would have never picked them up anyway. I checked 2.2 and 2.3 and they behave the same way.

          The gradebookroles change was made in 2.3 and this is merely bringing this code up to speed. The current code (before this commit) can create completion records for users who are not enrolled in a course but have one of the tracked roles and nothing is ever done with this data - the record will just sit their orphaned.

          Due to the gradebookroles still being used in 2.2 I am going to recommend we don't apply this patch to 2.2.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Hi Sam, No longer checking if a user is deleted is fine as in the delete_user() code the user is unenrolled from all courses - so this SQL would have never picked them up anyway. I checked 2.2 and 2.3 and they behave the same way. The gradebookroles change was made in 2.3 and this is merely bringing this code up to speed. The current code (before this commit) can create completion records for users who are not enrolled in a course but have one of the tracked roles and nothing is ever done with this data - the record will just sit their orphaned. Due to the gradebookroles still being used in 2.2 I am going to recommend we don't apply this patch to 2.2. Cheers, Aaron
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks Aaron, I've put this up for integration now.
          I agree best to apply this to 2.3 and master only.
          Thanks!

          Show
          Sam Hemelryk added a comment - Awesome thanks Aaron, I've put this up for integration now. I agree best to apply this to 2.3 and master only. Thanks!
          Hide
          Dan Poltawski 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
          Dan Poltawski 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 Poltawski added a comment -

          Hi Aaron, we need some testing instructions for this.

          Show
          Dan Poltawski added a comment - Hi Aaron, we need some testing instructions for this.
          Hide
          Dan Poltawski added a comment -

          I'm afraid I don't know how to test this, so reopening.

          Show
          Dan Poltawski added a comment - I'm afraid I don't know how to test this, so reopening.
          Hide
          Aaron Barnes added a comment -

          Major overhaul of patch to improve performance and include test coverage

          Show
          Aaron Barnes added a comment - Major overhaul of patch to improve performance and include test coverage
          Hide
          Dan Poltawski added a comment -

          Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..

          Show
          Dan Poltawski added a comment - Sending all 'waiting for peer review' issues to integration before freeze, as agreed in Integrators Meeting 19/10/12. We are doing this to ensure any 'integratable issues' will not got missed before freeze..
          Hide
          Aaron Barnes added a comment -

          MDL-34481 is a far better implementation than this, but if it can't make it into 2.4 code freeze then this definitely would be an improvement.

          Show
          Aaron Barnes added a comment - MDL-34481 is a far better implementation than this, but if it can't make it into 2.4 code freeze then this definitely would be an improvement.
          Hide
          Dan Poltawski added a comment -

          I've integrated this to master, thanks Aaron.

          Show
          Dan Poltawski added a comment - I've integrated this to master, thanks Aaron.
          Hide
          Dan Poltawski added a comment -

          BAH! There are no testing instructions for this one.

          Show
          Dan Poltawski added a comment - BAH! There are no testing instructions for this one.
          Hide
          Rajesh Taneja added a comment -

          Added basic test instructions, let me know if we need to test something specific.

          Show
          Rajesh Taneja added a comment - Added basic test instructions, let me know if we need to test something specific.
          Hide
          Rajesh Taneja added a comment -

          Sorry guys, unit test is failing

          Ran completion/tests/completion_completion_test.php and getting following error
          There was 1 error:

          1) completion_completion_testcase::test_completion_completion_save
          dml_read_exception: Error reading from database (You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '* FROM ut_course_completions WHERE course = '2'' at line 1
          SELECT userid, * FROM ut_course_completions WHERE course = ?
          [array (
          0 => '2',
          )])

          /var/www/im/lib/dml/moodle_database.php:424
          /var/www/im/lib/dml/mysqli_native_moodle_database.php:1023
          /var/www/im/lib/dml/moodle_database.php:1209
          /var/www/im/lib/dml/moodle_database.php:1160
          /var/www/im/completion/tests/completion_completion_test.php:86
          /var/www/im/lib/phpunit/classes/advanced_testcase.php:76

          completion/tests/cron_test.php
          There was 1 error:

          1) completioncron_testcase::test_completion_cron_mark_started
          dml_read_exception: Error reading from database (You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '* FROM ut_course_completions WHERE course = '2'' at line 1
          SELECT userid, * FROM ut_course_completions WHERE course = ?
          [array (
          0 => '2',
          )])

          /var/www/im/lib/dml/moodle_database.php:424
          /var/www/im/lib/dml/mysqli_native_moodle_database.php:1023
          /var/www/im/lib/dml/moodle_database.php:1209
          /var/www/im/lib/dml/moodle_database.php:1160
          /var/www/im/completion/tests/cron_test.php:68
          /var/www/im/lib/phpunit/classes/advanced_testcase.php:76

          To re-run:
          phpunit completioncron_testcase completion/tests/cron_test.php

          FAILURES!
          Tests: 1, Assertions: 0, Errors: 1.

          Show
          Rajesh Taneja added a comment - Sorry guys, unit test is failing Ran completion/tests/completion_completion_test.php and getting following error There was 1 error: 1) completion_completion_testcase::test_completion_completion_save dml_read_exception: Error reading from database (You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '* FROM ut_course_completions WHERE course = '2'' at line 1 SELECT userid, * FROM ut_course_completions WHERE course = ? [array ( 0 => '2', )]) /var/www/im/lib/dml/moodle_database.php:424 /var/www/im/lib/dml/mysqli_native_moodle_database.php:1023 /var/www/im/lib/dml/moodle_database.php:1209 /var/www/im/lib/dml/moodle_database.php:1160 /var/www/im/completion/tests/completion_completion_test.php:86 /var/www/im/lib/phpunit/classes/advanced_testcase.php:76 completion/tests/cron_test.php There was 1 error: 1) completioncron_testcase::test_completion_cron_mark_started dml_read_exception: Error reading from database (You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '* FROM ut_course_completions WHERE course = '2'' at line 1 SELECT userid, * FROM ut_course_completions WHERE course = ? [array ( 0 => '2', )]) /var/www/im/lib/dml/moodle_database.php:424 /var/www/im/lib/dml/mysqli_native_moodle_database.php:1023 /var/www/im/lib/dml/moodle_database.php:1209 /var/www/im/lib/dml/moodle_database.php:1160 /var/www/im/completion/tests/cron_test.php:68 /var/www/im/lib/phpunit/classes/advanced_testcase.php:76 To re-run: phpunit completioncron_testcase completion/tests/cron_test.php FAILURES! Tests: 1, Assertions: 0, Errors: 1.
          Hide
          Dan Poltawski added a comment -

          There is an issue that our integration system didn't pick this up too.

          Show
          Dan Poltawski added a comment - There is an issue that our integration system didn't pick this up too.
          Hide
          Dan Poltawski added a comment -

          Aaron,

          Can you comment on this one, I think we're going to be looking to revert this unfortunately.

          Show
          Dan Poltawski added a comment - Aaron, Can you comment on this one, I think we're going to be looking to revert this unfortunately.
          Hide
          Dan Poltawski added a comment -

          I've reverted this as we need to get the release out today.

          Show
          Dan Poltawski added a comment - I've reverted this as we need to get the release out today.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Dan Marsden added a comment -

          Aaron - is this still needed - do you need help picking this up again?

          Show
          Dan Marsden added a comment - Aaron - is this still needed - do you need help picking this up again?
          Hide
          Dan Marsden added a comment -

          ping - Aaron?

          Show
          Dan Marsden added a comment - ping - Aaron?

            People

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

              Dates

              • Created:
                Updated: