Add callbacks to add or keep section number #117

tuanngocnguyen · 2024-03-14T22:57:18Z

Hi @Syxton ,

I have created a PR for this issue:

#115

I have added 2 callbacks for course format to determine if a user can select "Add new section"/"Keep original section"

Would you please have a look and let me know your opinion?

Thanks

TomoTsuyuki

Thanks for making PR for the issue.
It looks ok but some variable/function name are confusing.

TomoTsuyuki · 2024-03-15T04:37:57Z

classes/form/section_select_form.php

@@ -45,6 +45,8 @@
 * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
 */
 class section_select_form extends moodleform {
+    /** Int array of allowed values */
+    protected $allowedvalues = [];


the variable name does not really mean the values.
maybe $targetsections or something is better?

Hi @TomoTsuyuki

Thanks for your review, I have changed it to $allowedsections, only contains section numbers which are allowed to be chosen.

TomoTsuyuki · 2024-03-15T04:42:56Z

classes/form/section_select_form.php

+     * @param array $attributes the attributes
+     * @return void
+     */
+    private function is_allowed(bool $allowcondition, int $optionnumber, array &$attributes): void {


this function name sounds like for checking value and return bool if condition is ok.
maybe set_radioption or something?

I have changed this to set_radio_option

TomoTsuyuki · 2024-03-15T04:43:39Z

classes/massactionutils.php

+     * @param string $callbackname the name of the callback function
+     * @return mixed|null none determined return type
+     */
+    private static function run_course_format_callback(int $courseid, string $format, string$callbackname) {


Do we need a space between string and $callbackname?

I have added a space

TomoTsuyuki · 2024-03-15T04:45:16Z

classes/form/section_select_form.php

+        $this->is_allowed($canaddnewsection, $targetsectionnum + 1, $attributes);
+        $radioarray[] = $mform->createElement('radio', 'targetsectionnum', '',
+            get_string('newsection', 'block_massaction'), $targetsectionnum + 1, $attributes);
+


FOUND 1 ERROR AFFECTING 1 LINE
140 | ERROR | [x] Functions must not contain multiple empty lines in a row; found 2 empty lines

I have removed the redundant empty line

Thanks @TomoTsuyuki for your review

I have updated the patch

tuanngocnguyen · 2024-03-26T22:48:49Z

Hi @Syxton ,

Hope you are doing fine.

Would you please help with the review once you have time and let me know if there is any issue I need to address?

Regards

Syxton

@tuanngocnguyen I have looked over these changes and they seem like they are good. @PhMemmel Do you have any concerns with moving this forward?

PhMemmel · 2024-03-27T20:21:16Z

@Syxton Won't be able to have a look for at least 2 weeks, sorry. If you want to wait, I will have a look and provide feedback.

PhMemmel · 2024-03-27T20:25:03Z

Just one thought when speaking of callbacks... Without having a closer look at the code ... would it be maybe better to use the new hook api for that implementation? So all kinds of Plugin could hook into this and we do not need to look for callbacks in course formats?

tuanngocnguyen · 2024-03-27T22:45:29Z

Hi @Syxton @PhMemmel ,

Thanks for your comment.

I am happy to wait for your review.

I think the limitation feature should be applied to course format.

If other plugins can implement the hooks/callbacks, we may go into a case where a course has multiple blocks with the feature. One allows to add new section, one disallows to add new section. I may lead to unpredictable behavior.
On the other hand, a course has only one format, so we will not run into this issue.

Please let me know your opinion.

Regards

PhMemmel · 2024-04-09T06:43:45Z

Hi everyone,

I now took some time to come back at this and thought a little bit more about this approach:

IMO it is not a good approach and not good style to create dependencies between plugins. Unfortunately, I could not find the place in the moodle dev docs where this is mentioned, but I'm pretty sure I've also read it there.
I admit, these kinds of callbacks are at least loosely coupled, but I consider it as very bad style that a course format main class contains some callback functions that are not related to the course format code or moodle core apis at all, but instead are hooked in by a plugin distantly related to the course format.
Also, block_massaction needs to contain course format related code when it is not necessary
Callbacks are being reworked and removed from the whole moodle code and be replaced by the hooks API, so I suggest to use the hooks API for this kind of scenario. This makes these scenarios much more controlled and clean.
The hooks API also allows other plugins to hook into block_massaction, so we do not have code for a super specific use case of special course formats, but instead we offer a hook for every plugin to use (if wanted).

Because of this I have reworked the current callback from #99 and included an alternative for this PR using a hook, see #118.
It's about 95% done IMO, it basically just lacks some unit tests which test the hook functionality.

To show, how this hook can be used I also pushed a moodle branch demonstrating the hook usage for format_topics: moodle/moodle@main...PhMemmel:moodle:Hook_example_block_massaction

@Syxton and also @tuanngocnguyen and @TomoTsuyuki: Let me know what you are thinking of this. From my point of view this would be the better approach for on the one hand addressing your (very specific) use cases and on the other hand keep the interactions with other plugins well structured and controlled.

Regarding your concerns @tuanngocnguyen about the course format having to be the only plugin controlling the section restrictrions: I designed the hook in a way so that every plugin which hooks in can disable additional sections and options, but never can add them again. So your course format callback should be good.
Also, the admin of a site can have a look in the hooks overview page in the website administration and see which hook listeners are registered, so it's easy to determine if there are other plugins hooking in. If you still do not feel well about this, it would also be possible to make the hook stoppable and to specify a priority to make sure your course format plugin will be the first.

@Syxton As my PR is more or less an API change we would have to think about how to deal with branches and releases. Also, this PR currently uses dependency injection features which is requiring moodle 4.4. If we need to be downwards compatible to moodle 4.3 this is easily reworkable however, makes it hard to unit test though.

Syxton · 2024-04-09T14:56:00Z

@PhMemmel I do like the hooks approach more due to the push towards hooks and their flexibility/control. I will create a branch for our current release through 4.3, and then we can push #118 onto master (the 4.4+ branch) moving forward once completed.

PhMemmel · 2024-04-10T17:02:03Z

@Syxton Glad to hear and thank you very much for the feedback. Will wait for some additional feedback from @tuanngocnguyen @TomoTsuyuki, especially if my implementation addresses all of their use cases.

After that I'll finalize my PR, will finally put it against master branch which then will be our new branch for Moodle 4.4 onwards.

tuanngocnguyen · 2024-04-11T01:28:20Z

Hi @PhMemmel @Syxton ,

Thanks for your work on the issue.
The PR looks good to me. However, we are still using Moodle 4.1 and PHP 7.4, there are some incompatible issues:

class "\core\di" does not exist in Moodle 4.1
Read only properties are only allowed since PHP 8.1
Constructor property promotion is only allowed since PHP 8.0

As Moodle 4.1 is LTS, would it still be possible for our PR to be integrated into a separated branch for the moodle version ? Please let me know if you require additional work for the MOODLE_401_STABLE branch?

Regards

tuanngocnguyen · 2024-04-11T01:44:46Z

I have rebased the PR to MOODLE_401_STABLE

PhMemmel · 2024-04-15T12:22:55Z

Thank you @tuanngocnguyen for the feedback.

I feared you would answer this :)

Trying to be as objective as possible while considering all concerns I personally come to the conclusion that it would be best overall to not integrate this PR but instead use the resources on the review of the hook rework/approach.

@tuanngocnguyen As we will not develop the current code any further it probably should be no big effort for you to integrate your PR in a temporary fork. As soon as you update, you will be able to switch to the new master branch and implement the new hook.

@Syxton @TomoTsuyuki @tuanngocnguyen Is this fine for everyone?

Syxton · 2024-04-15T13:13:37Z

@PhMemmel I agree. I think that it is late in the game to be adding features to the 401 branch. Moodle LTS general support ended in December of '23 with only security support till 2025. I can only speak for myself that my attention is spread very thin, so I think it best to instead of adding a feature for 4.1, we focus on the long term solution with hooks and @TomoTsuyuki & @tuanngocnguyen can keep their fork with this PR until they are able to upgrade their Moodle to the hooks capable version.

tuanngocnguyen · 2024-04-17T06:51:08Z

Hi @PhMemmel @Syxton

Thanks for your comments.
We will go with a fork for this PR.

Regards

tuanngocnguyen · 2024-05-10T00:24:19Z

Thanks all for your work.

I am closing this WR.

TomoTsuyuki reviewed Mar 15, 2024

View reviewed changes

tuanngocnguyen force-pushed the issue115_limitation_to_add_course_section branch 3 times, most recently from 33c3af5 to 3506e74 Compare March 17, 2024 23:10

Syxton reviewed Mar 27, 2024

View reviewed changes

PhMemmel mentioned this pull request Apr 9, 2024

Refactor section restriction callback by using a hook #118

Merged

Syxton force-pushed the master branch 2 times, most recently from b7ea080 to d3a086a Compare April 9, 2024 16:20

tuanngocnguyen changed the base branch from master to MOODLE_401_STABLE April 11, 2024 01:35

Add callbacks to add or keep section number

975d7cf

tuanngocnguyen force-pushed the issue115_limitation_to_add_course_section branch from 3506e74 to 975d7cf Compare April 11, 2024 01:40

tuanngocnguyen closed this May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add callbacks to add or keep section number #117

Add callbacks to add or keep section number #117

tuanngocnguyen commented Mar 14, 2024 •

edited

Loading

TomoTsuyuki left a comment

TomoTsuyuki Mar 15, 2024

tuanngocnguyen Mar 15, 2024 •

edited

Loading

TomoTsuyuki Mar 15, 2024

tuanngocnguyen Mar 15, 2024

TomoTsuyuki Mar 15, 2024

tuanngocnguyen Mar 15, 2024 •

edited

Loading

TomoTsuyuki Mar 15, 2024

tuanngocnguyen Mar 15, 2024

tuanngocnguyen Mar 15, 2024

tuanngocnguyen commented Mar 26, 2024

Syxton left a comment

PhMemmel commented Mar 27, 2024

PhMemmel commented Mar 27, 2024

tuanngocnguyen commented Mar 27, 2024 •

edited

Loading

PhMemmel commented Apr 9, 2024

Syxton commented Apr 9, 2024

PhMemmel commented Apr 10, 2024

tuanngocnguyen commented Apr 11, 2024 •

edited

Loading

tuanngocnguyen commented Apr 11, 2024

PhMemmel commented Apr 15, 2024

Syxton commented Apr 15, 2024

tuanngocnguyen commented Apr 17, 2024

tuanngocnguyen commented May 10, 2024

Add callbacks to add or keep section number #117

Add callbacks to add or keep section number #117

Conversation

tuanngocnguyen commented Mar 14, 2024 • edited Loading

TomoTsuyuki left a comment

Choose a reason for hiding this comment

TomoTsuyuki Mar 15, 2024

Choose a reason for hiding this comment

tuanngocnguyen Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

TomoTsuyuki Mar 15, 2024

Choose a reason for hiding this comment

tuanngocnguyen Mar 15, 2024

Choose a reason for hiding this comment

TomoTsuyuki Mar 15, 2024

Choose a reason for hiding this comment

tuanngocnguyen Mar 15, 2024 • edited Loading

Choose a reason for hiding this comment

TomoTsuyuki Mar 15, 2024

Choose a reason for hiding this comment

tuanngocnguyen Mar 15, 2024

Choose a reason for hiding this comment

tuanngocnguyen Mar 15, 2024

Choose a reason for hiding this comment

tuanngocnguyen commented Mar 26, 2024

Syxton left a comment

Choose a reason for hiding this comment

PhMemmel commented Mar 27, 2024

PhMemmel commented Mar 27, 2024

tuanngocnguyen commented Mar 27, 2024 • edited Loading

PhMemmel commented Apr 9, 2024

Syxton commented Apr 9, 2024

PhMemmel commented Apr 10, 2024

tuanngocnguyen commented Apr 11, 2024 • edited Loading

tuanngocnguyen commented Apr 11, 2024

PhMemmel commented Apr 15, 2024

Syxton commented Apr 15, 2024

tuanngocnguyen commented Apr 17, 2024

tuanngocnguyen commented May 10, 2024

tuanngocnguyen commented Mar 14, 2024 •

edited

Loading

tuanngocnguyen Mar 15, 2024 •

edited

Loading

tuanngocnguyen Mar 15, 2024 •

edited

Loading

tuanngocnguyen commented Mar 27, 2024 •

edited

Loading

tuanngocnguyen commented Apr 11, 2024 •

edited

Loading