MDL-45390 grades: Courses letters using system bad boundaries

Added unit tests to highlight the existing failures and adding
unset_config between each set of tests.

diff --git a/lib/db/upgradelib.php b/lib/db/upgradelib.php
index 9708aff..f618de2 100644 (file)
--- a/lib/db/upgradelib.php
+++ b/lib/db/upgradelib.php
@@ -379,39 +379,47 @@ function upgrade_course_letter_boundary($courseid = null) {
         $params['courseid'] = $courseid;
     }
 
         $params['courseid'] = $courseid;
     }
 

-    $contextselect = context_helper::get_preload_record_columns_sql('ctx');
+    // Check to see if the system letter boundaries are borked.
+    $systemcontext = context_system::instance();
+    $systemneedsfreeze = upgrade_letter_boundary_needs_freeze($systemcontext);

 
     // 3, 13, 23, 31, and 32 are the grade display types that incorporate showing letters. See lib/grade/constants/php.
 
     // 3, 13, 23, 31, and 32 are the grade display types that incorporate showing letters. See lib/grade/constants/php.

-    if (isset($CFG->grade_displaytype) && in_array($CFG->grade_displaytype, array(3, 13, 23, 31, 32))) {
-        // Check to see if the system letter boundaries are borked.
-        $systemcontext = context_system::instance();
-        if (upgrade_letter_boundary_needs_freeze($systemcontext)) {
-            // Select courses with no grade setting for display and a grade item that is using the default display,
-            // but have not altered the course letter boundary configuration. These courses are definitely affected.
-            $sql = "SELECT DISTINCT c.id AS courseid
-                      FROM {grade_items} gi
-                      JOIN {course} c ON c.id = gi.courseid
-                 LEFT JOIN {grade_settings} gs ON gs.courseid = c.id AND name = 'displaytype'
-                 LEFT JOIN (SELECT DISTINCT c.id
-                              FROM {grade_letters} gl
-                              JOIN {context} ctx ON gl.contextid = ctx.id
-                              JOIN {course} c ON ctx.instanceid = c.id
-                             WHERE ctx.contextlevel = :contextlevel) gl ON gl.id = c.id
-                     WHERE (gi.display = 0 AND (gs.value is NULL))
-                     AND gl.id is NULL $coursesql";
-            $affectedcourseids = $DB->get_recordset_sql($sql, $params);
-            foreach ($affectedcourseids as $courseid) {
-                set_config('gradebook_calculations_freeze_' . $courseid->courseid, 20160511);
-            }
-            $affectedcourseids->close();
+    $systemletters = (isset($CFG->grade_displaytype) && in_array($CFG->grade_displaytype, array(3, 13, 23, 31, 32)));
+
+    $contextselect = context_helper::get_preload_record_columns_sql('ctx');
+
+    if ($systemletters && $systemneedsfreeze) {
+        // Select courses with no grade setting for display and a grade item that is using the default display,
+        // but have not altered the course letter boundary configuration. These courses are definitely affected.
+
+        $sql = "SELECT DISTINCT c.id AS courseid
+                  FROM {grade_items} gi
+                  JOIN {course} c ON c.id = gi.courseid
+             LEFT JOIN {grade_settings} gs ON gs.courseid = c.id AND gs.name = 'displaytype'
+             LEFT JOIN (SELECT DISTINCT c.id
+                          FROM {grade_letters} gl
+                          JOIN {context} ctx ON gl.contextid = ctx.id
+                          JOIN {course} c ON ctx.instanceid = c.id
+                         WHERE ctx.contextlevel = :contextlevel) gl ON gl.id = c.id
+                 WHERE (gi.display = 0 AND (gs.value is NULL))
+                 AND gl.id is NULL $coursesql";
+        $affectedcourseids = $DB->get_recordset_sql($sql, $params);
+        foreach ($affectedcourseids as $courseid) {
+            set_config('gradebook_calculations_freeze_' . $courseid->courseid, 20160516);

         }
         }

+        $affectedcourseids->close();
+
+    }
+
+    if ($systemletters || $systemneedsfreeze) {
+

         // If the system letter boundary is okay proceed to check grade item and course grade display settings.
         $params['contextlevel2'] = CONTEXT_COURSE;
         $sql = "SELECT DISTINCT c.id AS courseid, $contextselect
                   FROM {course} c
                   JOIN {context} ctx ON ctx.instanceid = c.id AND ctx.contextlevel = :contextlevel
                   JOIN {grade_items} gi ON c.id = gi.courseid
         // If the system letter boundary is okay proceed to check grade item and course grade display settings.
         $params['contextlevel2'] = CONTEXT_COURSE;
         $sql = "SELECT DISTINCT c.id AS courseid, $contextselect
                   FROM {course} c
                   JOIN {context} ctx ON ctx.instanceid = c.id AND ctx.contextlevel = :contextlevel
                   JOIN {grade_items} gi ON c.id = gi.courseid

-             LEFT JOIN {grade_settings} gs ON c.id = gs.courseid AND name = 'displaytype'
+             LEFT JOIN {grade_settings} gs ON c.id = gs.courseid AND gs.name = 'displaytype'

              LEFT JOIN (SELECT DISTINCT c.id
                           FROM {grade_letters} gl
                           JOIN {context} ctx ON gl.contextid = ctx.id
              LEFT JOIN (SELECT DISTINCT c.id
                           FROM {grade_letters} gl
                           JOIN {context} ctx ON gl.contextid = ctx.id


@@ -422,13 +430,14 @@ function upgrade_course_letter_boundary($courseid = null) {
                     OR gl.id is NOT NULL)
                        $coursesql";
     } else {
                     OR gl.id is NOT NULL)
                        $coursesql";
     } else {

+

         // There is no site setting for letter grades. Just check the modified letter boundaries.
         $sql = "SELECT DISTINCT c.id AS courseid, $contextselect
         // There is no site setting for letter grades. Just check the modified letter boundaries.
         $sql = "SELECT DISTINCT c.id AS courseid, $contextselect

-                  FROM {grade_letters} l, {course} c
-                  JOIN {context} ctx ON ctx.instanceid = c.id AND ctx.contextlevel = :contextlevel
-                 WHERE l.contextid = ctx.id
-                   AND ctx.instanceid = c.id
-                       $coursesql";
+              FROM {grade_letters} l, {course} c
+              JOIN {context} ctx ON ctx.instanceid = c.id AND ctx.contextlevel = :contextlevel
+             WHERE l.contextid = ctx.id
+               AND ctx.instanceid = c.id
+                   $coursesql";

     }
 
     $potentialcourses = $DB->get_recordset_sql($sql, $params);
     }
 
     $potentialcourses = $DB->get_recordset_sql($sql, $params);

diff --git a/lib/tests/upgradelib_test.php b/lib/tests/upgradelib_test.php
index 3f7349e..02bc617 100644 (file)
--- a/lib/tests/upgradelib_test.php
+++ b/lib/tests/upgradelib_test.php
@@ -583,27 +583,29 @@ class core_upgradelib_testcase extends advanced_testcase {
         global $CFG, $DB;
         $this->resetAfterTest(true);
 
         global $CFG, $DB;
         $this->resetAfterTest(true);
 

+        require_once($CFG->libdir . '/db/upgradelib.php');
+

         // Create a user.
         $user = $this->getDataGenerator()->create_user();
 
         // Create some courses.
         $courses = array();
         $contexts = array();
         // Create a user.
         $user = $this->getDataGenerator()->create_user();
 
         // Create some courses.
         $courses = array();
         $contexts = array();

-        for ($i = 0; $i < 27; $i++) {
+        for ($i = 0; $i < 37; $i++) {

             $course = $this->getDataGenerator()->create_course();
             $context = context_course::instance($course->id);
             $course = $this->getDataGenerator()->create_course();
             $context = context_course::instance($course->id);

-            if (in_array($i, array(2, 5, 10, 13, 14, 19, 23, 25))) {
+            if (in_array($i, array(2, 5, 10, 13, 14, 19, 23, 25, 30, 34, 36))) {

                 // Assign good letter boundaries.
                 $this->assign_good_letter_boundary($context->id);
             }
                 // Assign good letter boundaries.
                 $this->assign_good_letter_boundary($context->id);
             }

-            if (in_array($i, array(3, 6, 11, 15, 20, 24, 26))) {
+            if (in_array($i, array(3, 6, 11, 15, 20, 24, 26, 31, 35))) {

                 // Assign bad letter boundaries.
                 $this->assign_bad_letter_boundary($context->id);
             }
 
                 // Assign bad letter boundaries.
                 $this->assign_bad_letter_boundary($context->id);
             }
 

-            if (in_array($i, array(9, 10, 11, 18, 19, 20))) {
+            if (in_array($i, array(9, 10, 11, 18, 19, 20, 29, 30, 31))) {

                 grade_set_setting($course->id, 'displaytype', '3');
                 grade_set_setting($course->id, 'displaytype', '3');

-            } else if (in_array($i, array(8, 17))) {
+            } else if (in_array($i, array(8, 17, 28))) {

                 grade_set_setting($course->id, 'displaytype', '2');
             }
 
                 grade_set_setting($course->id, 'displaytype', '2');
             }
 


@@ -614,10 +616,10 @@ class core_upgradelib_testcase extends advanced_testcase {
                               'itemmodule' => 'assign',
                               'iteminstance' => $assignrow->id,
                               'courseid' => $course->id));
                               'itemmodule' => 'assign',
                               'iteminstance' => $assignrow->id,
                               'courseid' => $course->id));

-                if (in_array($i, array(13, 14, 15, 22, 23, 24))) {
+                if (in_array($i, array(13, 14, 15, 23, 24, 34, 35, 36))) {

                     grade_item::set_properties($gi, array('display', 3));
                     $gi->update();
                     grade_item::set_properties($gi, array('display', 3));
                     $gi->update();

-                } else if (in_array($i, array(12, 21))) {
+                } else if (in_array($i, array(12, 21, 32))) {

                     grade_item::set_properties($gi, array('display', 2));
                     $gi->update();
                 }
                     grade_item::set_properties($gi, array('display', 2));
                     $gi->update();
                 }


@@ -657,7 +659,11 @@ class core_upgradelib_testcase extends advanced_testcase {
 
         // System setting for grade letter boundaries (default).
         set_config('grade_displaytype', '3');
 
         // System setting for grade letter boundaries (default).
         set_config('grade_displaytype', '3');

+        for ($i = 0; $i < 37; $i++) {
+            unset_config('gradebook_calculations_freeze_' . $courses[$i]->id);
+        }

         upgrade_course_letter_boundary();
         upgrade_course_letter_boundary();

+

         // [7] A course with no grade display settings for the course or grade items.
         $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[7]->id}));
         // [8] A course with grade display settings, but for something that isn't letters.
         // [7] A course with no grade display settings for the course or grade items.
         $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[7]->id}));
         // [8] A course with grade display settings, but for something that isn't letters.


@@ -680,6 +686,9 @@ class core_upgradelib_testcase extends advanced_testcase {
         // System setting for grade letter boundaries (custom with problem).
         $systemcontext = context_system::instance();
         $this->assign_bad_letter_boundary($systemcontext->id);
         // System setting for grade letter boundaries (custom with problem).
         $systemcontext = context_system::instance();
         $this->assign_bad_letter_boundary($systemcontext->id);

+        for ($i = 0; $i < 37; $i++) {
+            unset_config('gradebook_calculations_freeze_' . $courses[$i]->id);
+        }

         upgrade_course_letter_boundary();
 
         // [16] A course with no grade display settings for the course or grade items.
         upgrade_course_letter_boundary();
 
         // [16] A course with no grade display settings for the course or grade items.


@@ -704,14 +713,46 @@ class core_upgradelib_testcase extends advanced_testcase {
         $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[25]->id}));
         // [26] A course that is using the default display setting (letters) and altered the letter boundary with 57. Should be frozen.
         $this->assertEquals(20160516, $CFG->{'gradebook_calculations_freeze_' . $courses[26]->id});
         $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[25]->id}));
         // [26] A course that is using the default display setting (letters) and altered the letter boundary with 57. Should be frozen.
         $this->assertEquals(20160516, $CFG->{'gradebook_calculations_freeze_' . $courses[26]->id});

+
+        // System setting not showing letters.
+        set_config('grade_displaytype', '2');
+        for ($i = 0; $i < 37; $i++) {
+            unset_config('gradebook_calculations_freeze_' . $courses[$i]->id);
+        }
+        upgrade_course_letter_boundary();
+
+        // [27] A course with no grade display settings for the course or grade items.
+        $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[27]->id}));
+        // [28] A course with grade display settings, but for something that isn't letters.
+        $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[28]->id}));
+        // [29] A course with grade display settings of letters which are default.
+        $this->assertEquals(20160516, $CFG->{'gradebook_calculations_freeze_' . $courses[29]->id});
+        // [30] A course with grade display settings of letters which are not default, but not affected.
+        $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[30]->id}));
+        // [31] A course with grade display settings of letters which are not default, which will be affected.
+        $this->assertEquals(20160516, $CFG->{'gradebook_calculations_freeze_' . $courses[31]->id});
+        // [32] A grade item with display settings which are not letters.
+        $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[32]->id}));
+        // [33] All system defaults.
+        $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[33]->id}));
+        // [34] A grade item with display settings of letters which are not default, but not affected. Course uses new letter boundary setting.
+        $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[34]->id}));
+        // [35] A grade item with display settings of letters which are not default, which will be affected.
+        $this->assertEquals(20160516, $CFG->{'gradebook_calculations_freeze_' . $courses[35]->id});
+        // [36] A course with grade display settings of letters with modified and good boundary (not 57) Should not be frozen.
+        $this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $courses[36]->id}));

     }
 
     /**
      * Test upgrade_letter_boundary_needs_freeze function.
      */
     public function test_upgrade_letter_boundary_needs_freeze() {
     }
 
     /**
      * Test upgrade_letter_boundary_needs_freeze function.
      */
     public function test_upgrade_letter_boundary_needs_freeze() {

+        global $CFG;
+

         $this->resetAfterTest();
 
         $this->resetAfterTest();
 

+        require_once($CFG->libdir . '/db/upgradelib.php');
+

         $courses = array();
         $contexts = array();
         for ($i = 0; $i < 3; $i++) {
         $courses = array();
         $contexts = array();
         for ($i = 0; $i < 3; $i++) {


@@ -753,6 +794,8 @@ class core_upgradelib_testcase extends advanced_testcase {
                 array('contextid' => $contextid, 'lowerboundary' => 25.00000, 'letter' => 'D'),
                 array('contextid' => $contextid, 'lowerboundary' => 0.00000, 'letter' => 'F'),
             );
                 array('contextid' => $contextid, 'lowerboundary' => 25.00000, 'letter' => 'D'),
                 array('contextid' => $contextid, 'lowerboundary' => 0.00000, 'letter' => 'F'),
             );

+
+        $DB->delete_records('grade_letters', array('contextid' => $contextid));

         foreach ($newlettersscale as $record) {
             // There is no API to do this, so we have to manually insert into the database.
             $DB->insert_record('grade_letters', $record);
         foreach ($newlettersscale as $record) {
             // There is no API to do this, so we have to manually insert into the database.
             $DB->insert_record('grade_letters', $record);


@@ -779,6 +822,8 @@ class core_upgradelib_testcase extends advanced_testcase {
                 array('contextid' => $contextid, 'lowerboundary' => 25.00000, 'letter' => 'D'),
                 array('contextid' => $contextid, 'lowerboundary' => 0.00000, 'letter' => 'F'),
             );
                 array('contextid' => $contextid, 'lowerboundary' => 25.00000, 'letter' => 'D'),
                 array('contextid' => $contextid, 'lowerboundary' => 0.00000, 'letter' => 'F'),
             );

+
+        $DB->delete_records('grade_letters', array('contextid' => $contextid));

         foreach ($newlettersscale as $record) {
             // There is no API to do this, so we have to manually insert into the database.
             $DB->insert_record('grade_letters', $record);
         foreach ($newlettersscale as $record) {
             // There is no API to do this, so we have to manually insert into the database.
             $DB->insert_record('grade_letters', $record);