Skip to content

Add audit event log.#4556

Merged
demiankatz merged 20 commits into
vufind-org:devfrom
EreMaijala:dev-audit-event-log
Aug 27, 2025
Merged

Add audit event log.#4556
demiankatz merged 20 commits into
vufind-org:devfrom
EreMaijala:dev-audit-event-log

Conversation

@EreMaijala
Copy link
Copy Markdown
Contributor

@EreMaijala EreMaijala commented Aug 21, 2025

Audit event log can be used to store information about events such as user logins. This is also a prerequisite for the upcoming online payment support that needs to store events about payments.

This implementation is not exhaustive, but I tried to cover the functionality that we've had most issues dealing with, logins being the single most prevalent one.

TODO:

@EreMaijala EreMaijala requested a review from demiankatz August 21, 2025 18:20
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @EreMaijala -- I ran out of time before I could fully review all of the files here, but I found a few significant issues on my initial review. I'll look through the rest once you've had a chance to respond to this first round! (There are only two files I haven't looked at, so I'm nearly done in any case).

Comment thread module/VuFind/src/VuFind/Auth/ILSAuthenticator.php Outdated
Comment thread module/VuFind/src/VuFind/Auth/Manager.php Outdated
Comment thread module/VuFind/src/VuFind/Controller/MyResearchController.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Service/AuditEventServiceInterface.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Service/AuditEventService.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Service/AuditEventService.php
Comment thread module/VuFind/src/VuFind/Db/Service/AuditEventService.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Service/AuditEventService.php Outdated
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below for one more comment based on a review of your latest diffs. Also note that the open Auth Manager comment remains unresolved.

I still need to review a couple more files, but thanks for all the progress so far!

@EreMaijala EreMaijala requested a review from demiankatz August 26, 2025 11:54
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @EreMaijala, I've finished reviewing everything -- see below for a few more comments. The one about enums is the biggest issue; I don't mean to hold things up over it, but it seems like a decision we should make thoughtfully to be sure we keep our future options as open as possible.

Comment thread module/VuFind/src/VuFind/Db/Service/AuditEventService.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Service/AuditEventService.php Outdated
Comment thread module/VuFind/src/VuFind/Controller/AbstractBase.php Outdated
@EreMaijala EreMaijala requested a review from demiankatz August 27, 2025 08:42
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @EreMaijala -- see below for more discussion of enums and a trivial suggestion.

Comment thread module/VuFind/src/VuFind/Auth/ILSAuthenticator.php Outdated
Comment thread module/VuFind/src/VuFind/Db/Service/AuditEventService.php Outdated
@EreMaijala
Copy link
Copy Markdown
Contributor Author

@demiankatz Your suggestion is reasonable. I changed the type/subtype getters to always return a string. If, for some reason, the caller would want the enum, it can use the enum's tryFrom itself.

@EreMaijala EreMaijala requested a review from demiankatz August 27, 2025 10:44
@demiankatz demiankatz added this to the 11.0 milestone Aug 27, 2025
Copy link
Copy Markdown
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @EreMaijala!

@demiankatz demiankatz merged commit 0491c18 into vufind-org:dev Aug 27, 2025
6 checks passed
@demiankatz demiankatz deleted the dev-audit-event-log branch August 27, 2025 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants