From c660807bbddb3172a9e3b67a08391619ec87f597 Mon Sep 17 00:00:00 2001 From: Simon Thornett Date: Wed, 15 Jan 2025 14:19:30 +0000 Subject: [PATCH] WR422914 Peer review fixes. Added following fixes for the peer review from Dale Davis: * Update scope of __contruct classes as using Singleton * Updated getter function names * Re-added trailing comma to task file * Removed additional ci.yml file * Removed commented code * Updated assign and quiz objects to use stdClass for dynamic properties * Updated lib.php to use frankenstyle prefix --- classes/report_base.php | 10 ++++---- classes/source_base.php | 10 ++++---- lib.php | 8 ++++--- .../activities_in_progress/classes/report.php | 4 ++-- report/activity_dashboard/classes/report.php | 4 ++-- report/heatmap/classes/report.php | 4 ++-- .../classes/output/user_table.php | 4 ++-- report/student_search/classes/report.php | 4 ++-- report/summary_graphs/classes/report.php | 2 +- source/quiz/classes/task/quiz_tracking.php | 3 +-- templates/index.mustache | 4 ++-- tests/quiz_test.php | 24 +++++++++---------- version.php | 2 +- 13 files changed, 42 insertions(+), 41 deletions(-) diff --git a/classes/report_base.php b/classes/report_base.php index 29e1a59..25ccd3b 100644 --- a/classes/report_base.php +++ b/classes/report_base.php @@ -39,9 +39,9 @@ abstract class report_base { /** * Contructor. */ - public function __construct() { - $this->get_required_js(); - $this->get_required_css(); + private function __construct() { + $this->add_required_js(); + $this->add_required_css(); } /** @@ -97,13 +97,13 @@ public function has_access(): bool { * Set up the required JS in the global $PAGE object. * @return void */ - protected function get_required_js() { + protected function add_required_js() { } /** * Set up the required CSS in the global $PAGE object. * @return void */ - protected function get_required_css() { + protected function add_required_css() { } } diff --git a/classes/source_base.php b/classes/source_base.php index 5fefcce..390edf5 100644 --- a/classes/source_base.php +++ b/classes/source_base.php @@ -46,9 +46,9 @@ abstract class source_base { /** * Constructor. */ - public function __construct() { - $this->get_required_js(); - $this->get_required_css(); + private function __construct() { + $this->add_required_js(); + $this->add_required_css(); } /** @@ -121,14 +121,14 @@ abstract public function get_name(): string; * Set up the required JS in the global $PAGE object. * @return void */ - protected function get_required_js() { + protected function add_required_js() { } /** * Set up the required CSS in the global $PAGE object. * @return void */ - protected function get_required_css() { + protected function add_required_css() { } /** diff --git a/lib.php b/lib.php index 9c8f99c..642ce3c 100644 --- a/lib.php +++ b/lib.php @@ -47,7 +47,7 @@ function local_assessfreq_extend_navigation_course(navigation_node $navigation, /** * Get all of the subplugin reports that are enabled and instantiate the class. * - * @param bool $ignoreenabled + * @param bool $ignoreenabled Ignore the is_enabled check. * @return array */ function local_assessfreq_get_reports(bool $ignoreenabled = false): array { @@ -70,7 +70,8 @@ function local_assessfreq_get_reports(bool $ignoreenabled = false): array { * Get all of the subplugin sources that are enabled and instantiate the class. * * @param bool $ignoreenabled - * @param string $requiredmethod + * @param string $requiredmethod Only load sources that contain the required method. + * Usesful for calling specific sources for a report. * @return array */ function local_assessfreq_get_sources(bool $ignoreenabled = false, $requiredmethod = ''): array { @@ -159,7 +160,8 @@ function local_assessfreq_get_years($preference): array { * This is based on which sources have been enabled. * * @param array $preferences - * @param string $requiredmethod + * @param string $requiredmethod Only load sources that contain the required method. + * Usesful for calling specific sources for a report. * @return array $modules The enabled modules. * @throws coding_exception */ diff --git a/report/activities_in_progress/classes/report.php b/report/activities_in_progress/classes/report.php index 60bdbfe..462285e 100644 --- a/report/activities_in_progress/classes/report.php +++ b/report/activities_in_progress/classes/report.php @@ -117,7 +117,7 @@ public function get_contents(): string { /** * {@inheritDoc} */ - protected function get_required_js(): void { + protected function add_required_js(): void { global $PAGE; $PAGE->requires->js_call_amd( @@ -130,7 +130,7 @@ protected function get_required_js(): void { /** * {@inheritDoc} */ - protected function get_required_css(): void { + protected function add_required_css(): void { global $PAGE; $PAGE->requires->css('/local/assessfreq/report/activities_in_progress/styles.css'); diff --git a/report/activity_dashboard/classes/report.php b/report/activity_dashboard/classes/report.php index 86a335f..221dd9d 100644 --- a/report/activity_dashboard/classes/report.php +++ b/report/activity_dashboard/classes/report.php @@ -86,7 +86,7 @@ public function get_contents(): string { /** * {@inheritDoc} */ - protected function get_required_js(): void { + protected function add_required_js(): void { global $PAGE; $PAGE->requires->js_call_amd( @@ -99,7 +99,7 @@ protected function get_required_js(): void { /** * {@inheritDoc} */ - protected function get_required_css(): void { + protected function add_required_css(): void { global $PAGE; $PAGE->requires->css('/local/assessfreq/report/activity_dashboard/styles.css'); diff --git a/report/heatmap/classes/report.php b/report/heatmap/classes/report.php index c3741ec..7c1b3fc 100644 --- a/report/heatmap/classes/report.php +++ b/report/heatmap/classes/report.php @@ -85,7 +85,7 @@ public function get_contents(): string { /** * {@inheritDoc} */ - protected function get_required_js(): void { + protected function add_required_js(): void { global $PAGE; $PAGE->requires->js_call_amd('assessfreqreport_heatmap/heatmap', 'init', [$PAGE->course->id]); @@ -94,7 +94,7 @@ protected function get_required_js(): void { /** * {@inheritDoc} */ - protected function get_required_css(): void { + protected function add_required_css(): void { global $PAGE; // The CSS for the heatmap is based on plugin config. As such this needs to be in-line. $PAGE->requires->css('/local/assessfreq/report/heatmap/dynamic-styles.php'); diff --git a/report/student_search/classes/output/user_table.php b/report/student_search/classes/output/user_table.php index 94ec4df..a05fb4f 100644 --- a/report/student_search/classes/output/user_table.php +++ b/report/student_search/classes/output/user_table.php @@ -29,7 +29,7 @@ require_once($CFG->libdir . '/tablelib.php'); -use assessfreqsource_quiz\Source; +use assessfreqsource_quiz; use coding_exception; use html_writer; use local_assessfreq\frequency; @@ -474,7 +474,7 @@ public function query_db($pagesize, $useinitialsbar = false): void { $capabilities = $frequency->get_module_capabilities('quiz'); // Get the quizzes that we want users for. - $quizsource = new Source(); + $quizsource = assessfreqsource_quiz\source::get_instance(); $allquizzes = $quizsource->get_quiz_summaries($this->now, $this->hoursahead, $this->hoursbehind, false); $inprogressquizzes = $allquizzes['inprogress']; diff --git a/report/student_search/classes/report.php b/report/student_search/classes/report.php index 9887e36..15ac49b 100644 --- a/report/student_search/classes/report.php +++ b/report/student_search/classes/report.php @@ -86,7 +86,7 @@ public function get_contents(): string { /** * {@inheritDoc} */ - protected function get_required_js(): void { + protected function add_required_js(): void { global $PAGE; $PAGE->requires->js_call_amd( @@ -99,7 +99,7 @@ protected function get_required_js(): void { /** * {@inheritDoc} */ - protected function get_required_css(): void { + protected function add_required_css(): void { global $PAGE; $PAGE->requires->css('/local/assessfreq/report/student_search/styles.css'); diff --git a/report/summary_graphs/classes/report.php b/report/summary_graphs/classes/report.php index ed8cafe..89dfacc 100644 --- a/report/summary_graphs/classes/report.php +++ b/report/summary_graphs/classes/report.php @@ -102,7 +102,7 @@ public function get_contents(): string { /** * {@inheritDoc} */ - protected function get_required_js(): void { + protected function add_required_js(): void { global $PAGE; $PAGE->requires->js_call_amd('assessfreqreport_summary_graphs/summary_graphs', 'init'); diff --git a/source/quiz/classes/task/quiz_tracking.php b/source/quiz/classes/task/quiz_tracking.php index 4da148d..e9d7e47 100644 --- a/source/quiz/classes/task/quiz_tracking.php +++ b/source/quiz/classes/task/quiz_tracking.php @@ -23,7 +23,6 @@ */ namespace assessfreqsource_quiz\task; -use assessfreqsource_quiz\source; use core\task\scheduled_task; use local_assessfreq\frequency; use stdClass; @@ -66,7 +65,7 @@ public function execute(): void { $actionstart = 1594788000; } - $source = new source(); + $source = \assessfreqsource_quiz\source::get_instance(); $frequency = new frequency(); $quizzes = $source->get_tracked_quizzes_with_overrides($actionstart); diff --git a/templates/index.mustache b/templates/index.mustache index 11aee6b..ced29a0 100644 --- a/templates/index.mustache +++ b/templates/index.mustache @@ -25,9 +25,9 @@
-

+

-

+
{{> local_assessfreq/tabs}}
diff --git a/tests/quiz_test.php b/tests/quiz_test.php index 7d55236..9a86365 100644 --- a/tests/quiz_test.php +++ b/tests/quiz_test.php @@ -601,7 +601,7 @@ public function setup_mock_quiz_data(): array { * Test getting quiz override info. */ public function test_get_quiz_override_info(): void { - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $context = \context_module::instance($this->quiz1->cmid); // We're testing a private method, so we need to setup reflector magic. @@ -620,7 +620,7 @@ public function test_get_quiz_override_info(): void { */ public function test_get_quiz_questions(): void { global $DB; - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); [$course, $cm] = get_course_and_cm_from_instance($this->quiz1->id, 'quiz'); @@ -645,7 +645,7 @@ public function test_get_quiz_questions(): void { */ public function test_get_quiz_data(): void { - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $result = $quizdata->get_quiz_data($this->quiz1); $this->assertEquals('5 July 2020, 9:00 AM', $result->earlyopen); @@ -661,7 +661,7 @@ public function test_get_quiz_data(): void { * Test quiz override tracking. */ public function test_get_tracked_overrides(): void { - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $method = new \ReflectionMethod('\assessfreqsource_quiz\Source', 'get_tracked_overrides'); $method->setAccessible(true); // Allow accessing of private method. @@ -680,7 +680,7 @@ public function test_get_tracked_overrides(): void { * Test quiz tracking. */ public function test_get_tracked_quizzes(): void { - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $method = new \ReflectionMethod('\assessfreqsource_quiz\Source', 'get_tracked_quizzes'); $method->setAccessible(true); // Allow accessing of private method. @@ -713,7 +713,7 @@ public function test_get_tracked_quizzes_with_overrides(): void { $DB->insert_record('quiz_overrides', $override); - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $method = new \ReflectionMethod('\assessfreqsource_quiz\Source', 'get_tracked_quizzes_with_overrides'); $method->setAccessible(true); // Allow accessing of private method. @@ -756,7 +756,7 @@ public function test_local_assessfreq_get_loggedin_users(): void { */ public function test_get_quiz_attempts(): void { - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $method = new \ReflectionMethod('\assessfreqsource_quiz\Source', 'get_quiz_attempts'); $method->setAccessible(true); // Allow accessing of private method. @@ -795,7 +795,7 @@ public function test_get_quiz_tracking(): void { $now = 1594788000; $this->setup_quiz_tracking($now, $this->quiz1->id); - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $method = new \ReflectionMethod('\assessfreqsource_quiz\Source', 'get_tracking'); $method->setAccessible(true); // Allow accessing of private method. @@ -816,7 +816,7 @@ public function test_get_inprogress_counts(): void { $this->setup_quiz_tracking($now, $this->quiz3->id); $this->setup_quiz_tracking($now, $this->quiz4->id); - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $result = $quizdata->get_inprogress_count($now, 0, 0, false); $this->assertEquals(3, $result['assessments']); @@ -827,7 +827,7 @@ public function test_get_inprogress_counts(): void { * Test getting finished quizzes. */ public function test_get_quizzes_finished(): void { - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $now = 1594788000; $result = $quizdata->get_quiz_summaries($now, HOURSECS, HOURSECS); @@ -843,7 +843,7 @@ public function test_get_quizzes_inprogress(): void { $this->setup_quiz_tracking($now, $this->quiz3->id); $this->setup_quiz_tracking($now, $this->quiz4->id); - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $result = $quizdata->get_quiz_summaries($now, HOURSECS, HOURSECS); $this->assertCount(3, $result['inprogress']); @@ -857,7 +857,7 @@ public function test_get_quizzes_inprogress(): void { * Test getting upcomming quizzes. */ public function test_get_quizzes_upcomming(): void { - $quizdata = new \assessfreqsource_quiz\source(); + $quizdata = \assessfreqsource_quiz\source::get_instance(); $now = 1594780800; $result = $quizdata->get_quiz_summaries($now, HOURSECS, HOURSECS); diff --git a/version.php b/version.php index 6a688c9..c32d852 100644 --- a/version.php +++ b/version.php @@ -29,5 +29,5 @@ $plugin->release = 2025010300; $plugin->version = 2025010300; $plugin->requires = 2023100900; // Requires 4.3. -$plugin->supported = [403, 405]; +$plugin->supported = [403, 404]; $plugin->maturity = MATURITY_STABLE;