search

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

Issue with newly added events #564

Closed mdjnelson closed 1 year ago

mdjnelson commented 1 year ago

Thanks to @leonstr for pointing these out.

  1. Creating a new certificate or template results in both a template_created and a template_updated event. The latter is a result of add_page() triggering both a page_created and a template_updated. Having both template_created and template_updated on creating a template doesn't make sense to me as 'create' is one action.
  2. Adding a new element results in two template_updated events. The second one is a result of save_page() triggering both a page_updated and a template_updated. Again, two events -- this time the same event -- for one action feels like a bug.
  3. If the certificate has more than one page and you add an element to one page then a page_updated is triggered for each page even though only one has been changed.
  4. For the element_* and page_* events I think get_description() should use 'the', not 'a'/'an', e.g. The user with id '...' updated the page with id '...', not The user with id '...' updated a page with id '...'. Trivial but the later reads wrong to me, and it seems to be what Moodle uses, e.g. for course events.

I'll try to find time to see if I can patch these in which case I'll open an issue with a PR for you to review. For 1. and 2. it's not a case of simply removing the template_created and template_updated from add_page() and save_page(), otherwise there'd be no template_updated when you click + Add page.

mdjnelson commented 1 year ago

@leonstr I have pushed a commit to fix number 4.

mdjnelson commented 1 year ago

@leonstr I believe this is all fixed now, can you confirm? Thanks.

leonstr commented 1 year ago

@mdjnelson Apologies for the delay in responding. I've had a test 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

Opened a new issue for this. Sorry if my posts were noisy.

mdjnelson commented 1 year ago

Hi @leonstr, no problem at all. I am in the middle of moving country for a job and don't have time to work on this. Would you be able to? Cheers.

leonstr commented 1 year ago

I'll try! Good luck with your move.