Merge branch 'MDL-56646-33' of https://github.com/xow/moodle into MOO…

…DLE_33_STABLE

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

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;
 }

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 <a href="{$a->link}">automatically repair</a> 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}

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;

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;
+    }
 }
 
 /**

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 "."

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);
+    }
 }

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);
+    }
 }