Moodle
  1. Moodle
  2. MDL-35163

Quiz attempts for unenrolled users can't be deleted

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.3.3, 2.4
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      To test patch:

      1. Create a quiz
      2. Ensure there is more than one student enrolled on the course
      3. Log in as a student and make an attempt on the quiz
      4. Unenrol the student from the course
      5. Go into Quiz -> Results -> Responses and select "all users who have attempted the quiz"
      6. Select the attempt and click "delete selected attempts"
      7. Reselect "all users who have attempted the quiz"
      8. The quiz attempt should be deleted
      Show
      To test patch: Create a quiz Ensure there is more than one student enrolled on the course Log in as a student and make an attempt on the quiz Unenrol the student from the course Go into Quiz -> Results -> Responses and select "all users who have attempted the quiz" Select the attempt and click "delete selected attempts" Reselect "all users who have attempted the quiz" The quiz attempt should be deleted
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-35163-master
    • Rank:
      43795

      Description

      After users have been unenrolled from a course it appears to be impossible to delete their quiz attempts using the attempts report. The exception to this is where there are no longer any users enrolled on the course at all, when the attempts are deleted as expected.

      Alternate path to the same base problem:
      Create a course with a teacher and a student. Create a quiz. As the teacher, 'Switch to role Student'. Attempt the quiz. Now return to the teachers default role. You won't be able to delete that attempt.

        Issue Links

          Activity

          Hide
          Michael Aherne added a comment -

          An initial look at this suggests it's because the user ID is not contained in the $allowed array passed to quiz_attempts_report::delete_selected_attempts. If there are no users enrolled on the course this array is empty so the deletion is allowed.

          This seems to happen regardless of whether groups are enabled for the quiz or not.

          Show
          Michael Aherne added a comment - An initial look at this suggests it's because the user ID is not contained in the $allowed array passed to quiz_attempts_report::delete_selected_attempts. If there are no users enrolled on the course this array is empty so the deletion is allowed. This seems to happen regardless of whether groups are enabled for the quiz or not.
          Hide
          Michael Aherne added a comment -

          I've attached a patch that appears to fix this, although I'm not sure if it's the right answer or just a hack.

          The problem seems to be that the report options and the attempts list are in different forms and the attempts form didn't have access to the report options values (specifically attemptid). The original code seemed to be expecting these to be available in the table's displayoptions property, but this doesn't seem to be set anywhere (https://github.com/moodle/moodle/blob/master/mod/quiz/report/attemptsreport_table.php#L521).

          Show
          Michael Aherne added a comment - I've attached a patch that appears to fix this, although I'm not sure if it's the right answer or just a hack. The problem seems to be that the report options and the attempts list are in different forms and the attempts form didn't have access to the report options values (specifically attemptid). The original code seemed to be expecting these to be available in the table's displayoptions property, but this doesn't seem to be set anywhere ( https://github.com/moodle/moodle/blob/master/mod/quiz/report/attemptsreport_table.php#L521 ).
          Hide
          John Coughlin added a comment -

          I'm pretty sure we are seeing this same basic issue as well, although in our case we are seeing it happen when a user in the teacher role changes his/her role to "student" and then makes an attempt on the quiz. After doing this, you can't delete the attempts the teacher has made on the quiz (until/unless you de-enroll everybody in the course).

          One additional wrinkle is that if the teacher goes back to their original role and then clicks "preview quiz" (in the quiz administration area) an error will appear saying "error writing to database". With debugging turned on, this error expands to : Duplicate key value violates constraint "mdl_quizatte_quiuseaatt_uix" DETAIL: key (quiz,userid,attempt)=(2,5,1) already exists". I haven't tried the patch yet, but we'll take a look next week.

          Show
          John Coughlin added a comment - I'm pretty sure we are seeing this same basic issue as well, although in our case we are seeing it happen when a user in the teacher role changes his/her role to "student" and then makes an attempt on the quiz. After doing this, you can't delete the attempts the teacher has made on the quiz (until/unless you de-enroll everybody in the course). One additional wrinkle is that if the teacher goes back to their original role and then clicks "preview quiz" (in the quiz administration area) an error will appear saying "error writing to database". With debugging turned on, this error expands to : Duplicate key value violates constraint "mdl_quizatte_quiuseaatt_uix" DETAIL: key (quiz,userid,attempt)=(2,5,1) already exists". I haven't tried the patch yet, but we'll take a look next week.
          Hide
          Eric Merrill added a comment -

          I agree that the patch here seems to be the right way to do - it maintains the current report view between states, which is what is needed to properly populate $allowed.

          I've added Tim H. on this to get his feedback, it is his baby after all.

          Show
          Eric Merrill added a comment - I agree that the patch here seems to be the right way to do - it maintains the current report view between states, which is what is needed to properly populate $allowed. I've added Tim H. on this to get his feedback, it is his baby after all.
          Hide
          Tim Hunt added a comment -

          The fix looks correct to me. Good catch.

          Please

          1. Rebase after this week's weekly build is out.
          2. Change then testing instructions, so that the are instructions to test that the fix is correct, rather that being steps to reproduce.

          Then this can be submitted for integration.

          Show
          Tim Hunt added a comment - The fix looks correct to me. Good catch. Please 1. Rebase after this week's weekly build is out. 2. Change then testing instructions, so that the are instructions to test that the fix is correct, rather that being steps to reproduce. Then this can be submitted for integration.
          Hide
          Michael Aherne added a comment - - edited

          Thanks for the feedback, Tim (and others). I've rebased this and changed the instructions. I've also added a 2.3 branch as I think it's important to get this fix into 2.3 as well as master.

          Tim, would you mind submitting this for integration? I don't have the permissions to do this (I've asked!)

          Show
          Michael Aherne added a comment - - edited Thanks for the feedback, Tim (and others). I've rebased this and changed the instructions. I've also added a 2.3 branch as I think it's important to get this fix into 2.3 as well as master. Tim, would you mind submitting this for integration? I don't have the permissions to do this (I've asked!)
          Hide
          Tim Hunt added a comment -

          Thanks Michael.

          Show
          Tim Hunt added a comment - Thanks Michael.
          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 Poltawski added a comment -

          Integrated to 2.3 and master. Thanks Michael!

          Show
          Dan Poltawski added a comment - Integrated to 2.3 and master. Thanks Michael!
          Hide
          Jason Fowler added a comment -

          works fine, thanks for the fix

          Show
          Jason Fowler added a comment - works fine, thanks for the fix
          Hide
          Dan Poltawski added a comment -

          Congratulations, you've done it!

          Thanks, this change is now in the latest weekly release!

          Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

          Show
          Dan Poltawski added a comment - Congratulations, you've done it! Thanks, this change is now in the latest weekly release! Join the crowds of people tomorrow from 8am and download this Moodle release from your local apple store!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: