Moodle
  1. Moodle
  2. MDL-35698

Quiz cron uses wrong quiz close date when group overrides are being used

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.3
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      The particular case that was failing is:

      1. Create a quiz with a close date in the future
      2. Create a quiz group override with a close date in the past
      3. Start an attempt using a student not in the overridden group
      4. Run the moodle cron (and ensure update_overdue_attempts() runs). The student's attempt should NOT be closed.

      You may also wish to test some other cases. For example, create an attempt by a student in the overridden group before the group close date passes, as well, and make sure that is still closed.

      Show
      The particular case that was failing is: 1. Create a quiz with a close date in the future 2. Create a quiz group override with a close date in the past 3. Start an attempt using a student not in the overridden group 4. Run the moodle cron (and ensure update_overdue_attempts() runs). The student's attempt should NOT be closed. You may also wish to test some other cases. For example, create an attempt by a student in the overridden group before the group close date passes, as well, and make sure that is still closed.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      44440

      Description

      To reproduce:

      1. Create a quiz with a close date in the future
      2. Create a quiz group override with a close date in the past
      3. Start an attempt using a student not in the overridden group
      4. Run the moodle cron (and ensure update_overdue_attempts() runs)

      The active student quiz attempt will be closed.

        Issue Links

          Activity

          Hide
          Matt Petro added a comment - - edited

          The problem is due to a LEFT JOIN which is returning group overrides with no matching group assignment. It's a easy fix so I'll just include here:

          In mod/quiz/cronlib.php function get_list_of_overdue_attempts()

          Change:

                LEFT JOIN {quiz_overrides} qgo ON qgo.quiz = quiz.id
                LEFT JOIN {groups_members} gm ON gm.userid = iquiza.userid AND gm.groupid = qgo.groupid
          

          To:

                LEFT JOIN (
          
                   SELECT qo.quiz,
                          gm.userid,
                          qo.timeclose,
                          qo.timelimit
                     FROM {quiz_overrides} qo
                     JOIN {groups_members} gm ON gm.groupid = qo.groupid
          
                          ) qgo ON qgo.quiz = quiz.id AND qgo.userid = iquiza.userid
          

          There might be more efficient SQL to do this without a subquery, but it wasn't obvious to me. (Associativity of JOINS / LEFT JOINS is confusing to me

          Show
          Matt Petro added a comment - - edited The problem is due to a LEFT JOIN which is returning group overrides with no matching group assignment. It's a easy fix so I'll just include here: In mod/quiz/cronlib.php function get_list_of_overdue_attempts() Change: LEFT JOIN {quiz_overrides} qgo ON qgo.quiz = quiz.id LEFT JOIN {groups_members} gm ON gm.userid = iquiza.userid AND gm.groupid = qgo.groupid To: LEFT JOIN ( SELECT qo.quiz, gm.userid, qo.timeclose, qo.timelimit FROM {quiz_overrides} qo JOIN {groups_members} gm ON gm.groupid = qo.groupid ) qgo ON qgo.quiz = quiz.id AND qgo.userid = iquiza.userid There might be more efficient SQL to do this without a subquery, but it wasn't obvious to me. (Associativity of JOINS / LEFT JOINS is confusing to me
          Hide
          Tim Hunt added a comment -

          I have to believe you that this is happening, but I really don't understand why. The logic in get_list_of_overdue_attempts in mod/quiz/cronlib.php looks right.

          Do you feel brave enough to try to debug that monster SQL query? If not, I will try to look some time next week.

          Show
          Tim Hunt added a comment - I have to believe you that this is happening, but I really don't understand why. The logic in get_list_of_overdue_attempts in mod/quiz/cronlib.php looks right. Do you feel brave enough to try to debug that monster SQL query? If not, I will try to look some time next week.
          Hide
          Tim Hunt added a comment - - edited

          Sorry, overlapping comments. You are brave enough!

          I think a simpler fix is to just reverse the order of the two left joins:

          LEFT JOIN {groups_members} gm ON gm.userid = iquiza.userid
          LEFT JOIN {quiz_overrides} qgo ON qgo.quiz = quiz.id AND gm.groupid = qgo.groupid
          

          Are you able to test that?

          Show
          Tim Hunt added a comment - - edited Sorry, overlapping comments. You are brave enough! I think a simpler fix is to just reverse the order of the two left joins: LEFT JOIN {groups_members} gm ON gm.userid = iquiza.userid LEFT JOIN {quiz_overrides} qgo ON qgo.quiz = quiz.id AND gm.groupid = qgo.groupid Are you able to test that?
          Hide
          Matt Petro added a comment -

          Aha, I missed the simple solution. I've tested that and it works.

          Show
          Matt Petro added a comment - Aha, I missed the simple solution. I've tested that and it works.
          Hide
          Tim Hunt added a comment -

          Thanks Matt. Fix submitted to integration.

          We really should have unit tests for this query.

          Show
          Tim Hunt added a comment - Thanks Matt. Fix submitted to integration. We really should have unit tests for this query.
          Hide
          Dan Poltawski added a comment -

          Thanks Matt/Tim, i've integrated this now.

          Show
          Dan Poltawski added a comment - Thanks Matt/Tim, i've integrated this now.
          Hide
          Adrian Greeve added a comment -

          Tested on master to observe the problem.
          Tested on the 2.3 and master integration branches. The status of the quiz attempt is as it should be.
          No problems encountered.
          Test passed.

          Show
          Adrian Greeve added a comment - Tested on master to observe the problem. Tested on the 2.3 and master integration branches. The status of the quiz attempt is as it should be. No problems encountered. Test passed.
          Hide
          Matthew G. Switlik added a comment -

          We are having a very very similar issue. A Teacher had a quiz set for 10/3/2012. They granted a user a override to take the quiz early ending at 11am on 9/29/2012. The user override row in mdl_quiz_overrides has a groupid of NULL. No user groups are defined for the course. Since there are no groups every user would join on mdl_quiz_overrides with the groupid of NULL (at least on our PostgreSQL server. Is it different for MySQL?). On 10/3/2012 when users were taking the quiz their attempts were being erroneously closed when the cron ran.

          Tim's SQL change is identical to the fix I worked out for us except I added " qgo.groupid IS NOT NULL AND " as an "optimization"

          LEFT JOIN {groups_members} gm ON gm.userid = iquiza.userid
          LEFT JOIN {quiz_overrides} qgo ON qgo.groupid IS NOT NULL AND qgo.quiz = quiz.id AND gm.groupid = qgo.groupid AND 
          
          Show
          Matthew G. Switlik added a comment - We are having a very very similar issue. A Teacher had a quiz set for 10/3/2012. They granted a user a override to take the quiz early ending at 11am on 9/29/2012. The user override row in mdl_quiz_overrides has a groupid of NULL. No user groups are defined for the course. Since there are no groups every user would join on mdl_quiz_overrides with the groupid of NULL (at least on our PostgreSQL server. Is it different for MySQL?). On 10/3/2012 when users were taking the quiz their attempts were being erroneously closed when the cron ran. Tim's SQL change is identical to the fix I worked out for us except I added " qgo.groupid IS NOT NULL AND " as an "optimization" LEFT JOIN {groups_members} gm ON gm.userid = iquiza.userid LEFT JOIN {quiz_overrides} qgo ON qgo.groupid IS NOT NULL AND qgo.quiz = quiz.id AND gm.groupid = qgo.groupid AND
          Hide
          Tim Hunt added a comment -

          Did you try the query with and without that "optimization"? In other words, do you know if it has a significant effect on the query run-time on your database, or is it just an educated guess on your part?

          Also, I am afraid that if you care about this bug, then you will also care about MDL-35717, which is more serious, and has not been fixed yet, although there is a work-around.

          Show
          Tim Hunt added a comment - Did you try the query with and without that "optimization"? In other words, do you know if it has a significant effect on the query run-time on your database, or is it just an educated guess on your part? Also, I am afraid that if you care about this bug, then you will also care about MDL-35717 , which is more serious, and has not been fixed yet, although there is a work-around.
          Hide
          Matthew G. Switlik added a comment -

          Tim,
          It is just an educated guess. I don't know if it has a significant effect on the query run-time on my database yet, but I'll give it a try tomorrow. I just assume most databases will execute the IS NOT NULL bit first and disqualify all user override rows sooner and with fewer compares. I suppose if the database had no user overrides and tons of group overrides the IS NOT NULL bit would slow things down.

          Show
          Matthew G. Switlik added a comment - Tim, It is just an educated guess. I don't know if it has a significant effect on the query run-time on my database yet, but I'll give it a try tomorrow. I just assume most databases will execute the IS NOT NULL bit first and disqualify all user override rows sooner and with fewer compares. I suppose if the database had no user overrides and tons of group overrides the IS NOT NULL bit would slow things down.
          Hide
          Tim Hunt added a comment -

          My experience is don't guess. Run the query, and use EXPLAIN. In this case, the data is going to be very rapidly fed to MAX, which will throw away the NULLS, so I am not sure if the NOT NULL will have an effect or not.

          Show
          Tim Hunt added a comment - My experience is don't guess. Run the query, and use EXPLAIN. In this case, the data is going to be very rapidly fed to MAX, which will throw away the NULLS, so I am not sure if the NOT NULL will have an effect or not.
          Hide
          Matt Petro added a comment -

          JOIN's generally ignore NULL values, so "ON gm.groupid = qgo.groupid" should implicitly require that qgo.groupid IS NOT NULL. At least that's how I thought it worked.

          Show
          Matt Petro added a comment - JOIN's generally ignore NULL values, so "ON gm.groupid = qgo.groupid" should implicitly require that qgo.groupid IS NOT NULL. At least that's how I thought it worked.
          Hide
          Tim Hunt added a comment -

          Yes, but if you look at the SQL in Matthew's first comment, you will see that we are actually talking about LEFT JOINs.

          Show
          Tim Hunt added a comment - Yes, but if you look at the SQL in Matthew's first comment, you will see that we are actually talking about LEFT JOINs.
          Hide
          Matt Petro added a comment -

          Left join or not, NULL is never equal to anything. So "A IS NOT NULL AND A = B" should be equivalent to "A=B". Anyways, we'll see what Matthew reports back.

          Show
          Matt Petro added a comment - Left join or not, NULL is never equal to anything. So "A IS NOT NULL AND A = B" should be equivalent to "A=B". Anyways, we'll see what Matthew reports back.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed, many thanks for your awesome collaboration.

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your awesome collaboration.
          Hide
          Susan Mangan added a comment -

          Help! I need help to reproduce this error.

          I would just apply the fix but I need to confirm it will in fact fix the problem our online instructors are having as they are now refusing to use moodle until it's confirmed as being fixed due to the problems it has caused them.

          I'm 99% sure this is our problem - everything seems to match up with the over-ride, students getting kicked out, etc.

          I've recreated the scenario as described above and am running cron through the browser. Perhaps I'm not as familiar with cron as I need to be. how do I make sure update_overdue_attempts() runs? would it not automatically run if cron is triggered?

          Thank you SO MUCH in advance if you can help me out.

          Show
          Susan Mangan added a comment - Help! I need help to reproduce this error. I would just apply the fix but I need to confirm it will in fact fix the problem our online instructors are having as they are now refusing to use moodle until it's confirmed as being fixed due to the problems it has caused them. I'm 99% sure this is our problem - everything seems to match up with the over-ride, students getting kicked out, etc. I've recreated the scenario as described above and am running cron through the browser. Perhaps I'm not as familiar with cron as I need to be. how do I make sure update_overdue_attempts() runs? would it not automatically run if cron is triggered? Thank you SO MUCH in advance if you can help me out.
          Hide
          Matt Petro added a comment -

          Can you upgrade your site? This problem is fixed in moodle 2.3.2+.

          To reproduce the problem, the update_overdue_attempts() function runs only once an hour. So, if you don't run cron for an hour, and then run cron, that function will certainly get called.

          Show
          Matt Petro added a comment - Can you upgrade your site? This problem is fixed in moodle 2.3.2+. To reproduce the problem, the update_overdue_attempts() function runs only once an hour. So, if you don't run cron for an hour, and then run cron, that function will certainly get called.
          Hide
          Susan Mangan added a comment - - edited

          Yes I can upgrade but because our online instructors have already experienced the problems and now refuse to use Moodle I need to reproduce the error, patch it, and confirm it is in fact fixed. Even though I am 99% sure this is the problem, I want to make sure I see it not working and then working. Thanks for the cron tip, I will try that!

          Question: are there any implications to simply patching the code as indicated above? This will be faster for us than waiting for a scheduled shut-down to do a full upgrade. Thanks again, for your input.

          Show
          Susan Mangan added a comment - - edited Yes I can upgrade but because our online instructors have already experienced the problems and now refuse to use Moodle I need to reproduce the error, patch it, and confirm it is in fact fixed. Even though I am 99% sure this is the problem, I want to make sure I see it not working and then working. Thanks for the cron tip, I will try that! Question: are there any implications to simply patching the code as indicated above? This will be faster for us than waiting for a scheduled shut-down to do a full upgrade. Thanks again, for your input.
          Hide
          Dušan Ristić added a comment -

          This is still not working on Moodle 2.3.2+ (Build: 20120914)
          I've reproduced the bug with student override (didn't had time to test with group)

          Show
          Dušan Ristić added a comment - This is still not working on Moodle 2.3.2+ (Build: 20120914) I've reproduced the bug with student override (didn't had time to test with group)
          Hide
          Tim Hunt added a comment -

          You may actually be seeing MDL-36842.

          Show
          Tim Hunt added a comment - You may actually be seeing MDL-36842 .
          Hide
          Dušan Ristić added a comment -

          No, it's actually this bug. How to reproduce it:
          1. create quiz with close date in future and time limit
          2. add student override with close date in past
          3. start quiz with any student (it doesn't have to be the student with override)
          4. run cron (with update_overdue_attempts() executing) and student attempts closes before time is up and before close date for quiz

          I've noticed this after we had problems with one quiz. There were 74 attempts, with 34 starting before first update_overdue_attempts() run and 16 of them were closed. On second run of update_overdue_attempts() from 48 open attempts 25 were closed.

          Show
          Dušan Ristić added a comment - No, it's actually this bug. How to reproduce it: 1. create quiz with close date in future and time limit 2. add student override with close date in past 3. start quiz with any student (it doesn't have to be the student with override) 4. run cron (with update_overdue_attempts() executing) and student attempts closes before time is up and before close date for quiz I've noticed this after we had problems with one quiz. There were 74 attempts, with 34 starting before first update_overdue_attempts() run and 16 of them were closed. On second run of update_overdue_attempts() from 48 open attempts 25 were closed.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: