Skip to content

chore: Add Audit Log components and service to load from the API #3782

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

Merged
merged 31 commits into from
Sep 7, 2022

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma commented Aug 31, 2022

Preview:
Screen Shot 2022-08-31 at 15 32 31

Demo:

Screen.Recording.2022-09-07.at.15.31.43.mov

⚠️ Technical considerations:

  • I decided to build a diff component instead of getting Monaco or any other editor. Having a custom component was easier to customize and easier to get the format - which is not JSON or any other language but a list of values - done.
  • Some resources don't have an internal link like User or TemplateVersion so instead of being a link/blue, they are strong text.
  • I used test-ids to test the page loading since the message is pretty dynamic
  • I used the HTTP status number since it can be helpful to debug. The colors will change accordingly.
  • As "extra data" on the right I just put the IP and the Agent. I'm not sure if we should add other info.
  • The collapse button only works if the log has diffs and it works with keyboard navigation as well.

Closes #3607 #3429 #3428 #3608

@BrunoQuaresma BrunoQuaresma self-assigned this Aug 31, 2022
@BrunoQuaresma BrunoQuaresma requested a review from a team as a code owner August 31, 2022 18:40
@BrunoQuaresma BrunoQuaresma requested review from presleyp and removed request for a team August 31, 2022 18:40
@presleyp
Copy link
Contributor

Love your diff component! Small issue though, the line numbers aren't aligned - I guess it has to do with the text wrapping?
Screen Shot 2022-08-31 at 3 19 08 PM

@BrunoQuaresma
Copy link
Collaborator Author

Love your diff component! Small issue though, the line numbers aren't aligned - I guess it has to do with the text wrapping? Screen Shot 2022-08-31 at 3 19 08 PM

Good catch! I will play around with smaller viewports and see what other bugs I can fix. Thanks.

@presleyp
Copy link
Contributor

I like the color-coded pills for status codes, but wonder if it will be unclear to some users what they represent. Maybe they could say Status 200 or Status: 200 instead of just 200.

@presleyp
Copy link
Contributor

I denied some of the Chromatic changes - it's all to do with indentations making values not line up with their headers. Not sure what caused it.

@kylecarbs
Copy link
Member

Does this handle pagination?

@BrunoQuaresma
Copy link
Collaborator Author

Does this handle pagination?

Nops, but I can do it tomorrow - it would be a diff PR tho. What do you think? Or the port thing should be a priority?

@BrunoQuaresma BrunoQuaresma requested a review from coadler August 31, 2022 19:43
@BrunoQuaresma
Copy link
Collaborator Author

I like the color-coded pills for status codes, but wonder if it will be unclear to some users what they represent. Maybe they could say Status 200 or Status: 200 instead of just 200.

I think HTTP status is very well known 🤔 . For this layout specifically, adding the Status on every row can be too much visually because it increases the "accent color" area. I would wait for users to complain about that. But, if you think we should, I would be happy to add. Here is a comparison:

Screen Shot 2022-08-31 at 20 38 50

Screen Shot 2022-08-31 at 20 39 23

@presleyp
Copy link
Contributor

presleyp commented Sep 1, 2022

Sure, let's leave the status code as just the number and see if people ask about it.

@presleyp
Copy link
Contributor

presleyp commented Sep 1, 2022

I denied a few more Chromatic changes, this time it's just that a color change you made affected the Schedule switch, I don't think it looks "on" enough in the dark blue-grey color.

@BrunoQuaresma
Copy link
Collaborator Author

I made some minor updates:

  • on auditXService I updated the invoke to use only one function that loads the count and the results. Having multiple invokes doesn't guarantee both services are completed at the end so we would have to use parallel states making it more complex than necessary only to load the count.
  • The URL also changes on pagination so it can be a shareable URL

const readableActionMessage = (auditLog: AuditLog) => {
return `${actionLabelByAction[auditLog.action]} ${
resourceLabelByResourceType[auditLog.resource_type]
}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's how to do interpolation with react-i18n https://www.i18next.com/translation-function/interpolation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are planning to move this "message builder" to the BE so the CLI and FE can use the same language so I'm going just let this as it is but thanks for sharing the docs. Do you know how they could support interpolation with components?

Copy link
Contributor

Choose a reason for hiding this comment

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

@BrunoQuaresma BrunoQuaresma merged commit a00fdd6 into main Sep 7, 2022
@BrunoQuaresma BrunoQuaresma deleted the bq/feat-3251 branch September 7, 2022 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display audit log diffs in a git diff format
3 participants