Skip to content
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

feat: [UIE-8136] - IAM RBAC: add new users table component (part 1) #11367

Merged

Conversation

aaleksee-akamai
Copy link
Contributor

Description 📝

IAM RBAC - users table component.
Adds the component itself, which is a copy of an existing one in the Account, with some adjustments.
Second PR (part 2) will include the table for proxy users and the logic for deleting and filtering them.

Changes 🔄

  • new users table

Target release date 🗓️

Dec 10, 2024 (dev)

Preview 📷

Include a screenshot or screen recording of the change.

🔒 Use the Mask Sensitive Data setting for security.

💡 Use <video src="" /> tag when including recordings in table.

Before After
📷 image
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@aaleksee-akamai aaleksee-akamai requested a review from a team as a code owner December 4, 2024 14:19
@aaleksee-akamai aaleksee-akamai requested review from mjac0bs and hana-akamai and removed request for a team December 4, 2024 14:19
@aaleksee-akamai aaleksee-akamai self-assigned this Dec 4, 2024
@cpathipa cpathipa requested review from cpathipa and removed request for hana-akamai December 4, 2024 14:47
@aaleksee-akamai aaleksee-akamai force-pushed the UIE-8136-iam-rbac-users-table-part-1 branch from 290bd8e to e1707ba Compare December 5, 2024 16:32
Copy link

github-actions bot commented Dec 5, 2024

Coverage Report:
Base Coverage: 86.84%
Current Coverage: 86.84%

@aaleksee-akamai aaleksee-akamai force-pushed the UIE-8136-iam-rbac-users-table-part-1 branch from e1707ba to 20f9781 Compare December 6, 2024 12:15
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, @aaleksee-akamai!

Requesting changes for a few relatively small bits of feedback here.

In addition to the inline feedback, we'll want to address the linting warnings present in most files. Expand the accordion below for screenshots of most of them. We strongly recommend an es-lint extension to help catch and fix these easily (developer docs).

Requires Linting

Screenshot 2024-12-09 at 7 32 28 PM
Screenshot 2024-12-09 at 7 32 37 PM
Screenshot 2024-12-09 at 7 32 44 PM
Screenshot 2024-12-09 at 7 33 09 PM
Screenshot 2024-12-09 at 7 33 27 PM
Screenshot 2024-12-09 at 7 33 35 PM
Screenshot 2024-12-09 at 7 33 42 PM
Screenshot 2024-12-09 at 7 34 12 PM

In the future, please keep the How to Test section of the pull request template in your PR description. It helps anyone on the team easily test the PR at a glance when by calling out how to set up the environment and what reviewers should look out for (and what you confirmed yourself) there. For example, since this PR is pretty straight forward: "Ensure the Identity and Access Beta flag is enabled in dev tools", "Confirm table renders. Confirm correct columns are shown at smaller screen sizes. Confirm loading state at all screen sizes".

},
});

const numCols = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to need to make this dynamic to account for hidden columns at smaller screen sizes.

A good way to test this is to make use of the React Query dev tools. I find it helpful to 'Trigger Loading' action for an endpoint and then resize the screen to check that loading (and error) states are handled correctly.

Screen.Recording.2024-12-09.at.7.19.39.PM.mov

Screenshot 2024-12-09 at 7 21 50 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @mjac0bs , i didn't know about this tools, your video helped me to do that!

@aaleksee-akamai aaleksee-akamai force-pushed the UIE-8136-iam-rbac-users-table-part-1 branch from 26e0e7c to e11b9ef Compare December 10, 2024 13:59
@aaleksee-akamai
Copy link
Contributor Author

thanks for the review, @mjac0bs!

sure, I won't remove this section with how to test in the future. In addition, I've fixed the linting warnings in my changes.

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 465 passing tests on test run #5 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing465 Passing2 Skipped117m 46s

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Nice work! Confirming on the functionality.

@@ -0,0 +1,117 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding test coverage

@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label Dec 10, 2024
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for addressing feedback, @aaleksee-akamai! This all looks good now. I'm going to go ahead and merge since the last thing I committed was just a changeset file rename, and all tests passed previously.

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Dec 10, 2024
@mjac0bs mjac0bs merged commit 964842c into linode:develop Dec 10, 2024
20 of 22 checks passed
Copy link

cypress bot commented Dec 10, 2024

Cloud Manager E2E    Run #6946

Run Properties:  status check passed Passed #6946  •  git commit 964842c107: feat: [UIE-8136] - IAM RBAC: add new users table component (part 1) (#11367)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6946
Run duration 29m 22s
Commit git commit 964842c107: feat: [UIE-8136] - IAM RBAC: add new users table component (part 1) (#11367)
Committer aaleksee-akamai
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 467
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! IAM (Identity & Access Management)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants