From 8408cb10509f35df76c05a2456c3c2b53a846572 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 15 Apr 2021 05:27:00 +1000 Subject: [PATCH 01/11] MDL-71410 core: Introduce override_customdata() The get_custom_data() method now calls obtain_dynamic_data() because $this->customdata can be dynamic now. --- lib/modinfolib.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/modinfolib.php b/lib/modinfolib.php index 9ada74eb416..02fe0b5f4ac 100644 --- a/lib/modinfolib.php +++ b/lib/modinfolib.php @@ -701,7 +701,7 @@ class course_modinfo { * {@link cached_cm_info} * * Stage 2 - dynamic data. - * Dynamic data is user-dependend, it is stored in request-level cache. To reset this cache + * Dynamic data is user-dependent, it is stored in request-level cache. To reset this cache * {@link get_fast_modinfo()} with $reset argument may be called. * * Dynamic data is obtained when any of the following properties/methods is requested: @@ -722,6 +722,7 @@ class course_modinfo { * - {@link cm_info::set_user_visible()} * - {@link cm_info::set_on_click()} * - {@link cm_info::set_icon_url()} + * - {@link cm_info::override_customdata()} * Any methods affecting view elements can also be set in this callback. * * Stage 3 (view data). @@ -1422,6 +1423,7 @@ class cm_info implements IteratorAggregate { * @return mixed Optional custom data stored in modinfo cache for this activity, or null if none */ private function get_custom_data() { + $this->obtain_dynamic_data(); return $this->customdata; } @@ -1679,6 +1681,16 @@ class cm_info implements IteratorAggregate { $this->onclick = $onclick; } + /** + * Overrides the value of an element in the customdata array. + * + * @param string $name The key in the customdata array + * @param mixed $value The value + */ + public function override_customdata($name, $value) { + $this->customdata[$name] = $value; + } + /** * Sets HTML that displays after link on course view page. * @param string $afterlink HTML string (empty string if none) From 7b54f5b2e6ba8e3ca4021981fcc8e339b670388b Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Fri, 23 Apr 2021 21:07:39 +1000 Subject: [PATCH 02/11] MDL-71410 mod_assign: Cache user and group overrides --- mod/assign/classes/cache/overrides.php | 110 +++++++++++++++++++++++++ mod/assign/db/caches.php | 35 ++++++++ mod/assign/lang/en/assign.php | 1 + mod/assign/locallib.php | 36 ++++++-- mod/assign/overrideedit.php | 4 + mod/assign/version.php | 2 +- 6 files changed, 181 insertions(+), 7 deletions(-) create mode 100644 mod/assign/classes/cache/overrides.php create mode 100644 mod/assign/db/caches.php diff --git a/mod/assign/classes/cache/overrides.php b/mod/assign/classes/cache/overrides.php new file mode 100644 index 00000000000..a9722b91393 --- /dev/null +++ b/mod/assign/classes/cache/overrides.php @@ -0,0 +1,110 @@ +. + +/** + * Cache data source for the assign overrides. + * + * @package mod_assign + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_assign\cache; + +use cache_definition; + +/** + * Class assign_overrides + * + * @package mod_assign + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class overrides implements \cache_data_source { + + /** @var overrides the singleton instance of this class. */ + protected static $instance = null; + + /** + * Returns an instance of the data source class that the cache can use for loading data using the other methods + * specified by this interface. + * + * @param cache_definition $definition + * @return object + */ + public static function get_instance_for_cache(cache_definition $definition): overrides { + if (is_null(self::$instance)) { + self::$instance = new overrides(); + } + return self::$instance; + } + + /** + * Loads the data for the key provided ready formatted for caching. + * + * @param string|int $key The key to load. + * @return mixed What ever data should be returned, or false if it can't be loaded. + * @throws \coding_exception + */ + public function load_for_cache($key) { + global $DB; + + [$assignid, $ug, $ugid] = explode('_', $key); + $assignid = (int) $assignid; + + switch ($ug) { + case 'u': + $userid = (int) $ugid; + $override = $DB->get_record( + 'assign_overrides', + ['assignid' => $assignid, 'userid' => $userid], + 'duedate, cutoffdate, allowsubmissionsfromdate' + ); + break; + case 'g': + $groupid = (int) $ugid; + $override = $DB->get_record( + 'assign_overrides', + ['assignid' => $assignid, 'groupid' => $groupid], + 'sortorder, duedate, cutoffdate, allowsubmissionsfromdate' + ); + break; + default: + throw new \coding_exception('Invalid cache key'); + } + + // Return null instead of false, because false will not be cached. + return $override ?: null; + } + + /** + * Loads several keys for the cache. + * + * @param array $keys An array of keys each of which will be string|int. + * @return array An array of matching data items. + */ + public function load_many_for_cache(array $keys) { + $results = []; + + foreach ($keys as $key) { + $results[] = $this->load_for_cache($key); + } + + return $results; + } +} diff --git a/mod/assign/db/caches.php b/mod/assign/db/caches.php new file mode 100644 index 00000000000..736e05a6314 --- /dev/null +++ b/mod/assign/db/caches.php @@ -0,0 +1,35 @@ +. + +/** + * Defined caches used internally by the plugin. + * + * @package mod_assign + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +defined('MOODLE_INTERNAL') || die(); + +$definitions = [ + 'overrides' => [ + 'mode' => cache_store::MODE_APPLICATION, + 'simplekeys' => true, + 'datasource' => '\mod_assign\cache\overrides', + ], +]; diff --git a/mod/assign/lang/en/assign.php b/mod/assign/lang/en/assign.php index 728a80726dd..1e0ae3bdad4 100644 --- a/mod/assign/lang/en/assign.php +++ b/mod/assign/lang/en/assign.php @@ -112,6 +112,7 @@ $string['batchsetmarkingworkflowstateforusers'] = 'Set marking workflow state fo $string['blindmarking'] = 'Anonymous submissions'; $string['blindmarkingenabledwarning'] = 'Anonymous submissions are enabled for this activity. Grades will not be added to the gradebook until student identities are revealed via the grading action menu.'; $string['blindmarking_help'] = 'Anonymous submissions hide the identity of students from markers. Anonymous submission settings will be locked once a submission or grade has been made in relation to this assignment.'; +$string['cachedef_overrides'] = 'User and group override information'; $string['calendardue'] = '{$a} is due'; $string['calendargradingdue'] = '{$a} is due to be graded'; $string['changeuser'] = 'Change user'; diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 5fdb19d6760..c2764753011 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -874,8 +874,10 @@ class assign { $conds = array('modulename' => 'assign', 'instance' => $this->get_instance()->id); if (isset($override->userid)) { $conds['userid'] = $override->userid; + $cachekey = "{$cm->instance}_u_{$override->userid}"; } else { $conds['groupid'] = $override->groupid; + $cachekey = "{$cm->instance}_g_{$override->groupid}"; } $events = $DB->get_records('event', $conds); foreach ($events as $event) { @@ -884,6 +886,7 @@ class assign { } $DB->delete_records('assign_overrides', array('id' => $overrideid)); + cache::make('mod_assign', 'overrides')->delete($cachekey); // Set the common parameters for one of the events we will be triggering. $params = array( @@ -1203,6 +1206,8 @@ class assign { } } + $purgeoverrides = false; + // Remove user overrides. if (!empty($data->reset_assign_user_overrides)) { $DB->delete_records_select('assign_overrides', @@ -1211,6 +1216,7 @@ class assign { 'component' => $componentstr, 'item' => get_string('useroverridesdeleted', 'assign'), 'error' => false); + $purgeoverrides = true; } // Remove group overrides. if (!empty($data->reset_assign_group_overrides)) { @@ -1220,6 +1226,7 @@ class assign { 'component' => $componentstr, 'item' => get_string('groupoverridesdeleted', 'assign'), 'error' => false); + $purgeoverrides = true; } // Updating dates - shift may be negative too. @@ -1237,6 +1244,8 @@ class assign { WHERE assignid =? AND cutoffdate <> 0", array($data->timeshift, $this->get_instance()->id)); + $purgeoverrides = true; + // Any changes to the list of dates that needs to be rolled should be same during course restore and course reset. // See MDL-9367. shift_course_mod_dates('assign', @@ -1248,6 +1257,10 @@ class assign { 'error'=>false); } + if ($purgeoverrides) { + cache::make('mod_assign', 'overrides')->purge(); + } + return $status; } @@ -9790,14 +9803,14 @@ function assign_process_group_deleted_in_course($courseid, $groupid = null) { if ($groupid) { $params['groupid'] = $groupid; // We just update the group that was deleted. - $sql = "SELECT o.id, o.assignid + $sql = "SELECT o.id, o.assignid, o.groupid FROM {assign_overrides} o JOIN {assign} assign ON assign.id = o.assignid WHERE assign.course = :courseid AND o.groupid = :groupid"; } else { // No groupid, we update all orphaned group overrides for all assign in course. - $sql = "SELECT o.id, o.assignid + $sql = "SELECT o.id, o.assignid, o.groupid FROM {assign_overrides} o JOIN {assign} assign ON assign.id = o.assignid LEFT JOIN {groups} grp ON grp.id = o.groupid @@ -9805,11 +9818,15 @@ function assign_process_group_deleted_in_course($courseid, $groupid = null) { AND o.groupid IS NOT NULL AND grp.id IS NULL"; } - $records = $DB->get_records_sql_menu($sql, $params); + $records = $DB->get_records_sql($sql, $params); if (!$records) { return; // Nothing to do. } $DB->delete_records_list('assign_overrides', 'id', array_keys($records)); + $cache = cache::make('mod_assign', 'overrides'); + foreach ($records as $record) { + $cache->delete("{$record->assignid}_g_{$record->groupid}"); + } } /** @@ -9824,7 +9841,7 @@ function move_group_override($id, $move, $assignid) { global $DB; // Get the override object. - if (!$override = $DB->get_record('assign_overrides', array('id' => $id), 'id, sortorder')) { + if (!$override = $DB->get_record('assign_overrides', ['id' => $id], 'id, sortorder, groupid')) { return false; } // Count the number of group overrides. @@ -9840,8 +9857,8 @@ function move_group_override($id, $move, $assignid) { } // Retrieve the override object that is currently residing in the new position. - $params = array('sortorder' => $neworder, 'assignid' => $assignid); - if ($swapoverride = $DB->get_record('assign_overrides', $params, 'id, sortorder')) { + $params = ['sortorder' => $neworder, 'assignid' => $assignid]; + if ($swapoverride = $DB->get_record('assign_overrides', $params, 'id, sortorder, groupid')) { // Swap the sortorders. $swapoverride->sortorder = $override->sortorder; @@ -9850,6 +9867,11 @@ function move_group_override($id, $move, $assignid) { // Update the override records. $DB->update_record('assign_overrides', $override); $DB->update_record('assign_overrides', $swapoverride); + + // Delete cache for the 2 records we updated above. + $cache = cache::make('mod_assign', 'overrides'); + $cache->delete("{$override->assignid}_g_{$override->groupid}"); + $cache->delete("{$swapoverride->assignid}_g_{$swapoverride->groupid}"); } reorder_group_overrides($assignid); @@ -9866,11 +9888,13 @@ function reorder_group_overrides($assignid) { $i = 1; if ($overrides = $DB->get_records('assign_overrides', array('userid' => null, 'assignid' => $assignid), 'sortorder ASC')) { + $cache = cache::make('mod_assign', 'overrides'); foreach ($overrides as $override) { $f = new stdClass(); $f->id = $override->id; $f->sortorder = $i++; $DB->update_record('assign_overrides', $f); + $cache->delete("{$assignid}_g_{$override->groupid}"); // Update priorities of group overrides. $params = [ diff --git a/mod/assign/overrideedit.php b/mod/assign/overrideedit.php index 1a39c097f09..af6b3cc7bce 100644 --- a/mod/assign/overrideedit.php +++ b/mod/assign/overrideedit.php @@ -187,6 +187,8 @@ if ($mform->is_cancelled()) { if (!empty($override->id)) { $fromform->id = $override->id; $DB->update_record('assign_overrides', $fromform); + $cachekey = $groupmode ? "{$fromform->assignid}_g_{$fromform->groupid}" : "{$fromform->assignid}_u_{$fromform->userid}"; + cache::make('mod_assign', 'overrides')->delete($cachekey); // Determine which override updated event to fire. $params['objectid'] = $override->id; @@ -219,6 +221,8 @@ if ($mform->is_cancelled()) { $DB->update_record('assign_overrides', $fromform); reorder_group_overrides($assigninstance->id); } + $cachekey = $groupmode ? "{$fromform->assignid}_g_{$fromform->groupid}" : "{$fromform->assignid}_u_{$fromform->userid}"; + cache::make('mod_assign', 'overrides')->delete($cachekey); // Determine which override created event to fire. $params['objectid'] = $fromform->id; diff --git a/mod/assign/version.php b/mod/assign/version.php index 419c31c066c..1382e25bc86 100644 --- a/mod/assign/version.php +++ b/mod/assign/version.php @@ -25,5 +25,5 @@ defined('MOODLE_INTERNAL') || die(); $plugin->component = 'mod_assign'; // Full name of the plugin (used for diagnostics). -$plugin->version = 2020110900; // The current module version (Date: YYYYMMDDXX). +$plugin->version = 2021042700; // The current module version (Date: YYYYMMDDXX). $plugin->requires = 2020110300; // Requires this Moodle version. From 279def66dcbb8c26ffefc342a086be7b06df66d2 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 15 Apr 2021 05:55:09 +1000 Subject: [PATCH 03/11] MDL-71410 mod_assign: Cache assign times in modinfo for performance We update the dates with user/group overrides --- mod/assign/lib.php | 78 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 2 deletions(-) diff --git a/mod/assign/lib.php b/mod/assign/lib.php index 450aac46040..0de738c006d 100644 --- a/mod/assign/lib.php +++ b/mod/assign/lib.php @@ -483,10 +483,11 @@ function assign_extend_settings_navigation(settings_navigation $settings, naviga * will know about (most noticeably, an icon). */ function assign_get_coursemodule_info($coursemodule) { - global $CFG, $DB; + global $DB; $dbparams = array('id'=>$coursemodule->instance); - $fields = 'id, name, alwaysshowdescription, allowsubmissionsfromdate, intro, introformat, completionsubmit'; + $fields = 'id, name, alwaysshowdescription, allowsubmissionsfromdate, intro, introformat, completionsubmit, + duedate, cutoffdate, allowsubmissionsfromdate'; if (! $assignment = $DB->get_record('assign', $dbparams, $fields)) { return false; } @@ -505,9 +506,82 @@ function assign_get_coursemodule_info($coursemodule) { $result->customdata['customcompletionrules']['completionsubmit'] = $assignment->completionsubmit; } + // Populate some other values that can be used in calendar or on dashboard. + if ($assignment->duedate) { + $result->customdata['duedate'] = $assignment->duedate; + } + if ($assignment->cutoffdate) { + $result->customdata['cutoffdate'] = $assignment->cutoffdate; + } + if ($assignment->allowsubmissionsfromdate) { + $result->customdata['allowsubmissionsfromdate'] = $assignment->allowsubmissionsfromdate; + } + return $result; } +/** + * Sets dynamic information about a course module + * + * This function is called from cm_info when displaying the module + * + * @param cm_info $cm + */ +function mod_assign_cm_info_dynamic(cm_info $cm) { + global $USER; + + $cache = cache::make('mod_assign', 'overrides'); + $override = $cache->get("{$cm->instance}_u_{$USER->id}"); + + if (!$override) { + $override = (object) [ + 'allowsubmissionsfromdate' => null, + 'duedate' => null, + 'cutoffdate' => null, + ]; + } + + // No need to look for group overrides if there are user overrides for all allowsubmissionsfromdate, duedate and cutoffdate. + if (is_null($override->allowsubmissionsfromdate) || is_null($override->duedate) || is_null($override->cutoffdate)) { + $selectedgroupoverride = (object) [ + 'allowsubmissionsfromdate' => null, + 'duedate' => null, + 'cutoffdate' => null, + 'sortorder' => PHP_INT_MAX, // So that every sortorder read from DB is less than this. + ]; + $groupings = groups_get_user_groups($cm->course, $USER->id); + foreach ($groupings[0] as $groupid) { + $groupoverride = $cache->get("{$cm->instance}_g_{$groupid}"); + if ($groupoverride) { + if ($groupoverride->sortorder < $selectedgroupoverride->sortorder) { + $selectedgroupoverride = $groupoverride; + } + } + } + // If there is a user override for a setting, ignore the group override. + if (is_null($override->allowsubmissionsfromdate)) { + $override->allowsubmissionsfromdate = $selectedgroupoverride->allowsubmissionsfromdate; + } + if (is_null($override->duedate)) { + $override->duedate = $selectedgroupoverride->duedate; + } + if (is_null($override->cutoffdate)) { + $override->cutoffdate = $selectedgroupoverride->cutoffdate; + } + } + + // Populate some other values that can be used in calendar or on dashboard. + if (!is_null($override->allowsubmissionsfromdate)) { + $cm->override_customdata('allowsubmissionsfromdate', $override->allowsubmissionsfromdate); + } + if (!is_null($override->duedate)) { + $cm->override_customdata('duedate', $override->duedate); + } + if (!is_null($override->cutoffdate)) { + $cm->override_customdata('cutoffdate', $override->cutoffdate); + } +} + /** * Callback which returns human-readable strings describing the active completion custom rules for the module instance. * From b700f690abaec7d0228cec091398cfa7c7a9830a Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 25 Feb 2021 18:56:58 +1100 Subject: [PATCH 04/11] MDL-71410 mod_assign: implement activity_dates for the assignment module --- mod/assign/classes/dates.php | 67 +++++++++++ mod/assign/lang/en/assign.php | 3 + mod/assign/tests/dates_test.php | 175 +++++++++++++++++++++++++++++ mod/assign/tests/generator/lib.php | 24 ++++ 4 files changed, 269 insertions(+) create mode 100644 mod/assign/classes/dates.php create mode 100644 mod/assign/tests/dates_test.php diff --git a/mod/assign/classes/dates.php b/mod/assign/classes/dates.php new file mode 100644 index 00000000000..f2a4c9177de --- /dev/null +++ b/mod/assign/classes/dates.php @@ -0,0 +1,67 @@ +. + +/** + * Contains the class for fetching the important dates in mod_assign for a given module instance and a user. + * + * @package mod_assign + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_assign; + +use core\activity_dates; + +/** + * Class for fetching the important dates in mod_assign for a given module instance and a user. + * + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class dates extends activity_dates { + + /** + * Returns a list of important dates in mod_assign + * + * @return array + */ + protected function get_dates(): array { + $timeopen = $this->cm->customdata['allowsubmissionsfromdate'] ?? null; + $timedue = $this->cm->customdata['duedate'] ?? null; + $now = time(); + $dates = []; + + if ($timeopen) { + $openlabelid = $timeopen > $now ? 'activitydate:submissionsopen' : 'activitydate:submissionsopened'; + $dates[] = [ + 'label' => get_string($openlabelid, 'mod_assign'), + 'timestamp' => (int) $timeopen, + ]; + } + + if ($timedue) { + $dates[] = [ + 'label' => get_string('activitydate:submissionsdue', 'mod_assign'), + 'timestamp' => (int) $timedue, + ]; + } + + return $dates; + } +} diff --git a/mod/assign/lang/en/assign.php b/mod/assign/lang/en/assign.php index 1e0ae3bdad4..b68423d0c91 100644 --- a/mod/assign/lang/en/assign.php +++ b/mod/assign/lang/en/assign.php @@ -22,6 +22,9 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ +$string['activitydate:submissionsdue'] = 'Submissions due:'; +$string['activitydate:submissionsopen'] = 'Submissions open:'; +$string['activitydate:submissionsopened'] = 'Submissions opened:'; $string['activityoverview'] = 'You have assignments that need attention'; $string['addsubmission'] = 'Add submission'; $string['addsubmission_help'] = 'You have not made a submission yet.'; diff --git a/mod/assign/tests/dates_test.php b/mod/assign/tests/dates_test.php new file mode 100644 index 00000000000..35bb7e9787f --- /dev/null +++ b/mod/assign/tests/dates_test.php @@ -0,0 +1,175 @@ +. + +/** + * Contains unit tests for mod_assign\dates. + * + * @package mod_assign + * @category test + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_assign; + +use advanced_testcase; +use cm_info; +use core\activity_dates; + +/** + * Class for unit testing mod_assign\dates. + * + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class dates_test extends advanced_testcase { + + /** + * Data provider for get_dates_for_module(). + * @return array[] + */ + public function get_dates_for_module_provider(): array { + $now = time(); + $before = $now - DAYSECS; + $earlier = $before - DAYSECS; + $after = $now + DAYSECS; + $later = $after + DAYSECS; + + return [ + 'without any dates' => [ + null, null, null, null, null, null, [] + ], + 'only with opening time' => [ + $after, null, null, null, null, null, [ + ['label' => get_string('activitydate:submissionsopen', 'mod_assign'), 'timestamp' => $after], + ] + ], + 'only with closing time' => [ + null, $after, null, null, null, null, [ + ['label' => get_string('activitydate:submissionsdue', 'mod_assign'), 'timestamp' => $after], + ] + ], + 'with both times' => [ + $after, $later, null, null, null, null, [ + ['label' => get_string('activitydate:submissionsopen', 'mod_assign'), 'timestamp' => $after], + ['label' => get_string('activitydate:submissionsdue', 'mod_assign'), 'timestamp' => $later], + ] + ], + 'between the dates' => [ + $before, $after, null, null, null, null, [ + ['label' => get_string('activitydate:submissionsopened', 'mod_assign'), 'timestamp' => $before], + ['label' => get_string('activitydate:submissionsdue', 'mod_assign'), 'timestamp' => $after], + ] + ], + 'dates are past' => [ + $earlier, $before, null, null, null, null, [ + ['label' => get_string('activitydate:submissionsopened', 'mod_assign'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:submissionsdue', 'mod_assign'), 'timestamp' => $before], + ] + ], + 'with user override' => [ + $before, $after, $earlier, $later, null, null, [ + ['label' => get_string('activitydate:submissionsopened', 'mod_assign'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:submissionsdue', 'mod_assign'), 'timestamp' => $later], + ] + ], + 'with group override' => [ + $before, $after, null, null, $earlier, $later, [ + ['label' => get_string('activitydate:submissionsopened', 'mod_assign'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:submissionsdue', 'mod_assign'), 'timestamp' => $later], + ] + ], + 'with both user and group overrides' => [ + $before, $after, $earlier, $later, $earlier - DAYSECS, $later + DAYSECS, [ + ['label' => get_string('activitydate:submissionsopened', 'mod_assign'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:submissionsdue', 'mod_assign'), 'timestamp' => $later], + ] + ], + ]; + } + + /** + * Test for get_dates_for_module(). + * + * @dataProvider get_dates_for_module_provider + * @param int|null $from Time of opening submissions in the assignment. + * @param int|null $due Assignment's due date. + * @param int|null $userfrom The user override for opening submissions. + * @param int|null $userdue The user override for due date. + * @param int|null $groupfrom The group override for opening submissions. + * @param int|null $groupdue The group override for due date. + * @param array $expected The expected value of calling get_dates_for_module() + */ + public function test_get_dates_for_module(?int $from, ?int $due, + ?int $userfrom, ?int $userdue, + ?int $groupfrom, ?int $groupdue, + array $expected) { + + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + /** @var \mod_assign_generator $assigngenerator */ + $assigngenerator = $generator->get_plugin_generator('mod_assign'); + + $course = $generator->create_course(); + $user = $generator->create_user(); + $generator->enrol_user($user->id, $course->id); + + $data = ['course' => $course->id]; + if ($from) { + $data['allowsubmissionsfromdate'] = $from; + } + if ($due) { + $data['duedate'] = $due; + } + $assign = $assigngenerator->create_instance($data); + + if ($userfrom || $userdue || $groupfrom || $groupdue) { + $generator->enrol_user($user->id, $course->id); + $group = $generator->create_group(['courseid' => $course->id]); + $generator->create_group_member(['groupid' => $group->id, 'userid' => $user->id]); + + if ($userfrom || $userdue) { + $assigngenerator->create_override([ + 'assignid' => $assign->id, + 'userid' => $user->id, + 'allowsubmissionsfromdate' => $userfrom, + 'duedate' => $userdue, + ]); + } + + if ($groupfrom || $groupdue) { + $assigngenerator->create_override([ + 'assignid' => $assign->id, + 'groupid' => $group->id, + 'allowsubmissionsfromdate' => $groupfrom, + 'duedate' => $groupdue, + ]); + } + } + + $this->setUser($user); + + $cm = get_coursemodule_from_instance('assign', $assign->id); + // Make sure we're using a cm_info object. + $cm = cm_info::create($cm); + + $dates = activity_dates::get_dates_for_module($cm, (int) $user->id); + + $this->assertEquals($expected, $dates); + } +} diff --git a/mod/assign/tests/generator/lib.php b/mod/assign/tests/generator/lib.php index e9d4fa7d0b9..fd14b86578e 100644 --- a/mod/assign/tests/generator/lib.php +++ b/mod/assign/tests/generator/lib.php @@ -59,4 +59,28 @@ class mod_assign_generator extends testing_module_generator { return parent::create_instance($record, (array)$options); } + + /** + * Create an assign override (either user or group). + * + * @param array $data must specify assignid, and one of userid or groupid. + * @throws coding_exception + */ + public function create_override(array $data): void { + global $DB; + + if (!isset($data['assignid'])) { + throw new coding_exception('Must specify assignid when creating an assign override.'); + } + + if (!isset($data['userid']) && !isset($data['groupid'])) { + throw new coding_exception('Must specify one of userid or groupid when creating an assign override.'); + } + + if (isset($data['userid']) && isset($data['groupid'])) { + throw new coding_exception('Cannot specify both userid and groupid when creating an assign override.'); + } + + $DB->insert_record('assign_overrides', (object) $data); + } } From 04e6d285d83e41fcbe99959c0c6e27654f99c205 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Sun, 25 Apr 2021 04:19:24 +1000 Subject: [PATCH 05/11] MDL-71410 mod_lesson: Cache user and group overrides --- mod/lesson/classes/cache/overrides.php | 110 +++++++++++++++++++++++++ mod/lesson/db/caches.php | 35 ++++++++ mod/lesson/lang/en/lesson.php | 1 + mod/lesson/lib.php | 22 +++-- mod/lesson/locallib.php | 13 ++- mod/lesson/overrideedit.php | 4 + mod/lesson/version.php | 2 +- 7 files changed, 177 insertions(+), 10 deletions(-) create mode 100644 mod/lesson/classes/cache/overrides.php create mode 100644 mod/lesson/db/caches.php diff --git a/mod/lesson/classes/cache/overrides.php b/mod/lesson/classes/cache/overrides.php new file mode 100644 index 00000000000..889d9728c14 --- /dev/null +++ b/mod/lesson/classes/cache/overrides.php @@ -0,0 +1,110 @@ +. + +/** + * Cache data source for the lesson overrides. + * + * @package mod_lesson + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_lesson\cache; + +use cache_definition; + +/** + * Class lesson_overrides + * + * @package mod_lesson + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class overrides implements \cache_data_source { + + /** @var overrides the singleton instance of this class. */ + protected static $instance = null; + + /** + * Returns an instance of the data source class that the cache can use for loading data using the other methods + * specified by this interface. + * + * @param cache_definition $definition + * @return object + */ + public static function get_instance_for_cache(cache_definition $definition): overrides { + if (is_null(self::$instance)) { + self::$instance = new overrides(); + } + return self::$instance; + } + + /** + * Loads the data for the key provided ready formatted for caching. + * + * @param string|int $key The key to load. + * @return mixed What ever data should be returned, or false if it can't be loaded. + * @throws \coding_exception + */ + public function load_for_cache($key) { + global $DB; + + [$lessonid, $ug, $ugid] = explode('_', $key); + $lessonid = (int) $lessonid; + + switch ($ug) { + case 'u': + $userid = (int) $ugid; + $override = $DB->get_record( + 'lesson_overrides', + ['lessonid' => $lessonid, 'userid' => $userid], + 'available, deadline, timelimit, review, maxattempts, retake, password' + ); + break; + case 'g': + $groupid = (int) $ugid; + $override = $DB->get_record( + 'lesson_overrides', + ['lessonid' => $lessonid, 'groupid' => $groupid], + 'available, deadline, timelimit, review, maxattempts, retake, password' + ); + break; + default: + throw new \coding_exception('Invalid cache key'); + } + + // Return null instead of false, because false will not be cached. + return $override ?: null; + } + + /** + * Loads several keys for the cache. + * + * @param array $keys An array of keys each of which will be string|int. + * @return array An array of matching data items. + */ + public function load_many_for_cache(array $keys) { + $results = []; + + foreach ($keys as $key) { + $results[] = $this->load_for_cache($key); + } + + return $results; + } +} diff --git a/mod/lesson/db/caches.php b/mod/lesson/db/caches.php new file mode 100644 index 00000000000..30ad05c6e6f --- /dev/null +++ b/mod/lesson/db/caches.php @@ -0,0 +1,35 @@ +. + +/** + * Defined caches used internally by the plugin. + * + * @package mod_lesson + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +defined('MOODLE_INTERNAL') || die(); + +$definitions = [ + 'overrides' => [ + 'mode' => cache_store::MODE_APPLICATION, + 'simplekeys' => true, + 'datasource' => '\mod_lesson\cache\overrides', + ], +]; diff --git a/mod/lesson/lang/en/lesson.php b/mod/lesson/lang/en/lesson.php index c4f4b2499a8..5a2fed83495 100644 --- a/mod/lesson/lang/en/lesson.php +++ b/mod/lesson/lang/en/lesson.php @@ -83,6 +83,7 @@ $string['averagescore'] = 'Average score'; $string['averagetime'] = 'Average time'; $string['branch'] = 'Content'; $string['branchtable'] = 'Content'; +$string['cachedef_overrides'] = 'User and group override information'; $string['cancel'] = 'Cancel'; $string['cannotfindanswer'] = 'Error: could not find answer'; $string['cannotfindattempt'] = 'Error: could not find attempt'; diff --git a/mod/lesson/lib.php b/mod/lesson/lib.php index b0a0761cc36..05bd9016d0b 100644 --- a/mod/lesson/lib.php +++ b/mod/lesson/lib.php @@ -924,23 +924,27 @@ function lesson_reset_userdata($data) { $status[] = array('component'=>$componentstr, 'item'=>get_string('deleteallattempts', 'lesson'), 'error'=>false); } + $purgeoverrides = false; + // Remove user overrides. if (!empty($data->reset_lesson_user_overrides)) { $DB->delete_records_select('lesson_overrides', 'lessonid IN (SELECT id FROM {lesson} WHERE course = ?) AND userid IS NOT NULL', array($data->courseid)); $status[] = array( - 'component' => $componentstr, - 'item' => get_string('useroverridesdeleted', 'lesson'), - 'error' => false); + 'component' => $componentstr, + 'item' => get_string('useroverridesdeleted', 'lesson'), + 'error' => false); + $purgeoverrides = true; } // Remove group overrides. if (!empty($data->reset_lesson_group_overrides)) { $DB->delete_records_select('lesson_overrides', 'lessonid IN (SELECT id FROM {lesson} WHERE course = ?) AND groupid IS NOT NULL', array($data->courseid)); $status[] = array( - 'component' => $componentstr, - 'item' => get_string('groupoverridesdeleted', 'lesson'), - 'error' => false); + 'component' => $componentstr, + 'item' => get_string('groupoverridesdeleted', 'lesson'), + 'error' => false); + $purgeoverrides = true; } /// updating dates - shift may be negative too if ($data->timeshift) { @@ -953,12 +957,18 @@ function lesson_reset_userdata($data) { WHERE lessonid IN (SELECT id FROM {lesson} WHERE course = ?) AND deadline <> 0", array($data->timeshift, $data->courseid)); + $purgeoverrides = true; + // Any changes to the list of dates that needs to be rolled should be same during course restore and course reset. // See MDL-9367. shift_course_mod_dates('lesson', array('available', 'deadline'), $data->timeshift, $data->courseid); $status[] = array('component'=>$componentstr, 'item'=>get_string('datechanged'), 'error'=>false); } + if ($purgeoverrides) { + cache::make('mod_lesson', 'overrides')->purge(); + } + return $status; } diff --git a/mod/lesson/locallib.php b/mod/lesson/locallib.php index d5dc3be6e34..fec6308a1db 100644 --- a/mod/lesson/locallib.php +++ b/mod/lesson/locallib.php @@ -657,14 +657,14 @@ function lesson_process_group_deleted_in_course($courseid, $groupid = null) { if ($groupid) { $params['groupid'] = $groupid; // We just update the group that was deleted. - $sql = "SELECT o.id, o.lessonid + $sql = "SELECT o.id, o.lessonid, o.groupid FROM {lesson_overrides} o JOIN {lesson} lesson ON lesson.id = o.lessonid WHERE lesson.course = :courseid AND o.groupid = :groupid"; } else { // No groupid, we update all orphaned group overrides for all lessons in course. - $sql = "SELECT o.id, o.lessonid + $sql = "SELECT o.id, o.lessonid, o.groupid FROM {lesson_overrides} o JOIN {lesson} lesson ON lesson.id = o.lessonid LEFT JOIN {groups} grp ON grp.id = o.groupid @@ -672,11 +672,15 @@ function lesson_process_group_deleted_in_course($courseid, $groupid = null) { AND o.groupid IS NOT NULL AND grp.id IS NULL"; } - $records = $DB->get_records_sql_menu($sql, $params); + $records = $DB->get_records_sql($sql, $params); if (!$records) { return; // Nothing to do. } $DB->delete_records_list('lesson_overrides', 'id', array_keys($records)); + $cache = cache::make('mod_lesson', 'overrides'); + foreach ($records as $record) { + $cache->delete("{$record->lessonid}_g_{$record->groupid}"); + } } /** @@ -1740,8 +1744,10 @@ class lesson extends lesson_base { 'instance' => $this->properties->id); if (isset($override->userid)) { $conds['userid'] = $override->userid; + $cachekey = "{$cm->instance}_u_{$override->userid}"; } else { $conds['groupid'] = $override->groupid; + $cachekey = "{$cm->instance}_g_{$override->groupid}"; } $events = $DB->get_records('event', $conds); foreach ($events as $event) { @@ -1750,6 +1756,7 @@ class lesson extends lesson_base { } $DB->delete_records('lesson_overrides', array('id' => $overrideid)); + cache::make('mod_lesson', 'overrides')->delete($cachekey); // Set the common parameters for one of the events we will be triggering. $params = array( diff --git a/mod/lesson/overrideedit.php b/mod/lesson/overrideedit.php index 68f8251d9dc..dd7d9b93402 100644 --- a/mod/lesson/overrideedit.php +++ b/mod/lesson/overrideedit.php @@ -175,6 +175,8 @@ if ($mform->is_cancelled()) { if (!empty($override->id)) { $fromform->id = $override->id; $DB->update_record('lesson_overrides', $fromform); + $cachekey = $groupmode ? "{$fromform->lessonid}_g_{$fromform->groupid}" : "{$fromform->lessonid}_u_{$fromform->userid}"; + cache::make('mod_lesson', 'overrides')->delete($cachekey); // Determine which override updated event to fire. $params['objectid'] = $override->id; @@ -191,6 +193,8 @@ if ($mform->is_cancelled()) { } else { unset($fromform->id); $fromform->id = $DB->insert_record('lesson_overrides', $fromform); + $cachekey = $groupmode ? "{$fromform->lessonid}_g_{$fromform->groupid}" : "{$fromform->lessonid}_u_{$fromform->userid}"; + cache::make('mod_lesson', 'overrides')->delete($cachekey); // Determine which override created event to fire. $params['objectid'] = $fromform->id; diff --git a/mod/lesson/version.php b/mod/lesson/version.php index 0a3bb8bc883..fa1656a99df 100644 --- a/mod/lesson/version.php +++ b/mod/lesson/version.php @@ -24,7 +24,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2020110900; // The current module version (Date: YYYYMMDDXX) +$plugin->version = 2021042700; // The current module version (Date: YYYYMMDDXX) $plugin->requires = 2020110300; // Requires this Moodle version $plugin->component = 'mod_lesson'; // Full name of the plugin (used for diagnostics) $plugin->cron = 0; From 5f9ca816f48a3ab0faa122ed6955eab2df7e8356 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Fri, 5 Mar 2021 22:29:12 +1100 Subject: [PATCH 06/11] MDL-71410 mod_lesson: Cache lesson times in modinfo for performance We update the dates with user/group overrides. The calculation of the override in the lesson module is different from the assignment module as the lesson_overrides table des not have a sortorder column. See lesson::update_effective_access(). --- mod/lesson/lib.php | 66 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/mod/lesson/lib.php b/mod/lesson/lib.php index 05bd9016d0b..fdecc3de61f 100644 --- a/mod/lesson/lib.php +++ b/mod/lesson/lib.php @@ -1552,7 +1552,7 @@ function lesson_get_coursemodule_info($coursemodule) { global $DB; $dbparams = ['id' => $coursemodule->instance]; - $fields = 'id, name, intro, introformat, completionendreached, completiontimespent'; + $fields = 'id, name, intro, introformat, completionendreached, completiontimespent, available, deadline'; if (!$lesson = $DB->get_record('lesson', $dbparams, $fields)) { return false; } @@ -1571,9 +1571,73 @@ function lesson_get_coursemodule_info($coursemodule) { $result->customdata['customcompletionrules']['completiontimespent'] = $lesson->completiontimespent; } + // Populate some other values that can be used in calendar or on dashboard. + if ($lesson->available) { + $result->customdata['available'] = $lesson->available; + } + if ($lesson->deadline) { + $result->customdata['deadline'] = $lesson->deadline; + } + return $result; } +/** + * Sets dynamic information about a course module + * + * This function is called from cm_info when displaying the module + * + * @param cm_info $cm + */ +function mod_lesson_cm_info_dynamic(cm_info $cm) { + global $USER; + + $cache = cache::make('mod_lesson', 'overrides'); + $override = $cache->get("{$cm->instance}_u_{$USER->id}"); + + if (!$override) { + $override = (object) [ + 'available' => null, + 'deadline' => null, + ]; + } + + // No need to look for group overrides if there are user overrides for both available and deadline. + if (is_null($override->available) || is_null($override->deadline)) { + $availables = []; + $deadlines = []; + $groupings = groups_get_user_groups($cm->course, $USER->id); + foreach ($groupings[0] as $groupid) { + $groupoverride = $cache->get("{$cm->instance}_g_{$groupid}"); + if (isset($groupoverride->available)) { + $availables[] = $groupoverride->available; + } + if (isset($groupoverride->deadline)) { + $deadlines[] = $groupoverride->deadline; + } + } + // If there is a user override for a setting, ignore the group override. + if (is_null($override->available) && count($availables)) { + $override->available = min($availables); + } + if (is_null($override->deadline) && count($deadlines)) { + if (in_array(0, $deadlines)) { + $override->deadline = 0; + } else { + $override->deadline = max($deadlines); + } + } + } + + // Populate some other values that can be used in calendar or on dashboard. + if (!is_null($override->available)) { + $cm->override_customdata('available', $override->available); + } + if (!is_null($override->deadline)) { + $cm->override_customdata('deadline', $override->deadline); + } +} + /** * Callback which returns human-readable strings describing the active completion custom rules for the module instance. * From 9bf74e7e8513e8561ae02012a02fca0ced9c551e Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Fri, 5 Mar 2021 23:37:23 +1100 Subject: [PATCH 07/11] MDL-71410 mod_lesson: implement activity_dates for the lesson module --- mod/lesson/classes/dates.php | 68 +++++++++++ mod/lesson/tests/dates_test.php | 175 +++++++++++++++++++++++++++++ mod/lesson/tests/generator/lib.php | 23 ++++ 3 files changed, 266 insertions(+) create mode 100644 mod/lesson/classes/dates.php create mode 100644 mod/lesson/tests/dates_test.php diff --git a/mod/lesson/classes/dates.php b/mod/lesson/classes/dates.php new file mode 100644 index 00000000000..cf18db24bd9 --- /dev/null +++ b/mod/lesson/classes/dates.php @@ -0,0 +1,68 @@ +. + +/** + * Contains the class for fetching the important dates in mod_lesson for a given module instance and a user. + * + * @package mod_lesson + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_lesson; + +use core\activity_dates; + +/** + * Class for fetching the important dates in mod_lesson for a given module instance and a user. + * + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class dates extends activity_dates { + + /** + * Returns a list of important dates in mod_lesson + * + * @return array + */ + protected function get_dates(): array { + $timeopen = $this->cm->customdata['available'] ?? null; + $timeclose = $this->cm->customdata['deadline'] ?? null; + $now = time(); + $dates = []; + + if ($timeopen) { + $openlabelid = $timeopen > $now ? 'activitydate:opens' : 'activitydate:opened'; + $dates[] = [ + 'label' => get_string($openlabelid, 'course'), + 'timestamp' => (int) $timeopen, + ]; + } + + if ($timeclose) { + $closelabelid = $timeclose > $now ? 'activitydate:closes' : 'activitydate:closed'; + $dates[] = [ + 'label' => get_string($closelabelid, 'course'), + 'timestamp' => (int) $timeclose, + ]; + } + + return $dates; + } +} diff --git a/mod/lesson/tests/dates_test.php b/mod/lesson/tests/dates_test.php new file mode 100644 index 00000000000..dbe906709cf --- /dev/null +++ b/mod/lesson/tests/dates_test.php @@ -0,0 +1,175 @@ +. + +/** + * Contains unit tests for mod_lesson\dates. + * + * @package mod_lesson + * @category test + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_lesson; + +use advanced_testcase; +use cm_info; +use core\activity_dates; + +/** + * Class for unit testing mod_lesson\dates. + * + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class dates_test extends advanced_testcase { + + /** + * Data provider for get_dates_for_module(). + * @return array[] + */ + public function get_dates_for_module_provider(): array { + $now = time(); + $before = $now - DAYSECS; + $earlier = $before - DAYSECS; + $after = $now + DAYSECS; + $later = $after + DAYSECS; + + return [ + 'without any dates' => [ + null, null, null, null, null, null, [] + ], + 'only with opening time' => [ + $after, null, null, null, null, null, [ + ['label' => get_string('activitydate:opens', 'course'), 'timestamp' => $after], + ] + ], + 'only with closing time' => [ + null, $after, null, null, null, null, [ + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $after], + ] + ], + 'with both times' => [ + $after, $later, null, null, null, null, [ + ['label' => get_string('activitydate:opens', 'course'), 'timestamp' => $after], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $later], + ] + ], + 'between the dates' => [ + $before, $after, null, null, null, null, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $before], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $after], + ] + ], + 'dates are past' => [ + $earlier, $before, null, null, null, null, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:closed', 'course'), 'timestamp' => $before], + ] + ], + 'with user override' => [ + $before, $after, $earlier, $later, null, null, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $later], + ] + ], + 'with group override' => [ + $before, $after, null, null, $earlier, $later, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $later], + ] + ], + 'with both user and group overrides' => [ + $before, $after, $earlier, $later, $earlier - DAYSECS, $later + DAYSECS, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $later], + ] + ], + ]; + } + + /** + * Test for get_dates_for_module(). + * + * @dataProvider get_dates_for_module_provider + * @param int|null $available The 'available from' value of the lesson. + * @param int|null $deadline The lesson's deadline. + * @param int|null $useravailable The user override for opening the lesson. + * @param int|null $userdeadline The user override for deadline of the lesson. + * @param int|null $groupavailable The group override for opening the lesson. + * @param int|null $groupuserdeadline The group override for deadline of the lesson. + * @param array $expected The expected value of calling get_dates_for_module() + */ + public function test_get_dates_for_module(?int $available, ?int $deadline, + ?int $useravailable, ?int $userdeadline, + ?int $groupavailable, ?int $groupuserdeadline, + array $expected) { + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + /** @var \mod_lesson_generator $lessongenerator */ + $lessongenerator = $generator->get_plugin_generator('mod_lesson'); + + $course = $generator->create_course(); + $user = $generator->create_user(); + $generator->enrol_user($user->id, $course->id); + + $data = ['course' => $course->id]; + if ($available) { + $data['available'] = $available; + } + if ($deadline) { + $data['deadline'] = $deadline; + } + $this->setAdminUser(); + $lesson = $lessongenerator->create_instance($data); + + if ($useravailable || $userdeadline || $groupavailable || $groupuserdeadline) { + $generator->enrol_user($user->id, $course->id); + $group = $generator->create_group(['courseid' => $course->id]); + $generator->create_group_member(['groupid' => $group->id, 'userid' => $user->id]); + + if ($useravailable || $userdeadline) { + $lessongenerator->create_override([ + 'lessonid' => $lesson->id, + 'userid' => $user->id, + 'available' => $useravailable, + 'deadline' => $userdeadline, + ]); + } + + if ($groupavailable || $groupuserdeadline) { + $lessongenerator->create_override([ + 'lessonid' => $lesson->id, + 'groupid' => $group->id, + 'available' => $groupavailable, + 'deadline' => $groupuserdeadline, + ]); + } + } + + $this->setUser($user); + + $cm = get_coursemodule_from_instance('lesson', $lesson->id); + // Make sure we're using a cm_info object. + $cm = cm_info::create($cm); + + $dates = activity_dates::get_dates_for_module($cm, (int) $user->id); + + $this->assertEquals($expected, $dates); + } +} diff --git a/mod/lesson/tests/generator/lib.php b/mod/lesson/tests/generator/lib.php index 777fdb2d362..53fcfa6a251 100644 --- a/mod/lesson/tests/generator/lib.php +++ b/mod/lesson/tests/generator/lib.php @@ -426,4 +426,27 @@ class mod_lesson_generator extends testing_module_generator { $page = lesson_page::create((object)$record, new lesson($lesson), $context, $CFG->maxbytes); return $DB->get_record('lesson_pages', array('id' => $page->id), '*', MUST_EXIST); } + + /** + * Create a lesson override (either user or group). + * + * @param array $data must specify lessonid, and one of userid or groupid. + */ + public function create_override(array $data): void { + global $DB; + + if (!isset($data['lessonid'])) { + throw new coding_exception('Must specify lessonid when creating a lesson override.'); + } + + if (!isset($data['userid']) && !isset($data['groupid'])) { + throw new coding_exception('Must specify one of userid or groupid when creating a lesson override.'); + } + + if (isset($data['userid']) && isset($data['groupid'])) { + throw new coding_exception('Cannot specify both userid and groupid when creating a lesson override.'); + } + + $DB->insert_record('lesson_overrides', (object) $data); + } } From e8caa2d2a22b1f58fed37e793d8e9ecf096769a1 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Mon, 26 Apr 2021 02:44:25 +1000 Subject: [PATCH 08/11] MDL-71410 mod_quiz: Cache user and group overrides --- mod/quiz/classes/cache/overrides.php | 110 +++++++++++++++++++++++++++ mod/quiz/db/caches.php | 35 +++++++++ mod/quiz/lang/en/quiz.php | 1 + mod/quiz/lib.php | 13 ++++ mod/quiz/locallib.php | 10 ++- mod/quiz/overrideedit.php | 4 + mod/quiz/version.php | 2 +- 7 files changed, 171 insertions(+), 4 deletions(-) create mode 100644 mod/quiz/classes/cache/overrides.php create mode 100644 mod/quiz/db/caches.php diff --git a/mod/quiz/classes/cache/overrides.php b/mod/quiz/classes/cache/overrides.php new file mode 100644 index 00000000000..839b3b29554 --- /dev/null +++ b/mod/quiz/classes/cache/overrides.php @@ -0,0 +1,110 @@ +. + +/** + * Cache data source for the quiz overrides. + * + * @package mod_quiz + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_quiz\cache; + +use cache_definition; + +/** + * Class quiz_overrides + * + * @package mod_quiz + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class overrides implements \cache_data_source { + + /** @var overrides the singleton instance of this class. */ + protected static $instance = null; + + /** + * Returns an instance of the data source class that the cache can use for loading data using the other methods + * specified by this interface. + * + * @param cache_definition $definition + * @return object + */ + public static function get_instance_for_cache(cache_definition $definition): overrides { + if (is_null(self::$instance)) { + self::$instance = new overrides(); + } + return self::$instance; + } + + /** + * Loads the data for the key provided ready formatted for caching. + * + * @param string|int $key The key to load. + * @return mixed What ever data should be returned, or false if it can't be loaded. + * @throws \coding_exception + */ + public function load_for_cache($key) { + global $DB; + + [$quizid, $ug, $ugid] = explode('_', $key); + $quizid = (int) $quizid; + + switch ($ug) { + case 'u': + $userid = (int) $ugid; + $override = $DB->get_record( + 'quiz_overrides', + ['quiz' => $quizid, 'userid' => $userid], + 'timeopen, timeclose, timelimit, attempts, password' + ); + break; + case 'g': + $groupid = (int) $ugid; + $override = $DB->get_record( + 'quiz_overrides', + ['quiz' => $quizid, 'groupid' => $groupid], + 'timeopen, timeclose, timelimit, attempts, password' + ); + break; + default: + throw new \coding_exception('Invalid cache key'); + } + + // Return null instead of false, because false will not be cached. + return $override ?: null; + } + + /** + * Loads several keys for the cache. + * + * @param array $keys An array of keys each of which will be string|int. + * @return array An array of matching data items. + */ + public function load_many_for_cache(array $keys) { + $results = []; + + foreach ($keys as $key) { + $results[] = $this->load_for_cache($key); + } + + return $results; + } +} diff --git a/mod/quiz/db/caches.php b/mod/quiz/db/caches.php new file mode 100644 index 00000000000..0b0e541c79c --- /dev/null +++ b/mod/quiz/db/caches.php @@ -0,0 +1,35 @@ +. + +/** + * Defined caches used internally by the plugin. + * + * @package mod_quiz + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +defined('MOODLE_INTERNAL') || die(); + +$definitions = [ + 'overrides' => [ + 'mode' => cache_store::MODE_APPLICATION, + 'simplekeys' => true, + 'datasource' => '\mod_quiz\cache\overrides', + ], +]; diff --git a/mod/quiz/lang/en/quiz.php b/mod/quiz/lang/en/quiz.php index 2bb95b02ca8..c5089568ae0 100644 --- a/mod/quiz/lang/en/quiz.php +++ b/mod/quiz/lang/en/quiz.php @@ -131,6 +131,7 @@ $string['browsersecurity_help'] = 'If "Full screen pop-up with some JavaScript s * The quiz will only start if the student has a JavaScript-enabled web-browser * The quiz appears in a full screen popup window that covers all the other windows and has no navigation controls * Students are prevented, as far as is possible, from using facilities like copy and paste'; +$string['cachedef_overrides'] = 'User and group override information'; $string['calculated'] = 'Calculated'; $string['calculatedquestion'] = 'Calculated question not supported at line {$a}. The question will be ignored'; $string['cannotcreatepath'] = 'Path cannot be created ({$a})'; diff --git a/mod/quiz/lib.php b/mod/quiz/lib.php index f20bbfd0329..7d5d3afa836 100644 --- a/mod/quiz/lib.php +++ b/mod/quiz/lib.php @@ -232,10 +232,12 @@ function quiz_delete_override($quiz, $overrideid, $log = true) { // Create the search array for a group override. $eventsearcharray = array('modulename' => 'quiz', 'instance' => $quiz->id, 'groupid' => (int)$override->groupid); + $cachekey = "{$quiz->id}_g_{$override->groupid}"; } else { // Create the search array for a user override. $eventsearcharray = array('modulename' => 'quiz', 'instance' => $quiz->id, 'userid' => (int)$override->userid); + $cachekey = "{$quiz->id}_u_{$override->userid}"; } $events = $DB->get_records('event', $eventsearcharray); foreach ($events as $event) { @@ -244,6 +246,7 @@ function quiz_delete_override($quiz, $overrideid, $log = true) { } $DB->delete_records('quiz_overrides', array('id' => $overrideid)); + cache::make('mod_quiz', 'overrides')->delete($cachekey); if ($log) { // Set the common parameters for one of the events we will be triggering. @@ -1536,6 +1539,8 @@ function quiz_reset_userdata($data) { 'error' => false); } + $purgeoverrides = false; + // Remove user overrides. if (!empty($data->reset_quiz_user_overrides)) { $DB->delete_records_select('quiz_overrides', @@ -1544,6 +1549,7 @@ function quiz_reset_userdata($data) { 'component' => $componentstr, 'item' => get_string('useroverridesdeleted', 'quiz'), 'error' => false); + $purgeoverrides = true; } // Remove group overrides. if (!empty($data->reset_quiz_group_overrides)) { @@ -1553,6 +1559,7 @@ function quiz_reset_userdata($data) { 'component' => $componentstr, 'item' => get_string('groupoverridesdeleted', 'quiz'), 'error' => false); + $purgeoverrides = true; } // Updating dates - shift may be negative too. @@ -1566,6 +1573,8 @@ function quiz_reset_userdata($data) { WHERE quiz IN (SELECT id FROM {quiz} WHERE course = ?) AND timeclose <> 0", array($data->timeshift, $data->courseid)); + $purgeoverrides = true; + // Any changes to the list of dates that needs to be rolled should be same during course restore and course reset. // See MDL-9367. shift_course_mod_dates('quiz', array('timeopen', 'timeclose'), @@ -1577,6 +1586,10 @@ function quiz_reset_userdata($data) { 'error' => false); } + if ($purgeoverrides) { + cache::make('mod_quiz', 'overrides')->purge(); + } + return $status; } diff --git a/mod/quiz/locallib.php b/mod/quiz/locallib.php index c78df8220ca..9cda10953f1 100644 --- a/mod/quiz/locallib.php +++ b/mod/quiz/locallib.php @@ -1923,7 +1923,7 @@ function quiz_process_group_deleted_in_course($courseid) { // It would be nice if we got the groupid that was deleted. // Instead, we just update all quizzes with orphaned group overrides. - $sql = "SELECT o.id, o.quiz + $sql = "SELECT o.id, o.quiz, o.groupid FROM {quiz_overrides} o JOIN {quiz} quiz ON quiz.id = o.quiz LEFT JOIN {groups} grp ON grp.id = o.groupid @@ -1931,12 +1931,16 @@ function quiz_process_group_deleted_in_course($courseid) { AND o.groupid IS NOT NULL AND grp.id IS NULL"; $params = array('courseid' => $courseid); - $records = $DB->get_records_sql_menu($sql, $params); + $records = $DB->get_records_sql($sql, $params); if (!$records) { return; // Nothing to do. } $DB->delete_records_list('quiz_overrides', 'id', array_keys($records)); - quiz_update_open_attempts(array('quizid' => array_unique(array_values($records)))); + $cache = cache::make('mod_quiz', 'overrides'); + foreach ($records as $record) { + $cache->delete("{$record->quiz}_g_{$record->groupid}"); + } + quiz_update_open_attempts(['quizid' => array_unique(array_column($records, 'quiz'))]); } /** diff --git a/mod/quiz/overrideedit.php b/mod/quiz/overrideedit.php index e3c01eb9e18..d02bba6d7e8 100644 --- a/mod/quiz/overrideedit.php +++ b/mod/quiz/overrideedit.php @@ -176,6 +176,8 @@ if ($mform->is_cancelled()) { if (!empty($override->id)) { $fromform->id = $override->id; $DB->update_record('quiz_overrides', $fromform); + $cachekey = $groupmode ? "{$fromform->quiz}_g_{$fromform->groupid}" : "{$fromform->quiz}_u_{$fromform->userid}"; + cache::make('mod_quiz', 'overrides')->delete($cachekey); // Determine which override updated event to fire. $params['objectid'] = $override->id; @@ -192,6 +194,8 @@ if ($mform->is_cancelled()) { } else { unset($fromform->id); $fromform->id = $DB->insert_record('quiz_overrides', $fromform); + $cachekey = $groupmode ? "{$fromform->quiz}_g_{$fromform->groupid}" : "{$fromform->quiz}_u_{$fromform->userid}"; + cache::make('mod_quiz', 'overrides')->delete($cachekey); // Determine which override created event to fire. $params['objectid'] = $fromform->id; diff --git a/mod/quiz/version.php b/mod/quiz/version.php index e93456d755a..5e19c6da52d 100644 --- a/mod/quiz/version.php +++ b/mod/quiz/version.php @@ -24,6 +24,6 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2020121100; +$plugin->version = 2021042700; $plugin->requires = 2020110300; $plugin->component = 'mod_quiz'; From d89d9840483f8f68a8add2e21fac0e71faf1db1c Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Thu, 15 Apr 2021 08:08:36 +1000 Subject: [PATCH 09/11] MDL-71410 mod_quiz: Cache quiz times in modinfo for performance We update the dates with user/group overrides. The calculation of the override in the quiz module is different from the assignment module as the quiz_overrides table des not have a sortorder column. See quiz_update_effective_access(). --- mod/quiz/lib.php | 67 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/mod/quiz/lib.php b/mod/quiz/lib.php index 7d5d3afa836..36d31128fa5 100644 --- a/mod/quiz/lib.php +++ b/mod/quiz/lib.php @@ -2186,7 +2186,8 @@ function quiz_get_coursemodule_info($coursemodule) { global $DB; $dbparams = ['id' => $coursemodule->instance]; - $fields = 'id, name, intro, introformat, completionattemptsexhausted, completionpass, completionminattempts'; + $fields = 'id, name, intro, introformat, completionattemptsexhausted, completionpass, completionminattempts, + timeopen, timeclose'; if (!$quiz = $DB->get_record('quiz', $dbparams, $fields)) { return false; } @@ -2213,9 +2214,73 @@ function quiz_get_coursemodule_info($coursemodule) { $result->customdata['customcompletionrules']['completionminattempts'] = $quiz->completionminattempts; } + // Populate some other values that can be used in calendar or on dashboard. + if ($quiz->timeopen) { + $result->customdata['timeopen'] = $quiz->timeopen; + } + if ($quiz->timeclose) { + $result->customdata['timeclose'] = $quiz->timeclose; + } + return $result; } +/** + * Sets dynamic information about a course module + * + * This function is called from cm_info when displaying the module + * + * @param cm_info $cm + */ +function mod_quiz_cm_info_dynamic(cm_info $cm) { + global $USER; + + $cache = cache::make('mod_quiz', 'overrides'); + $override = $cache->get("{$cm->instance}_u_{$USER->id}"); + + if (!$override) { + $override = (object) [ + 'timeopen' => null, + 'timeclose' => null, + ]; + } + + // No need to look for group overrides if there are user overrides for both timeopen and timeclose. + if (is_null($override->timeopen) || is_null($override->timeclose)) { + $opens = []; + $closes = []; + $groupings = groups_get_user_groups($cm->course, $USER->id); + foreach ($groupings[0] as $groupid) { + $groupoverride = $cache->get("{$cm->instance}_g_{$groupid}"); + if (isset($groupoverride->timeopen)) { + $opens[] = $groupoverride->timeopen; + } + if (isset($groupoverride->timeclose)) { + $closes[] = $groupoverride->timeclose; + } + } + // If there is a user override for a setting, ignore the group override. + if (is_null($override->timeopen) && count($opens)) { + $override->timeopen = min($opens); + } + if (is_null($override->timeclose) && count($closes)) { + if (in_array(0, $closes)) { + $override->timeclose = 0; + } else { + $override->timeclose = max($closes); + } + } + } + + // Populate some other values that can be used in calendar or on dashboard. + if (!is_null($override->timeopen)) { + $cm->override_customdata('timeopen', $override->timeopen); + } + if (!is_null($override->timeclose)) { + $cm->override_customdata('timeclose', $override->timeclose); + } +} + /** * Callback which returns human-readable strings describing the active completion custom rules for the module instance. * From d8806cc8aa55bbb5583a7bcb4f7f268c2f019504 Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Mon, 8 Mar 2021 02:52:04 +1100 Subject: [PATCH 10/11] MDL-71410 mod_quiz: implement activity_dates for the quiz module --- mod/quiz/classes/dates.php | 68 +++++++++++++ mod/quiz/tests/dates_test.php | 175 ++++++++++++++++++++++++++++++++++ 2 files changed, 243 insertions(+) create mode 100644 mod/quiz/classes/dates.php create mode 100644 mod/quiz/tests/dates_test.php diff --git a/mod/quiz/classes/dates.php b/mod/quiz/classes/dates.php new file mode 100644 index 00000000000..ce0f6058406 --- /dev/null +++ b/mod/quiz/classes/dates.php @@ -0,0 +1,68 @@ +. + +/** + * Contains the class for fetching the important dates in mod_quiz for a given module instance and a user. + * + * @package mod_quiz + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_quiz; + +use core\activity_dates; + +/** + * Class for fetching the important dates in mod_quiz for a given module instance and a user. + * + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class dates extends activity_dates { + + /** + * Returns a list of important dates in mod_quiz + * + * @return array + */ + protected function get_dates(): array { + $timeopen = $this->cm->customdata['timeopen'] ?? null; + $timeclose = $this->cm->customdata['timeclose'] ?? null; + $now = time(); + $dates = []; + + if ($timeopen) { + $openlabelid = $timeopen > $now ? 'activitydate:opens' : 'activitydate:opened'; + $dates[] = [ + 'label' => get_string($openlabelid, 'core_course'), + 'timestamp' => (int) $timeopen, + ]; + } + + if ($timeclose) { + $closelabelid = $timeclose > $now ? 'activitydate:closes' : 'activitydate:closed'; + $dates[] = [ + 'label' => get_string($closelabelid, 'core_course'), + 'timestamp' => (int) $timeclose, + ]; + } + + return $dates; + } +} diff --git a/mod/quiz/tests/dates_test.php b/mod/quiz/tests/dates_test.php new file mode 100644 index 00000000000..372ba729d14 --- /dev/null +++ b/mod/quiz/tests/dates_test.php @@ -0,0 +1,175 @@ +. + +/** + * Contains unit tests for mod_quiz\dates. + * + * @package mod_quiz + * @category test + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +declare(strict_types=1); + +namespace mod_quiz; + +use advanced_testcase; +use cm_info; +use core\activity_dates; + +/** + * Class for unit testing mod_quiz\dates. + * + * @copyright 2021 Shamim Rezaie + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class dates_test extends advanced_testcase { + + /** + * Data provider for get_dates_for_module(). + * @return array[] + */ + public function get_dates_for_module_provider(): array { + $now = time(); + $before = $now - DAYSECS; + $earlier = $before - DAYSECS; + $after = $now + DAYSECS; + $later = $after + DAYSECS; + + return [ + 'without any dates' => [ + null, null, null, null, null, null, [] + ], + 'only with opening time' => [ + $after, null, null, null, null, null, [ + ['label' => get_string('activitydate:opens', 'course'), 'timestamp' => $after], + ] + ], + 'only with closing time' => [ + null, $after, null, null, null, null, [ + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $after], + ] + ], + 'with both times' => [ + $after, $later, null, null, null, null, [ + ['label' => get_string('activitydate:opens', 'course'), 'timestamp' => $after], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $later], + ] + ], + 'between the dates' => [ + $before, $after, null, null, null, null, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $before], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $after], + ] + ], + 'dates are past' => [ + $earlier, $before, null, null, null, null, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:closed', 'course'), 'timestamp' => $before], + ] + ], + 'with user override' => [ + $before, $after, $earlier, $later, null, null, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $later], + ] + ], + 'with group override' => [ + $before, $after, null, null, $earlier, $later, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $later], + ] + ], + 'with both user and group overrides' => [ + $before, $after, $earlier, $later, $earlier - DAYSECS, $later + DAYSECS, [ + ['label' => get_string('activitydate:opened', 'course'), 'timestamp' => $earlier], + ['label' => get_string('activitydate:closes', 'course'), 'timestamp' => $later], + ] + ], + ]; + } + + /** + * Test for get_dates_for_module(). + * + * @dataProvider get_dates_for_module_provider + * @param int|null $timeopen Time of opening the quiz. + * @param int|null $timeclose Time of closing the quiz. + * @param int|null $usertimeopen The user override for opening the quiz. + * @param int|null $usertimeclose The user override for closing the quiz. + * @param int|null $grouptimeopen The group override for opening the quiz. + * @param int|null $grouptimeclose The group override for closing the quiz. + * @param array $expected The expected value of calling get_dates_for_module() + */ + public function test_get_dates_for_module(?int $timeopen, ?int $timeclose, + ?int $usertimeopen, ?int $usertimeclose, + ?int $grouptimeopen, ?int $grouptimeclose, + array $expected) { + + $this->resetAfterTest(); + $generator = $this->getDataGenerator(); + /** @var \mod_quiz_generator $quizgenerator */ + $quizgenerator = $generator->get_plugin_generator('mod_quiz'); + + $course = $generator->create_course(); + $user = $generator->create_user(); + $generator->enrol_user($user->id, $course->id); + + $data = ['course' => $course->id]; + if ($timeopen) { + $data['timeopen'] = $timeopen; + } + if ($timeclose) { + $data['timeclose'] = $timeclose; + } + $quiz = $quizgenerator->create_instance($data); + + if ($usertimeopen || $usertimeclose || $grouptimeopen || $grouptimeclose) { + $generator->enrol_user($user->id, $course->id); + $group = $generator->create_group(['courseid' => $course->id]); + $generator->create_group_member(['groupid' => $group->id, 'userid' => $user->id]); + + if ($usertimeopen || $usertimeclose) { + $quizgenerator->create_override([ + 'quiz' => $quiz->id, + 'userid' => $user->id, + 'timeopen' => $usertimeopen, + 'timeclose' => $usertimeclose, + ]); + } + + if ($grouptimeopen || $grouptimeclose) { + $quizgenerator->create_override([ + 'quiz' => $quiz->id, + 'groupid' => $group->id, + 'timeopen' => $grouptimeopen, + 'timeclose' => $grouptimeclose, + ]); + } + } + + $this->setUser($user); + + $cm = get_coursemodule_from_instance('quiz', $quiz->id); + // Make sure we're using a cm_info object. + $cm = cm_info::create($cm); + + $dates = activity_dates::get_dates_for_module($cm, (int) $user->id); + + $this->assertEquals($expected, $dates); + } +} From b315affbd43538ce51c9a8e0ec0dab462ed83cea Mon Sep 17 00:00:00 2001 From: Shamim Rezaie Date: Mon, 19 Apr 2021 07:19:10 +1000 Subject: [PATCH 11/11] MDL-71410 core: Prevent recursively calling getters --- lib/completionlib.php | 4 +++- lib/modinfolib.php | 13 +++++++++++-- mod/folder/lib.php | 2 +- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/lib/completionlib.php b/lib/completionlib.php index e72de6ed75d..451cd8a4bc6 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -1117,7 +1117,9 @@ class completion_info { // Custom activity module completion data. // Cast custom data to array before checking for custom completion rules. - $customdata = (array)$cm->customdata; + // We call ->get_custom_data() instead of ->customdata here because there is the chance of recursive calling, + // and we cannot call a getter from a getter in PHP. + $customdata = (array) $cm->get_custom_data(); // Return early if the plugin does not define custom completion rules. if (empty($customdata['customcompletionrules'])) { return $data; diff --git a/lib/modinfolib.php b/lib/modinfolib.php index 02fe0b5f4ac..0949413b499 100644 --- a/lib/modinfolib.php +++ b/lib/modinfolib.php @@ -1420,9 +1420,14 @@ class cm_info implements IteratorAggregate { return $this->onclick; } /** + * Getter method for property $customdata, ensures that dynamic data is retrieved. + * + * This method is normally called by the property ->customdata, but can be called directly if there + * is a case when it might be called recursively (you can't call property values recursively). + * * @return mixed Optional custom data stored in modinfo cache for this activity, or null if none */ - private function get_custom_data() { + public function get_custom_data() { $this->obtain_dynamic_data(); return $this->customdata; } @@ -1688,6 +1693,9 @@ class cm_info implements IteratorAggregate { * @param mixed $value The value */ public function override_customdata($name, $value) { + if (!is_array($this->customdata)) { + $this->customdata = []; + } $this->customdata[$name] = $value; } @@ -1893,7 +1901,8 @@ class cm_info implements IteratorAggregate { * the module or not. * * As part of this function, the module's _cm_info_dynamic function from its lib.php will - * be called (if it exists). + * be called (if it exists). Make sure that the functions that are called here do not use + * any getter magic method from cm_info. * @return void */ private function obtain_dynamic_data() { diff --git a/mod/folder/lib.php b/mod/folder/lib.php index c450915cb1d..2fe3f23e0f6 100644 --- a/mod/folder/lib.php +++ b/mod/folder/lib.php @@ -439,7 +439,7 @@ function folder_get_coursemodule_info($cm) { * @param cm_info $cm */ function folder_cm_info_dynamic(cm_info $cm) { - if ($cm->customdata) { + if ($cm->get_custom_data()) { // the field 'customdata' is not empty IF AND ONLY IF we display contens inline $cm->set_no_view_link(); }