From 9a3af544ffdbd671bdfc18169af955fed08dc040 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 13 Jun 2019 16:39:24 +0100 Subject: [PATCH] MDL-65915 quiz regrading: better progress information This makes it easier to diagnose problems if the regrade fails in the middle. Along the way, I added one un-declared field to the class, and converted some SQL-construction code to $DB->get_in_or_equal(). I also added the ability to set attributes on the label in html_writer::checkbox. --- lib/outputcomponents.php | 10 +- mod/quiz/lang/en/quiz.php | 1 + mod/quiz/report/attemptsreport_table.php | 3 +- .../report/overview/lang/en/quiz_overview.php | 2 + mod/quiz/report/overview/report.php | 45 +++++-- .../report/overview/tests/behat/basic.feature | 10 -- .../overview/tests/behat/regrading.feature | 115 ++++++++++++++++++ 7 files changed, 160 insertions(+), 26 deletions(-) create mode 100644 mod/quiz/report/overview/tests/behat/regrading.feature diff --git a/lib/outputcomponents.php b/lib/outputcomponents.php index 91c0a54e73a..8f1acefdb2d 100644 --- a/lib/outputcomponents.php +++ b/lib/outputcomponents.php @@ -1785,10 +1785,12 @@ class html_writer { * @param bool $checked Whether the checkbox is checked * @param string $label The label for the checkbox * @param array $attributes Any attributes to apply to the checkbox + * @param array $labelattributes Any attributes to apply to the label, if present * @return string html fragment */ - public static function checkbox($name, $value, $checked = true, $label = '', array $attributes = null) { - $attributes = (array)$attributes; + public static function checkbox($name, $value, $checked = true, $label = '', + array $attributes = null, array $labelattributes = null) { + $attributes = (array) $attributes; $output = ''; if ($label !== '' and !is_null($label)) { @@ -1804,7 +1806,9 @@ class html_writer { $output .= self::empty_tag('input', $attributes); if ($label !== '' and !is_null($label)) { - $output .= self::tag('label', $label, array('for'=>$attributes['id'])); + $labelattributes = (array) $labelattributes; + $labelattributes['for'] = $attributes['id']; + $output .= self::tag('label', $label, $labelattributes); } return $output; diff --git a/mod/quiz/lang/en/quiz.php b/mod/quiz/lang/en/quiz.php index bed899502f5..4cb853f7217 100644 --- a/mod/quiz/lang/en/quiz.php +++ b/mod/quiz/lang/en/quiz.php @@ -861,6 +861,7 @@ $string['sectionheadingremove'] = 'Remove heading \'{$a}\''; $string['seequestions'] = '(See questions)'; $string['select'] = 'Select'; $string['selectall'] = 'Select all'; +$string['selectattempt'] = 'Select attempt'; $string['selectcategory'] = 'Select category'; $string['selectedattempts'] = 'Selected attempts...'; $string['selectmultipleitems'] = 'Select multiple items'; diff --git a/mod/quiz/report/attemptsreport_table.php b/mod/quiz/report/attemptsreport_table.php index 7cb42069171..5feaa9d4be0 100644 --- a/mod/quiz/report/attemptsreport_table.php +++ b/mod/quiz/report/attemptsreport_table.php @@ -109,7 +109,8 @@ abstract class quiz_attempts_report_table extends table_sql { */ public function col_checkbox($attempt) { if ($attempt->attempt) { - return ''; + return html_writer::checkbox('attemptid[]', $attempt->attempt, false, + get_string('selectattempt', 'quiz'), [], ['class' => 'accesshide']); } else { return ''; } diff --git a/mod/quiz/report/overview/lang/en/quiz_overview.php b/mod/quiz/report/overview/lang/en/quiz_overview.php index 60d89679a8e..67024620999 100644 --- a/mod/quiz/report/overview/lang/en/quiz_overview.php +++ b/mod/quiz/report/overview/lang/en/quiz_overview.php @@ -64,9 +64,11 @@ $string['regradealldrydogroup'] = 'Regrade attempts ({$a->countregradeneeded}) m $string['regradealldrygroup'] = 'Dry run a full regrade for group \'{$a->groupname}\''; $string['regradeallgroup'] = 'Full regrade for group \'{$a->groupname}\''; $string['regradecomplete'] = 'Regrade completed successfully'; +$string['regradedsuccessfullyxofy'] = 'Successfully regraded ({$a->done}/{$a->count})'; $string['regradeheader'] = 'Regrading'; $string['regradeselected'] = 'Regrade selected attempts'; $string['regradingattemptxofy'] = 'Regrading attempt ({$a->done}/{$a->count})'; +$string['regradingattemptxofywithdetails'] = 'Regrading attempt ({$a->done}/{$a->count}) - Attempt {$a->attemptnum} by {$a->name} (id {$a->attemptid})'; $string['show'] = 'Show / download'; $string['showattempts'] = 'Only show / download attempts'; $string['showdetailedmarks'] = 'Marks for each question'; diff --git a/mod/quiz/report/overview/report.php b/mod/quiz/report/overview/report.php index c0596560a06..337c4d3868c 100644 --- a/mod/quiz/report/overview/report.php +++ b/mod/quiz/report/overview/report.php @@ -39,6 +39,11 @@ require_once($CFG->dirroot . '/mod/quiz/report/overview/overview_table.php'); */ class quiz_overview_report extends quiz_attempts_report { + /** + * @var bool whether there are actually students to show, given the options. + */ + protected $hasgroupstudents; + public function display($quiz, $cm, $course) { global $DB, $OUTPUT, $PAGE; @@ -376,21 +381,22 @@ class quiz_overview_report extends quiz_attempts_report { global $DB; $this->unlock_session(); - $sql = "SELECT quiza.* - FROM {quiz_attempts} quiza"; + $sql = "SELECT quiza.*, " . get_all_user_name_fields(true, 'u') . " + FROM {quiz_attempts} quiza + JOIN {user} u ON u.id = quiza.userid"; $where = "quiz = :qid AND preview = 0"; $params = array('qid' => $quiz->id); if ($this->hasgroupstudents && !empty($groupstudentsjoins->joins)) { - $sql .= "\nJOIN {user} u ON u.id = quiza.userid - {$groupstudentsjoins->joins}"; + $sql .= "\n{$groupstudentsjoins->joins}"; $where .= " AND {$groupstudentsjoins->wheres}"; $params += $groupstudentsjoins->params; } if ($attemptids) { - $aids = join(',', $attemptids); - $where .= " AND quiza.id IN ({$aids})"; + list($attemptidcondition, $attemptidparams) = $DB->get_in_or_equal($attemptids, SQL_PARAMS_NAMED); + $where .= " AND quiza.id $attemptidcondition"; + $params += $attemptidparams; } $sql .= "\nWHERE {$where}"; @@ -407,11 +413,16 @@ class quiz_overview_report extends quiz_attempts_report { 'done' => 0, ); foreach ($attempts as $attempt) { - $this->regrade_attempt($attempt, $dryrun); $a['done']++; + $a['attemptnum'] = $attempt->attempt; + $a['name'] = fullname($attempt); + $a['attemptid'] = $attempt->id; $progressbar->update($a['done'], $a['count'], - get_string('regradingattemptxofy', 'quiz_overview', $a)); + get_string('regradingattemptxofywithdetails', 'quiz_overview', $a)); + $this->regrade_attempt($attempt, $dryrun); } + $progressbar->update($a['done'], $a['count'], + get_string('regradedsuccessfullyxofy', 'quiz_overview', $a)); if (!$dryrun) { $this->update_overall_grades($quiz); @@ -457,8 +468,13 @@ class quiz_overview_report extends quiz_attempts_report { return; } - $attempts = $DB->get_records_list('quiz_attempts', 'uniqueid', - array_keys($attemptquestions)); + list($uniqueidcondition, $params) = $DB->get_in_or_equal(array_keys($attemptquestions)); + $attempts = $DB->get_records_sql(" + SELECT quiza.*, " . get_all_user_name_fields(true, 'u') . " + FROM {quiz_attempts} quiza + JOIN {user} u ON u.id = quiza.userid + WHERE quiza.uniqueid $uniqueidcondition + ", $params); $this->clear_regrade_table($quiz, $groupstudentsjoins); @@ -468,11 +484,16 @@ class quiz_overview_report extends quiz_attempts_report { 'done' => 0, ); foreach ($attempts as $attempt) { - $this->regrade_attempt($attempt, false, $attemptquestions[$attempt->uniqueid]); $a['done']++; + $a['attemptnum'] = $attempt->attempt; + $a['name'] = fullname($attempt); + $a['attemptid'] = $attempt->id; $progressbar->update($a['done'], $a['count'], - get_string('regradingattemptxofy', 'quiz_overview', $a)); + get_string('regradingattemptxofywithdetails', 'quiz_overview', $a)); + $this->regrade_attempt($attempt, false, $attemptquestions[$attempt->uniqueid]); } + $progressbar->update($a['done'], $a['count'], + get_string('regradedsuccessfullyxofy', 'quiz_overview', $a)); $this->update_overall_grades($quiz); } diff --git a/mod/quiz/report/overview/tests/behat/basic.feature b/mod/quiz/report/overview/tests/behat/basic.feature index 766f37a83e6..a24fcf42d77 100644 --- a/mod/quiz/report/overview/tests/behat/basic.feature +++ b/mod/quiz/report/overview/tests/behat/basic.feature @@ -78,13 +78,3 @@ Feature: Basic use of the Grades report And I should see "25.00" in the "S1 Student1" "table_row" # Check student2's grade And I should see "100.00" in the "S2 Student2" "table_row" - - # Check regrade and delete attempts. - And I set the field with xpath "//tr[contains(normalize-space(.), 'student1@example.com')]//input[@type='checkbox']" to "1" - And I press "Regrade selected attempts" - And I press "Continue" - And I should see "student1@example.com" - And I set the field with xpath "//tr[contains(normalize-space(.), 'student1@example.com')]//input[@type='checkbox']" to "1" - And I press "Delete selected attempts" - And I press "Yes" - And I should not see "student1@example.com" diff --git a/mod/quiz/report/overview/tests/behat/regrading.feature b/mod/quiz/report/overview/tests/behat/regrading.feature new file mode 100644 index 00000000000..5aba26eb663 --- /dev/null +++ b/mod/quiz/report/overview/tests/behat/regrading.feature @@ -0,0 +1,115 @@ +@mod @mod_quiz @quiz @quiz_overview @javascript +Feature: Regrading quiz attempts using the Grades report + In order to be able to correct mistakes I made setting up my quiz + As a teacher + I need to be able to re-grade attempts after editing questions + + Background: + Given the following "users" exist: + | username | firstname | lastname | + | teacher | Mark | Allwright | + | student1 | Student | One | + | student2 | Student | Two | + And the following "courses" exist: + | fullname | shortname | category | + | Course 1 | C1 | 0 | + And the following "course enrolments" exist: + | user | course | role | + | teacher | C1 | editingteacher | + | student1 | C1 | student | + | student2 | C1 | student | + And the following "question categories" exist: + | contextlevel | reference | name | + | Course | C1 | Test questions | + And the following "activities" exist: + | activity | name | course | idnumber | + | quiz | Quiz for testing regrading | C1 | quiz1 | + And the following "questions" exist: + | questioncategory | qtype | name | + | Test questions | truefalse | TF | + | Test questions | shortanswer | SA | + And quiz "Quiz for testing regrading" contains the following questions: + | question | page | maxmark | + | TF | 1 | 5.0 | + | SA | 1 | 5.0 | + And user "student1" has attempted "Quiz for testing regrading" with responses: + | slot | response | + | 1 | True | + | 2 | frog | + And user "student2" has attempted "Quiz for testing regrading" with responses: + | slot | response | + | 1 | True | + | 2 | toad | + + Scenario: Regrade all attempts + Given I log in as "teacher" + And I am on "Course 1" course homepage + And I follow "Quiz for testing regrading" + And I navigate to "Results > Grades" in current page administration + When I press "Regrade all" + + # Note, the order is not defined, so we can only check part of the message. + # Also, nothing has changed in the quiz, so the regrade won't alter any scores, + # but this is still a useful test that the regrade process completes without errors. + Then I should see "Quiz for testing regrading" + And I should see "Successfully regraded (2/2)" + And I should see "Regrade completed successfully" + And I press "Continue" + + # These next tests just serve to check we got back to the report. + And I should see "Quiz for testing regrading" + And I should see "Overall number of students achieving grade ranges" + + Scenario: Regrade selected attempts + Given I log in as "teacher" + And I am on "Course 1" course homepage + And I follow "Quiz for testing regrading" + And I navigate to "Results > Grades" in current page administration + When I click on "Select attempt" "checkbox" in the "Student Two" "table_row" + + And I press "Regrade selected attempts" + Then I should see "Quiz for testing regrading" + And I should see "Successfully regraded (1/1)" + And I should see "Regrade completed successfully" + And I press "Continue" + + # These next tests just serve to check we got back to the report. + And I should see "Quiz for testing regrading" + And I should see "Overall number of students achieving grade ranges" + + Scenario: Dry-run a full regrade, then regrade the attempts that will need it. + Given I log in as "teacher" + And I am on "Course 1" course homepage + And I follow "Quiz for testing regrading" + When I navigate to "Edit quiz" in current page administration + And I follow "Edit question SA" + And I set the field "id_fraction_1" to "50%" + And I press "id_submitbutton" + And I follow "Attempts: 2" + And I press "Dry run a full regrade" + + # Note, the order is not defined, so we can only check part of the message. + Then I should see "Quiz for testing regrading" + And I should see "Successfully regraded (2/2)" + And I should see "Regrade completed successfully" + And I press "Continue" + + And "Student One" row "Regrade" column of "attempts" table should not contain "Needed" + And "Student TwoReview attempt" row "Regrade" column of "attempts" table should contain "Needed" + # In the following, the first number is strike-through, and the second is not, but Behat can't see that. + # At this point, it is showing what would change. + And "Student TwoReview attempt" row "Q. 2/50.00Sort by Q. 2/50.00 Ascending" column of "attempts" table should contain "40.00/25.00" + And "Student TwoReview attempt" row "Grade/100.00Sort by Grade/100.00 Ascending" column of "attempts" table should contain "90.00/75.00" + And I press "Regrade attempts marked as needing regrading (1)" + And I should see "Quiz for testing regrading" + And I should see "Successfully regraded (1/1)" + And I should see "Regrade completed successfully" + And I press "Continue" + + # These next tests just serve to check we got back to the report. + And I should see "Quiz for testing regrading" + And I should see "Overall number of students achieving grade ranges" + # Now, both old-score strike-through and new score plain, are still shown, but now it indicates what did change. + And "Student TwoReview attempt" row "Q. 2/50.00Sort by Q. 2/50.00 Ascending" column of "attempts" table should contain "40.00/25.00" + And "Student TwoReview attempt" row "Grade/100.00Sort by Grade/100.00 Ascending" column of "attempts" table should contain "90.00/75.00" + And "Regrade attempts marked as needing regrading" "button" should not exist