Conversation
| export const getCreateLinkForEntityType = ( | ||
| entityType: EntityType | EntityTypePermissions | ||
| ): string => { | ||
| // TODO - find the exceptions to this rule - most use the route of /{entityType}s/create (note the "s") |
There was a problem hiding this comment.
I can add one. I don't know if QA will be testing all the various entity types well or not.
| direction="row" | ||
| spacing={2} | ||
| > | ||
| <Typography variant={'h3'}>Roles</Typography> |
There was a problem hiding this comment.
Brackets not necessary variant='h3'
Cloud Manager UI test results🔺 2 failing tests on test run #4 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/clone-linode.spec.ts,cypress/e2e/core/objectStorageMulticluster/bucket-details-multicluster.spec.ts" |
||||||||||||||||||||
| <Notice spacingTop={8} variant="warning"> | ||
| <Typography fontSize="inherit"> | ||
| <Link to={getCreateLinkForEntityType(type)}> | ||
| Create a {capitalizeAllWords(type)} Entity |
There was a problem hiding this comment.
We need an override for "VPC", "NodeBalancer" and "StackScript"
| @@ -86,7 +87,7 @@ export const AssignSingleRole = ({ | |||
| }} | |||
| > | |||
| <Button disabled={index === 0} onClick={() => onRemove(index)}> | |||
There was a problem hiding this comment.
Instead of disabling deleting the first role, could we disable the button if it was the only rule? An example use case: if I assigned multiple roles to a user and I wanted to delete the first role in the list.
There was a problem hiding this comment.
(1/2) Can we add a unit test for this component (or create a ticket to do this):
- Tests that the drawer opens and renders
- Tests that the drawer closes
- Tests that the drawer displays multiple roles
There was a problem hiding this comment.
(2/2) Can we add a unit test for this component (or create a ticket to do this):
- Tests adding a role
- Tests the entity select
- Tests the "Create an X entity first" message appears
- Tests deleting a role
hkhalil-akamai
left a comment
hkhalil-akamai
left a comment
There was a problem hiding this comment.
Overall functionality looks good. Above comments are nits; feel free to consider them in follow-up tickets.
Co-authored-by: Hussain Khalil <122488130+hkhalil-akamai@users.noreply.github.com>
|
Approved and merging this PR addressing the feedback for test coverage in the followup tickets AssignSingleRole
AssignNewRoleDrawer
CC: @rodonnel-akamai Will merge this once CI passes. |
|
@rodonnel-akamai There is unit test failing on this PR can you look into this. |
7 participants
Description 📝
Highlight the Pull Request's context and intentions.
Changes 🔄
List any change(s) relevant to the reviewer.
Target release date 🗓️
4/22/2025
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
Verification steps
(How to verify changes)
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
As an Author, before moving this PR from Draft to Open, I confirmed ✅