Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor section restriction callback by using a hook #118

Merged
merged 2 commits into from
May 7, 2024

Conversation

PhMemmel
Copy link
Collaborator

@PhMemmel PhMemmel commented Apr 9, 2024

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

@PhMemmel PhMemmel force-pushed the Add_hook_for_filtering_sections branch 2 times, most recently from 5914711 to 52268ff Compare April 9, 2024 06:35
@Syxton Syxton force-pushed the master branch 2 times, most recently from b7ea080 to d3a086a Compare April 9, 2024 16:20
@PhMemmel PhMemmel force-pushed the Add_hook_for_filtering_sections branch from 52268ff to 81d6af9 Compare May 3, 2024 06:12
@PhMemmel
Copy link
Collaborator Author

PhMemmel commented May 3, 2024

@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 Syxton self-requested a review May 3, 2024 18:33
Copy link
Owner

@Syxton Syxton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how this works. With how rare its use case is, I think this will work nicely to fulfill the need to filter the section lists.

classes/hook/filter_sections.php Outdated Show resolved Hide resolved
classes/hook/filter_sections.php Outdated Show resolved Hide resolved
@Syxton
Copy link
Owner

Syxton commented May 3, 2024

@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
Copy link
Collaborator Author

PhMemmel commented May 3, 2024

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

@PhMemmel
Copy link
Collaborator Author

PhMemmel commented May 6, 2024

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 :)

@PhMemmel PhMemmel force-pushed the Add_hook_for_filtering_sections branch from 5d770a7 to 96219b1 Compare May 6, 2024 13:01
@Syxton
Copy link
Owner

Syxton commented May 6, 2024

@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
Copy link
Collaborator Author

PhMemmel commented May 7, 2024

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.

@Syxton Syxton merged commit 70b0b7e into master May 7, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants