From 8238f53d710a172b925cc5cdf66871023f8ada53 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Fri, 1 Aug 2014 16:05:31 +0800 Subject: [PATCH] MDL-30668 gradereport_grader: split user preferences into individual courses This solves the DB exception when users were trying to insert very large string in DB --- grade/report/grader/index.php | 2 +- grade/report/grader/lib.php | 131 +++++++++++++++++++++--- grade/tests/report_graderlib_test.php | 137 +++++++++++++++++++++++++- 3 files changed, 252 insertions(+), 18 deletions(-) diff --git a/grade/report/grader/index.php b/grade/report/grader/index.php index 57b4b910a35..435fb5734d1 100644 --- a/grade/report/grader/index.php +++ b/grade/report/grader/index.php @@ -107,7 +107,7 @@ grade_regrade_final_grades($courseid); // Perform actions if (!empty($target) && !empty($action) && confirm_sesskey()) { - grade_report_grader::do_process_action($target, $action); + grade_report_grader::do_process_action($target, $action, $courseid); } $reportname = get_string('pluginname', 'gradereport_grader'); diff --git a/grade/report/grader/lib.php b/grade/report/grader/lib.php index 56d3bf11388..8a6d67beaa8 100644 --- a/grade/report/grader/lib.php +++ b/grade/report/grader/lib.php @@ -110,11 +110,7 @@ class grade_report_grader extends grade_report { $this->canviewhidden = has_capability('moodle/grade:viewhidden', context_course::instance($this->course->id)); // load collapsed settings for this report - if ($collapsed = get_user_preferences('grade_report_grader_collapsed_categories')) { - $this->collapsed = unserialize($collapsed); - } else { - $this->collapsed = array('aggregatesonly' => array(), 'gradesonly' => array()); - } + $this->collapsed = static::get_collapsed_preferences($this->course->id); if (empty($CFG->enableoutcomes)) { $nooutcomes = false; @@ -1535,32 +1531,139 @@ class grade_report_grader extends grade_report { } public function process_action($target, $action) { - return self::do_process_action($target, $action); + return self::do_process_action($target, $action, $this->course->id); + } + + /** + * From the list of categories that this user prefers to collapse choose ones that belong to the current course. + * + * This function serves two purposes. + * Mainly it helps migrating from user preference style when all courses were stored in one preference. + * Also it helps to remove the settings for categories that were removed if the array for one course grows too big. + * + * @param int $courseid + * @param array $collapsed + * @return array + */ + protected static function filter_collapsed_categories($courseid, $collapsed) { + global $DB; + if (empty($collapsed)) { + $collapsed = array('aggregatesonly' => array(), 'gradesonly' => array()); + } + if (empty($collapsed['aggregatesonly']) && empty($collapsed['gradesonly'])) { + return $collapsed; + } + $cats = $DB->get_fieldset_select('grade_categories', 'id', 'courseid = ?', array($courseid)); + $collapsed['aggregatesonly'] = array_values(array_intersect($collapsed['aggregatesonly'], $cats)); + $collapsed['gradesonly'] = array_values(array_intersect($collapsed['gradesonly'], $cats)); + return $collapsed; + } + + /** + * Returns the list of categories that this user wants to collapse or display aggregatesonly + * + * This method also migrates on request from the old format of storing user preferences when they were stored + * in one preference for all courses causing DB error when trying to insert very big value. + * + * @param int $courseid + * @return array + */ + protected static function get_collapsed_preferences($courseid) { + if ($collapsed = get_user_preferences('grade_report_grader_collapsed_categories'.$courseid)) { + return json_decode($collapsed, true); + } + + // Try looking for old location of user setting that used to store all courses in one serialized user preference. + if (($oldcollapsedpref = get_user_preferences('grade_report_grader_collapsed_categories')) !== null) { + if ($collapsedall = @unserialize($oldcollapsedpref)) { + // We found the old-style preference, filter out only categories that belong to this course and update the prefs. + $collapsed = static::filter_collapsed_categories($courseid, $collapsedall); + if (!empty($collapsed['aggregatesonly']) || !empty($collapsed['gradesonly'])) { + static::set_collapsed_preferences($courseid, $collapsed); + $collapsedall['aggregatesonly'] = array_diff($collapsedall['aggregatesonly'], $collapsed['aggregatesonly']); + $collapsedall['gradesonly'] = array_diff($collapsedall['gradesonly'], $collapsed['gradesonly']); + if (!empty($collapsedall['aggregatesonly']) || !empty($collapsedall['gradesonly'])) { + set_user_preference('grade_report_grader_collapsed_categories', serialize($collapsedall)); + } else { + unset_user_preference('grade_report_grader_collapsed_categories'); + } + } + } else { + // We found the old-style preference, but it is unreadable, discard it. + unset_user_preference('grade_report_grader_collapsed_categories'); + } + } else { + $collapsed = array('aggregatesonly' => array(), 'gradesonly' => array()); + } + return $collapsed; + } + + /** + * Sets the list of categories that user wants to see collapsed in user preferences + * + * This method may filter or even trim the list if it does not fit in DB field. + * + * @param int $courseid + * @param array $collapsed + */ + protected static function set_collapsed_preferences($courseid, $collapsed) { + global $DB; + // In an unlikely case that the list of collapsed categories for one course is too big for the user preference size, + // try to filter the list of categories since array may contain categories that were deleted. + if (strlen(json_encode($collapsed)) >= 1333) { + $collapsed = static::filter_collapsed_categories($courseid, $collapsed); + } + + // If this did not help, "forget" about some of the collapsed categories. Still better than to loose all information. + while (strlen(json_encode($collapsed)) >= 1333) { + if (count($collapsed['aggregatesonly'])) { + array_pop($collapsed['aggregatesonly']); + } + if (count($collapsed['gradesonly'])) { + array_pop($collapsed['gradesonly']); + } + } + + if (!empty($collapsed['aggregatesonly']) || !empty($collapsed['gradesonly'])) { + set_user_preference('grade_report_grader_collapsed_categories'.$courseid, json_encode($collapsed)); + } else { + unset_user_preference('grade_report_grader_collapsed_categories'.$courseid); + } } /** * Processes a single action against a category, grade_item or grade. * @param string $target eid ({type}{id}, e.g. c4 for category4) * @param string $action Which action to take (edit, delete etc...) + * @param int $courseid affected course. * @return */ - public static function do_process_action($target, $action) { + public static function do_process_action($target, $action, $courseid = null) { + global $DB; // TODO: this code should be in some grade_tree static method $targettype = substr($target, 0, 1); $targetid = substr($target, 1); // TODO: end - if ($collapsed = get_user_preferences('grade_report_grader_collapsed_categories')) { - $collapsed = unserialize($collapsed); - } else { - $collapsed = array('aggregatesonly' => array(), 'gradesonly' => array()); + if ($targettype !== 'c') { + // The following code only works with categories. + return true; } + if (!$courseid) { + // Absence of argument $courseid will display debugging message in 2.8. + if (!$courseid = $DB->get_field('grade_categories', 'courseid', array('id' => $targetid), IGNORE_MISSING)) { + return true; + } + } + + $collapsed = static::get_collapsed_preferences($courseid); + switch ($action) { case 'switch_minus': // Add category to array of aggregatesonly if (!in_array($targetid, $collapsed['aggregatesonly'])) { $collapsed['aggregatesonly'][] = $targetid; - set_user_preference('grade_report_grader_collapsed_categories', serialize($collapsed)); + static::set_collapsed_preferences($courseid, $collapsed); } break; @@ -1572,13 +1675,13 @@ class grade_report_grader extends grade_report { if (!in_array($targetid, $collapsed['gradesonly'])) { $collapsed['gradesonly'][] = $targetid; } - set_user_preference('grade_report_grader_collapsed_categories', serialize($collapsed)); + static::set_collapsed_preferences($courseid, $collapsed); break; case 'switch_whole': // Remove the category from the array of collapsed cats $key = array_search($targetid, $collapsed['gradesonly']); if ($key !== false) { unset($collapsed['gradesonly'][$key]); - set_user_preference('grade_report_grader_collapsed_categories', serialize($collapsed)); + static::set_collapsed_preferences($courseid, $collapsed); } break; diff --git a/grade/tests/report_graderlib_test.php b/grade/tests/report_graderlib_test.php index f3fd1ee6a16..9faf37eab03 100644 --- a/grade/tests/report_graderlib_test.php +++ b/grade/tests/report_graderlib_test.php @@ -45,7 +45,6 @@ class core_grade_report_graderlib_testcase extends advanced_testcase { $this->resetAfterTest(true); $course = $this->getDataGenerator()->create_course(); - $coursecontext = context_course::instance($course->id); // Create and enrol a student. $student = $this->getDataGenerator()->create_user(array('username' => 'Student Sam')); @@ -60,7 +59,7 @@ class core_grade_report_graderlib_testcase extends advanced_testcase { // Switch the stdClass instance for a grade item instance. $forum1 = grade_item::fetch(array('itemtype' => 'mod', 'itemmodule' => 'forum', 'iteminstance' => $forum1->id, 'courseid' => $course->id)); - $report = $this->create_report($course, $coursecontext); + $report = $this->create_report($course); $testgrade = 60.00; $data = new stdClass(); @@ -106,8 +105,140 @@ class core_grade_report_graderlib_testcase extends advanced_testcase { $this->assertEquals($studentgrade->finalgrade, $toobig); } - private function create_report($course, $coursecontext) { + public function test_collapsed_preferences() { + $this->resetAfterTest(true); + $emptypreferences = array('aggregatesonly' => array(), 'gradesonly' => array()); + + $user1 = $this->getDataGenerator()->create_user(); + $course1 = $this->getDataGenerator()->create_course(); + $course2 = $this->getDataGenerator()->create_course(); + $course3 = $this->getDataGenerator()->create_course(); + + $this->setUser($user1); + + $report = $this->create_report($course1); + $this->assertEquals($emptypreferences, $report->collapsed); + + // Validating preferences set/get for one course. + $report->process_action('c13', 'switch_minus'); + $report = $this->create_report($course1); + $this->assertEquals(array(13), $report->collapsed['aggregatesonly']); + $this->assertEmpty($report->collapsed['gradesonly']); + + $report->process_action('c13', 'switch_plus'); + $report = $this->create_report($course1); + $this->assertEmpty($report->collapsed['aggregatesonly']); + $this->assertEquals(array(13), $report->collapsed['gradesonly']); + + $report->process_action('c13', 'switch_whole'); + $report = $this->create_report($course1); + $this->assertEquals($emptypreferences, $report->collapsed); + + // Validating preferences set/get for several courses. + + $course1cats = $course2cats = $course3cats = array(); + for ($i=0;$i<10;$i++) { + $course1cats[] = $this->create_grade_category($course1)->id; + $course2cats[] = $this->create_grade_category($course2)->id; + $course3cats[] = $this->create_grade_category($course3)->id; + } + + $report1 = $this->create_report($course1); + foreach ($course1cats as $catid) { + $report1->process_action('c'.$catid, 'switch_minus'); + } + $report2 = $this->create_report($course2); + foreach ($course2cats as $catid) { + $report2->process_action('c'.$catid, 'switch_minus'); + $report2->process_action('c'.$catid, 'switch_plus'); + } + $report3 = $this->create_report($course3); + foreach ($course3cats as $catid) { + $report3->process_action('c'.$catid, 'switch_minus'); + if (($i++)%2) { + $report3->process_action('c'.$catid, 'switch_plus'); + } + } + + $report1 = $this->create_report($course1); + $this->assertEquals(10, count($report1->collapsed['aggregatesonly'])); + $this->assertEquals(0, count($report1->collapsed['gradesonly'])); + $report2 = $this->create_report($course2); + $this->assertEquals(0, count($report2->collapsed['aggregatesonly'])); + $this->assertEquals(10, count($report2->collapsed['gradesonly'])); + $report3 = $this->create_report($course3); + $this->assertEquals(5, count($report3->collapsed['aggregatesonly'])); + $this->assertEquals(5, count($report3->collapsed['gradesonly'])); + + // Test upgrade script. + // Combine data generated for user1 and set it in the old format for user2, Try to retrieve it and make sure it is converted. + + $user2 = $this->getDataGenerator()->create_user(); + $alldata = array( + 'aggregatesonly' => array_merge($report1->collapsed['aggregatesonly'], $report2->collapsed['aggregatesonly'], $report3->collapsed['aggregatesonly']), + 'gradesonly' => array_merge($report1->collapsed['gradesonly'], $report2->collapsed['gradesonly'], $report3->collapsed['gradesonly']), + ); + set_user_preference('grade_report_grader_collapsed_categories', serialize($alldata), $user2); + + $this->setUser($user2); + $convertedreport1 = $this->create_report($course1); + $this->assertEquals($report1->collapsed, $convertedreport1->collapsed); + $convertedreport2 = $this->create_report($course2); + $this->assertEquals($report2->collapsed, $convertedreport2->collapsed); + $convertedreport3 = $this->create_report($course3); + $this->assertEquals($report3->collapsed, $convertedreport3->collapsed); + // Make sure the old style user preference is removed now. + $this->assertEmpty(get_user_preferences('grade_report_grader_collapsed_categories')); + + // Test overflowing the setting with non-existing categories (only validated if new setting size exceeds 1333 chars). + + $toobigvalue = $expectedvalue = $report1->collapsed; + for ($i = 0; strlen(json_encode($toobigvalue)) < 1333; $i++) { + $toobigvalue[($i < 7) ? 'gradesonly' : 'aggregatesonly'][] = $course1cats[9] + 1 + $i; + } + $lastvalue = array_pop($toobigvalue['gradesonly']); + set_user_preference('grade_report_grader_collapsed_categories'.$course1->id, json_encode($toobigvalue)); + + $report1 = $this->create_report($course1); + $report1->process_action('c'.$lastvalue, 'switch_minus'); + + $report1 = $this->create_report($course1); + $this->assertEquals($expectedvalue, $report1->collapsed); + + // Test overflowing the setting with existing categories. + + $toobigvalue = $report1->collapsed; + for ($i = 0; strlen(json_encode($toobigvalue)) < 1333; $i++) { + $catid = $this->create_grade_category($course1)->id; + $toobigvalue[($i < 7) ? 'gradesonly' : 'aggregatesonly'][] = $catid; + } + $lastcatid = array_pop($toobigvalue['gradesonly']); + set_user_preference('grade_report_grader_collapsed_categories'.$course1->id, json_encode($toobigvalue)); + $toobigvalue['aggregatesonly'][] = $lastcatid; + + $report1 = $this->create_report($course1); + $report1->process_action('c'.$lastcatid, 'switch_minus'); + + // One last value should be removed from both arrays. + $report1 = $this->create_report($course1); + $this->assertEquals(count($toobigvalue['aggregatesonly']) - 1, count($report1->collapsed['aggregatesonly'])); + $this->assertEquals(count($toobigvalue['gradesonly']) - 1, count($report1->collapsed['gradesonly'])); + } + + private function create_grade_category($course) { + static $cnt = 0; + $cnt++; + $grade_category = new grade_category(array('courseid' => $course->id, 'fullname' => 'Cat '.$cnt), false); + $grade_category->apply_default_settings(); + $grade_category->apply_forced_settings(); + $grade_category->insert(); + return $grade_category; + } + + private function create_report($course) { + + $coursecontext = context_course::instance($course->id); $gpr = new grade_plugin_return(array('type' => 'report', 'plugin'=>'grader', 'courseid' => $course->id)); $report = new grade_report_grader($course->id, $gpr, $coursecontext);