-
Notifications
You must be signed in to change notification settings - Fork 878
chore: clean up groups page #16259
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
chore: clean up groups page #16259
Changes from 7 commits
3928c9f
6ad8d8a
3edd915
7791f86
9dccf36
afed20d
9d4c705
6679635
95f295c
eea6cf6
e0521c8
ed8e84f
670ef77
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
import { expect, test } from "@playwright/test"; | ||
import { createUser, getCurrentOrgId, setupApiCalls } from "../../api"; | ||
import { defaultOrganizationName } from "../../constants"; | ||
import { requiresLicense } from "../../helpers"; | ||
import { login } from "../../helpers"; | ||
import { beforeCoderTest } from "../../hooks"; | ||
|
@@ -17,16 +18,20 @@ test(`Every user should be automatically added to the default '${DEFAULT_GROUP_N | |
}) => { | ||
requiresLicense(); | ||
await setupApiCalls(page); | ||
|
||
const orgName = defaultOrganizationName; | ||
const orgId = await getCurrentOrgId(); | ||
const numberOfMembers = 3; | ||
const users = await Promise.all( | ||
Array.from({ length: numberOfMembers }, () => createUser(orgId)), | ||
); | ||
|
||
await page.goto(`${baseURL}/groups`, { waitUntil: "domcontentloaded" }); | ||
await page.goto(`${baseURL}/organizations/${orgName}/groups`, { | ||
waitUntil: "domcontentloaded", | ||
}); | ||
await expect(page).toHaveTitle("Groups - Coder"); | ||
|
||
const groupRow = page.getByRole("row", { name: DEFAULT_GROUP_NAME }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally, we try to use getByRole as much as possible, just curious why you switched this to getByText? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It stopped working 🤷♀️ I tried to figure out what was going on but the dom looks basically identical to me. No idea why it broke. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok got it, I having been seeing the same type of things sometimes. Trying to figure out best practices to avoid test flakes. |
||
const groupRow = page.getByText(DEFAULT_GROUP_NAME); | ||
await groupRow.click(); | ||
await expect(page).toHaveTitle(`${DEFAULT_GROUP_NAME} - Coder`); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import type { CSSObject, Interpolation, Theme } from "@emotion/react"; | |
import { Stack } from "components/Stack/Stack"; | ||
import { type ClassName, useClassName } from "hooks/useClassName"; | ||
import type { ElementType, FC, ReactNode } from "react"; | ||
import { Link, NavLink, useMatch } from "react-router-dom"; | ||
import { Link, NavLink } from "react-router-dom"; | ||
import { cn } from "utils/cn"; | ||
|
||
interface SidebarProps { | ||
|
@@ -61,21 +61,16 @@ export const SettingsSidebarNavItem: FC<SettingsSidebarNavItemProps> = ({ | |
href, | ||
end, | ||
}) => { | ||
// 2025-01-10: useMatch is a workaround for a bug we encountered when you | ||
// pass a render function to NavLink's className prop, and try to access | ||
// NavLinks's isActive state value for the conditional styling. isActive | ||
// wasn't always evaluating to true when it should be, but useMatch worked | ||
const matchResult = useMatch(href); | ||
return ( | ||
<NavLink | ||
end={end} | ||
to={href} | ||
className={cn( | ||
"relative text-sm text-content-secondary no-underline font-medium py-2 px-3 hover:bg-surface-secondary rounded-md transition ease-in-out duration-150", | ||
{ | ||
"font-semibold text-content-primary": matchResult !== null, | ||
}, | ||
)} | ||
className={({ isActive }) => | ||
cn( | ||
"relative text-sm text-content-secondary no-underline font-medium py-2 px-3 hover:bg-surface-secondary rounded-md transition ease-in-out duration-150", | ||
isActive && "font-semibold text-content-primary", | ||
) | ||
} | ||
Comment on lines
-64
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
> | ||
{children} | ||
</NavLink> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,44 +1,18 @@ | ||
import type { AuthorizationResponse, Organization } from "api/typesGenerated"; | ||
import { FeatureStageBadge } from "components/FeatureStageBadge/FeatureStageBadge"; | ||
import { | ||
Sidebar as BaseSidebar, | ||
SettingsSidebarNavItem as SidebarNavItem, | ||
} from "components/Sidebar/Sidebar"; | ||
import { Stack } from "components/Stack/Stack"; | ||
import type { Permissions } from "contexts/auth/permissions"; | ||
import { useFeatureVisibility } from "modules/dashboard/useFeatureVisibility"; | ||
import { ArrowUpRight } from "lucide-react"; | ||
import type { FC } from "react"; | ||
|
||
export interface OrganizationWithPermissions extends Organization { | ||
permissions: AuthorizationResponse; | ||
} | ||
|
||
interface DeploymentSidebarProps { | ||
/** Site-wide permissions. */ | ||
permissions: Permissions; | ||
} | ||
|
||
/** | ||
* A combined deployment settings and organization menu. | ||
*/ | ||
export const DeploymentSidebarView: FC<DeploymentSidebarProps> = ({ | ||
permissions, | ||
}) => { | ||
const { multiple_organizations: hasPremiumLicense } = useFeatureVisibility(); | ||
|
||
return ( | ||
<BaseSidebar> | ||
<DeploymentSettingsNavigation | ||
permissions={permissions} | ||
isPremium={hasPremiumLicense} | ||
/> | ||
</BaseSidebar> | ||
); | ||
}; | ||
|
||
interface DeploymentSettingsNavigationProps { | ||
interface DeploymentSidebarViewProps { | ||
/** Site-wide permissions. */ | ||
permissions: Permissions; | ||
isPremium: boolean; | ||
showOrganizations: boolean; | ||
hasPremiumLicense: boolean; | ||
} | ||
|
||
/** | ||
|
@@ -48,12 +22,13 @@ interface DeploymentSettingsNavigationProps { | |
* Menu items are shown based on the permissions. If organizations can be | ||
* viewed, groups are skipped since they will show under each org instead. | ||
*/ | ||
const DeploymentSettingsNavigation: FC<DeploymentSettingsNavigationProps> = ({ | ||
export const DeploymentSidebarView: FC<DeploymentSidebarViewProps> = ({ | ||
permissions, | ||
isPremium, | ||
showOrganizations, | ||
hasPremiumLicense, | ||
}) => { | ||
return ( | ||
<div> | ||
<BaseSidebar> | ||
<div className="flex flex-col gap-1"> | ||
{permissions.viewDeploymentValues && ( | ||
<SidebarNavItem href="/deployment/general">General</SidebarNavItem> | ||
|
@@ -100,7 +75,11 @@ const DeploymentSettingsNavigation: FC<DeploymentSettingsNavigationProps> = ({ | |
<SidebarNavItem href="/deployment/users">Users</SidebarNavItem> | ||
)} | ||
{permissions.viewAnyGroup && ( | ||
<SidebarNavItem href="/deployment/groups">Groups</SidebarNavItem> | ||
<SidebarNavItem href="/deployment/groups"> | ||
<Stack direction="row" alignItems="center" spacing={0.5}> | ||
Groups {showOrganizations && <ArrowUpRight size={16} />} | ||
</Stack> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some precedent for using ArrowUpRight to convey this type of meaning? Did you get some feedback from christin? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was imagining something a bit more explicit to let users know they are be taken to the default organization There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just trying something new, and keep hearing about how she's already busy enough. If we don't like it we can change it later. 😅 |
||
</SidebarNavItem> | ||
)} | ||
{permissions.viewNotificationTemplate && ( | ||
<SidebarNavItem href="/deployment/notifications"> | ||
|
@@ -115,10 +94,10 @@ const DeploymentSettingsNavigation: FC<DeploymentSettingsNavigationProps> = ({ | |
IdP Organization Sync | ||
</SidebarNavItem> | ||
)} | ||
{!isPremium && ( | ||
{!hasPremiumLicense && ( | ||
<SidebarNavItem href="/deployment/premium">Premium</SidebarNavItem> | ||
)} | ||
</div> | ||
</div> | ||
</BaseSidebar> | ||
); | ||
}; |
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.
What was the reason to only test the organizations routes. Do you feel it is redundant to test both the deployment and organizations groups functionality?
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 only run the e2e tests with a premium license, which cannot manage groups from the deployment settings.