-
Notifications
You must be signed in to change notification settings - Fork 497
Angular: Audit Trail feature #4576
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
base: main
Are you sure you want to change the base?
Conversation
…template, adjust tables references and models
|
Hi @FrancescoMolinaro, |
|
General feedback from the DSpace Developers Meeting on Sept 25:
|
|
Why are we using Solr as the primary store? Audit data should be logged to the simplest, most bullet-proof format possible, one that is easy to replicate to offline storage, like a simple sequential file. They can be indexed into a cache in batches by a back-end process. The cache could be Solr, but why -- I see nothing here that uses text analysis. |
|
Hi @FrancescoMolinaro, |
|
@FrancescoMolinaro : If you can find time, it'd be good to resolve the merge conflicts on this PR. The backend PR is already at +1 approval, so it may be possible to move both forward together quickly if we can solve the conflicts here. Thanks! |
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.
@FrancescoMolinaro : Thanks for this PR! I finally had a chance to test this today. Overall, it works well. But, I did find a few things we likely should fix. Some are described inline below, but I've also marked up this screenshot:
The above is a screenshot of the audit trail for a specific item. A few improvements I'd like to see:
- The breadcrumbs should include the Item page because this is a specific item (and we know its UUID). So, I think we need to enhance these breadcrumbs
- The title of the Item is displayed with no information about what it means. I think we need to say something like "Logs for object: [name of object]" rather than just putting the item name with no explanation. (Should we also consider renaming this page to be "Object Audit Logs" instead of "Subject Audit Logs"??)
- The Back button is "squished". The arrow overlaps the text.
- In this case, this Item has no audit trail. We should have an empty table displayed or some sort of message that says "No audit logs exist" (or similar).
- In general, I also think you can remove the word "Overview" from all these pages. I think it's better to just say "Subject Audit Logs" as the word "Overview" doesn't add anything. This is the same for the global page, it could just say "All Audit Logs" instead of "Audit Log Overview"
I have not had a chance to do a detailed code review yet. But, I've noted a few things I found in my testing inline below. I'll try to do a more detailed code review next week.
| cy.get('ds-audit-overview').should('be.visible'); | ||
| // Analyze <ds-audit-overview> for accessibility issues | ||
| testA11y('ds-audit-overview'); | ||
| }); |
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.
I would appreciate it if we could add some more basic e2e tests for this page... even just check that the basic page structure exists.
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.
It still would be good to add more basic e2e tests here, testing that the basic page structure exists.
| text: 'menu.section.audit', | ||
| link: '/auditlogs', | ||
| }, | ||
| icon: 'key', |
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.
Same here, let's not use key and use something new.
src/app/audit-page/object-audit-overview/object-audit-overview.component.html
Outdated
Show resolved
Hide resolved
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.
@FrancescoMolinaro and @steph-ieffam : Thanks for the updates here. I'm including both of you on this message as I've found some slightly odd behavior in more detailed testing today. I'm not sure if it's coming from the frontend or backend.
First, thanks @FrancescoMolinaro for addressing my prior feedback. This is looking good overall and the code looks good too, other than a few minor inline comments noted below.
In testing today, this feature is working well, but I've found a few bugs or odd behaviors:
- First, when
audit.enabled=falseon the backend, the Audit menu on items is wrongly displayed. This should not appear if the feature is disabled. (The main audit admin sidebar menu is working properly though and doesn't appear when disabled)
- When I create a new submission by importing a publication from CrossRef (not sure if the import matters here), the audit logs for that one action are massive. I end up with 56 entries in the audit log, all of them
MODIFY_METADATAactions. Some are also very odd as I'll see the same field listed twice, both forREMOVEandADD(even though I never removed the field). Here's what it looks like:
In this scenario I literally just imported metadata from CrossRef, added any missing required fields, uploaded a file and submitted it. I didn't make any metadata modifications. Oddly, too, I don't see information about the file I uploaded in the audit trail for this newly submitted item. It's just 56 entries for MODIFY_METADATA.
My expectation is that Item creation/submission doesn't need this level of detail. I was expecting to see a much smaller list of audit actions. Perhaps just an Item CREATE action and a Bitstream CREATE action. I'm not sure this detail about each metadata edit is necessary during submission. (UPDATE: I should add that I have audit.item.in-workspace set to the default of false. If it was set to true, then I could understand also capturing detail about individual metadata edits.)
- I also tested uploading a new bistream to an existing archived Item. In this case, I also found the audit trail to be overly detailed and duplicative:
Again, I'm a little surprised that we have METADATA_MODIFY audit entries here as the file was just added. However, what's more surprising is we have two of the same METADATA_MODIFY entries followed by two of the same CREATE entries.
Overall, again, I think this is working. I'm just worried that if we log every single metadata modification during submission it makes the audit logs extremely long/detailed. In my opinion the audit logs are more useful if they track CREATE for new objects, and then track modifications after creation in more detail.
If we need to bring this up for discussion in a future meeting, let me know. Overall though, I do want to clarify that I like this feature. Just trying to understand why there are so many unexpected entries being generated.
| ], | ||
| standalone: true, | ||
| }) | ||
| export class AuditTableComponent { |
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.
Please add a code comment which describes where this component is used. All components should have a basic description in either a comment or TypeDocs.
| cy.get('ds-audit-overview').should('be.visible'); | ||
| // Analyze <ds-audit-overview> for accessibility issues | ||
| testA11y('ds-audit-overview'); | ||
| }); |
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.
It still would be good to add more basic e2e tests here, testing that the basic page structure exists.
src/assets/i18n/en.json5
Outdated
|
|
||
| "audit.overview.table.other": "Other Object", | ||
|
|
||
| "audit.overview.table.timestamp": "Time", |
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.
Should this say "Time (UTC)"? It might make sense to clarify that these times all appear to be in UTC. Either that, or change the format of the time to include the timezone.
|
Hey @tdonohue thanks for spotting these issues. Most of what you have reported was actually a REST issue, in the meantime @FrancescoMolinaro has addressed all the issues on the Angular side.
We spotted an issue on the dispatching of the events so the same set of modified metadata was being dispatched multiple times even though these changes were already stored and shouldn't have been processed twice. This has been introduced by mistake during the refactoring for the review so we didn't notice it at fist. Now the set of information which is tracked is way smaller and doesn't contain duplicate information
I can confirm the above issue was happening right after the item was moved to the archived state. Plenty of duplicated dc.description.provenance, dc.identifier.uri and dc.date.accessioned was being tracked. You will still see this information in the audit trail since these metadata are currently added right after the item is set to archived but now the information is only displayed once for each modification.
We indeed noticed the creation Event was being tracked twice and also some information set on the newely created bitstream/bundle was being tracked. We just introduced a fix to track the creation of the objects once and skip the tracking of information about the related objects (bundles & bitstreams) while the item is still in the workspace. During our tests we managed to reduce the number of tracked events from 56 events (clearly something odd was happening) to just 11 event (plenty of bitstreams were also added in our test case). Please let us know if anything odd is still left behind and needs to be changed. |
References
Fixes DSpace/DSpace#8824
Based on DSpace/DSpace#10684 to improve and better manage the audit information through the Event's details
Related to Rest Contract DSpace/RestContract#311
Related to Rest Request DSpace/DSpace#11072
Description
Ui Implementation for Audits Overview, menu accessible in admin sidebar:
Implementation of context menu and related detail page for item:
Instructions for Reviewers
For the backend setup please refer to DSpace/DSpace#11072.
To configure the Audit Overview menu we added a new property enableAuditLogsOverview in app config, currently set to true as default.
The context menu on item page is configurable via rest property context-menu-entry.audit.enabled.
List of changes in this PR:
Implemented new page components for Audits overview
Implemented new page components for Audits deatail for item
Implemented presentational table component
Added e2e te
To test the PR, once both front and rest are installed, you need to do some change on an item, for instance add a metadata, the changes to the item will appear in the audit pages, containing detail of the changes made.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.