Cascade last_modified stamping to Submission on Chapter changes (extends pkp-lib#13074) - #2420
Cascade last_modified stamping to Submission on Chapter changes (extends pkp-lib#13074)#2420monmarzia-archi wants to merge 1 commit into
Conversation
…nds pkp-lib#13074)
|
@asmecher Back with the reworked cascade. Thanks for the feedback on the call, it made the design a lot clearer. How the cascade works now There are three new events, one per entity: AuthorMetadataChanged, CitationMetadataChanged and PublicationMetadataChanged. Each one has a small listener. All three listeners queue the same shared job. The cascade moves one step at a time. An author change emits an author event. The job handles it, finds the publication, and emits a publication event. A second job handles that one and stamps the submission. I chose one step at a time so that each hop is its own unit of work, with its own retry semantics if the connection ever changes. It also leaves a public event at the publication level, in case anything else needs to react to publication changes later. A change from the version you saw The earlier version emitted MetadataChanged from the author dispatch points. That event already has a listener, MetadataChangedListener, which reindexes the search index. So editing an author also triggered a reindex. Nobody asked for that, and it went against the "purely additive" argument I gave for concrete events. The new events fix this. MetadataChanged is no longer emitted from any new point. Editing an author no longer reindexes. Every author test asserts this. Targeted update You asked for an update that only touches last_modified. SubmissionDAO::updateLastModified() does that. It writes one column, instead of going through EntityDAO::_update(), which writes every mapped column and could overwrite concurrent changes to other fields. One consequence: Repository::stampModified() no longer goes through edit(), so it does not call Hook::call('Submission::edit', ...). This is on purpose. Going through edit() would fire that hook in situations where it never fired before, and third-party plugins would see that as a change in behaviour. Why the job runs synchronously The shared job sets the sync connection. The reason: default_connection is database. If job_runner is off and no worker is running, a queued job never runs, and last_modified is never written. The config template discourages job_runner on high-traffic sites, so this can happen in practice. Let me know if you would rather have it go through the normal queue. Finding the parent entity This used to be repeated at each dispatch point. It is now in one place, in the shared job, as a match on the event origin. I looked at building on getParentColumn(), which several DAOs already implement. I chose the match instead, to keep the change small: CitationDAO is a legacy DAO and does not implement it, and adding it there felt like a bigger change than this fix needs. Happy to do it the other way if you prefer. What gets stamped Only submissions.last_modified. The intermediate events route the cascade, they do not stamp their own level. That is enough for the OAI-PMH datestamp, which uses GREATEST(submissions, issues, publications). One thing I noticed while testing: on the citation path publications.last_modified changes anyway, because saving citations goes through Repo::publication()->edit(). That is existing behaviour, not something this change adds. Bulk operations setAuthorsOrder() and deleteByPublicationId() emit one event per operation, not one per row. There are tests for the count. One side effect worth mentioning. importCitations() calls deleteByPublicationId() and then insertObject() for each entry, so a bulk import emits one event per operation it performs internally: replacing 15 references with 20 emits 21. I did not add a dispatch in importCitations() itself, since the inner calls already cover it. Suppressing them would need a flag through the DAO, which seemed more than this fix needs. Happy to add the suppression if you think it's worth it. Tests Thirteen tests in three files: the shared job, the author repository, the citation DAO. I checked that each one fails when I break it on purpose. importCitations() has unit tests only. It takes a raw citation list and is not reachable from the editorial workflow, so I could not test it by hand. LibraryClasses and LibraryJobs pass locally. The only failures are two in FilterDAOTest, which fail because a backup dump is missing in my environment. |
|
@asmecher there is an update to the PR pkp/pkp-lib#13104 |
Draft PR — companion to pkp/pkp-lib#13074 (see pkp/pkp-lib#13104)
Extends the same
last_modifiedcascade fix to Chapters (OMP-specific), confirmed inscope by @asmecher.
Draft PR — work in progress, opened for early feedback as requested by @asmecher
Summary
Editing, adding, deleting, or resequencing a Chapter did not update
submissions.last_modifiedon the parent Submission, the same gap addressed for Author/Citation in pkp-lib#13074.
This PR adds the corresponding dispatch in
ChapterDAO.php, reusing the sameStampSubmissionModifiedlistener introduced in pkp-lib#13074.Testing done so far
confirmed
last_modifiedon the parent Submission updates in all cases.Still to do before this leaves draft status
Dependency
This PR assumes
StampSubmissionModified(from pkp-lib#13074) is merged or at leaststable on the target branch; it does not duplicate that listener.
AI-assisted development disclosure
Portions of this PR's code analysis and drafting were assisted by an AI tool (Claude).
All technical claims and code have been independently reviewed and verified by the author
prior to submission, per PKP's AI contribution policy.