|
History
| Log In
|
|
|
|
[
Permlink
| « Hide
]
Anthony Borrow - 19/Feb/07 03:49 AM
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
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. 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.
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
patch for \mod\quiz\report\overview\report.php
Tim - Do you have any objection to me checking in the above patch? Peace - Anthony
* 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. 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 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.
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 ; 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 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?
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. 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. 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.
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. 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 ;-) 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 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 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
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! 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. Sounds good, Tim.
I just found one more bug that was marked as a dup of this one - 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"; } 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
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bug
Closed
Major
Quiz results overview: not all combinations of Show attempts with ... & Groups settings work properly
