Skip to content

Commit

Permalink
MDL-74231 grader report: Fix case where old settings are empty / null
Browse files Browse the repository at this point in the history 
There is an edge case in the conversion from old collapse/expand
settings (stored as user preference) to the new ones that can lead
to notices / warnings / errors.

Before PHP 8, they weren't being detected, but now they are.

Basically, if some of the expected components of the preferences
are missing, then some array (array_diff(), array_intersect()..)
operations now are failing, because now those parameter functions
do require to be arrays (so empty strings or nulls aren't allowed).

Also, there were some cases where the old preference was not being
removed ever, so its removal has been moved to a common place at
the end of the function, where it's always evaluated in case
the preference still exists but is empty of contents.

I've tried to cover all the cases I've been able to imagine with
unit tests (empty information, some component not set, all components
not set...), have moved it to a new unit test because the one
already performing some tests with those old preferences was
already too long to add more cases.
  • Loading branch information
@stronk7
stronk7 committed Mar 29, 2022
1 parent 7ce003b commit 4e2b4a1
Show file tree
Hide file tree
Showing 2 changed files with 247 additions and 11 deletions.
38 changes: 27 additions & 11 deletions grade/report/grader/lib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1768,9 +1768,10 @@ public function process_action($target, $action) {
*/
protected static function filter_collapsed_categories($courseid, $collapsed) {
global $DB;
if (empty($collapsed)) {
$collapsed = array('aggregatesonly' => array(), 'gradesonly' => array());
}
// Ensure we always have an element for aggregatesonly and another for gradesonly, no matter it's empty.
$collapsed['aggregatesonly'] = $collapsed['aggregatesonly'] ?? [];
$collapsed['gradesonly'] = $collapsed['gradesonly'] ?? [];

if (empty($collapsed['aggregatesonly']) && empty($collapsed['gradesonly'])) {
return $collapsed;
}
Expand All @@ -1791,12 +1792,23 @@ protected static function filter_collapsed_categories($courseid, $collapsed) {
*/
protected static function get_collapsed_preferences($courseid) {
if ($collapsed = get_user_preferences('grade_report_grader_collapsed_categories'.$courseid)) {
return json_decode($collapsed, true);
$collapsed = json_decode($collapsed, true);
// Ensure we always have an element for aggregatesonly and another for gradesonly, no matter it's empty.
$collapsed['aggregatesonly'] = $collapsed['aggregatesonly'] ?? [];
$collapsed['gradesonly'] = $collapsed['gradesonly'] ?? [];
return $collapsed;
}

// Try looking for old location of user setting that used to store all courses in one serialized user preference.
$collapsed = ['aggregatesonly' => [], 'gradesonly' => []]; // Use this if old settings are not found.
$collapsedall = [];
$oldprefexists = false;
if (($oldcollapsedpref = get_user_preferences('grade_report_grader_collapsed_categories')) !== null) {
$oldprefexists = true;
if ($collapsedall = unserialize_array($oldcollapsedpref)) {
// Ensure we always have an element for aggregatesonly and another for gradesonly, no matter it's empty.
$collapsedall['aggregatesonly'] = $collapsedall['aggregatesonly'] ?? [];
$collapsedall['gradesonly'] = $collapsedall['gradesonly'] ?? [];
// 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'])) {
Expand All @@ -1805,17 +1817,21 @@ protected static function get_collapsed_preferences($courseid) {
$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());
}

// Arrived here, if the old pref exists and it doesn't contain
// more information, it means that the migration of all the
// data to new, by course, preferences is completed, so
// the old one can be safely deleted.
if ($oldprefexists &&
empty($collapsedall['aggregatesonly']) &&
empty($collapsedall['gradesonly'])) {
unset_user_preference('grade_report_grader_collapsed_categories');
}

return $collapsed;
}

Expand Down
220 changes: 220 additions & 0 deletions grade/tests/report_graderlib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,226 @@ public function test_collapsed_preferences() {
$this->assertEquals(count($toobigvalue['gradesonly']) - 1, count($report1->collapsed['gradesonly']));
}

/**
* Test some special cases of the conversion from old preferences to new ones
*
* @covers \grade_report_grader::get_collapsed_preferences
* @covers \grade_report_grader::filter_collapsed_categories
*/
public function test_old_collapsed_preferences() {
$this->resetAfterTest(true);

$user1 = $this->getDataGenerator()->create_user();
$course1 = $this->getDataGenerator()->create_course();
$course2 = $this->getDataGenerator()->create_course();
$course3 = $this->getDataGenerator()->create_course();

$course1cats = $course2cats = $course3cats = [];
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);
// Collapse all the cats in course1.
foreach ($course1cats as $catid) {
$report1->process_action('cg'. $catid, 'switch_minus');
}

// Expand all the cats in course2.
$report2 = $this->create_report($course2);
foreach ($course2cats as $catid) {
$report2->process_action('cg'.$catid, 'switch_minus');
$report2->process_action('cg'.$catid, 'switch_plus');
}

// Collapse odd cats and expand even cats in course3.
$report3 = $this->create_report($course3);
foreach ($course3cats as $catid) {
$report3->process_action('cg'.$catid, 'switch_minus');
if (($i++) % 2) {
$report3->process_action('cg'.$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']));

// Use the preferences generated for user1 and set it in the old format for other users.

// User2: both gradesonly and aggregatesonly.
$user2 = $this->getDataGenerator()->create_user();
$alldata = [
'gradesonly' => array_merge(
$report1->collapsed['gradesonly'],
$report2->collapsed['gradesonly'],
$report3->collapsed['gradesonly']),
'aggregatesonly' => array_merge(
$report1->collapsed['aggregatesonly'],
$report2->collapsed['aggregatesonly'],
$report3->collapsed['aggregatesonly']),
];
set_user_preference('grade_report_grader_collapsed_categories', serialize($alldata), $user2);

$this->setUser($user2);
$convertedreport1 = $this->create_report($course1);
$this->assertEquals($report1->collapsed['gradesonly'], $convertedreport1->collapsed['gradesonly']);
$this->assertEquals($report1->collapsed['aggregatesonly'], $convertedreport1->collapsed['aggregatesonly']);
$newprefs1 = get_user_preferences('grade_report_grader_collapsed_categories' . $course1->id); // Also verify new prefs.
$this->assertEquals($report1->collapsed['gradesonly'], json_decode($newprefs1, true)['gradesonly']);
$this->assertEquals($report1->collapsed['aggregatesonly'], json_decode($newprefs1, true)['aggregatesonly']);

$convertedreport2 = $this->create_report($course2);
$this->assertEquals($report2->collapsed['gradesonly'], $convertedreport2->collapsed['gradesonly']);
$this->assertEquals($report2->collapsed['aggregatesonly'], $convertedreport2->collapsed['aggregatesonly']);
$newprefs2 = get_user_preferences('grade_report_grader_collapsed_categories' . $course2->id); // Also verify new prefs.
$this->assertEquals($report2->collapsed['gradesonly'], json_decode($newprefs2, true)['gradesonly']);
$this->assertEquals($report2->collapsed['aggregatesonly'], json_decode($newprefs2, true)['aggregatesonly']);

$convertedreport3 = $this->create_report($course3);
$this->assertEquals($report3->collapsed['gradesonly'], $convertedreport3->collapsed['gradesonly']);
$this->assertEquals($report3->collapsed['aggregatesonly'], $convertedreport3->collapsed['aggregatesonly']);
$newprefs3 = get_user_preferences('grade_report_grader_collapsed_categories' . $course3->id); // Also verify new prefs.
$this->assertEquals($report3->collapsed['gradesonly'], json_decode($newprefs3, true)['gradesonly']);
$this->assertEquals($report3->collapsed['aggregatesonly'], json_decode($newprefs3, true)['aggregatesonly']);

// Make sure the old style user preference is removed now.
$this->assertEmpty(get_user_preferences('grade_report_grader_collapsed_categories'));

// User3: only gradesonly (missing aggregatesonly).
$user3 = $this->getDataGenerator()->create_user();
$alldata = [
'gradesonly' => array_merge(
$report1->collapsed['gradesonly'],
$report2->collapsed['gradesonly'],
$report3->collapsed['gradesonly']),
];
set_user_preference('grade_report_grader_collapsed_categories', serialize($alldata), $user3);

$this->setUser($user3);
$convertedreport1 = $this->create_report($course1);
$this->assertEquals($report1->collapsed['gradesonly'], $convertedreport1->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport1->collapsed['aggregatesonly']);
$newprefs1 = get_user_preferences('grade_report_grader_collapsed_categories' . $course1->id); // Also verify new prefs.
$this->assertNull($newprefs1);

$convertedreport2 = $this->create_report($course2);
$this->assertEquals($report2->collapsed['gradesonly'], $convertedreport2->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport2->collapsed['aggregatesonly']);
$newprefs2 = get_user_preferences('grade_report_grader_collapsed_categories' . $course2->id); // Also verify new prefs.
$this->assertEquals($report2->collapsed['gradesonly'], json_decode($newprefs2, true)['gradesonly']);
$this->assertEquals([], json_decode($newprefs2, true)['aggregatesonly']);

$convertedreport3 = $this->create_report($course3);
$this->assertEquals($report3->collapsed['gradesonly'], $convertedreport3->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport3->collapsed['aggregatesonly']);
$newprefs3 = get_user_preferences('grade_report_grader_collapsed_categories' . $course3->id); // Also verify new prefs.
$this->assertEquals($report3->collapsed['gradesonly'], json_decode($newprefs3, true)['gradesonly']);
$this->assertEquals([], json_decode($newprefs3, true)['aggregatesonly']);

// Make sure the old style user preference is removed now.
$this->assertEmpty(get_user_preferences('grade_report_grader_collapsed_categories'));

// User4: only aggregatesonly (missing gradesonly).
$user4 = $this->getDataGenerator()->create_user();
$alldata = [
'aggregatesonly' => array_merge(
$report1->collapsed['aggregatesonly'],
$report2->collapsed['aggregatesonly'],
$report3->collapsed['aggregatesonly']),
];
set_user_preference('grade_report_grader_collapsed_categories', serialize($alldata), $user4);

$this->setUser($user4);
$convertedreport1 = $this->create_report($course1);
$this->assertEquals([], $convertedreport1->collapsed['gradesonly']);
$this->assertEquals($report1->collapsed['aggregatesonly'], $convertedreport1->collapsed['aggregatesonly']);
$newprefs1 = get_user_preferences('grade_report_grader_collapsed_categories' . $course1->id); // Also verify new prefs.
$this->assertEquals([], json_decode($newprefs1, true)['gradesonly']);
$this->assertEquals($report1->collapsed['aggregatesonly'], json_decode($newprefs1, true)['aggregatesonly']);

$convertedreport2 = $this->create_report($course2);
$this->assertEquals([], $convertedreport2->collapsed['gradesonly']);
$this->assertEquals($report2->collapsed['aggregatesonly'], $convertedreport2->collapsed['aggregatesonly']);
$newprefs2 = get_user_preferences('grade_report_grader_collapsed_categories' . $course2->id); // Also verify new prefs.
$this->assertNull($newprefs2);

$convertedreport3 = $this->create_report($course3);
$this->assertEquals([], $convertedreport3->collapsed['gradesonly']);
$this->assertEquals($report3->collapsed['aggregatesonly'], $convertedreport3->collapsed['aggregatesonly']);
$newprefs3 = get_user_preferences('grade_report_grader_collapsed_categories' . $course3->id); // Also verify new prefs.
$this->assertEquals([], json_decode($newprefs3, true)['gradesonly']);
$this->assertEquals($report3->collapsed['aggregatesonly'], json_decode($newprefs3, true)['aggregatesonly']);

// Make sure the old style user preference is removed now.
$this->assertEmpty(get_user_preferences('grade_report_grader_collapsed_categories'));

// User5: both missing gradesonly and aggregatesonly.
$user5 = $this->getDataGenerator()->create_user();
$alldata = [];
set_user_preference('grade_report_grader_collapsed_categories', serialize($alldata), $user5);

$this->setUser($user5);
$convertedreport1 = $this->create_report($course1);
$this->assertEquals([], $convertedreport1->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport1->collapsed['aggregatesonly']);
$newprefs1 = get_user_preferences('grade_report_grader_collapsed_categories' . $course1->id); // Also verify new prefs.
$this->assertNull($newprefs1);

$convertedreport2 = $this->create_report($course2);
$this->assertEquals([], $convertedreport2->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport2->collapsed['aggregatesonly']);
$newprefs2 = get_user_preferences('grade_report_grader_collapsed_categories' . $course2->id); // Also verify new prefs.
$this->assertNull($newprefs2);

$convertedreport3 = $this->create_report($course3);
$this->assertEquals([], $convertedreport3->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport3->collapsed['aggregatesonly']);
$newprefs3 = get_user_preferences('grade_report_grader_collapsed_categories' . $course3->id); // Also verify new prefs.
$this->assertNull($newprefs3);

// Make sure the old style user preference is removed now.
$this->assertEmpty(get_user_preferences('grade_report_grader_collapsed_categories'));

// User6: both empty gradesonly and aggregatesonly.
$user6 = $this->getDataGenerator()->create_user();
$alldata = [
'gradesonly' => [],
'aggregatesonly' => []
];
set_user_preference('grade_report_grader_collapsed_categories', serialize($alldata), $user6);

$this->setUser($user6);
$convertedreport1 = $this->create_report($course1);
$this->assertEquals([], $convertedreport1->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport1->collapsed['aggregatesonly']);
$newprefs1 = get_user_preferences('grade_report_grader_collapsed_categories' . $course1->id); // Also verify new prefs.
$this->assertNull($newprefs1);

$convertedreport2 = $this->create_report($course2);
$this->assertEquals([], $convertedreport2->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport2->collapsed['aggregatesonly']);
$newprefs2 = get_user_preferences('grade_report_grader_collapsed_categories' . $course2->id); // Also verify new prefs.
$this->assertNull($newprefs2);

$convertedreport3 = $this->create_report($course3);
$this->assertEquals([], $convertedreport3->collapsed['gradesonly']);
$this->assertEquals([], $convertedreport3->collapsed['aggregatesonly']);
$newprefs3 = get_user_preferences('grade_report_grader_collapsed_categories' . $course3->id); // Also verify new prefs.
$this->assertNull($newprefs3);

// Make sure the old style user preference is removed now.
$this->assertEmpty(get_user_preferences('grade_report_grader_collapsed_categories'));
}

/**
* Tests the get_right_rows function with one 'normal' and one 'ungraded' quiz.
*
Expand Down

0 comments on commit 4e2b4a1

Please sign in to comment.