Moodle
  1. Moodle
  2. MDL-7772

Quiz results overview: not all combinations of Show attempts with ... & Groups settings work properly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9.1
    • Component/s: Quiz
    • Labels:
      None
    • Environment:
      Linux (Ubuntu 6.06), PHP 5.1.2, MySQL 5.0.22, Apache 2.0.55, Moodle 1.6.3+
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      27680

      Description

      As the semester is coming to a close, I wanted to go to a quiz and view all student with no attempts (from the drop down box at the bottom of the screen). I had 3 users show that had not attempted the quiz. Then I wanted to see how many from a particular group (section) had not taken the quiz and it indicates that there was nothing to show despite the fact that one of the users with no attempt belonged to the selected group. It appears that these 2 query options are not working together. I would like for it to limit the students who had not taken the quiz to those that are a member of the visible group rather than returning a false nothing to show.

      1. 18stable_mod_quiz_report_overview_report.php.diff
        2 kB
        Anthony Borrow
      2. quizreport_patch.txt
        8 kB
        Ann Adamcik
      3. report.php.diff
        1 kB
        Anthony Borrow
      4. report.php.patch
        1 kB
        Anthony Borrow

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          Tim - I am beginning to look in to why the queries as not functioning as intended by looking at the mess that begins around line 241 in \mod\quiz\report\overview\report.php this seems like a classic example of various combinations not working that you spoke of. Since this is (IMO) an instance of an actual bug in so far as it produces a false result, perhaps the priority of this issue might be bumped up. Peace - Anthony

          Show
          Anthony Borrow added a comment - Tim - I am beginning to look in to why the queries as not functioning as intended by looking at the mess that begins around line 241 in \mod\quiz\report\overview\report.php this seems like a classic example of various combinations not working that you spoke of. Since this is (IMO) an instance of an actual bug in so far as it produces a false result, perhaps the priority of this issue might be bumped up. Peace - Anthony
          Hide
          Tim Hunt added a comment -

          I think the problem is at around line 259 of report.php, which you get to if !empty($currentgroup) and $noattempts == 1.

          I don't have time to look into this further. If you have time, I suggest that just before like 350:

          $attempts = get_records_sql($select.$from.$where.$sort.$limit);

          you insert

          echo '<pre>', htmlspecialchars($select.$from.$where.$sort.$limit), '</pre>';

          That will display the SQL on-screen when you run the report. Try running the SQL directly against your database and experiment with it to try to work out how to change it to make it return the right results.

          If you can tell me how the SQL needs to change, I can probably fix the code.

          Show
          Tim Hunt added a comment - I think the problem is at around line 259 of report.php, which you get to if !empty($currentgroup) and $noattempts == 1. I don't have time to look into this further. If you have time, I suggest that just before like 350: $attempts = get_records_sql($select.$from.$where.$sort.$limit); you insert echo '<pre>', htmlspecialchars($select.$from.$where.$sort.$limit), '</pre>'; That will display the SQL on-screen when you run the report. Try running the SQL directly against your database and experiment with it to try to work out how to change it to make it return the right results. If you can tell me how the SQL needs to change, I can probably fix the code.
          Hide
          Anthony Borrow added a comment -

          Not a problem, I am currently working on some patches to the feedback module for our production server so that we can do our end of year evaluations which has my immediate attention. I will look into the queries and see what I can flush out and report my findings here if I'm able to sort out the sql.

          Show
          Anthony Borrow added a comment - Not a problem, I am currently working on some patches to the feedback module for our production server so that we can do our end of year evaluations which has my immediate attention. I will look into the queries and see what I can flush out and report my findings here if I'm able to sort out the sql.
          Hide
          Anthony Borrow added a comment -

          Tim - I found that in the query for Show Students With No Attempts Only there was an additional where condition (qa.preview=0) that was causing no records to be returned. When I remove it, the query returns as I would expect it. Since I did not fully understand what other scenarios may want to make use of the where condition, I removed it from the case I knew that I did not want it and created an else clause to use it all other cases since it was being used previously be default (line 257). I have tested this on my test server and implemented it on my production server. I also cleaned up line 266 where a double quote was used instead of a single quote. This was causing the course id to be put in single quotations which while not causing any problems was not consistent with how it was being coded elsewhere (line 251 for example). I am attaching a patch file so that this can be committed. Let me know if you have any questions or if you are swamped (and approve) I could commit it. Peace - Anthony

          Show
          Anthony Borrow added a comment - Tim - I found that in the query for Show Students With No Attempts Only there was an additional where condition (qa.preview=0) that was causing no records to be returned. When I remove it, the query returns as I would expect it. Since I did not fully understand what other scenarios may want to make use of the where condition, I removed it from the case I knew that I did not want it and created an else clause to use it all other cases since it was being used previously be default (line 257). I have tested this on my test server and implemented it on my production server. I also cleaned up line 266 where a double quote was used instead of a single quote. This was causing the course id to be put in single quotations which while not causing any problems was not consistent with how it was being coded elsewhere (line 251 for example). I am attaching a patch file so that this can be committed. Let me know if you have any questions or if you are swamped (and approve) I could commit it. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          patch for \mod\quiz\report\overview\report.php

          Show
          Anthony Borrow added a comment - patch for \mod\quiz\report\overview\report.php
          Hide
          Anthony Borrow added a comment -

          Tim - Do you have any objection to me checking in the above patch? Peace - Anthony

          Show
          Anthony Borrow added a comment - Tim - Do you have any objection to me checking in the above patch? Peace - Anthony
          Hide
          Tim Hunt added a comment -
          • Can you explain to me why this works?
          • Before committing, you will need to update the comment saying "No else ...".

          And this bit of code has changed in Moodle 1.8, because of changes to the groups system, are you able to make patches for all versions after 1.6? in which case I will check in the 1.6 changes as well, after you have dealt with the two points above.

          Show
          Tim Hunt added a comment - Can you explain to me why this works? Before committing, you will need to update the comment saying "No else ...". And this bit of code has changed in Moodle 1.8, because of changes to the groups system, are you able to make patches for all versions after 1.6? in which case I will check in the 1.6 changes as well, after you have dealt with the two points above.
          Hide
          Anthony Borrow added a comment -

          Tim - Sorry for the delay, I have been away from my office and will be away next week as well. In any case, when trying to view students with no attempts you cannot simultaneously request that the qa.preview=0 (because there is no record). We need to just have the LEFT JOIN where qa.userid IS NULL. Otherwise, they conflict and we get no results returned. In cases where we are not trying to find the students with no attempts, we must add in the WHERE condition to prevent the preview attempt (if one exists) from being included. Does that explain it?

          As for Moodle 1.8, I will dive in and take a look at the code and see what has changed and what needs to be done because it is not working in 1.8. Peace - Anthony

          Show
          Anthony Borrow added a comment - Tim - Sorry for the delay, I have been away from my office and will be away next week as well. In any case, when trying to view students with no attempts you cannot simultaneously request that the qa.preview=0 (because there is no record). We need to just have the LEFT JOIN where qa.userid IS NULL. Otherwise, they conflict and we get no results returned. In cases where we are not trying to find the students with no attempts, we must add in the WHERE condition to prevent the preview attempt (if one exists) from being included. Does that explain it? As for Moodle 1.8, I will dive in and take a look at the code and see what has changed and what needs to be done because it is not working in 1.8. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          Tim - Here is the requested diff for 18STABLE. It basically follows the same logic; however, I noticed that the combination All Participants and No Attempts was not working. I will comment on this in a second. The limitation to this patch is that it shows more than just the student role context so it is not precisely students without attempts but rather any user including admins, teachers, etc. For students only the context list will need to be narrowed down. That is my next project.

          Show
          Anthony Borrow added a comment - Tim - Here is the requested diff for 18STABLE. It basically follows the same logic; however, I noticed that the combination All Participants and No Attempts was not working. I will comment on this in a second. The limitation to this patch is that it shows more than just the student role context so it is not precisely students without attempts but rather any user including admins, teachers, etc. For students only the context list will need to be narrowed down. That is my next project.
          Hide
          Anthony Borrow added a comment -

          Tim - For the sake of consistency, I would like for lines 314 and 323 (version 1.75.2.9) to both use single quotes.

          314:
          $where = ' WHERE ra.contextid ' .$contextlists . ' AND '.groups_members_where_sql($currentgroup).' AND qa.preview = 0';

          323:
          $where = " WHERE ra.contextid $contextlists AND qa.preview = 0";

          I think it should be:
          $where = ' WHERE ra.contextid'.$contextlists.' AND qa.preview=0' ;

          although for the patch - I think we need to get rid of the qa.preview=0 as it causes the query to return an empty set so ultimately I think it should be:
          $where = ' WHERE ra.contextid'.$contextlists ;

          Show
          Anthony Borrow added a comment - Tim - For the sake of consistency, I would like for lines 314 and 323 (version 1.75.2.9) to both use single quotes. 314: $where = ' WHERE ra.contextid ' .$contextlists . ' AND '.groups_members_where_sql($currentgroup).' AND qa.preview = 0'; 323: $where = " WHERE ra.contextid $contextlists AND qa.preview = 0"; I think it should be: $where = ' WHERE ra.contextid'.$contextlists.' AND qa.preview=0' ; although for the patch - I think we need to get rid of the qa.preview=0 as it causes the query to return an empty set so ultimately I think it should be: $where = ' WHERE ra.contextid'.$contextlists ;
          Hide
          Anthony Borrow added a comment -

          Tim - I have a question about line 294:
          $contextlists = get_related_contexts_string(get_context_instance(CONTEXT_COURSE, $course->id));

          Who all can take a quiz? Whose results are we trying to see? Currently this seems to pulls back anyone who has some type of context for the course. Is this discriminate enough for what this page is trying to do - namely show information about quiz attempts? Should it just be students? Or perhaps better yet anyone with the /mod/quiz:attempt capability since those are the only users who should have an attempt in the database (which would make it a viable solution for custom roles). Fixing line 294 to make it appropriate discriminate the context list of users with attempts would resolve the issue that my 18STABLE patch produces of returning students and non-students (i.e. admins, teachers, etc.) who have not made an attempt. Let me know if you have any questions, suggestions or how you would like me to proceed with this. Peace - Anthony

          Show
          Anthony Borrow added a comment - Tim - I have a question about line 294: $contextlists = get_related_contexts_string(get_context_instance(CONTEXT_COURSE, $course->id)); Who all can take a quiz? Whose results are we trying to see? Currently this seems to pulls back anyone who has some type of context for the course. Is this discriminate enough for what this page is trying to do - namely show information about quiz attempts? Should it just be students? Or perhaps better yet anyone with the /mod/quiz:attempt capability since those are the only users who should have an attempt in the database (which would make it a viable solution for custom roles). Fixing line 294 to make it appropriate discriminate the context list of users with attempts would resolve the issue that my 18STABLE patch produces of returning students and non-students (i.e. admins, teachers, etc.) who have not made an attempt. Let me know if you have any questions, suggestions or how you would like me to proceed with this. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          I also noticed with 1.8 that the URL is not remembering the "funky" setting (i.e. the noattempts variable passed in the URL and also the pagesize variable) - in other words, the page seems to forget the options selected at the bottom. Would you like for me to file a separate tracker issue on that?

          Show
          Anthony Borrow added a comment - I also noticed with 1.8 that the URL is not remembering the "funky" setting (i.e. the noattempts variable passed in the URL and also the pagesize variable) - in other words, the page seems to forget the options selected at the bottom. Would you like for me to file a separate tracker issue on that?
          Hide
          Tim Hunt added a comment -

          Hmm perhaps the right thing to do is actually to use get_users_by_capability() from lib/accesslib.php to get all the users with capability mod/quiz:attempt in the context of the quiz. (And perhaps use it again to reject people with mod/quiz:preview, which gets rid of teachers.)

          I thought I had fixed the forgetting URL parameters bug. At least I fixed some occurrences of it. For 1.9, Jamie Pratt has come up with a robust solution to it in all cases, so I don't really want to spend more time on it in the 1.8 branch if possible.

          Show
          Tim Hunt added a comment - Hmm perhaps the right thing to do is actually to use get_users_by_capability() from lib/accesslib.php to get all the users with capability mod/quiz:attempt in the context of the quiz. (And perhaps use it again to reject people with mod/quiz:preview, which gets rid of teachers.) I thought I had fixed the forgetting URL parameters bug. At least I fixed some occurrences of it. For 1.9, Jamie Pratt has come up with a robust solution to it in all cases, so I don't really want to spend more time on it in the 1.8 branch if possible.
          Hide
          Anthony Borrow added a comment -

          Tim - Sounds good, I will be out of the office until July 1st and then I will look at changing the code to use the get_users_by_capability function as suggested; however, if you get a chance (and I understand that is not likely - but nevertheless, if you do) to patch the 1.8 code it would be greatly appreciated. If nothing else, if we can get it working in 1.9 that would be fine with me.

          I'm all for putting our efforts into 1.9 as that is what I am hoping to use for the fall semester (late August). I'll do another CVS download and merge with our custom code to see if any of the forgetting URL parameters bugs get fixed but if not I am not overly concerned about them.

          Show
          Anthony Borrow added a comment - Tim - Sounds good, I will be out of the office until July 1st and then I will look at changing the code to use the get_users_by_capability function as suggested; however, if you get a chance (and I understand that is not likely - but nevertheless, if you do) to patch the 1.8 code it would be greatly appreciated. If nothing else, if we can get it working in 1.9 that would be fine with me. I'm all for putting our efforts into 1.9 as that is what I am hoping to use for the fall semester (late August). I'll do another CVS download and merge with our custom code to see if any of the forgetting URL parameters bugs get fixed but if not I am not overly concerned about them.
          Hide
          Tim Hunt added a comment -

          OK, I checked your changes into 1.8. They looked sensible.

          I was wondering whether it would be better just to replace "qa.preview = 0" with "(qa.preview = 0 OR qa.preview IS NULL)" everywhere?

          Show
          Tim Hunt added a comment - OK, I checked your changes into 1.8. They looked sensible. I was wondering whether it would be better just to replace "qa.preview = 0" with "(qa.preview = 0 OR qa.preview IS NULL)" everywhere?
          Hide
          Anthony Borrow added a comment -

          Tim - (qa.preview = 0 OR qa.preview IS NULL) seems like a simpler fix from a more experienced programmer although I have not tested it to make sure that it does in fact work. I will so when I get back to the office and if it does then I will post a patch file for that way too. Then you can choose. Either way is fine with me and since you maintain the quiz module I would say go with whatever is easier for you long term - in other words use whichever code is easier to maintain. It seems to me that if statements would complicate things and that the (qa.preview = 0 OR qa.preview IS NULL) is easier to maintain and follow.

          Show
          Anthony Borrow added a comment - Tim - (qa.preview = 0 OR qa.preview IS NULL) seems like a simpler fix from a more experienced programmer although I have not tested it to make sure that it does in fact work. I will so when I get back to the office and if it does then I will post a patch file for that way too. Then you can choose. Either way is fine with me and since you maintain the quiz module I would say go with whatever is easier for you long term - in other words use whichever code is easier to maintain. It seems to me that if statements would complicate things and that the (qa.preview = 0 OR qa.preview IS NULL) is easier to maintain and follow.
          Hide
          Ann Adamcik added a comment -

          Regarding the changes checked in to 1.8.1:

          We have a number of site admins, and having them all show up when trying to 'view students with no attempts' or 'view all students' isn't really an improvement. Also, when I select 'view all students', preview attempts are showing up in the list.

          Show
          Ann Adamcik added a comment - Regarding the changes checked in to 1.8.1: We have a number of site admins, and having them all show up when trying to 'view students with no attempts' or 'view all students' isn't really an improvement. Also, when I select 'view all students', preview attempts are showing up in the list.
          Hide
          Tim Hunt added a comment -

          For long term maintainability the best solution would be to rewrite the whole block of code that gets the data from the database. It sucks. However, I am hesitant to do this, because I am not sure I know exactly what it should be doing for every possible combination of options. As this bug report shows, the current code is not getting it right.

          What would be really helpful would be two things.

          1. document describing the correct behavior in every possible case. I guess there are eight possible cases, coming from the four choices in the Show ... dropdown, and whether we are showing a group or the whole course, and a good place to do this might be to expand what is already on the page http://docs.moodle.org/en/Quiz_reports.

          2. even more valuable, to work out the minimal test dataset that would be needed to confirm that the report was actually working. That is, a description saying you need a course with X teachers and Y students arranged into groups like this. Student 1 has attempted the quiz. Student 2 hasn't. Student 3 used to be enrolled on the course and attempted the quiz, but has not beed unerolled. Teacher has previewed the quiz, and so on, and so forth. And then for each of the 8 possible report settings, say who should be listed (and obviously all 8 should be different lists). Probably, to be really thorough, in these modern roles and capabilites times, you need to include a user who has been assigned the student role on a whole course category, and also some users who can only access the quiz because of a role override. Such a description would belong on a new page linked from http://docs.moodle.org/en/Development:Developer_notes#Quiz_.26_question_types.

          Doing 2. would a wonderful contribution, and you could argue that until we have such a thing, we are really wasting our time fiddling around just trying to fix particular cases of this bug

          Show
          Tim Hunt added a comment - For long term maintainability the best solution would be to rewrite the whole block of code that gets the data from the database. It sucks. However, I am hesitant to do this, because I am not sure I know exactly what it should be doing for every possible combination of options. As this bug report shows, the current code is not getting it right. What would be really helpful would be two things. 1. document describing the correct behavior in every possible case. I guess there are eight possible cases, coming from the four choices in the Show ... dropdown, and whether we are showing a group or the whole course, and a good place to do this might be to expand what is already on the page http://docs.moodle.org/en/Quiz_reports . 2. even more valuable, to work out the minimal test dataset that would be needed to confirm that the report was actually working. That is, a description saying you need a course with X teachers and Y students arranged into groups like this. Student 1 has attempted the quiz. Student 2 hasn't. Student 3 used to be enrolled on the course and attempted the quiz, but has not beed unerolled. Teacher has previewed the quiz, and so on, and so forth. And then for each of the 8 possible report settings, say who should be listed (and obviously all 8 should be different lists). Probably, to be really thorough, in these modern roles and capabilites times, you need to include a user who has been assigned the student role on a whole course category, and also some users who can only access the quiz because of a role override. Such a description would belong on a new page linked from http://docs.moodle.org/en/Development:Developer_notes#Quiz_.26_question_types . Doing 2. would a wonderful contribution, and you could argue that until we have such a thing, we are really wasting our time fiddling around just trying to fix particular cases of this bug
          Hide
          Ann Adamcik added a comment -

          Okay, here's what I've worked out -

          1. Correct behavior

          Case 1: All participants
          1A: Show students with attempts only - list all currently enrolled students who have made a quiz attempt
          1B: Show students with no attempts only - list all currently enrolled students who have not attempted this quiz
          1C: Show all students - list all currently enrolled students
          1D: Show all attempts - list both currently enrolled and previously enrolled students who have made a quiz attempt

          Case 2: Group
          2A: Show students with attempts only - list all currently enrolled students who are members of the selected group, and who have made a quiz attempt
          2B: Show students with no attempts only - list all currently enrolled students who are members of the selected group, and who have not yet attempted the quiz
          2C: Show all students - list all currently enrolled students who are members of the selected group
          2D: Show all attempts - ? Same as 2A, since any previously enrolled students who attempted the quiz will not belong to any group.

          2. I'm not sure I covered all cases, but here's the test course I'm using and the expected results -

          Student A, student role in course context, in Group A - 1 quiz attempt
          Student B, student role in course category context, in Group A - 0 quiz attempts
          Student C, student role in course context, in Group B - 0 quiz attempts
          Student D, student role in course context, no group, 0 quiz attempts
          Student E, no longer a student in this course, 1 quiz attempt

          Preview attempts: 1 teacher, 1 admin

          Results Overview:

          Case 1: All participants
          1A: Show students with attempts only - Student A
          1B: Show students with no attempts only - Student B, Student C, Student D
          1C: Show all students - Students A through D
          1D: Show all attempts - Student A and Student E

          Case 2: Group A
          2A: Show students with attempts only - Student A
          2B: Show students with no attempts only - Student B
          2C: Show all students - Student A and Student B
          2D: Show all attempts - Student A

          Case 3: Group B
          3A: Show students with attempts only - no results
          3B: Show students with no attempts only - Student C
          3C: Show all students - Student C
          3D: Show all attempts - no results

          Finally, here's the re-worked sql to produce the above results -

          $contextlists = get_related_contexts_string(get_context_instance(CONTEXT_COURSE, $course->id));

          // Get list of users to exclude from results - e.g. teachers, admins, etc.
          $excluded = join(',', array_keys(get_users_by_capability($context, "mod/quiz:preview")));

          // Construct the SQL
          $select = 'SELECT '.sql_concat('u.id', '\'#\'', $db->IfNull('qa.attempt', '0')).' AS uniqueid, '.
          'qa.uniqueid as attemptuniqueid, qa.id AS attempt, u.id AS userid, u.firstname, u.lastname, u.picture, '.
          'qa.sumgrades, qa.timefinish, qa.timestart, qa.timefinish - qa.timestart AS duration ';

          if ($course->id != SITEID) {

          // This part is the same for all cases - join users and quiz_attempts tables
          $from = 'FROM '.$CFG->prefix.'user u ';
          $from .= 'LEFT JOIN '.$CFG->prefix.'quiz_attempts qa ON qa.userid = u.id AND qa.quiz = '.$quiz->id;

          // Viewing all participants

          if ( empty( $currentgroup ) ) {

          if ( $noattempts == 3 )

          { // Show all attempts - including students who are no longer in the course // Anyone who has made a quiz attempt, but does not have quiz preview capabilities $where = ' WHERE qa.id IS NOT NULL AND u.id NOT IN (' .$excluded. ')'; }

          else {

          // Get everyone with a role in the course context, who does not have quiz preview capabilities - e.g. all students
          $from .= ' JOIN '.$CFG->prefix.'role_assignments ra ON ra.userid = u.id ';
          $where = ' WHERE ra.contextid ' . $contextlists . ' AND u.id NOT IN (' .$excluded. ')';

          if ( empty( $noattempts ) )

          { // Show only those students with attempts $where .= ' AND qa.id IS NOT NULL'; }

          else if ( $noattempts == 1 )

          { // Show only those students without attempts $where .= ' AND qa.id IS NULL'; }

          }
          }

          // Viewing selected group only.

          else if ( !empty ( $currentgroup ) ) {

          // Get everyone in the selected group, who does not have quiz preview capabilities - e.g. all students
          $from .= groups_members_join_sql(). ' JOIN '.$CFG->prefix.'role_assignments ra ON ra.userid = u.id ';
          $where = ' WHERE ra.contextid ' . $contextlists . ' AND ' .groups_members_where_sql($currentgroup). ' AND u.id NOT IN (' .$
          excluded. ')';
          if ( empty( $noattempts ) || $noattempts == 3 )

          { // Show only those students with attempts $where .= ' AND qa.id IS NOT NULL'; }

          else if ( $noattempts == 1 )

          { // Show only those students without attempts $where .= ' AND qa.id IS NULL'; }

          }

          $countsql = 'SELECT COUNT(DISTINCT('.sql_concat('u.id', '\'#\'', $db->IfNull('qa.attempt', '0')).')) '.$from.$where;
          }

          Let me know if this makes sense...

          -Ann

          Show
          Ann Adamcik added a comment - Okay, here's what I've worked out - 1. Correct behavior Case 1: All participants 1A: Show students with attempts only - list all currently enrolled students who have made a quiz attempt 1B: Show students with no attempts only - list all currently enrolled students who have not attempted this quiz 1C: Show all students - list all currently enrolled students 1D: Show all attempts - list both currently enrolled and previously enrolled students who have made a quiz attempt Case 2: Group 2A: Show students with attempts only - list all currently enrolled students who are members of the selected group, and who have made a quiz attempt 2B: Show students with no attempts only - list all currently enrolled students who are members of the selected group, and who have not yet attempted the quiz 2C: Show all students - list all currently enrolled students who are members of the selected group 2D: Show all attempts - ? Same as 2A, since any previously enrolled students who attempted the quiz will not belong to any group. 2. I'm not sure I covered all cases, but here's the test course I'm using and the expected results - Student A, student role in course context, in Group A - 1 quiz attempt Student B, student role in course category context, in Group A - 0 quiz attempts Student C, student role in course context, in Group B - 0 quiz attempts Student D, student role in course context, no group, 0 quiz attempts Student E, no longer a student in this course, 1 quiz attempt Preview attempts: 1 teacher, 1 admin Results Overview: Case 1: All participants 1A: Show students with attempts only - Student A 1B: Show students with no attempts only - Student B, Student C, Student D 1C: Show all students - Students A through D 1D: Show all attempts - Student A and Student E Case 2: Group A 2A: Show students with attempts only - Student A 2B: Show students with no attempts only - Student B 2C: Show all students - Student A and Student B 2D: Show all attempts - Student A Case 3: Group B 3A: Show students with attempts only - no results 3B: Show students with no attempts only - Student C 3C: Show all students - Student C 3D: Show all attempts - no results Finally, here's the re-worked sql to produce the above results - $contextlists = get_related_contexts_string(get_context_instance(CONTEXT_COURSE, $course->id)); // Get list of users to exclude from results - e.g. teachers, admins, etc. $excluded = join(',', array_keys(get_users_by_capability($context, "mod/quiz:preview"))); // Construct the SQL $select = 'SELECT '.sql_concat('u.id', '\'#\'', $db->IfNull('qa.attempt', '0')).' AS uniqueid, '. 'qa.uniqueid as attemptuniqueid, qa.id AS attempt, u.id AS userid, u.firstname, u.lastname, u.picture, '. 'qa.sumgrades, qa.timefinish, qa.timestart, qa.timefinish - qa.timestart AS duration '; if ($course->id != SITEID) { // This part is the same for all cases - join users and quiz_attempts tables $from = 'FROM '.$CFG->prefix.'user u '; $from .= 'LEFT JOIN '.$CFG->prefix.'quiz_attempts qa ON qa.userid = u.id AND qa.quiz = '.$quiz->id; // Viewing all participants if ( empty( $currentgroup ) ) { if ( $noattempts == 3 ) { // Show all attempts - including students who are no longer in the course // Anyone who has made a quiz attempt, but does not have quiz preview capabilities $where = ' WHERE qa.id IS NOT NULL AND u.id NOT IN (' .$excluded. ')'; } else { // Get everyone with a role in the course context, who does not have quiz preview capabilities - e.g. all students $from .= ' JOIN '.$CFG->prefix.'role_assignments ra ON ra.userid = u.id '; $where = ' WHERE ra.contextid ' . $contextlists . ' AND u.id NOT IN (' .$excluded. ')'; if ( empty( $noattempts ) ) { // Show only those students with attempts $where .= ' AND qa.id IS NOT NULL'; } else if ( $noattempts == 1 ) { // Show only those students without attempts $where .= ' AND qa.id IS NULL'; } } } // Viewing selected group only. else if ( !empty ( $currentgroup ) ) { // Get everyone in the selected group, who does not have quiz preview capabilities - e.g. all students $from .= groups_members_join_sql(). ' JOIN '.$CFG->prefix.'role_assignments ra ON ra.userid = u.id '; $where = ' WHERE ra.contextid ' . $contextlists . ' AND ' .groups_members_where_sql($currentgroup). ' AND u.id NOT IN (' .$ excluded. ')'; if ( empty( $noattempts ) || $noattempts == 3 ) { // Show only those students with attempts $where .= ' AND qa.id IS NOT NULL'; } else if ( $noattempts == 1 ) { // Show only those students without attempts $where .= ' AND qa.id IS NULL'; } } $countsql = 'SELECT COUNT(DISTINCT('.sql_concat('u.id', '\'#\'', $db->IfNull('qa.attempt', '0')).')) '.$from.$where; } Let me know if this makes sense... -Ann
          Hide
          Joseph Rézeau added a comment -

          BUMP!
          Ann Adamcik - [23/Jun/07 01:24 PM ] wrote;
          Regarding the changes checked in to 1.8.1:
          We have a number of site admins, and having them all show up when trying to 'view students with no attempts' or 'view all students' isn't really an improvement.

          I have exactly the same problem. Tim, could you possibly find a fix to display only "real students" in the 'Show all students' and 'Show students with no attempts' lists, excluding Teacher(s), Admin(s) and any other role which is not a student?

          Joseph

          Show
          Joseph Rézeau added a comment - BUMP! Ann Adamcik - [23/Jun/07 01:24 PM ] wrote; Regarding the changes checked in to 1.8.1: We have a number of site admins, and having them all show up when trying to 'view students with no attempts' or 'view all students' isn't really an improvement. I have exactly the same problem. Tim, could you possibly find a fix to display only "real students" in the 'Show all students' and 'Show students with no attempts' lists, excluding Teacher(s), Admin(s) and any other role which is not a student? Joseph
          Hide
          Tim Hunt added a comment -

          I know, I know. This is a bad problem. Unfortunately, that does not change the fact I have not time. Maybe next week, if I get something else finished first.

          Show
          Tim Hunt added a comment - I know, I know. This is a bad problem. Unfortunately, that does not change the fact I have not time. Maybe next week, if I get something else finished first.
          Hide
          Anthony Borrow added a comment -

          If I can get some time to look at this I will see if I can come up with a patch. Of course, Ann has been good about providing patches too. So hopefully one of us might be able to help out. Peace - Anthony

          Show
          Anthony Borrow added a comment - If I can get some time to look at this I will see if I can come up with a patch. Of course, Ann has been good about providing patches too. So hopefully one of us might be able to help out. Peace - Anthony
          Hide
          Ann Adamcik added a comment -

          Attached is a patch against the 1.9 stable branch. The logic is as follows:

          Show students with attempts -> shows attempts by non-admin users who have mod/quiz:attempt capability.
          Show students with no attempts -> shows list of non-admin users who have mod/quiz:attempt capability, and who have not attempted the quiz.
          Show all students -> list of all non-admin users who have mod/quiz:attempt capability, with or without attempts.
          Show all attempts -> list of all attempts.

          In all 4 cases, preview attempts are filtered out. There is one line that can be commented out if preview attempts are desired in the Show all attempts case. If a group is selected, results are filtered by group membership.

          We've tested this with various group/groupings settings, and various multiple role assignments. Additional testing and/or comments welcome!

          Show
          Ann Adamcik added a comment - Attached is a patch against the 1.9 stable branch. The logic is as follows: Show students with attempts -> shows attempts by non-admin users who have mod/quiz:attempt capability. Show students with no attempts -> shows list of non-admin users who have mod/quiz:attempt capability, and who have not attempted the quiz. Show all students -> list of all non-admin users who have mod/quiz:attempt capability, with or without attempts. Show all attempts -> list of all attempts. In all 4 cases, preview attempts are filtered out. There is one line that can be commented out if preview attempts are desired in the Show all attempts case. If a group is selected, results are filtered by group membership. We've tested this with various group/groupings settings, and various multiple role assignments. Additional testing and/or comments welcome!
          Hide
          Tim Hunt added a comment -

          This looks like good work. Thank you very much.

          The situation is that I am currently very busy with other things. However, the OU (The Open University, UK, where I work) is about to outsource a lot of work on the quiz reports, so when I was writing the specification for that, I added fixing a lot of quiz report bugs to the requirements (http://docs.moodle.org/en/Development:Quiz_report_enhancements), as well as adding new features. So this should get looked at soon.

          Show
          Tim Hunt added a comment - This looks like good work. Thank you very much. The situation is that I am currently very busy with other things. However, the OU (The Open University, UK, where I work) is about to outsource a lot of work on the quiz reports, so when I was writing the specification for that, I added fixing a lot of quiz report bugs to the requirements ( http://docs.moodle.org/en/Development:Quiz_report_enhancements ), as well as adding new features. So this should get looked at soon.
          Hide
          Ann Adamcik added a comment -

          Sounds good, Tim.

          I just found one more bug that was marked as a dup of this one - MDL-9830. If the quiz has overall feedback defined, only users with attempts are shown. 'Students with no attempts' returns nothing at all, and 'All students' only lists the ones with attempts. Interestingly, there are page numbers at the top even when the table is empty.

          The problem is that if the quiz has feedback, there is a join on the quiz_feedback table that results in the query not returning any users without attempts. This needs to be a left join in order to return users both with and without attempts.

          @@ -412,7 +413,7 @@
          if ($hasfeedback) {
          $factor = $quiz->grade/$quiz->sumgrades;
          $select .= ', qf.feedbacktext ';

          • $from .= " JOIN {$CFG->prefix}quiz_feedback qf ON " .
            + $from .= " LEFT JOIN {$CFG->prefix}quiz_feedback qf ON " .
            "qf.quizid = $quiz->id AND qf.mingrade <= qa.sumgrades * $factor AND qa.sumgrades * $factor < qf.maxgrade";
            }
          Show
          Ann Adamcik added a comment - Sounds good, Tim. I just found one more bug that was marked as a dup of this one - MDL-9830 . If the quiz has overall feedback defined, only users with attempts are shown. 'Students with no attempts' returns nothing at all, and 'All students' only lists the ones with attempts. Interestingly, there are page numbers at the top even when the table is empty. The problem is that if the quiz has feedback, there is a join on the quiz_feedback table that results in the query not returning any users without attempts. This needs to be a left join in order to return users both with and without attempts. @@ -412,7 +413,7 @@ if ($hasfeedback) { $factor = $quiz->grade/$quiz->sumgrades; $select .= ', qf.feedbacktext '; $from .= " JOIN {$CFG->prefix}quiz_feedback qf ON " . + $from .= " LEFT JOIN {$CFG->prefix}quiz_feedback qf ON " . "qf.quizid = $quiz->id AND qf.mingrade <= qa.sumgrades * $factor AND qa.sumgrades * $factor < qf.maxgrade"; }
          Hide
          Anthony Borrow added a comment -

          Ann - I will definitely check out the patch in the coming week and give it some testing and comment back with any questions/concerns. Thanks for your work on this! Peace - Anthony

          Show
          Anthony Borrow added a comment - Ann - I will definitely check out the patch in the coming week and give it some testing and comment back with any questions/concerns. Thanks for your work on this! Peace - Anthony
          Hide
          Tim Hunt added a comment -

          Assigning quiz report issues mentioned in http://docs.moodle.org/en/Development:Quiz_report_enhancements to Jamie.

          Show
          Tim Hunt added a comment - Assigning quiz report issues mentioned in http://docs.moodle.org/en/Development:Quiz_report_enhancements to Jamie.
          Hide
          Anthony Borrow added a comment -

          Ann - For 1.8, here is a dirty hack that I used to help a production site. Essentially I indicate that only folks with a student role (where the id field in the roles table is 5) should be included. Obviously, if there are custom roles then this dirty hack will not allow them to be seen; however, despite its limitations I wanted to share it with you and others in case it might be helpful. I have not had a chance to look that the 1.9 patches yet but hopefully this weekend I might find some time. Peace - Anthony

          Show
          Anthony Borrow added a comment - Ann - For 1.8, here is a dirty hack that I used to help a production site. Essentially I indicate that only folks with a student role (where the id field in the roles table is 5) should be included. Obviously, if there are custom roles then this dirty hack will not allow them to be seen; however, despite its limitations I wanted to share it with you and others in case it might be helpful. I have not had a chance to look that the 1.9 patches yet but hopefully this weekend I might find some time. Peace - Anthony
          Hide
          Jamie Pratt added a comment -

          Have been considering what to do here.

          Back in the days of Moodle 1.6 mjollnir_ removed sql code that imploded all user ids into an array :

          "Fixing bug 4303 - quiz reports dying on large enrolments - getting all users into an array and imploding it into an IN() replaced with joins. This changes the functionality SLIGHTLY in two ways - 1. show students with no attempts now JUST shows students with no attempts, rather than both. Show students with no attempts is now disabled for the site course as is not really relevant and could be ridiculously large"

          See MDL-4303

          Ann's second proposed patch is doing the same thing.

          Show
          Jamie Pratt added a comment - Have been considering what to do here. Back in the days of Moodle 1.6 mjollnir_ removed sql code that imploded all user ids into an array : "Fixing bug 4303 - quiz reports dying on large enrolments - getting all users into an array and imploding it into an IN() replaced with joins. This changes the functionality SLIGHTLY in two ways - 1. show students with no attempts now JUST shows students with no attempts, rather than both. Show students with no attempts is now disabled for the site course as is not really relevant and could be ridiculously large" See MDL-4303 Ann's second proposed patch is doing the same thing.
          Hide
          Jamie Pratt added a comment -

          I think that in general to query all attempts on a quiz we query all attempts with preview = 0. It is the responsibility of attempt.php to check capabilities and mark who is previewing a question.

          Where we want to check who is expected to do a quiz - ie. the old student role. We could query all users who have a role assignment in the module or inherited in the module context and then exclude those users returned by get_users_by_capability($context, "mod/quiz:preview"). This is not perfect as for instance this will still include users assigned the role 'course creator' at the course category or global level. But I assume a IN () clause in sql including nearly all users with a role assignment is not going to be scalable.

          I think we need to use the module context in the query rather than the course context as Ann has done above.

          Show
          Jamie Pratt added a comment - I think that in general to query all attempts on a quiz we query all attempts with preview = 0. It is the responsibility of attempt.php to check capabilities and mark who is previewing a question. Where we want to check who is expected to do a quiz - ie. the old student role. We could query all users who have a role assignment in the module or inherited in the module context and then exclude those users returned by get_users_by_capability($context, "mod/quiz:preview"). This is not perfect as for instance this will still include users assigned the role 'course creator' at the course category or global level. But I assume a IN () clause in sql including nearly all users with a role assignment is not going to be scalable. I think we need to use the module context in the query rather than the course context as Ann has done above.
          Hide
          Jamie Pratt added a comment -

          Where a group has been selected we can ignore role assignments and just query the groups_members table and then exclude users with the preview capability.

          Show
          Jamie Pratt added a comment - Where a group has been selected we can ignore role assignments and just query the groups_members table and then exclude users with the preview capability.
          Hide
          Jamie Pratt added a comment -

          Added a forum post here http://moodle.org/mod/forum/discuss.php?d=95905 about whether it is a good idea to write an addition to get_users_by_capability to allow a db JOIN to other data to fetch from the db.

          Show
          Jamie Pratt added a comment - Added a forum post here http://moodle.org/mod/forum/discuss.php?d=95905 about whether it is a good idea to write an addition to get_users_by_capability to allow a db JOIN to other data to fetch from the db.
          Hide
          Tim Hunt added a comment -

          I think get_users_by_capability can check groups for you, which is good. I replied in the forum too.

          Show
          Tim Hunt added a comment - I think get_users_by_capability can check groups for you, which is good. I replied in the forum too.
          Hide
          Anthony Borrow added a comment -

          Jamie - I noticed the commit to HEAD, are you planning on patching 1.9? Peace - Anthony

          Show
          Anthony Borrow added a comment - Jamie - I noticed the commit to HEAD, are you planning on patching 1.9? Peace - Anthony
          Hide
          Jamie Pratt added a comment -

          I committed a patch for this into 1.9 and HEAD

          Show
          Jamie Pratt added a comment - I committed a patch for this into 1.9 and HEAD
          Hide
          Jamie Pratt added a comment -

          Looks like I was working on the patch for 1.9 while you were writing your comment Anthony! Committed the patch.

          Show
          Jamie Pratt added a comment - Looks like I was working on the patch for 1.9 while you were writing your comment Anthony! Committed the patch.
          Hide
          Anthony Borrow added a comment -

          I suspected you were on top of it but just wanted to make sure. Thanks for your work. I'll try to do a little testing this weekend. Peace - Anthony

          Show
          Anthony Borrow added a comment - I suspected you were on top of it but just wanted to make sure. Thanks for your work. I'll try to do a little testing this weekend. Peace - Anthony
          Hide
          Tim Hunt added a comment -

          As far as I have seen while testing the quiz reports over the last few months, and having merged this new code into OU Moodle, and so reviewed it, this is now working, so closing.

          Show
          Tim Hunt added a comment - As far as I have seen while testing the quiz reports over the last few months, and having merged this new code into OU Moodle, and so reviewed it, this is now working, so closing.

            People

            • Votes:
              10 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: