-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send audit results to DOR event service #1375
Conversation
ae5ad9b
to
9939f40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good, and covers all the events i can think of to send (definitely seems to cover what's described in the linked tickets, AFAICT).
i have some minor naming quibbles that i don't feel strongly about, and would be happy to forget about them if you prefer the names to stay as-is.
also i think there might be a parameter ordering issue with one of the refactored class methods, and if that is indeed the case, i also have a suggestion for a test expectation that might've sussed that out.
and i certainly wouldn't hold up this PR over it, but now i sorta wanna rename the WorkflowReporter
class to something more general, since its responsibilities are a bit wider now. can't think of any names i really like off-hand. EventsReporter
seems accurate but also feels like it sways too far towards the newly added reporting while obscuring the existing workflow reporting mechanics.
9939f40
to
063f9e1
Compare
@jmartin-sul Thanks for the review! All changes made. 🍻 Have another look? |
Fixes #1357 Fixes #1351 This pull request sends audit results to the new DOR event service which is provided by dor-services-app. As part of this, I refactored the WorkflowReporter such that the class methods (all that was exposed previously) create an instance of the class, with the instance doing all the work. This preserves the prior contract and allows for less passing around of state which now lives in the instance.
063f9e1
to
49ed05e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the changes!
Fixes #1357
Fixes #1351
Why was this change made?
This pull request sends audit results to the new DOR event service which is provided by
dor-services-app
. As part of this, I refactored theWorkflowReporter
such that the class methods (all that was exposed previously) create an instance of the class, with the instance doing all the work. This preserves the prior contract and allows for less passing around of state which now lives in the instance.Also includes:
Was the usage documentation (e.g. wiki, README, queue or DB specific README) updated?
no.