-
Notifications
You must be signed in to change notification settings - Fork 332
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
OBJ-242 [OBJ] Primary Nav and Restricted Users #5640
OBJ-242 [OBJ] Primary Nav and Restricted Users #5640
Conversation
Previously OBJ was only displayed for users with Object Storage in account.capabilities. Now, everyone will see Object Storage in the Primary Nav (hidden behind feature flag). Additionally, restricted users can now view the Object Storage feature, though they will see Notices explaining that they can't actually create any resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore previous comment, I was wrong 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working as expected for all scenarios (restricted & unrestricted, w/ and w/out tag)
} | ||
|
||
const mapStateToProps: MapState<StateProps, {}> = state => ({ | ||
bucketsLastUpdated: state.__resources.buckets.lastUpdated, | ||
clustersLastUpdated: state.__resources.clusters.lastUpdated | ||
clustersLastUpdated: state.__resources.clusters.lastUpdated, | ||
isRestrictedUser: pathOr( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is going to default to true
, that means that the above useEffect
might not fire at all if for whatever reason the component renders before the profile finishes loading
I think to prevent any unwanted race conditions, we should add isRestrictedUser
to the dependency array for the useEffect
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't an issue now because the IdentifyUser.tsx
render blocks the app if the profile data hasn't finished loading or fails, but that's something we should fix as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinmckenna fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good (though I agree with Marty's comment), I did see a small visual thing unrelated to this PR that might be worth fixing: if you open the create access key drawer, focus the label field, then click Cancel, validation gets triggered and you see the "label is required" message as the drawer is animating out of view.
@Jskobos good catch; I notice it happening on the Create Bucket form as well. I'll make a ticket and address separately. |
We actually introduced handling for restricted users in linode#5640, but this was reversed by a regression in linode#5785. Re-adding handling for restricted users is especially important for the upcoming promo. I also adjusted padding and margins.
Description
This PR accomplishes two things:
object-storage
feature flag).Previously we were looking at
account.capabilities
to determine whether to show Object Storage in Primary Nav. This works for account owners, but in order show Object Storage to restricted users, we need to include it in the Primary Nav by default (since we don't have access to/account
for restricted users). This will currently be hidden behind theobject-storage
feature flag.Restricted users currently can't use Object Storage. This PR makes it so that restricted users can see everything within the feature, with notices explaining they need to contact an account admin (following the pattern we have elsewhere in the app).
Type of Change
Note to Reviewers
Please test all combinations (with unrestricted as well as restricted users):