fix deprecated function get_participants_counts and fix js error#204
fix deprecated function get_participants_counts and fix js error#204emily-lambrou wants to merge 5 commits intocatalyst:MOODLE_405_STABLEfrom
Conversation
|
@danmarsden for your review and aprroval |
emily-lambrou
changed the title
|
@danmarsden it seems that there is a warning while running the workflow Warning: The phpcpd command is deprecated and will be removed in 5.0.0. No replacement is planned. Please advice if there is something pending from my side. |
|
thanks - ignore the phpcpd thing - the bigger issue is the codechecker errors a bit higher in that output (which you can also run using the local_codechecker plugin locally) - most of those look to be pre-existing though so best to ignore any that relate to files not changed here - I'll drop some comments inline on the PR with some issues.. |
There was a problem hiding this comment.
this file shouldn't be included in the PR - it's related to your IDE and not something that should sit within the plugin codebase.
There was a problem hiding this comment.
thanks, i deleted it
|
|
||
| foreach ($rs as $user) { | ||
| if ($total === 0) { | ||
| $total = (int)$user->fullcount; // <- confirmed by your dump |
There was a problem hiding this comment.
the phpdoc comment here doesn't comply with the coding guidelines - must start with a capital letter and end with a fullstop so will throw codechecker errors - I'd just remove the comment.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
this added carriage return will throw coding guideline errors as there are two empty lines/carriage returns.
| $options->noteStateNames = note_get_state_names(); | ||
| } | ||
| echo '</form>'; | ||
| $PAGE->requires->js_call_amd('core_user/participants', 'init', [$options]); |
There was a problem hiding this comment.
is the $options array used anymore? - if not we should be deleting/clearing it properly above?
There was a problem hiding this comment.
Good point — the only other use of $options was setting noteStateNames (via note_get_state_names()). That was only ever consumed by the AMD init call. Since we’ve replaced the AMD args with a trimmed $jsoptions object, the noteStateNames assignment is now unused too.
So yes, it’s safe to delete the $options->noteStateNames line along with the old $options block. If we ever need those values in JS again, we can add them explicitly to $jsoptions.
Delete .idea/.gitignore
remove the comment
no empty lines
delete the unnecessary variable for options
|
@danmarsden Thank you for your time and review. Could you please have a look again in my commits/corrections and my comments? |
|
Thanks for the updated PR! - I've rebased it manually and pulled out the $options code that isn't needed anymore and merged it into the MOODLE_405_STABLE branch. |
|
Thanks @danmarsden ! |
2 participants
Fixes #203