Moodle
  1. Moodle
  2. MDL-32107

Completion Info reset state throws debugging error

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.1
    • Fix Version/s: 2.3
    • Component/s: Activity completion
    • Labels:
    • Testing Instructions:
      Hide

      Note: The actual bug that occurs, for some reason I don't understand I haven't been able to reproduce it without using the OU's custom dual pane module. However, I did test this fix against that and it works. These test instructions exercise the activity completion report, because that page uses the functions I changed - just to make sure I didn't break it.

      1. Set up a new course website. Ensure that completion is enabled.

      2. Enrol two test users as students in the course.

      3. Add a Label with text 'Ticky ticky' and manual completion enabled.

      4. While logged in as admin (or another user who is not enrolled on the course), tick the box.

      5. Log in as the first user who is enrolled on the course, and tick the box.

      6. Log back in as admin (or whoever) and view the 'Activity completion' report for the course.

      • The list of users should show both of the enrolled students (one of whom has ticked the box and the other hasn't).
      • It should not show the admin/unenrolled user.

      7. Using the 'First name' selector at the top, click to different values (one which does not match any user names; one which does) and ensure the result is as expected

      Show
      Note: The actual bug that occurs, for some reason I don't understand I haven't been able to reproduce it without using the OU's custom dual pane module. However, I did test this fix against that and it works. These test instructions exercise the activity completion report, because that page uses the functions I changed - just to make sure I didn't break it. 1. Set up a new course website. Ensure that completion is enabled. 2. Enrol two test users as students in the course. 3. Add a Label with text 'Ticky ticky' and manual completion enabled. 4. While logged in as admin (or another user who is not enrolled on the course), tick the box. 5. Log in as the first user who is enrolled on the course, and tick the box. 6. Log back in as admin (or whoever) and view the 'Activity completion' report for the course. The list of users should show both of the enrolled students (one of whom has ticked the box and the other hasn't). It should not show the admin/unenrolled user. 7. Using the 'First name' selector at the top, click to different values (one which does not match any user names; one which does) and ensure the result is as expected
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-32107-master
    • Rank:
      38816

      Description

      On one Moodle course, when updating a module I am seeing the following error -

      Did you remember to make the first column something unique in your call to get_records? Duplicate value '333182' found in column 'id'.
      
          line 716 of /lib/dml/pgsql_native_moodle_database.php: call to debugging()
          line 1094 of /lib/completionlib.php: call to pgsql_native_moodle_database->get_records_sql()
          line 786 of /lib/completionlib.php: call to completion_info->get_tracked_users()
          line 391 of /course/modedit.php: call to completion_info->reset_all_state()
      
      Did you remember to make the first column something unique in your call to get_records? Duplicate value '339734' found in column 'id'.
      
          line 716 of /lib/dml/pgsql_native_moodle_database.php: call to debugging()
          line 1094 of /lib/completionlib.php: call to pgsql_native_moodle_database->get_records_sql()
          line 786 of /lib/completionlib.php: call to completion_info->get_tracked_users()
          line 391 of /course/modedit.php: call to completion_info->reset_all_state()
      
      Did you remember to make the first column something unique in your call to get_records? Duplicate value '407824' found in column 'id'.
      
          line 716 of /lib/dml/pgsql_native_moodle_database.php: call to debugging()
          line 1094 of /lib/completionlib.php: call to pgsql_native_moodle_database->get_records_sql()
          line 786 of /lib/completionlib.php: call to completion_info->get_tracked_users()
          line 391 of /course/modedit.php: call to completion_info->reset_all_state()
      

      I have attempted reproducing and replicating the error but haven't succeeded.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Anthony.

          Could you be more specific about what you mean by "updating a module"? Do you mean configuring an activity module?

          How are you sure this is related to course completion?

          Show
          Michael de Raadt added a comment - Hi, Anthony. Could you be more specific about what you mean by "updating a module"? Do you mean configuring an activity module? How are you sure this is related to course completion?
          Hide
          Anthony Forth added a comment -

          I mean configuring an activity module.

          My assumption was it is related to completion because the error occurs when it's pulling out a list of tracked users (see the error). I assume the query used allows the possibility of multiple results with the same user id (which the unique factor). I did look at the data in the db to see if there was anything that appeared irregular, but couldn't see anything. I can't replicate the error or replicate the query exactly because I don't know some of the params.

          Show
          Anthony Forth added a comment - I mean configuring an activity module. My assumption was it is related to completion because the error occurs when it's pulling out a list of tracked users (see the error). I assume the query used allows the possibility of multiple results with the same user id (which the unique factor). I did look at the data in the db to see if there was anything that appeared irregular, but couldn't see anything. I can't replicate the error or replicate the query exactly because I don't know some of the params.
          Hide
          Michael de Raadt added a comment -

          Thanks for the additional information, Anthony.

          Actually, the errors indicate this is related to activity completion.

          We would really need some way to replicate the problem before we can attack it.

          Was the activity duplicated? Could you experiment with turning activity completion off and on in the course and see if that has an effect?

          Show
          Michael de Raadt added a comment - Thanks for the additional information, Anthony. Actually, the errors indicate this is related to activity completion. We would really need some way to replicate the problem before we can attack it. Was the activity duplicated? Could you experiment with turning activity completion off and on in the course and see if that has an effect?
          Hide
          Anthony Forth added a comment -

          Done a bit more testing -

          It happens to every activity within one course on the Moodle instance (not on any others that I can locate).

          It only happens when an activity is updated (not when it is created).

          It only happens if completion tracking is set to 'do not indicate activity completion'.

          It doesn't matter whether an activity was duplicated.

          Switching off activity completion for the course stops the error appearing, switching it back on makes the error reappear.

          Show
          Anthony Forth added a comment - Done a bit more testing - It happens to every activity within one course on the Moodle instance (not on any others that I can locate). It only happens when an activity is updated (not when it is created). It only happens if completion tracking is set to 'do not indicate activity completion'. It doesn't matter whether an activity was duplicated. Switching off activity completion for the course stops the error appearing, switching it back on makes the error reappear.
          Hide
          Sam Marshall added a comment -

          We had a problem once with the completion system that has now been resolved, where it was possible there would be duplicate rows which could cause this kind of issue, but that was fixed a while back (I forget which version but it's in OU live for instance) - there is a unique index on userid,coursemoduleid on course_modules_completion.

          Is that unique index definitely in place on whatever server this happens on?

          Show
          Sam Marshall added a comment - We had a problem once with the completion system that has now been resolved, where it was possible there would be duplicate rows which could cause this kind of issue, but that was fixed a while back (I forget which version but it's in OU live for instance) - there is a unique index on userid,coursemoduleid on course_modules_completion. Is that unique index definitely in place on whatever server this happens on?
          Hide
          Anthony Forth added a comment -

          Yes - it's there.

          The only server where I can see this behaviour is learn2acct.

          I'll reassign our Redmine bug to you.

          Show
          Anthony Forth added a comment - Yes - it's there. The only server where I can see this behaviour is learn2acct. I'll reassign our Redmine bug to you.
          Hide
          Michael de Raadt added a comment -

          I'll leave this with you, Sam.

          Show
          Michael de Raadt added a comment - I'll leave this with you, Sam.
          Hide
          Sam Marshall added a comment -

          By using dbdebug script I was able to identify the query that is causing the problem:

          SELECT u.id, u.firstname, u.lastname, u.idnumber 
          FROM mdl_user u 
          INNER JOIN mdl_role_assignments ra ON ra.userid = u.id 
          INNER JOIN mdl_role r ON r.id = ra.roleid 
          INNER JOIN mdl_user_enrolments ue ON ue.userid = u.id 
          INNER JOIN mdl_enrol e ON e.id = ue.enrolid 
          INNER JOIN mdl_course c ON c.id = e.courseid
           WHERE (ra.contextid = 171455 OR ra.contextid IN (1,160501,160502,602509,171455 )) 
           AND c.id = 200182 AND ue.status = 0 AND e.status = 0 AND ue.timestart < 1332506418 
           AND (ue.timeend > 1332506418 OR ue.timeend = 0) AND ra.roleid IN (15,5) 
          

          This query will return multiple entries for the same user, giving an error, if:

          • they have more than one role
          • or they have more than one enrolment

          Not sure if the last is permitted but the first definitely is.

          Also I am not sure why we are checking role - seems like enrolment would be sufficient. Seems to me that when moving to 2.x we should probably have ditched the role part and replaced this with standard sql from get_enrolled_sql or whatever that is, rather than custom sql. Looking at it now.

          Show
          Sam Marshall added a comment - By using dbdebug script I was able to identify the query that is causing the problem: SELECT u.id, u.firstname, u.lastname, u.idnumber FROM mdl_user u INNER JOIN mdl_role_assignments ra ON ra.userid = u.id INNER JOIN mdl_role r ON r.id = ra.roleid INNER JOIN mdl_user_enrolments ue ON ue.userid = u.id INNER JOIN mdl_enrol e ON e.id = ue.enrolid INNER JOIN mdl_course c ON c.id = e.courseid WHERE (ra.contextid = 171455 OR ra.contextid IN (1,160501,160502,602509,171455 )) AND c.id = 200182 AND ue.status = 0 AND e.status = 0 AND ue.timestart < 1332506418 AND (ue.timeend > 1332506418 OR ue.timeend = 0) AND ra.roleid IN (15,5) This query will return multiple entries for the same user, giving an error, if: they have more than one role or they have more than one enrolment Not sure if the last is permitted but the first definitely is. Also I am not sure why we are checking role - seems like enrolment would be sufficient. Seems to me that when moving to 2.x we should probably have ditched the role part and replaced this with standard sql from get_enrolled_sql or whatever that is, rather than custom sql. Looking at it now.
          Hide
          Sam Marshall added a comment -

          I have now done changes to make the completion system use the standard 'enrolled' SQL instead of some weird custom combination between that and what it used to do before 'enrolled' existed. As the standard enrolled SQL already does a DISTINCT, this also fixes the reported problem.

          In addition to the test script above, I also verified that unit tests do not give errors.

          I suggest this fix is only applied to 2.3 because:

          • Listing users twice (and then dropping the extras in get_records_sql) will not cause any practical problems beyond the warning message.
          • The patch doesn't apply (at all) to 2.2 due to whitespace/comment changes.
          Show
          Sam Marshall added a comment - I have now done changes to make the completion system use the standard 'enrolled' SQL instead of some weird custom combination between that and what it used to do before 'enrolled' existed. As the standard enrolled SQL already does a DISTINCT, this also fixes the reported problem. In addition to the test script above, I also verified that unit tests do not give errors. I suggest this fix is only applied to 2.3 because: Listing users twice (and then dropping the extras in get_records_sql) will not cause any practical problems beyond the warning message. The patch doesn't apply (at all) to 2.2 due to whitespace/comment changes.
          Hide
          Dan Poltawski added a comment -

          Hi Sam,

          Looks fine to me. Only comment is that you are not using what seems to be the 'standard pattern' for get_enrolled_sql of joining explicitly on the enrolled sql (see for example forum_get_potential_subscribers).

          The only reason I mention it is that perhaps it has advantages for some DB engines to write that way (I don't know), or perhaps everyone just copied the same way and you didn't

          Show
          Dan Poltawski added a comment - Hi Sam, Looks fine to me. Only comment is that you are not using what seems to be the 'standard pattern' for get_enrolled_sql of joining explicitly on the enrolled sql (see for example forum_get_potential_subscribers ). The only reason I mention it is that perhaps it has advantages for some DB engines to write that way (I don't know), or perhaps everyone just copied the same way and you didn't
          Hide
          Sam Marshall added a comment -

          Thanks Dan. I had a look at the example you pointed to with the join, I see what they're doing but it looks a teeny bit weird to me I'm not aware of a reason why that would be any faster but that may of course just be me not knowing so much about whichever database...

          Maybe see what the integration reviewer thinks? If they agree it should be changed, I'll go ahead and change it, not a problem. Hope that's ok, submitting for review now.

          Show
          Sam Marshall added a comment - Thanks Dan. I had a look at the example you pointed to with the join, I see what they're doing but it looks a teeny bit weird to me I'm not aware of a reason why that would be any faster but that may of course just be me not knowing so much about whichever database... Maybe see what the integration reviewer thinks? If they agree it should be changed, I'll go ahead and change it, not a problem. Hope that's ok, submitting for review now.
          Hide
          Dan Poltawski added a comment -

          Yep fine here to submit for integration.

          (note I only mentioned it because its the way used almost universally across codebase)

          Show
          Dan Poltawski added a comment - Yep fine here to submit for integration. (note I only mentioned it because its the way used almost universally across codebase)
          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
          Sam Hemelryk added a comment -

          Just reading through comments I see that this is drawing on the get_enrolled_sql use.
          Personally I don't have anything against the approach you've taken here Sam, however I'm definitely no DB expert of over to Eloy!

          Show
          Sam Hemelryk added a comment - Just reading through comments I see that this is drawing on the get_enrolled_sql use. Personally I don't have anything against the approach you've taken here Sam, however I'm definitely no DB expert of over to Eloy!
          Hide
          Sam Hemelryk added a comment -

          (Forgot to mention everything else looks fine)

          Show
          Sam Hemelryk added a comment - (Forgot to mention everything else looks fine)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, the key here is that "IN" is, generally, evil. It's NEVER quicker than the EXISTS/JOIN alternatives, NEVER!

          Some RDBMS have them optimized to be converted to one of the laters so differences are not noticeable. But not all.

          And, more yet, MySQL sucks with all subqueries (more or less), so always we can, we try the JOIN alternative, that is the one working better under all DBs (comparable to EXISTS in all them but MySQL, and better than IN in al them, but in PostgreSQL and MSSQL).

          So I'd say that JOIN is the best candidate without doubt. And IN is evil and should be avoided unless 100% necessary.

          That's the thing, basically, afaik, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, the key here is that "IN" is, generally, evil. It's NEVER quicker than the EXISTS/JOIN alternatives, NEVER! Some RDBMS have them optimized to be converted to one of the laters so differences are not noticeable. But not all. And, more yet, MySQL sucks with all subqueries (more or less), so always we can, we try the JOIN alternative, that is the one working better under all DBs (comparable to EXISTS in all them but MySQL, and better than IN in al them, but in PostgreSQL and MSSQL). So I'd say that JOIN is the best candidate without doubt. And IN is evil and should be avoided unless 100% necessary. That's the thing, basically, afaik, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          So, in your changes, for the one simply counting users, you don't need to join anything. It's just the count of the records returned by get_enrolled_sql().

          And for the other query, I'd change it to JOIN as commented above.

          So I'm reopening this... thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - So, in your changes, for the one simply counting users, you don't need to join anything. It's just the count of the records returned by get_enrolled_sql(). And for the other query, I'd change it to JOIN as commented above. So I'm reopening this... thanks!
          Hide
          Sam Marshall added a comment -

          Wow, I completely forgot about this one. Sorry for the delay. Re-submitting.

          Based on Eloy's comments I have now changed the code. Unfortunately there is a reason I did this with subqueries before - it's basically because that's the only thing that was possible with the get_enrolled_sql output. So, in order to make this change work, I needed to extend the get_enrolled_sql function so that it can be used in joins. I have also updated the unit tests (and rerun them for both completionlib and accesslib). I redid the manual test script as well.

          Show
          Sam Marshall added a comment - Wow, I completely forgot about this one. Sorry for the delay. Re-submitting. Based on Eloy's comments I have now changed the code. Unfortunately there is a reason I did this with subqueries before - it's basically because that's the only thing that was possible with the get_enrolled_sql output. So, in order to make this change work, I needed to extend the get_enrolled_sql function so that it can be used in joins. I have also updated the unit tests (and rerun them for both completionlib and accesslib). I redid the manual test script as well.
          Hide
          Sam Marshall added a comment -

          Dan pointed out to me that I misunderstood Eloy's request - it was not to remove ALL use of subqueries, which I originally coded, but to change it from 'IN (subquery)' to 'FROM (subquery) eu'. Which was a much simpler change. I have updated my branch ready for integration review. There is now no change to accesslib or anything scary.

          Show
          Sam Marshall added a comment - Dan pointed out to me that I misunderstood Eloy's request - it was not to remove ALL use of subqueries, which I originally coded, but to change it from 'IN (subquery)' to 'FROM (subquery) eu'. Which was a much simpler change. I have updated my branch ready for integration review. There is now no change to accesslib or anything scary.
          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
          Sam Marshall added a comment -

          Rebased (no conflicts).

          Show
          Sam Marshall added a comment - Rebased (no conflicts).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It looks perfect, so I'm about to integrate it.

          The only point, commented above, is that I think it's NOT necessary to join with the

          {user}

          table as far as the get_enrolled_sql() query already has that table joined, if I'm not wrong. For the count() operation I mean, obviously for the get_tracked_users() it's necessary in order to fetch user details.

          Anyway, that way both methods seem more "paired", so no probs.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - It looks perfect, so I'm about to integrate it. The only point, commented above, is that I think it's NOT necessary to join with the {user} table as far as the get_enrolled_sql() query already has that table joined, if I'm not wrong. For the count() operation I mean, obviously for the get_tracked_users() it's necessary in order to fetch user details. Anyway, that way both methods seem more "paired", so no probs. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23 only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (23 only), thanks!
          Hide
          Rajesh Taneja added a comment -

          Works Great

          Thanks for fixing this Sam.

          Show
          Rajesh Taneja added a comment - Works Great Thanks for fixing this Sam.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao
          Hide
          Sam Marshall added a comment -

          thanks all

          Just for the record - I know it looks like it's not, but in fact, I think the join with user table is necessary for the count operation. The reason is that we allow people to pass in a WHERE condition, which refers to fields in the user table. And the SELECT done by get_enrolled_sql does not include all fields of the user table.

          Show
          Sam Marshall added a comment - thanks all Just for the record - I know it looks like it's not, but in fact, I think the join with user table is necessary for the count operation. The reason is that we allow people to pass in a WHERE condition, which refers to fields in the user table. And the SELECT done by get_enrolled_sql does not include all fields of the user table.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: