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(cjux-278): maintenance root roles #8875

Merged
merged 4 commits into from
Dec 10, 2024
Merged

feat(cjux-278): maintenance root roles #8875

merged 4 commits into from
Dec 10, 2024

Conversation

Tymek
Copy link
Member

@Tymek Tymek commented Nov 27, 2024

Custom root roles for changing maintenance mode state and banners.

Internal ticket: CJUX-278

Copy link

vercel bot commented Nov 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2024 3:06pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Dec 9, 2024 3:06pm

Copy link
Contributor

github-actions bot commented Nov 27, 2024

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Files

Copy link
Contributor

@kwasniew kwasniew left a comment

Choose a reason for hiding this comment

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

How did you test it? Manually?

@@ -6,10 +6,11 @@ import { Box, styled } from '@mui/material';
import useUiConfig from 'hooks/api/getters/useUiConfig/useUiConfig';
import { MaintenanceTooltip } from './MaintenanceTooltip';
import { MaintenanceToggle } from './MaintenanceToggle';
import { UPDATE_MAINTENANCE_MODE } from '@server/types/permissions';
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know we could import stuff from the server? Should it go in AccessProvider/permissions to be consistent with how we're doing it today? I think only defining it in one place is cool, though, but why is it not something we're already doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for noticing this. It needs refactoring. I'll post another PR, because it turns out there are inconsistencies all over this.

Comment on lines +154 to +159
export const MAINTENANCE_MODE_PERMISSIONS = [
ADMIN,
READ_ROLE,
READ_CLIENT_API_TOKEN,
READ_FRONTEND_API_TOKEN,
UPDATE_MAINTENANCE_MODE,
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work? Do you need one of them or all of them? Seems a little strange to gain access to the full admin panel if you have read client API token only? Maybe I'm misunderstanding how this works?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have a way to selectively show some tabs (yet). It's a way to at least show all options, and PermissionGuard wrapper will do block further UI

image

@Tymek Tymek merged commit 5cc0e58 into main Dec 10, 2024
11 of 12 checks passed
@Tymek Tymek deleted the cjux-278 branch December 10, 2024 14:22
Tymek added a commit that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants