From f1f5ba36ad7893e3fcc98a54804e1dbd22c3bea8 Mon Sep 17 00:00:00 2001 From: Marcus Boon Date: Thu, 9 Apr 2020 13:00:29 +1000 Subject: [PATCH] MDL-68388 core_grades: Use MUC for grade letters In the get_grade_letters there is a static variable that is used to cache grade letters, we should use MUC for this so that it is reset properly between unit tests. --- grade/edit/letter/index.php | 16 ++++++++ lang/en/cache.php | 1 + lib/db/caches.php | 8 ++++ lib/gradelib.php | 20 +++++----- lib/tests/gradelib_test.php | 73 +++++++++++++++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 9 deletions(-) diff --git a/grade/edit/letter/index.php b/grade/edit/letter/index.php index 9f4e0758b7f..b72e2035fab 100644 --- a/grade/edit/letter/index.php +++ b/grade/edit/letter/index.php @@ -137,6 +137,10 @@ if (!$edit) { redirect($returnurl); } else if ($data = $mform->get_data()) { + + // Make sure we are updating the cache. + $cache = cache::make('core', 'grade_letters'); + if (!$admin and empty($data->override)) { $records = $DB->get_records('grade_letters', array('contextid' => $context->id)); foreach ($records as $record) { @@ -148,6 +152,9 @@ if (!$edit) { )); $event->trigger(); } + + // Make sure we clear the cache for this context. + $cache->delete($context->id); redirect($returnurl); } @@ -222,6 +229,15 @@ if (!$edit) { } } + // Cache the changed letters. + if (!empty($letters)) { + + // For some reason, the cache saves it in the order in which they were entered + // but we really want to order them in descending order so we sort it here. + krsort($letters); + $cache->set($context->id, $letters); + } + // Delete the unused records. foreach($pool as $leftover) { $DB->delete_records('grade_letters', array('id' => $leftover->id)); diff --git a/lang/en/cache.php b/lang/en/cache.php index cb595171bf7..6c24f1b9eeb 100644 --- a/lang/en/cache.php +++ b/lang/en/cache.php @@ -78,6 +78,7 @@ $string['cachedef_recommendation_favourite_course_content_items'] = 'Recommendat $string['cachedef_repositories'] = 'Repositories instances data'; $string['cachedef_roledefs'] = 'Role definitions'; $string['cachedef_grade_categories'] = 'Grade category queries'; +$string['cachedef_grade_letters'] = 'Grade letters queries'; $string['cachedef_string'] = 'Language string cache'; $string['cachedef_tags'] = 'Tags collections and areas'; $string['cachedef_temp_tables'] = 'Temporary tables cache'; diff --git a/lib/db/caches.php b/lib/db/caches.php index a5bd37e418e..ac2fa19aa2a 100644 --- a/lib/db/caches.php +++ b/lib/db/caches.php @@ -454,4 +454,12 @@ $definitions = array( 'mode' => cache_store::MODE_APPLICATION, 'simpledata' => true, ], + + // Cache the grade letters for faster retrival. + 'grade_letters' => [ + 'mode' => cache_store::MODE_REQUEST, + 'simplekeys' => true, + 'staticacceleration' => true, + 'staticaccelerationsize' => 100 + ], ); diff --git a/lib/gradelib.php b/lib/gradelib.php index ef192981c27..02919f16ae9 100644 --- a/lib/gradelib.php +++ b/lib/gradelib.php @@ -955,14 +955,11 @@ function grade_get_letters($context=null) { return array('93'=>'A', '90'=>'A-', '87'=>'B+', '83'=>'B', '80'=>'B-', '77'=>'C+', '73'=>'C', '70'=>'C-', '67'=>'D+', '60'=>'D', '0'=>'F'); } - static $cache = array(); + $cache = cache::make('core', 'grade_letters'); + $data = $cache->get($context->id); - if (array_key_exists($context->id, $cache)) { - return $cache[$context->id]; - } - - if (count($cache) > 100) { - $cache = array(); // cache size limit + if (!empty($data)) { + return $data; } $letters = array(); @@ -978,13 +975,15 @@ function grade_get_letters($context=null) { } if (!empty($letters)) { - $cache[$context->id] = $letters; + // Cache the grade letters for this context. + $cache->set($context->id, $letters); return $letters; } } $letters = grade_get_letters(null); - $cache[$context->id] = $letters; + // Cache the grade letters for this context. + $cache->set($context->id, $letters); return $letters; } @@ -1430,6 +1429,9 @@ function remove_grade_letters($context, $showfeedback) { if ($showfeedback) { echo $OUTPUT->notification($strdeleted.' - '.get_string('letters', 'grades'), 'notifysuccess'); } + + $cache = cache::make('core', 'grade_letters'); + $cache->delete($context->id); } /** diff --git a/lib/tests/gradelib_test.php b/lib/tests/gradelib_test.php index a3e2b3d5667..737aca9b2a4 100644 --- a/lib/tests/gradelib_test.php +++ b/lib/tests/gradelib_test.php @@ -76,10 +76,28 @@ class core_gradelib_testcase extends advanced_testcase { $letter->contextid = $context->id; $DB->insert_record('grade_letters', $letter); + // Pre-warm the cache, ensure that that the letter is cached. + $cache = cache::make('core', 'grade_letters'); + + // Check that the cache is empty beforehand. + $letters = $cache->get($context->id); + $this->assertFalse($letters); + + // Call the function. + grade_get_letters($context); + + $letters = $cache->get($context->id); + $this->assertEquals(1, count($letters)); + $this->assertEquals($letter->letter, $letters['100.00000']); + remove_grade_letters($context, false); // Confirm grade letter was deleted. $this->assertEquals(0, $DB->count_records('grade_letters')); + + // Confirm grade letter is also deleted from cache. + $letters = $cache->get($context->id); + $this->assertFalse($letters); } /** @@ -228,4 +246,59 @@ class core_gradelib_testcase extends advanced_testcase { ], ]; } + + /** + * Test the caching of grade letters. + */ + public function test_get_grade_letters() { + + $this->resetAfterTest(); + + // Setup some basics. + $course = $this->getDataGenerator()->create_course(); + $context = context_course::instance($course->id); + + $cache = cache::make('core', 'grade_letters'); + $letters = $cache->get($context->id); + + // Make sure the cache is empty. + $this->assertFalse($letters); + + // Now check to see if the letters get cached. + $actual = grade_get_letters($context); + + $expected = $cache->get($context->id); + + $this->assertEquals($expected, $actual); + } + + /** + * Test custom letters. + */ + public function test_get_grade_letters_custom() { + global $DB; + + $this->resetAfterTest(); + + $course = $this->getDataGenerator()->create_course(); + $context = context_course::instance($course->id); + + $cache = cache::make('core', 'grade_letters'); + $letters = $cache->get($context->id); + + // Make sure the cache is empty. + $this->assertFalse($letters); + + // Add a grade letter to the course. + $letter = new stdClass(); + $letter->letter = 'M'; + $letter->lowerboundary = '100'; + $letter->contextid = $context->id; + $DB->insert_record('grade_letters', $letter); + + $actual = grade_get_letters($context); + $expected = $cache->get($context->id); + + $this->assertEquals($expected, $actual); + } }