search

Syxton / moodle-block_massaction

THE OFFICIAL Mass Actions block for Moodle 3.9 and beyond
9 stars 14 forks source link

Refactor section restriction callback by using a hook #118

Closed PhMemmel closed 6 months ago

PhMemmel commented 7 months ago

This should rework the already merged #99 and replace #117

PhMemmel commented 6 months ago

@Syxton I would be happy about your input on this so far, especially about the design of the hook class. There of course are plenty of possibilities how to design them, for example only offer one method to remove restricted sections like I am doing currently, or offer a method to filter sections etc. Do you have any thoughts on this?

Syxton commented 6 months ago

@PhMemmel I think I want to have this in place for the 4.4 release. So I'll wait to create the release. But if we get close to the 10th, I'll make the release and we can pull this in later. Gotta have that badge :)

PhMemmel commented 6 months ago

We got this in time, no worries. Will probably manage to take care of your feedback and finalize this on Monday 👍

PhMemmel commented 6 months ago

Hi @Syxton,

I took another look and decided to rework the hook by splitting it up into two hooks. Reason: Hooks are some kind of external api without being well documented ;-), so they should be easy to understand and use. The current approach was a bit complicated, so I split it up. To avoid code duplication one is tempted to use inheritance to avoid this, however this is discouraged in the moodle hooks docs, so I used a trait. I'm not 100% happy with that, but it still lets me sleep peacefully... ;-)

Let me know, what you think. For testing I also updated the branch with my changes to format_topics (it's moodle 4.5 current develop now).

Ah, nearly forgot: I also reworked the way the filtered sections are being passed into the javascript: It was a bit ugly how the array is being passed through 3 and mores functions. So I just put it in the DOM now and let it pick up by the JS function which really needs it.

I also fixed some codechecker things. Sorry for being not very careful regarding the commit history. I suggest in the end you just squash everything here :)

Syxton commented 6 months ago

@PhMemmel Sorry it took so long to approve this. I ended up getting deep in the weeds about how Moodle implemented hooks. It is my understanding that Moodle hooks are more like events, but they called them hooks because events in Moodle are already a thing. Anyway. It makes good sense to me to split those and the trait and hidden input really are decent clean code solution. If you are good with where this request is, I'll merge and get a 4.4+ release out today.

PhMemmel commented 6 months ago

Hi @Syxton, first of all: that's definitely not a long time :)

Besides that thank you very much for the feedback. If you have no more, this is ready to integrate from my point of view. I did not update the version.php properly yet, so this still needs to be done before releasing.