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

ref(superuser): red sidebar for active superuser #61329

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Dec 7, 2023

When superuser is active or superuser is required to access an org, the sidebar will turn red.

Backend changes to come to add the scope org:superuser to request.access for active superuser.

red-superuser-bar.mov

@github-actions github-actions bot added Scope: Frontend Automatically applied to PRs that change frontend components Scope: Backend Automatically applied to PRs that change backend components labels Dec 7, 2023
Copy link
Contributor

github-actions bot commented Dec 7, 2023

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

static/app/components/sidebar/index.tsx Outdated Show resolved Hide resolved
@@ -2392,6 +2392,7 @@ def custom_parameter_sort(parameter: dict) -> tuple[str, int]:
"openid",
"profile",
"email",
"org:superuser",
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this up next to the org:* scopes, and add a comment that this is only for superusers and it is added when the superuser session is valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm going to isolate the FE from BE changes and do the FE first in this PR

static/app/components/sidebar/index.tsx Outdated Show resolved Hide resolved
static/app/constants/index.tsx Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #61329 (20ffa58) into master (ab3e9ca) will decrease coverage by 0.01%.
Report is 30 commits behind head on master.
The diff coverage is n/a.

❗ Current head 20ffa58 differs from pull request most recent head ef40ed5. Consider uploading reports for the commit ef40ed5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #61329      +/-   ##
==========================================
- Coverage   81.08%   81.08%   -0.01%     
==========================================
  Files        5186     5186              
  Lines      227817   227813       -4     
  Branches    38221    38221              
==========================================
- Hits       184720   184715       -5     
  Misses      37471    37471              
- Partials     5626     5627       +1     
Files Coverage Δ
src/sentry/conf/server.py 89.91% <ø> (ø)
static/app/components/sidebar/index.tsx 90.62% <ø> (-0.42%) ⬇️
static/app/constants/index.tsx 92.10% <ø> (ø)
static/app/utils/isActiveSuperuser.tsx 80.00% <ø> (-1.82%) ⬇️

... and 6 files with indirect coverage changes

@cathteng cathteng removed the Scope: Backend Automatically applied to PRs that change backend components label Dec 7, 2023
@cathteng cathteng changed the title wip: red sidebar for active superuser ref(superuser): red sidebar for active superuser Dec 7, 2023
@cathteng cathteng marked this pull request as ready for review December 7, 2023 21:35
@cathteng cathteng requested a review from a team as a code owner December 7, 2023 21:35
@cathteng cathteng requested review from a team and leedongwei December 7, 2023 21:35
@@ -973,6 +974,7 @@ export const darkTheme: Theme = {
},
sidebarGradient: `linear-gradient(180deg, ${sidebarBackground.dark} 0%, #1B1825 100%)`,
sidebarBorder: darkAliases.border,
superuserSidebar: '#620808',
Copy link
Member

Choose a reason for hiding this comment

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

Nit: All other sidebar-related keys started with the word "sidebar"

Suggested change
superuserSidebar: '#620808',
sidebarColorSuperuser: '#620808',

@cathteng cathteng merged commit f8cafd8 into master Dec 11, 2023
43 checks passed
@cathteng cathteng deleted the cathy/superuser/red-sidebar-when-active branch December 11, 2023 15:24
@SebastiaanKloos
Copy link

I think the sidebar should not be red if the user is also the organization owner. On self-hosted most of the time the org-owner is also super admin.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants