Skip to content

Commit

Permalink
MDL-75878 Course: Adding graded activity too slow when lots of grades
Browse files Browse the repository at this point in the history
Adding an activity with a grade item previously triggered a course
regrade, which can be extremely slow. This change makes it so that on
courses where regrading is slow, when you add an activity that
requires regrading, it takes you to an intermediate screen with a
progress bar.

This change also fixes a bug in the existing grade overview report,
which shows grades across multiple courses, but previously only
checked, and if necessary regraded, one course.
  • Loading branch information
sammarshallou committed Jan 3, 2023
1 parent 12e9d9e commit fbd61d1
Show file tree
Hide file tree
Showing 8 changed files with 243 additions and 11 deletions.
20 changes: 15 additions & 5 deletions course/modedit.php
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@
redirect(course_get_url($course, $cw->section, array('sr' => $sectionreturn)));
}
} else if ($fromform = $mform->get_data()) {
// Mark that this is happening in the front-end UI. This is used to indicate that we are able to
// do regrading with a progress bar and redirect, if necessary.
$fromform->frontend = true;
if (!empty($fromform->update)) {
list($cm, $fromform) = update_moduleinfo($cm, $fromform, $course, $mform);
} else if (!empty($fromform->add)) {
Expand All @@ -176,14 +179,21 @@

if (isset($fromform->submitbutton)) {
$url = new moodle_url("/mod/$module->name/view.php", array('id' => $fromform->coursemodule, 'forceview' => 1));
if (empty($fromform->showgradingmanagement)) {
redirect($url);
} else {
redirect($fromform->gradingman->get_management_url($url));
if (!empty($fromform->showgradingmanagement)) {
$url = $fromform->gradingman->get_management_url($url);
}
} else {
redirect(course_get_url($course, $cw->section, array('sr' => $sectionreturn)));
$url = course_get_url($course, $cw->section, array('sr' => $sectionreturn));
}

// If we need to regrade the course with a progress bar as a result of updating this module,
// redirect first to the page that will do this.
if (isset($fromform->needsfrontendregrade)) {
$url = new moodle_url('/course/modregrade.php', ['id' => $fromform->coursemodule,
'url' => $url->out_as_local_url(false)]);
}

redirect($url);
exit;

} else {
Expand Down
15 changes: 14 additions & 1 deletion course/modlib.php
Original file line number Diff line number Diff line change
Expand Up @@ -388,8 +388,21 @@ function edit_module_post_actions($moduleinfo, $course) {

\course_modinfo::purge_course_module_cache($course->id, $moduleinfo->coursemodule);
rebuild_course_cache($course->id, true, true);

if ($hasgrades) {
grade_regrade_final_grades($course->id);
// If regrading will be slow, and this is happening in response to front-end UI...
if (!empty($moduleinfo->frontend) && grade_needs_regrade_progress_bar($course->id)) {
// And if it actually needs regrading...
$courseitem = grade_item::fetch_course_item($course->id);
if ($courseitem->needsupdate) {
// Then don't do it as part of this form save, do it on an extra web request with a
// progress bar.
$moduleinfo->needsfrontendregrade = true;
}
} else {
// Regrade now.
grade_regrade_final_grades($course->id);
}
}

// To be removed (deprecated) with MDL-67526 (both lines).
Expand Down
60 changes: 60 additions & 0 deletions course/modregrade.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

/**
* Regrades a module that has been added or updated when it might take a long time.
*
* A progress bar will be displayed in that case, and then the user can click 'Continue' to proceed.
* If for some reason you get to this page when regrading will not take a long time, then it will
* redirect immediately.
*
* @package core_course
* @copyright 2022 The Open University
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/

define('NO_OUTPUT_BUFFERING', true);
require('../config.php');
require_once($CFG->libdir . '/gradelib.php');

$cmid = required_param('id', PARAM_INT);
$urlpath = required_param('url', PARAM_LOCALURL);
$url = new moodle_url($urlpath);

// Check that an absolute url/path was specified e.g. /course/view.php (it probably isn't a
// security risk to allow a relative one, but isn't necessary here).
if (!$url->get_host()) {
throw new moodle_exception('missingparam', 'error', '', 'url');
}

// Get course.
[$course, $cm] = get_course_and_cm_from_cmid($cmid);
$context = \context_module::instance($cm->id);

// Set up page for display.
$PAGE->set_url(new moodle_url('/course/modregrade.php', ['id' => $cmid, 'url' => $url]));
$PAGE->set_context($context);
$PAGE->set_title($course->shortname . ': ' . get_string('recalculatinggrades', 'grades'));

// Security check: must be logged in with manage activities permission.
require_login($course, false, $cm);
require_capability('moodle/course:manageactivities', $context);

// Do the regrade if necessary.
grade_regrade_final_grades_if_required($course);

// Redirect back to the target URL.
redirect($url);
5 changes: 3 additions & 2 deletions grade/report/overview/classes/external.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,13 @@ public static function get_course_grades($userid = 0) {
$course = get_course(SITEID);
$context = context_course::instance($course->id);

// Force a regrade if required.
grade_regrade_final_grades_if_required($course);
// Get the course final grades now.
$gpr = new grade_plugin_return(array('type' => 'report', 'plugin' => 'overview', 'courseid' => $course->id,
'userid' => $userid));
$report = new grade_report_overview($userid, $gpr, $context);
// Force a regrade if required. As this is the overview report, we need to do all courses
// the user is enrolled in, not just $course.
$report->regrade_all_courses_if_needed();
$coursesgrades = $report->setup_courses_data(true);

$grades = array();
Expand Down
6 changes: 3 additions & 3 deletions grade/report/overview/index.php
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,6 @@
}
$USER->grade_last_report[$course->id] = 'overview';

// First make sure we have proper final grades.
grade_regrade_final_grades_if_required($course);

$actionbar = new \core_grades\output\general_action_bar($context,
new moodle_url('/grade/report/overview/index.php', ['id' => $courseid]), 'report', 'overview');

Expand Down Expand Up @@ -126,6 +123,9 @@

} else { // Only show one user's report
$report = new grade_report_overview($userid, $gpr, $context);
// Make sure we have proper final grades - this report shows grades from other courses, not just the
// selected one, so we need to check and regrade all courses the user is enrolled in.
$report->regrade_all_courses_if_needed(true);
print_grade_page_head($courseid, 'report', 'overview', get_string('pluginname', 'gradereport_overview') .
' - ' . fullname($report->user), false, false, true, null, null,
$report->user, $actionbar);
Expand Down
19 changes: 19 additions & 0 deletions grade/report/overview/lib.php
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,25 @@ public function __construct($userid, $gpr, $context) {
$this->setup_table();
}

/**
* Regrades all courses if needed.
*
* If $frontend is true, this may show a progress bar and redirect back to the page (possibly
* several times if multiple courses need it). Otherwise, it will not return until all the
* courses have been updated.
*
* @param bool $frontend True if we are running front-end code and can safely redirect back
*/
public function regrade_all_courses_if_needed(bool $frontend = false): void {
foreach ($this->courses as $course) {
if ($frontend) {
grade_regrade_final_grades_if_required($course);
} else {
grade_regrade_final_grades($course->id);
}
}
}

/**
* Prepares the headers and attributes of the flexitable.
*/
Expand Down
125 changes: 125 additions & 0 deletions grade/report/overview/tests/lib_test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

namespace gradereport_overview;

/**
* Overview grade report lib functions unit tests
*
* @package gradereport_overview
* @copyright 2023 The Open University
* @covers \grade_report_overview
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class lib_test extends \externallib_advanced_testcase {

/**
* Require the library file we're about to test, and other requirements.
*/
protected function setUp(): void {
global $CFG;
require_once($CFG->dirroot . '/grade/report/overview/lib.php');
require_once($CFG->dirroot . '/grade/querylib.php');
}

/**
* Data provider for true or false value.
*
* @return array Two options, one with true and one with false
*/
public function true_or_false(): array {
return [
[true],
[false]
];
}

/**
* Tests the regrade_all_courses_if_needed function, which is supposed to regrade all the
* courses that the current user is enrolled on (if they need update).
*
* This is tested with both frontend and backend - the frontend option should not actually
* do the progress bar/continue button (which can't be tested from here because it calls exit)
* because these courses are small.
*
* @dataProvider true_or_false
* @param bool $frontend True to use the front-end parameter to the function under test
*/
public function test_regrade_all_courses_if_needed(bool $frontend): void {
global $DB;
$this->resetAfterTest(true);

$generator = $this->getDataGenerator();

// Create 3 courses and a test user. The test user belongs to 2 of them, while another user
// belongs to the other course.
$user = $generator->create_user();
$otheruser = $generator->create_user();
$course1 = $generator->create_course();
$generator->enrol_user($user->id, $course1->id, 'student');
$course2 = $generator->create_course();
$generator->enrol_user($otheruser->id, $course2->id, 'student');
$course3 = $generator->create_course();
$generator->enrol_user($user->id, $course3->id, 'student');

// We need permission to create grades (even though it's a data generator).
$this->setAdminUser();

// Set up each course grade.
foreach ([$course1, $course2, $course3] as $course) {
// Create an assignment and get its grade item.
$assign = $generator->create_module('assign', ['course' => $course->id]);
$modinfo = get_fast_modinfo($course->id);
$cm = $modinfo->get_cm($assign->cmid);
$items = grade_get_grade_items_for_activity($cm, true);
$item = reset($items);

// Set a grade in the assignment, either for the normal test user or the other user
// depending on course.
if ($course === $course2) {
$userid = $otheruser->id;
} else {
$userid = $user->id;
}
$generator->create_grade_grade([
'itemid' => $item->id,
'userid' => $userid,
'teamsubmission' => false,
'attemptnumber' => 0,
'grade' => 50
]);

// Bodge the final grade so that it needs regrading and is set wrong.
$course->gradeitemid = $DB->get_field('grade_items', 'id',
['courseid' => $course->id, 'itemtype' => 'course'], MUST_EXIST);
$DB->set_field('grade_items', 'needsupdate', 1, ['id' => $course->gradeitemid]);
$DB->set_field('grade_grades', 'finalgrade', 25.0, ['itemid' => $course->gradeitemid]);
}

// Construct the overview report and call regrade_all_courses_if_needed.
$gpr = new \stdClass();
$report = new \grade_report_overview($user->id, $gpr, '');
$report->regrade_all_courses_if_needed($frontend);

// This should have regraded courses 1 and 3, but not 2 (because the user doesn't belong).
$this->assertEqualsWithDelta(50.0, $DB->get_field('grade_grades', 'finalgrade',
['itemid' => $course1->gradeitemid]), 1.0);
$this->assertEqualsWithDelta(25.0, $DB->get_field('grade_grades', 'finalgrade',
['itemid' => $course2->gradeitemid]), 1.0);
$this->assertEqualsWithDelta(50.0, $DB->get_field('grade_grades', 'finalgrade',
['itemid' => $course3->gradeitemid]), 1.0);
}
}
4 changes: 4 additions & 0 deletions lib/gradelib.php
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,10 @@ function grade_needs_regrade_final_grades($courseid) {
function grade_needs_regrade_progress_bar($courseid) {
global $DB;
$grade_items = grade_item::fetch_all(array('courseid' => $courseid));
if (!$grade_items) {
// If there are no grade items then we definitely don't need a progress bar!
return false;
}

list($sql, $params) = $DB->get_in_or_equal(array_keys($grade_items), SQL_PARAMS_NAMED, 'gi');
$gradecount = $DB->count_records_select('grade_grades', 'itemid ' . $sql, $params);
Expand Down

0 comments on commit fbd61d1

Please sign in to comment.