WR422914 Subplugin refactor #161
Conversation
|
Hi @SimonThornett |
|
Hi @dmitriim, Not a problem. I know that there are some compatibility issues with the modal since 4.2, but I'll rebase this from 403, add in the modal fix that I have for that branch and add some version based loading for the different modal JS so that we can have one version that covers all. |
|
@SimonThornett yeah. Modal factory has been deprecated. See this example of how to deal with that a18328d |
|
@SimonThornett do you have ETA for this work to be completed? It's a big change so all other PRs can be affected as well as you'd need to rebase every time anything lands. I'm really keen to get this landed ASAP to make everyone life easier. Happy to review it when ready. |
|
Thanks @dmitriim, I actually did a similar thing on the 4.4 branch I was working on here. I'm working on the update as we speak and my colleague @sarahjcotton will likely be able to pick up the review as she is also familiar with the work that was done here having done the original specification. Would be great for another pair of eyes, but I appreciate it is a substantial piece of work. I'm just working on a couple of additional improvements to performance and functionality and I'll update the branch. |
Complete refactor of the plugin to move reports and modules into report and source subplugins respectively.
SimonThornett
force-pushed
the
WR422914
branch
from
1078c33 to
9bac8fb
Compare
SimonThornett
changed the base branch from
MOODLE_400_STABLE
to
MOODLE_403_STABLE
|
Hey @dmitriim, I've updated the branch to base off of the 403 stable. I haven't passed it over for internal review yet as I'm just sorting the PHP Unit tests. Those can be reviewed separately so I'll push those as a different commit and ask someone our side to have a look at the current commit. |
|
Latest commit contains the updated unit tests and behat tests. All unit tests pass with the following output: Time: 07:41.730, Memory: 137.00 MB behat pass 23 of 23 tests in 33 seconds |
daledavies
requested review from
daledavies
and removed request for
marxjohnson
| // 'other' => ['action' => 'user', 'duration' => $actionduration], | ||
| //]); | ||
| //$event->trigger(); | ||
| //mtrace('local_assessfreq: Processing user events finished in: ' . $actionduration . ' seconds'); |
There was a problem hiding this comment.
This appears to have been left in by accident?
There was a problem hiding this comment.
Worth addressing CI failures
- There seem to be cases where docblock and function definitiion don't match, parameters/return types missing or not matching etc.
- Instances of spaces before colons when defining return types for functions (report_base and source_base for example).
- I'm not 100% sure about the use of @inheritdoc.
- The new functions in lib.php should use frankenstyle prefix https://moodledev.io/general/development/policies/codingstyle#functions-and-methods. Or could they be moved into a more appropriate class?
- Multiple files complaining about not ending with blank line.
- A few instances where blocks of code are commented out.
- Test failures etc.
General
There are a lot of references (guarded my method_exists checks) to methods that do not exist in the report_base or source_base abstract classes. I wonder if you could create interfaces for these, then you have something that defines the method signature contract. At the moment you can't be sure that a subplugin has defined one of these methods correctly when you are calling it. Rather than using method_exists you could check to see if the class inherits the interface etc.
I've also made a few notes around the place where I think you could abstract some areas of repeated code.
PHP 8.2 compatibility
There are areas where dynamic properties are added to classes, in some cases this is for objects that will probably be instances of stdClass (quiz_settings::get_quiz() for example returns stdClass), but others appear to be instances of actual classes (like assign()). I'm wondering if this might cause issues with PHP 8.2?
| * @param string $module The module to get events for or all events. | ||
| * @param int $from The timestamp to get events from. | ||
| * @param int $to The timestamp to get events to. | ||
| * @param bool $cache If false cache won't be used fresh data will be retrieved from DB. | ||
| * @return array $events An array of site events | ||
| */ | ||
| public function get_site_events(string $module = 'all', int $from = 0, int $to = 0, bool $cache = true): array { | ||
| public function get_site_events(int $courseid, string $module = 'all', int $from = 0, int $to = 0, bool $cache = true): array { |
| } | ||
| } | ||
|
|
||
| $assign->overridecount = count($overridenparticipants); |
There was a problem hiding this comment.
I can't find this property in the assign() class
|
|
||
| $assign->submissions = $submissions; | ||
| $assign->firststart = $firststart; | ||
| $assign->laststart = $laststart; |
There was a problem hiding this comment.
Same for these and a bunch of others too.
|
|
||
| private static array $instances = []; | ||
|
|
||
| public function __construct() { |
There was a problem hiding this comment.
I think you can make this private to prevent instantiation without using get_instance(), if you only ever want report subplugin instances to be singletons.
| */ | ||
| private static array $cache = []; | ||
|
|
||
| public function __construct() { |
| $methodname = 'get_' . $data->call . '_chart'; | ||
| } else { | ||
| throw new moodle_exception('Call not allowed'); | ||
| function get_sources($ignoreenabled = false, $requiredmethod = '') : array { |
There was a problem hiding this comment.
Should this use a frankenstyle prefix?
| } else { | ||
| throw new moodle_exception('Call not allowed'); | ||
| } | ||
| function get_months_ordered() : array { |
There was a problem hiding this comment.
Should this use a frankenstyle prefix?
| */ | ||
| function local_assessfreq_output_fragment_new_base_form($args): string { | ||
| function get_years($preference) : array { |
There was a problem hiding this comment.
Should this use a frankenstyle prefix?
| */ | ||
| function local_assessfreq_output_fragment_get_student_search_table($args): string { | ||
| global $CFG, $PAGE; | ||
| function get_modules($preferences, $requiredmethod= '') : array { |
There was a problem hiding this comment.
Should this use a frankenstyle prefix?
| */ | ||
| function local_assessfreq_output_fragment_get_quizzes_inprogress_table($args): string { | ||
| global $PAGE; | ||
| function get_loggedin_users(array $userids): stdClass { |
There was a problem hiding this comment.
Should this use a frankenstyle prefix?
Taken from the README.md:
Post version
2024040300this plugin was completely refactored to support more reports and modules.In addition the report now loads in a tabbed format instead of in different locations.
Each report is now a subplugin within the
reportdirectoryThe subplugins report class should extend from the \local_assessfreq\report_base class
Capability checks were reworked to be relative to the location that they are being loading from. The initial version
has the following capabilities:
however each future subplugin can define their own access checks by using the abstract
has_accessmethod.Accessing the reports from a course (link now added to the course context menu under reports) will do the capability check at the
course context level, otherwise system level will be used.
The reports themselves should also be restricted based on the $PAGE->course if it is not the SITEID as this is set
during the intial load of the index.php file.
Each module is now a subplugin within the
sourcedirectoryThe subplugins source class should extend from the \local_assessfreq\source_base class
Along with general performance improvements additional settings have been added to reduce the load on the reports: