Ensure events are only triggered when necessary #595
Conversation
|
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. |
|
This issue should ideally solve all the issues found in #570. |
|
@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. |
|
Thanks @leonstr! |
|
Ping @leonstr! :) |
|
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. |
|
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. |
|
Looking now 👀. |
load_template.php
Outdated
|
|
||
| // Delete the pages. | ||
| $DB->delete_records('customcert_pages', array('templateid' => $template->get_id())); | ||
| if ($pages = $DB->get_records('customcert_pages', array('templateid' => $template->get_id()))) { |
There was a problem hiding this comment.
Please use [] instead of array().
|
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. |
|
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 |
mdjnelson
changed the title
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 CONTEXT_SYSTEM). 4. Previously with >1 pages each page_updated had template_updated too. Now only one template_updated is triggered when multiple page_updated events. 5. Previously there were two template_created events when copying a template. Now fixed (i.e. just one template_created). 6. Previously loading a template had page_created without corresponding page_deleted (as with elements). Now fixed.
|
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 |
|
Cherry-picked, thanks! |
Suppress template_updated when creating new template.
Fix Save changes/Add element so template_updated triggered if name changed.
Fix context for page_created when loading a template in course instance (was system now course module).