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.

Sign up for GitHub

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

fix: [M3-7571] - Missing label for Full Account Access Toggle #10931

Merged
merged 6 commits into from
Sep 23, 2024
Merged

fix: [M3-7571] - Missing label for Full Account Access Toggle #10931

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
16011c3
fix: [M3-7571] - Missing label for Full Account Access Toggle
carrillo-erik Sep 12, 2024
9c66182
Add changeset
carrillo-erik Sep 12, 2024
362e60c
Update e2e tests
carrillo-erik Sep 17, 2024
1a84ef3
Update headers based on PR feedback and lighthouse results
carrillo-erik Sep 17, 2024
f1597cc
Merge branch 'develop' of https://github.com/linode/manager into fix/…
carrillo-erik Sep 17, 2024
a8fa11c
Merge branch 'develop' of https://github.com/linode/manager into fix/…
carrillo-erik Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-10931-fixed-1726172174760.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Fixed
---

Missing label for Full Account Toggle ([#10931](https://github.com/linode/manager/pull/10931))
8 changes: 2 additions & 6 deletions packages/manager/cypress/e2e/core/account/user-permissions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,7 @@ describe('User permission management', () => {
// Restrict account access, confirm page updates to reflect change.
mockUpdateUser(mockUser.username, mockUserUpdated);
mockGetUserGrants(mockUser.username, mockUserGrantsUpdated);
cy.findByLabelText('Toggle Full Account Access')
.should('be.visible')
.click();
cy.findByText('Full Account Access').should('be.visible').click();

ui.toast.assertMessage('User permissions successfully saved.');

Expand Down Expand Up @@ -250,9 +248,7 @@ describe('User permission management', () => {
// Re-enable unrestricted account access, confirm page updates to reflect change.
mockUpdateUser(mockUser.username, mockUser);
mockGetUserGrantsUnrestrictedAccess(mockUser.username);
cy.findByLabelText('Toggle Full Account Access')
.should('be.visible')
.click();
cy.findByText('Full Account Access').should('be.visible').click();
Copy link
Member

Choose a reason for hiding this comment

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

This might be a dumb question, but wouldn't the fact that we switched to findByText over findByLabelText indicate that it isn't accessible? Is it expected we can't use findByLabelText?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnussman-akamai It's a valid question and I was confused by this as well. The error from running the cypress debug command states "The element is not visible because it has CSS property: opacity: 0."

Looking at the Cypress docs, as a best practice, they recommend using data-* attributes to provide context to selectors and isolate them from CSS or JS changes. With that in mind, I went ahead and implemented this and confirmed the tests are still passing and VoiceOver works as expected.

Copy link
Member

Choose a reason for hiding this comment

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

Understood! Thanks for the context 🙏


cy.findByText('General Permissions').should('be.visible');
cy.findByText(unrestrictedAccessMessage).should('be.visible');
Expand Down
14 changes: 7 additions & 7 deletions packages/manager/src/features/Users/UserPermissions.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Grid from '@mui/material/Unstable_Grid2';
import { styled } from '@mui/material/styles';
import Grid from '@mui/material/Unstable_Grid2';

import { CircleProgress } from 'src/components/CircleProgress';
import Select from 'src/components/EnhancedSelect/Select';
Expand Down Expand Up @@ -41,16 +41,17 @@ export const StyledHeaderGrid = styled(Grid, {
[theme.breakpoints.down('sm')]: {
marginLeft: theme.spacing(2),
marginTop: theme.spacing(1),
width: '100%',
},
width: '100%',
}));

export const StyledSubHeaderGrid = styled(Grid, {
label: 'StyledSubHeaderGrid',
export const StyledFullAccountAccessToggleGrid = styled(Grid, {
label: 'StyledFullAccountAccessToggleGrid',
})(({ theme }) => ({
marginTop: theme.spacing(2),
padding: 0,
[theme.breakpoints.down('sm')]: {
margin: theme.spacing(0.5),
padding: 0,
paddingLeft: theme.spacing(2),
},
}));

Expand All @@ -68,7 +69,6 @@ export const StyledPaper = styled(Paper, {
label: 'StyledPaper',
})(({ theme }) => ({
paddingBottom: 0,
paddingTop: 0,
[theme.breakpoints.down('sm')]: {
padding: 0,
},
Expand Down
143 changes: 73 additions & 70 deletions packages/manager/src/features/Users/UserPermissions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,19 @@
import {
GlobalGrantTypes,
Grant,
GrantLevel,
GrantType,
Grants,
User,
getGrants,
getUser,
updateGrants,
updateUser,
} from '@linode/api-v4/lib/account';
import { APIError } from '@linode/api-v4/lib/types';
import { Paper } from '@mui/material';
import Grid from '@mui/material/Unstable_Grid2';
import { QueryClient } from '@tanstack/react-query';
import { enqueueSnackbar } from 'notistack';
import { compose, flatten, lensPath, omit, set } from 'ramda';
import * as React from 'react';

import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel';
import { Box } from 'src/components/Box';
import { CircleProgress } from 'src/components/CircleProgress';
// import { Button } from 'src/components/Button/Button';
import { DocumentTitleSegment } from 'src/components/DocumentTitle';
import { Item } from 'src/components/EnhancedSelect/Select';
import { FormControlLabel } from 'src/components/FormControlLabel';
import { Notice } from 'src/components/Notice/Notice';
import { SelectionCard } from 'src/components/SelectionCard/SelectionCard';
Expand All @@ -34,14 +24,8 @@ import { TabPanels } from 'src/components/Tabs/TabPanels';
import { Tabs } from 'src/components/Tabs/Tabs';
import { Toggle } from 'src/components/Toggle/Toggle';
import { Typography } from 'src/components/Typography';
import {
WithFeatureFlagProps,
withFeatureFlags,
} from 'src/containers/flags.container';
import {
WithQueryClientProps,
withQueryClient,
} from 'src/containers/withQueryClient.container';
import { withFeatureFlags } from 'src/containers/flags.container';
import { withQueryClient } from 'src/containers/withQueryClient.container';
import { PARENT_USER, grantTypeMap } from 'src/features/Account/constants';
import { accountQueries } from 'src/queries/account/queries';
import { getAPIErrorOrDefault } from 'src/utilities/errorUtils';
Expand All @@ -51,14 +35,28 @@ import { scrollErrorIntoViewV2 } from 'src/utilities/scrollErrorIntoViewV2';
import {
StyledCircleProgress,
StyledDivWrapper,
StyledFullAccountAccessToggleGrid,
StyledHeaderGrid,
StyledPaper,
StyledPermPaper,
StyledSelect,
StyledSubHeaderGrid,
StyledUnrestrictedGrid,
} from './UserPermissions.styles';
import { UserPermissionsEntitySection } from './UserPermissionsEntitySection';

import type {
GlobalGrantTypes,
Grant,
GrantLevel,
GrantType,
Grants,
User,
} from '@linode/api-v4/lib/account';
import type { APIError } from '@linode/api-v4/lib/types';
import type { QueryClient } from '@tanstack/react-query';
import type { Item } from 'src/components/EnhancedSelect/Select';
import type { WithFeatureFlagProps } from 'src/containers/flags.container';
import type { WithQueryClientProps } from 'src/containers/withQueryClient.container';
interface Props {
accountUsername?: string;
currentUsername?: string;
Expand Down Expand Up @@ -91,30 +89,6 @@ interface State {
type CombinedProps = Props & WithQueryClientProps & WithFeatureFlagProps;

class UserPermissions extends React.Component<CombinedProps, State> {
componentDidMount() {
this.getUserGrants();
this.getUserType();
}

componentDidUpdate(prevProps: CombinedProps) {
if (prevProps.currentUsername !== this.props.currentUsername) {
this.getUserGrants();
this.getUserType();
}
}

render() {
const { loading } = this.state;
const { currentUsername } = this.props;

return (
<div ref={this.formContainerRef}>
<DocumentTitleSegment segment={`${currentUsername} - Permissions`} />
{loading ? <CircleProgress /> : this.renderBody()}
</div>
);
}

billingPermOnClick = (value: null | string) => () => {
const lp = lensPath(['grants', 'global', 'account_access']);
this.setState(set(lp, value));
Expand Down Expand Up @@ -371,7 +345,7 @@ class UserPermissions extends React.Component<CombinedProps, State> {
spacing={2}
>
<Grid>
<Typography data-qa-permissions-header="billing" variant="h3">
<Typography data-qa-permissions-header="billing" variant="h2">
Copy link
Contributor

Choose a reason for hiding this comment

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

same - the headings composition was already correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found the culprit that led me to miscalculate the header order based on the Lighthouse tool. I've reverted the headers and updated the variant for the appropriate element.
Screenshot 2024-09-17 at 11 49 22 AM

Billing Access
</Typography>
</Grid>
Expand Down Expand Up @@ -445,32 +419,41 @@ class UserPermissions extends React.Component<CombinedProps, State> {
textTransform: 'capitalize',
}}
data-qa-restrict-access={restricted}
variant="h2"
variant="h1"
>
{isProxyUser ? PARENT_USER : 'General'} Permissions
</Typography>
</StyledHeaderGrid>
<StyledSubHeaderGrid>
<Toggle
tooltipText={
currentUsername === accountUsername
? 'You cannot restrict the current active user.'
: ''
<StyledFullAccountAccessToggleGrid>
<FormControlLabel
control={
<Toggle
inputProps={{
'aria-label': 'Toggle Full Account Access',
}}
tooltipText={
currentUsername === accountUsername
? 'You cannot restrict the current active user.'
: ''
}
checked={!restricted}
disabled={currentUsername === accountUsername}
onChange={this.onChangeRestricted}
/>
}
aria-label="Toggle Full Account Access"
checked={!restricted}
disabled={currentUsername === accountUsername}
onChange={this.onChangeRestricted}
slotProps={{
typography: {
sx: (theme) => ({
fontFamily: theme.font.bold,
fontSize: '16px',
}),
},
}}
label="Full Account Access"
labelPlacement="end"
value={restricted}
/>
</StyledSubHeaderGrid>
<Grid sx={{ padding: 0 }}>
<Typography
sx={{ fontFamily: (theme) => theme.font.bold }}
variant="subtitle2"
>
Full Account Access
</Typography>
</Grid>
</StyledFullAccountAccessToggleGrid>
</Grid>
</StyledPaper>
{restricted ? this.renderPermissions() : this.renderUnrestricted()}
Expand Down Expand Up @@ -531,7 +514,7 @@ class UserPermissions extends React.Component<CombinedProps, State> {
<StyledPermPaper data-qa-global-section>
<Typography
data-qa-permissions-header="Global Permissions"
variant="subtitle2"
variant="body2"
>
Configure the specific rights and privileges this user has within the
account.{<br />}Remember that permissions related to actions with the
Expand Down Expand Up @@ -606,12 +589,11 @@ class UserPermissions extends React.Component<CombinedProps, State> {
<Grid>
<Typography
data-qa-permissions-header="Specific Permissions"
variant="h2"
variant="h1"
carrillo-erik marked this conversation as resolved.
Show resolved Hide resolved
>
Specific Permissions
</Typography>
</Grid>

<Grid style={{ marginTop: 5 }}>
<StyledSelect
defaultValue={defaultPerm}
Expand Down Expand Up @@ -679,9 +661,6 @@ class UserPermissions extends React.Component<CombinedProps, State> {
<Typography data-qa-unrestricted-msg>
This user has unrestricted access to the account.
</Typography>
{/* <Button buttonType="primary" onClick={this.onChangeRestricted}>
Save
</Button> */}
</StyledUnrestrictedGrid>
</Paper>
);
Expand Down Expand Up @@ -823,6 +802,30 @@ class UserPermissions extends React.Component<CombinedProps, State> {
setAllPerm: 'null',
userType: null,
};

componentDidMount() {
this.getUserGrants();
this.getUserType();
}

componentDidUpdate(prevProps: CombinedProps) {
if (prevProps.currentUsername !== this.props.currentUsername) {
this.getUserGrants();
this.getUserType();
}
}

render() {
const { loading } = this.state;
const { currentUsername } = this.props;

return (
<div ref={this.formContainerRef}>
<DocumentTitleSegment segment={`${currentUsername} - Permissions`} />
{loading ? <CircleProgress /> : this.renderBody()}
</div>
);
}
}

export default withQueryClient(withFeatureFlags(UserPermissions));
2 changes: 1 addition & 1 deletion packages/manager/src/features/Users/UserPermissionsEntitySection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export const UserPermissionsEntitySection = React.memo(
<Box key={entity} paddingBottom="0" marginTop={`${theme.spacing(2)}`}>
{showHeading && (
<Typography
variant="h3"
variant="h2"
carrillo-erik marked this conversation as resolved.
Show resolved Hide resolved
sx={{
marginTop: theme.spacing(3),
marginBottom: theme.spacing(2),
Expand Down
Loading