-
Notifications
You must be signed in to change notification settings - Fork 365
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
upcoming: [M3-7527] - Improve Login History restricted and child user experience #10125
upcoming: [M3-7527] - Improve Login History restricted and child user experience #10125
Conversation
4b03619
to
c62cb00
Compare
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.
We didn't have any test coverage for the Login History page, so I added this spec and included some test cases for the changes in this PR.
@@ -41,7 +41,11 @@ const AccountLoginsTableRow = (props: AccountLogin) => { | |||
</TableCell> | |||
</Hidden> | |||
<TableCell statusCell> | |||
<StatusIcon pulse={false} status={accessIconMap[status] ?? 'other'} /> | |||
<StatusIcon | |||
ariaLabel={`Status is ${status}`} |
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.
Added an ariaLabel here to make this more accessible and to verify the icon is present in the integration test.
const isAccountAccessRestricted = | ||
isRestrictedChildUser || | ||
(profile?.restricted && grants?.global.account_access !== 'read_write'); |
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.
We'll display the warning notice for child users and restricted users without account_access
.
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.
I'm also getting the unauthorized message for a restricted user with read/write permissions. I'm not sure if it's something particular about my account.
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.
Thanks @hkhalil-akamai - good catch. After looking into this, I realized that in production, the API will return a 403 Unauthorized
for GET account/logins*
for any restricted user that tries to access Login History - not just those with restricted account_access
. I made an update in f2984f0 to account for that. cc @jaalah-akamai
Before / Prod | After |
---|---|
Coverage Report: ✅ |
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.
Thanks for adding this test!
const isAccountAccessRestricted = | ||
isRestrictedChildUser || | ||
(profile?.restricted && grants?.global.account_access !== 'read_write'); |
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.
I'm also getting the unauthorized message for a restricted user with read/write permissions. I'm not sure if it's something particular about my account.
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.
Thanks for adding mocks!
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.
Apart from Hussain's comment, everything else is working as intended.
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.
Thanks for addressing feedback!
Description 📝
Currently, all restricted users receive an unauthorized message (see Before screenshot) after the table data fails to populate when they access the Login History tab. In the case of a child user (user_type: child), who is unrestricted by the API (account_access: read_write), but should be restricted in the UI, we must manually restrict the child user from viewing this page.
This PR updates the child view to not show login history information and display a warning notice to convey unauthorized access. For consistency, we will also make these changes for restricted users.
Note
The API has recently changed from
user_type: null
touser_type: "default"
. This is not reflected in this PR at this time because I didn't want to duplicate the work we'll need to make these changes on the front end, which will be done in another ticket.Changes 🔄
account_access
and does not display the login history table.account/logins
endpoint to the MSW so we can now mock that page.account/account-login-history.spec.ts
).Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
Reproduction steps
(How to reproduce the issue, if applicable)
account_access
grant) set to eitherRead-Only
orNone
.Verification steps
(How to verify changes)
user_type
to"child"
and make surerestricted
isfalse
in*/profile
.user_type
tonull
andrestricted
totrue
in*/profile
.As an Author I have considered 🤔
Check all that apply