mdjnelson / moodle-mod_customcert

Enables the creation of dynamically generated certificates with complete customisation via the web browser.
https://moodle.org/plugins/mod_customcert
GNU General Public License v3.0
89 stars 155 forks source link

Ensure events are only triggered when necessary #595

Closed mdjnelson closed 5 months ago

mdjnelson commented 5 months ago
  1. Suppress template_updated when creating new template.

  2. Fix Save changes/Add element so template_updated triggered if name changed.

  3. Fix context for page_created when loading a template in course instance (was system now course module).

mdjnelson commented 5 months ago

Hi @leonstr, sorry it took me a while to see you had pushed code but there was no PR yet, so created this. Does this include all fixes for events or is there still more to do? I would like to fix up the events code before I push another release.

mdjnelson commented 5 months ago

This issue should ideally solve all the issues found in https://github.com/mdjnelson/moodle-mod_customcert/issues/570.

leonstr commented 5 months ago

@mdjnelson From memory this is not yet complete. Apologies for the delay. I'll aim to pick this up in the next week with a view to getting this wrapped up.

mdjnelson commented 5 months ago

Thanks @leonstr!

mdjnelson commented 5 months ago

Ping @leonstr! :)

leonstr commented 5 months ago

Just acknowledging the ping. I've been working on this on Saturday and today. A bit more testing is required, I'll try to have this done in the next day.

leonstr commented 5 months ago

Have updated PR. Hopefully this fixes all issues listed in #570 and doesn't break anything (PHPUnit tests pass on my host).

Apologies for the time taken to respond to this.

mdjnelson commented 5 months ago

Looking now 👀.

leonstr commented 5 months ago

Thanks for the feedback @mdjnelson most of these look straightforward, give me another day or so to look at what PHPUnit testing I can add.

mdjnelson commented 5 months ago

No worries @leonstr, thanks for you help! I am currently doing a refactor to make pages, templates and elements persistents which can contain the logic for triggering these events (if we want (may be a bad idea havent thought abt it too much) but not just that but this was needed a long time ago cause the logic is all over the place). However, this will take me a bit so go ahead with what you are doing and yeh, unit tests would be great to know my refactor isnt screwing everything up. :D

leonstr commented 5 months ago

PHPUnit tests: I've added checks for events' contexts, currently this is "system" except for test_load_template which I've added to catch one of the problems originally reported (Loading a template for a course activity instance includes a page_created event with context: system). I've also removed the objectid checks in test_copy_to_template since a) these were incorrect (param1 and param2 are the same), and b) we're using copy_to_template() to create records so don't deterministically know what their IDs will be (likely the existing IDs + 1, but we shouldn't count on that).

mdjnelson commented 5 months ago

Cherry-picked, thanks!