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 157 forks source link

More issues with newly added events #570

Closed leonstr closed 4 months ago

leonstr commented 1 year ago

I've tested the fixes for #564 and see the following issues:

  1. Unexpected _templateupdated on create template (but not add activity instance). It's unexpected because it's in addition to the expected _templatecreated event.
  2. No event on clicking Save changes or Add element on customcert/edit.php. The latter action implicitly saves template changes, e.g. any name change. I'd expect a _templateupdated.
  3. Event descriptions when adding, updating and deleting a course activity instance show "created the certificate template", "updated the certificate template" and "deleted the certificate template". Whereas these should say "created the certificate", "updated the certificate" and "deleted the certificate" respectively, with "... template" only displayed when adding, updating and deleting templates under Site administration > Plugins > Activity modules > Custom certificate > Custom certificate settings, Manage templates).
  4. Loading a template for a course activity instance includes a _pagecreated event with context: system, whereas surely this should be context: cmid.

Tested with 7ee063e (MOODLE_401_STABLE) on Moodle 4.1.3+ (build: 20230526).

leonstr commented 1 year ago

As a first go at fixing these I've added branch 564-401 commit 1941274.

Another issue: I don't like that every _pageupdated also has a _templateupdated, if there's one action then two events seems wrong. Having one _templateupdated + one or more _pageupdated would make sense. I'd need to do a bit more testing to see if the _templateupdated could be removed from template->save_page(), i.e. does a _templatecreated or _templateupdated definitely get triggered every time save_page() is called? I haven't tried to fix this.

leonstr commented 1 year ago

Another issue: There are two _templatecreated events when copying a template. Possibly template->copy_to_template() doesn't need the:

                \mod_customcert\event\template_created::create_from_template($copytotemplate)->trigger();

because manage_templates.php calls:

            $newtemplate = \mod_customcert\template::create($name, $template->get_contextid());

which calls:

        \mod_customcert\event\template_created::create_from_template($template)->trigger();

?

leonstr commented 5 months ago

There's a further issue that rearranging elements by drag and drop then saving these this doesn't trigger any events. At this stage I think this specific issue should be added to it's own ticket, patching the above issues has taken long enough and retro-fitting events to rearrange.php will take a bit of thought.

Edit: Added this as #599 .

mdjnelson commented 4 months ago

@leonstr i've lost track. Can you help me out and let me know if this can be closed?

leonstr commented 4 months ago

Yes, as far as I know the issues were addressed in 62b2c2f and this can be closed.