Skip to content

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

Merged
BrunoQuaresma merged 31 commits into
mainfrom
bq/feat-3251
Sep 7, 2022
Merged

chore: Add Audit Log components and service to load from the API#3782
BrunoQuaresma merged 31 commits into
mainfrom
bq/feat-3251

Conversation

@BrunoQuaresma

@BrunoQuaresma BrunoQuaresma commented Aug 31, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
presleyp reviewed Aug 31, 2022
View reviewed changes
Comment thread site/src/api/typesGenerated.ts Outdated
presleyp
presleyp reviewed Aug 31, 2022
View reviewed changes
Comment thread site/src/components/AuditLogRow/AuditLogDiff.tsx Outdated
presleyp
presleyp reviewed Aug 31, 2022
View reviewed changes
Comment thread site/src/components/AuditLogRow/AuditLogRow.tsx Outdated
presleyp
presleyp reviewed Aug 31, 2022
View reviewed changes
Comment thread site/src/components/AuditLogRow/AuditLogRow.tsx Outdated
presleyp
presleyp reviewed Aug 31, 2022
View reviewed changes
Comment thread site/src/components/AuditLogRow/AuditLogRow.tsx
presleyp
presleyp reviewed Aug 31, 2022
View reviewed changes
Comment thread site/src/components/AuditLogRow/AuditLogRow.tsx
presleyp
presleyp reviewed Aug 31, 2022
View reviewed changes
Comment thread site/src/xServices/audit/auditXService.ts Outdated
presleyp
presleyp reviewed Aug 31, 2022
View reviewed changes
Comment thread site/src/pages/TemplatePage/TemplatePageView.tsx Outdated
@presleyp

presleyp commented Aug 31, 2022

Copy link
Copy Markdown
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

BrunoQuaresma commented Aug 31, 2022

Copy link
Copy Markdown
Contributor 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

presleyp commented Aug 31, 2022

Copy link
Copy Markdown
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

presleyp commented Aug 31, 2022

Copy link
Copy Markdown
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

kylecarbs commented Aug 31, 2022

Copy link
Copy Markdown
Member

Does this handle pagination?

@BrunoQuaresma

BrunoQuaresma commented Aug 31, 2022

Copy link
Copy Markdown
Contributor 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

BrunoQuaresma commented Aug 31, 2022

Copy link
Copy Markdown
Contributor 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

presleyp commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

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

@presleyp

presleyp commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

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.

presleyp
presleyp reviewed Sep 1, 2022
View reviewed changes
Comment thread site/src/xServices/audit/auditXService.ts Outdated
@BrunoQuaresma

BrunoQuaresma commented Sep 7, 2022

Copy link
Copy Markdown
Contributor 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

presleyp
presleyp reviewed Sep 7, 2022
View reviewed changes
Comment thread site/src/components/AuditLogRow/AuditLogRow.tsx
const readableActionMessage = (auditLog: AuditLog) => {
return `${actionLabelByAction[auditLog.action]} ${
resourceLabelByResourceType[auditLog.resource_type]
}`

@presleyp presleyp Sep 7, 2022

Copy link
Copy Markdown
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

@BrunoQuaresma BrunoQuaresma Sep 7, 2022

Copy link
Copy Markdown
Contributor 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?

@presleyp presleyp Sep 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the closest I've found https://www.i18next.com/translation-function/formatting

@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

Reviewers

@coadler coadler Awaiting requested review from coadler
1 more reviewer
@presleyp presleyp presleyp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@BrunoQuaresma BrunoQuaresma

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Display audit log diffs in a git diff format

3 participants

@BrunoQuaresma @presleyp @kylecarbs