Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve the Audit Trails views #17104

Merged
merged 11 commits into from
Dec 4, 2024
Merged

Improve the Audit Trails views #17104

merged 11 commits into from
Dec 4, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Dec 2, 2024

Fix #17105

@MikeAlhayek MikeAlhayek requested a review from Piedone December 2, 2024 20:15
@Piedone
Copy link
Member

Piedone commented Dec 2, 2024

These details are already tracked by Audit Trail.

@MikeAlhayek
Copy link
Member Author

So we have a way to view the user object as we do in content? I am not sure.

It's useful to that that info in the user object as we do in contentItems

@Piedone
Copy link
Member

Piedone commented Dec 3, 2024

This can be useful if the properties are included in UserIndex too, so one can filter/order on them. Without that, I don't see the benefit of duplicating these without the rich contextual info that Audit Trail provides.

@MikeAlhayek
Copy link
Member Author

MikeAlhayek commented Dec 3, 2024

@Piedone I don't think we should add these info to the UserIndex unless there is a need for it. We don't query against that date.

However, to make this more useful, I added it to the audit trail so that info is more useful over time.

User Event

image

Content Event

image

@Piedone
Copy link
Member

Piedone commented Dec 3, 2024

The last modified, created dates for content items are there for historical reasons (these predate Audit Trail) and because it's useful to query on them. I think it's the same for Users. Why would we just store the same data that's already stored in Audit Trail, and better? And then not make querying available, for which they'd be actually required (for e.g. "list the most recently registered users first" or "list users who registered in November").

Having a separate section for the general Event Info and the specific one below for Audit Trail events is nice, but I don't get why duplicate things for users:

  • Created at UTC should be the same as the event timestamp.
  • Created by user ID is in the event info.
  • The Modified* ones are the same, and we shouldn't display fields with blank values for different events (since there's also a User Updated event, recorded with all the basic details too).

@MikeAlhayek MikeAlhayek changed the title Add TimeStamps on User Improve the Audit Trails views Dec 3, 2024
@MikeAlhayek
Copy link
Member Author

@Piedone okay. I dropped the CreatedUtc, CreatedBy, ModifiedUtc and ModifiedBy. Instead, I updated the trail audit views so they are grouped in such a way someone will understand what the values mean. I think with this format, It's easier to determine who did what.

image

image

@Piedone
Copy link
Member

Piedone commented Dec 4, 2024

But why not also keep them, just add them to UserIndex? I do think with that, it's useful (but not without).

@MikeAlhayek
Copy link
Member Author

Why would we inflate a index with data that isn't used?

Also, @sebastienros kinda mention in the meeting today that he is also not a fan of these properties as you originally suggested

Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

Because querying on users by these is a use case I can imagine being useful. But not insisting on it if we throw out the properties anyway.

@MikeAlhayek MikeAlhayek requested a review from Piedone December 4, 2024 19:24
@Piedone Piedone merged commit a7a6675 into main Dec 4, 2024
17 checks passed
@Piedone Piedone deleted the ma/user-timestamps branch December 4, 2024 19:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
2 participants