diff --git a/mod/assign/backup/moodle2/restore_assign_stepslib.php b/mod/assign/backup/moodle2/restore_assign_stepslib.php index 64bc4b529d53a..4cd588d69b219 100644 --- a/mod/assign/backup/moodle2/restore_assign_stepslib.php +++ b/mod/assign/backup/moodle2/restore_assign_stepslib.php @@ -243,7 +243,10 @@ protected function process_assign_grade($data) { $flags->userid = $this->get_mappingid('user', $data->userid); $DB->insert_record('assign_user_flags', $flags); } - + // Fix null grades that were rescaled. + if ($data->grade < 0 && $data->grade != -1) { + $data->grade = -1; + } $newitemid = $DB->insert_record('assign_grades', $data); // Note - the old contextid is required in order to be able to restore files stored in diff --git a/mod/assign/db/upgrade.php b/mod/assign/db/upgrade.php index 0a1cb790c2e57..3a6b3f4a67cfb 100644 --- a/mod/assign/db/upgrade.php +++ b/mod/assign/db/upgrade.php @@ -318,5 +318,18 @@ function xmldb_assign_upgrade($oldversion) { upgrade_mod_savepoint(true, 2017051501, 'assign'); } + if ($oldversion < 2017051502) { + require_once($CFG->dirroot.'/mod/assign/upgradelib.php'); + $brokenassigns = get_assignments_with_rescaled_null_grades(); + + // Set config value. + foreach ($brokenassigns as $assign) { + set_config('has_rescaled_null_grades_' . $assign, 1, 'assign'); + } + + // Main savepoint reached. + upgrade_mod_savepoint(true, 2017051502, 'assign'); + } + return true; } diff --git a/mod/assign/lang/en/assign.php b/mod/assign/lang/en/assign.php index 8b34a0f2eaa32..5ca89772a59b7 100644 --- a/mod/assign/lang/en/assign.php +++ b/mod/assign/lang/en/assign.php @@ -196,6 +196,9 @@ $string['extensionduedate'] = 'Extension due date'; $string['extensionnotafterduedate'] = 'Extension date must be after the due date'; $string['extensionnotafterfromdate'] = 'Extension date must be after the allow submissions from date'; +$string['fixrescalednullgrades'] = 'This assignment contains some erroneous grades. You can automatically repair these grades. This may affect course totals.'; +$string['fixrescalednullgradesconfirm'] = 'Are you sure you want to fix erroneous grades? All broken grades will be removed. This may affect course totals.'; +$string['fixrescalednullgradesdone'] = 'Grades fixed.'; $string['gradecanbechanged'] = 'Grade can be changed'; $string['gradersubmissionupdatedtext'] = '{$a->username} has updated their assignment submission for \'{$a->assignment}\' at {$a->timeupdated} diff --git a/mod/assign/lib.php b/mod/assign/lib.php index 7c6b896fc4ec5..4e7c7ec1fdebf 100644 --- a/mod/assign/lib.php +++ b/mod/assign/lib.php @@ -1585,7 +1585,8 @@ function assign_rescale_activity_grades($course, $cm, $oldmin, $oldmax, $newmin, 'a' => $cm->instance ); - $sql = 'UPDATE {assign_grades} set grade = (((grade - :p1) * :p2) + :p3) where assignment = :a'; + // Only rescale grades that are greater than or equal to 0. Anything else is a special value. + $sql = 'UPDATE {assign_grades} set grade = (((grade - :p1) * :p2) + :p3) where assignment = :a and grade >= 0'; $dbupdate = $DB->execute($sql, $params); if (!$dbupdate) { return false; diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 0a9d8930942e7..76312d630a38c 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -600,6 +600,8 @@ public function view($action='', $args = array()) { $o .= $this->view_batch_markingallocation($mform); } else if ($action == 'viewsubmitforgradingerror') { $o .= $this->view_error_page(get_string('submitforgrading', 'assign'), $notices); + } else if ($action == 'fixrescalednullgrades') { + $o .= $this->view_fix_rescaled_null_grades(); } else { $o .= $this->view_submission_page(); } @@ -4232,6 +4234,8 @@ protected function view_grading_page() { $this->require_view_grades(); require_once($CFG->dirroot . '/mod/assign/gradeform.php'); + $this->add_grade_notices(); + // Only load this if it is. $o .= $this->view_grading_table(); @@ -5205,6 +5209,8 @@ protected function view_submission_page() { $instance = $this->get_instance(); + $this->add_grade_notices(); + $o = ''; $postfix = ''; @@ -8538,6 +8544,92 @@ public function set_module_viewed() { $completion = new completion_info($this->get_course()); $completion->set_module_viewed($this->get_course_module()); } + + /** + * Checks for any grade notices, and adds notifications. Will display on assignment main page and grading table. + * + * @return void The notifications API will render the notifications at the appropriate part of the page. + */ + protected function add_grade_notices() { + if (has_capability('mod/assign:grade', $this->get_context()) && get_config('assign', 'has_rescaled_null_grades_' . $this->get_instance()->id)) { + $link = new \moodle_url('/mod/assign/view.php', array('id' => $this->get_course_module()->id, 'action' => 'fixrescalednullgrades')); + \core\notification::warning(get_string('fixrescalednullgrades', 'mod_assign', ['link' => $link->out()])); + } + } + + /** + * View fix rescaled null grades. + * + * @return bool True if null all grades are now fixed. + */ + protected function fix_null_grades() { + global $DB; + $result = $DB->set_field_select( + 'assign_grades', + 'grade', + -1, + 'grade <> ? AND grade < 0', + [-1] + ); + $assign = clone $this->get_instance(); + $assign->cmidnumber = $this->get_course_module()->idnumber; + assign_update_grades($assign); + return $result; + } + + /** + * View fix rescaled null grades. + * + * @return void The notifications API will render the notifications at the appropriate part of the page. + */ + protected function view_fix_rescaled_null_grades() { + global $OUTPUT; + + $o = ''; + + require_capability('mod/assign:grade', $this->get_context()); + + $instance = $this->get_instance(); + + $o .= $this->get_renderer()->render( + new assign_header( + $instance, + $this->get_context(), + $this->show_intro(), + $this->get_course_module()->id + ) + ); + + $confirm = optional_param('confirm', 0, PARAM_BOOL); + + if ($confirm) { + confirm_sesskey(); + + // Fix the grades. + $this->fix_null_grades(); + unset_config('has_rescaled_null_grades_' . $instance->id, 'assign'); + + // Display the notice. + $o .= $this->get_renderer()->notification(get_string('fixrescalednullgradesdone', 'assign'), 'notifysuccess'); + $url = new moodle_url( + '/mod/assign/view.php', + array( + 'id' => $this->get_course_module()->id, + 'action' => 'grading' + ) + ); + $o .= $this->get_renderer()->continue_button($url); + } else { + // Ask for confirmation. + $continue = new \moodle_url('/mod/assign/view.php', array('id' => $this->get_course_module()->id, 'action' => 'fixrescalednullgrades', 'confirm' => true, 'sesskey' => sesskey())); + $cancel = new \moodle_url('/mod/assign/view.php', array('id' => $this->get_course_module()->id)); + $o .= $OUTPUT->confirm(get_string('fixrescalednullgradesconfirm', 'mod_assign'), $continue, $cancel); + } + + $o .= $this->view_footer(); + + return $o; + } } /** diff --git a/mod/assign/tests/behat/rescale_grades.feature b/mod/assign/tests/behat/rescale_grades.feature index 75b070141c930..11a54a81fd54d 100644 --- a/mod/assign/tests/behat/rescale_grades.feature +++ b/mod/assign/tests/behat/rescale_grades.feature @@ -12,10 +12,12 @@ Feature: Check that the assignment grade can be rescaled when the max grade is c | username | firstname | lastname | email | | teacher1 | Teacher | 1 | teacher1@example.com | | student1 | Student | 1 | student10@example.com | + | student2 | Student | 2 | student10@example.com | And the following "course enrolments" exist: | user | course | role | | teacher1 | C1 | editingteacher | | student1 | C1 | student | + | student2 | C1 | student | And the following "groups" exist: | name | course | idnumber | | Group 1 | C1 | G1 | @@ -69,3 +71,18 @@ Feature: Check that the assignment grade can be rescaled when the max grade is c When I press "Save and display" And I navigate to "View all submissions" in current page administration Then "Student 1" row "Grade" column of "generaltable" table should contain "20.00" + + Scenario: Rescaling should not produce negative grades + Given I navigate to "View all submissions" in current page administration + And I click on "Grade" "link" in the "Student 2" "table_row" + And I wait until the page is ready + And I follow "Assignment: Test assignment name" + And I navigate to "Edit settings" in current page administration + And I expand all fieldsets + And I set the field "Rescale existing grades" to "Yes" + And I set the field "Maximum grade" to "50" + When I press "Save and display" + And I navigate to "View all submissions" in current page administration + # Since the decimal places are always displayed (e.g. 0.00), this will catch any grade. The student should not have one. + # We can't just check for - (empty grade) because a negative grade would also have that character. + Then "Student 2" row "Grade" column of "generaltable" table should not contain "." diff --git a/mod/assign/tests/lib_test.php b/mod/assign/tests/lib_test.php index 3603c7626b98c..abcfc0008852e 100644 --- a/mod/assign/tests/lib_test.php +++ b/mod/assign/tests/lib_test.php @@ -655,4 +655,33 @@ public function test_mod_assign_completion_get_active_rule_descriptions() { $this->assertEquals(mod_assign_get_completion_active_rule_descriptions($moddefaults), $activeruledescriptions); $this->assertEquals(mod_assign_get_completion_active_rule_descriptions(new stdClass()), []); } + + /** + * Test that if some grades are not set, they are left alone and not rescaled + */ + public function test_assign_rescale_activity_grades_some_unset() { + $this->resetAfterTest(); + + // As a teacher... + $this->setUser($this->editingteachers[0]); + $assign = $this->create_instance(); + + // Grade the student. + $data = ['grade' => 50]; + $assign->testable_apply_grade_to_user((object)$data, $this->students[0]->id, 0); + + // Try getting another students grade. This will give a grade of -1. + $assign->get_user_grade($this->students[1]->id, true); + + // Rescale. + assign_rescale_activity_grades($this->course, $assign->get_course_module(), 0, 100, 0, 10); + + // Get the grades for both students. + $student0grade = $assign->get_user_grade($this->students[0]->id, true); + $student1grade = $assign->get_user_grade($this->students[1]->id, true); + + // Make sure the real grade is scaled, but the -1 stays the same. + $this->assertEquals($student0grade->grade, 5); + $this->assertEquals($student1grade->grade, -1); + } } diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index 193f7fa0b656b..884a68b4638db 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -2953,4 +2953,147 @@ public function test_update_activity_completion_records_team_submission() { $completiondata = $completion->get_data($cm, false, $student2->id); $this->assertEquals(1, $completiondata->completionstate); } + + public function get_assignments_with_rescaled_null_grades_provider() { + return [ + 'Negative less than one is errant' => [ + 'grade' => -0.64, + 'count' => 1, + ], + 'Negative more than one is errant' => [ + 'grade' => -30.18, + 'count' => 1, + ], + 'Negative one exactly is not errant' => [ + 'grade' => -1, + 'count' => 0, + ], + 'Positive grade is not errant' => [ + 'grade' => 1, + 'count' => 0, + ], + 'Large grade is not errant' => [ + 'grade' => 100, + 'count' => 0, + ], + 'Zero grade is not errant' => [ + 'grade' => 0, + 'count' => 0, + ], + ]; + } + + /** + * Test determining if the assignment as any null grades that were rescaled. + * @dataProvider get_assignments_with_rescaled_null_grades_provider + */ + public function test_get_assignments_with_rescaled_null_grades($grade, $count) { + global $DB; + + $this->resetAfterTest(); + + $assign = $this->create_instance(array('grade' => 100)); + + // Try getting a student's grade. This will give a grade of -1. + // Then we can override it with a bad negative grade. + $assign->get_user_grade($this->students[0]->id, true); + + // Set the grade to something errant. + $DB->set_field( + 'assign_grades', + 'grade', + $grade, + [ + 'userid' => $this->students[0]->id, + 'assignment' => $assign->get_instance()->id, + ] + ); + + $this->assertCount($count, get_assignments_with_rescaled_null_grades()); + } + + /** + * Data provider for test_fix_null_grades + * @return array[] Test data for test_fix_null_grades. Each element should contain grade, expectedcount and gradebookvalue + */ + public function fix_null_grades_provider() { + return [ + 'Negative less than one is errant' => [ + 'grade' => -0.64, + 'gradebookvalue' => null, + ], + 'Negative more than one is errant' => [ + 'grade' => -30.18, + 'gradebookvalue' => null, + ], + 'Negative one exactly is not errant, but shouldn\'t be pushed to gradebook' => [ + 'grade' => -1, + 'gradebookvalue' => null, + ], + 'Positive grade is not errant' => [ + 'grade' => 1, + 'gradebookvalue' => 1, + ], + 'Large grade is not errant' => [ + 'grade' => 100, + 'gradebookvalue' => 100, + ], + 'Zero grade is not errant' => [ + 'grade' => 0, + 'gradebookvalue' => 0, + ], + ]; + } + + /** + * Test fix_null_grades + * @param number $grade The grade we should set in the assign grading table. + * @param number $expectedcount The finalgrade we expect in the gradebook after fixing the grades. + * @dataProvider fix_null_grades_provider + */ + public function test_fix_null_grades($grade, $gradebookvalue) { + global $DB; + + $this->resetAfterTest(); + + $studentid = $this->students[0]->id; + + $assign = $this->create_instance(); + + // Try getting a student's grade. This will give a grade of -1. + // Then we can override it with a bad negative grade. + $assign->get_user_grade($studentid, true); + + // Set the grade to something errant. + $DB->set_field( + 'assign_grades', + 'grade', + $grade, + [ + 'userid' => $studentid, + 'assignment' => $assign->get_instance()->id, + ] + ); + $assign->grade = $grade; + $assigntemp = clone $assign->get_instance(); + $assigntemp->cmidnumber = $assign->get_course_module()->idnumber; + assign_update_grades($assigntemp); + + // Check that the gradebook was updated with the assign grade. So we can guarentee test results later on. + $expectedgrade = $grade == -1 ? null : $grade; // Assign sends null to the gradebook for -1 grades. + $gradegrade = grade_grade::fetch(array('userid' => $studentid, 'itemid' => $assign->get_grade_item()->id)); + $this->assertEquals($expectedgrade, $gradegrade->rawgrade); + + // Call fix_null_grades(). + $method = new ReflectionMethod(assign::class, 'fix_null_grades'); + $method->setAccessible(true); + $result = $method->invoke($assign); + + $this->assertSame(true, $result); + + $gradegrade = grade_grade::fetch(array('userid' => $studentid, 'itemid' => $assign->get_grade_item()->id)); + + // Check that the grade was updated in the gradebook by fix_null_grades. + $this->assertEquals($gradebookvalue, $gradegrade->finalgrade); + } } diff --git a/mod/assign/upgradelib.php b/mod/assign/upgradelib.php index c4fff77304300..10ab600920ac9 100644 --- a/mod/assign/upgradelib.php +++ b/mod/assign/upgradelib.php @@ -414,3 +414,27 @@ private function duplicate_course_module(stdClass $cm, $moduleid, $newinstanceid return $newcm; } } + +/** + * Determines if the assignment as any null grades that were rescaled. + * + * Null grades are stored as -1 but should never be rescaled. + * + * @return int[] Array of the ids of all the assignments with rescaled null grades. + */ +function get_assignments_with_rescaled_null_grades() { + global $DB; + + $query = 'SELECT id, assignment FROM {assign_grades} + WHERE grade < 0 AND grade <> -1'; + + $assignments = array_values($DB->get_records_sql($query)); + + $getassignmentid = function ($assignment) { + return $assignment->assignment; + }; + + $assignments = array_map($getassignmentid, $assignments); + + return $assignments; +} diff --git a/mod/assign/version.php b/mod/assign/version.php index 11c734888be57..5ca85f4bac4c6 100644 --- a/mod/assign/version.php +++ b/mod/assign/version.php @@ -25,6 +25,6 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'mod_assign'; // Full name of the plugin (used for diagnostics). -$plugin->version = 2017051501; // The current module version (Date: YYYYMMDDXX). +$plugin->version = 2017051502; // The current module version (Date: YYYYMMDDXX). $plugin->requires = 2017050500; // Requires this Moodle version. $plugin->cron = 60;