Skip to content

Commit bf24941

Browse files
authored
Fix UI bugs (#375)
* Fix org info page being empty on first load for non-admins * Store currently selected semester for room requests in query parameter <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Authorization now fetches core organization roles alongside app roles for more reliable access control. * Semester selections in room requests sync with the browser URL for sharing/bookmarking. * **Performance** * Organization role cache duration shortened for timelier permission updates. * **Bug Fixes** * Added null-guards and improved org-role loading to prevent incorrect page states. * **Tests** * Added end-to-end coverage for editing and persisting Organization Info. * **Chores** * Terraform provider versions pinned and Makefile targets updated for upgrades. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 400bba9 commit bf24941

File tree

10 files changed

+177
-58
lines changed

10 files changed

+177
-58
lines changed

Makefile

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ test_unit: install
7575
terraform -chdir=terraform/envs/qa init -reconfigure -backend=false -upgrade
7676
terraform -chdir=terraform/envs/qa fmt -check
7777
terraform -chdir=terraform/envs/qa validate
78-
terraform -chdir=terraform/envs/prod init -reconfigure -backend=false
78+
terraform -chdir=terraform/envs/prod init -reconfigure -backend=false -upgrade
7979
terraform -chdir=terraform/envs/prod fmt -check
8080
terraform -chdir=terraform/envs/prod validate
8181
yarn prettier
@@ -96,3 +96,7 @@ prod_health_check:
9696
lock_terraform:
9797
terraform -chdir=terraform/envs/qa providers lock -platform=windows_amd64 -platform=darwin_amd64 -platform=darwin_arm64 -platform=linux_amd64 -platform=linux_arm64
9898
terraform -chdir=terraform/envs/prod providers lock -platform=windows_amd64 -platform=darwin_amd64 -platform=darwin_arm64 -platform=linux_amd64 -platform=linux_arm64
99+
100+
upgrade_terraform:
101+
terraform -chdir=terraform/envs/qa init -reconfigure -backend=false -upgrade
102+
terraform -chdir=terraform/envs/prod init -reconfigure -backend=false -upgrade

src/ui/components/AuthGuard/index.tsx

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@ import { AcmAppShell, AcmAppShellProps } from "@ui/components/AppShell";
55
import FullScreenLoader from "@ui/components/AuthContext/LoadingScreen";
66
import { getRunEnvironmentConfig, ValidService } from "@ui/config";
77
import { useApi } from "@ui/util/api";
8-
import { AppRoles } from "@common/roles";
8+
import { AppRoles, OrgRoleDefinition } from "@common/roles";
99

1010
export const CACHE_KEY_PREFIX = "auth_response_cache_";
11-
const CACHE_DURATION = 2 * 60 * 60 * 1000; // 2 hours in milliseconds
11+
const CACHE_DURATION = 30 * 60 * 1000; // 30 minutes in milliseconds
1212

1313
type CacheData = {
1414
data: any; // Just the JSON response data
@@ -87,7 +87,6 @@ export const clearAuthCache = () => {
8787
/**
8888
* Retrieves the user's roles from the session cache for a specific service.
8989
* @param service The service to check the cache for.
90-
* @param route The authentication check route.
9190
* @returns A promise that resolves to an array of roles, or null if not found in cache.
9291
*/
9392
export const getUserRoles = async (
@@ -105,6 +104,25 @@ export const getUserRoles = async (
105104
return null;
106105
};
107106

107+
/**
108+
* Retrieves the user's org roles from the session cache for Core API.
109+
* @returns A promise that resolves to an array of roles, or null if not found in cache.
110+
*/
111+
export const getCoreOrgRoles = async (): Promise<
112+
OrgRoleDefinition[] | null
113+
> => {
114+
const { authCheckRoute } =
115+
getRunEnvironmentConfig().ServiceConfiguration.core;
116+
if (!authCheckRoute) {
117+
throw new Error("no auth check route");
118+
}
119+
const cachedData = await getCachedResponse("core", authCheckRoute);
120+
if (cachedData?.data?.orgRoles && Array.isArray(cachedData.data.orgRoles)) {
121+
return cachedData.data.orgRoles;
122+
}
123+
return null;
124+
};
125+
108126
export const AuthGuard: React.FC<
109127
{
110128
resourceDef: ResourceDefinition;

src/ui/pages/organization/OrgInfo.page.tsx

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,16 @@
11
import { useState, useEffect } from "react";
22
import { Title, Stack, Container, Select } from "@mantine/core";
3-
import { AuthGuard, getUserRoles } from "@ui/components/AuthGuard";
3+
import {
4+
AuthGuard,
5+
getUserRoles,
6+
getCoreOrgRoles,
7+
} from "@ui/components/AuthGuard";
48
import { useApi } from "@ui/util/api";
59
import { AppRoles } from "@common/roles";
610
import { notifications } from "@mantine/notifications";
711
import { IconAlertCircle } from "@tabler/icons-react";
812
import FullScreenLoader from "@ui/components/AuthContext/LoadingScreen";
913
import { AllOrganizationNameList, OrganizationName } from "@acm-uiuc/js-shared";
10-
import { useAuth } from "@ui/components/AuthContext";
1114
import { ManageOrganizationForm } from "./ManageOrganizationForm";
1215
import {
1316
LeadEntry,
@@ -21,7 +24,6 @@ type OrganizationData = z.infer<typeof setOrganizationMetaBody>;
2124

2225
export const OrgInfoPage = () => {
2326
const api = useApi("core");
24-
const { orgRoles } = useAuth();
2527
const [searchParams, setSearchParams] = useSearchParams();
2628
const [manageableOrgs, setManagableOrgs] = useState<
2729
OrganizationName[] | null
@@ -112,15 +114,19 @@ export const OrgInfoPage = () => {
112114
useEffect(() => {
113115
(async () => {
114116
const appRoles = await getUserRoles("core");
115-
if (appRoles?.includes(AppRoles.ALL_ORG_MANAGER)) {
117+
const orgRoles = await getCoreOrgRoles();
118+
if (appRoles === null || orgRoles === null) {
119+
return;
120+
}
121+
if (appRoles.includes(AppRoles.ALL_ORG_MANAGER)) {
116122
setManagableOrgs(AllOrganizationNameList);
117123
return;
118124
}
119125
setManagableOrgs(
120126
orgRoles.filter((x) => x.role === "LEAD").map((x) => x.org),
121127
);
122128
})();
123-
}, [orgRoles]);
129+
}, []);
124130

125131
// Update URL when selected org changes
126132
const handleOrgChange = (org: OrganizationName | null) => {

src/ui/pages/roomRequest/RoomRequestLanding.page.tsx

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,21 @@ import {
1414
type RoomRequestStatus,
1515
} from "@common/types/roomRequest";
1616
import { OrganizationName } from "@acm-uiuc/js-shared";
17+
import { useSearchParams } from "react-router-dom";
1718

1819
export const ManageRoomRequestsPage: React.FC = () => {
1920
const api = useApi("core");
20-
const [semester, setSemester] = useState<string | null>(null); // TODO: Create a selector for this
21+
const [semester, setSemesterState] = useState<string | null>(null);
2122
const [isLoading, setIsLoading] = useState(false);
2223
const nextSemesters = getSemesters();
2324
const semesterOptions = [...getPreviousSemesters(), ...nextSemesters];
25+
const [searchParams, setSearchParams] = useSearchParams();
26+
const setSemester = (semester: string | null) => {
27+
setSemesterState(semester);
28+
if (semester) {
29+
setSearchParams({ semester });
30+
}
31+
};
2432
const createRoomRequest = async (
2533
payload: RoomRequestFormValues,
2634
): Promise<RoomRequestPostResponse> => {
@@ -45,8 +53,16 @@ export const ManageRoomRequestsPage: React.FC = () => {
4553
};
4654

4755
useEffect(() => {
48-
setSemester(nextSemesters[0].value);
49-
}, []);
56+
const semeseterFromUrl = searchParams.get("semester") as string | null;
57+
if (
58+
semeseterFromUrl &&
59+
semesterOptions.map((x) => x.value).includes(semeseterFromUrl)
60+
) {
61+
setSemester(semeseterFromUrl);
62+
} else {
63+
setSemester(nextSemesters[0].value);
64+
}
65+
}, [searchParams, semesterOptions, nextSemesters]);
5066
return (
5167
<AuthGuard
5268
resourceDef={{

terraform/envs/prod/.terraform.lock.hcl

Lines changed: 21 additions & 21 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

terraform/envs/prod/main.tf

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ terraform {
22
required_providers {
33
aws = {
44
source = "hashicorp/aws"
5-
version = "~> 6.18.0"
5+
version = "= 6.19.0"
66
}
77
}
88

terraform/envs/qa/.terraform.lock.hcl

Lines changed: 21 additions & 21 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

terraform/envs/qa/main.tf

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ terraform {
22
required_providers {
33
aws = {
44
source = "hashicorp/aws"
5-
version = "~> 6.18.0"
5+
version = "= 6.19.0"
66
}
77
}
88

tests/e2e/base.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ import {
44
GetSecretValueCommand,
55
} from "@aws-sdk/client-secrets-manager";
66

7+
export interface RecursiveRecord
8+
extends Record<string, any | RecursiveRecord> {}
9+
710
export const getSecretValue = async (
811
secretId: string,
912
): Promise<Record<string, string | number | boolean> | null> => {
@@ -71,12 +74,12 @@ export async function getUpcomingEvents() {
7174
const data = await fetch(
7275
"https://core.aws.qa.acmuiuc.org/api/v1/events?upcomingOnly=true",
7376
);
74-
return (await data.json()) as Record<string, string>[];
77+
return (await data.json()) as RecursiveRecord[];
7578
}
7679

7780
export async function getAllEvents() {
7881
const data = await fetch("https://core.aws.qa.acmuiuc.org/api/v1/events");
79-
return (await data.json()) as Record<string, string>[];
82+
return (await data.json()) as RecursiveRecord[];
8083
}
8184

8285
export const test = base.extend<{ becomeUser: (page: Page) => Promise<void> }>({

tests/e2e/orgInfo.spec.ts

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
import { expect } from "@playwright/test";
2+
import { RecursiveRecord, test } from "./base.js";
3+
import { describe } from "node:test";
4+
5+
describe("Organization Info Tests", () => {
6+
test("A user can update org metadata", async ({ page, becomeUser }) => {
7+
const date = new Date().toISOString();
8+
await becomeUser(page);
9+
await expect(
10+
page.locator("a").filter({ hasText: "Management Portal DEV ENV" }),
11+
).toBeVisible();
12+
await expect(
13+
page.locator("a").filter({ hasText: "Organization Info" }),
14+
).toBeVisible();
15+
await page.locator("a").filter({ hasText: "Organization Info" }).click();
16+
await expect(page.getByRole("heading")).toContainText(
17+
"Manage Organization Info",
18+
);
19+
await page.getByRole("textbox", { name: "Select an organization" }).click();
20+
await page.getByText("Infrastructure Committee").click();
21+
await page.getByRole("textbox", { name: "Description" }).click();
22+
await page
23+
.getByRole("textbox", { name: "Description" })
24+
.fill(`Populated by E2E tests on ${date}`);
25+
await page
26+
.getByRole("textbox", { name: "Website" })
27+
.fill(`https://infra.acm.illinois.edu?date=${date}`);
28+
29+
const existingOtherLink = page.locator("text=Other").first();
30+
const hasExistingOther = await existingOtherLink
31+
.isVisible()
32+
.catch(() => false);
33+
34+
if (!hasExistingOther) {
35+
await page.getByRole("button", { name: "Add Link" }).click();
36+
await page.getByRole("textbox", { name: "Type" }).click();
37+
await page.getByRole("option", { name: "Other" }).click();
38+
}
39+
40+
await page.getByRole("textbox", { name: "URL" }).click();
41+
await page
42+
.getByRole("textbox", { name: "URL" })
43+
.fill(`https://infra.acm.illinois.edu/e2e?date=${date}`);
44+
await page
45+
.locator("form")
46+
.getByRole("button", { name: "Save Changes" })
47+
.click();
48+
await expect(
49+
page.getByText("Infrastructure Committee updated"),
50+
).toBeVisible();
51+
52+
const data = await fetch(
53+
`https://core.aws.qa.acmuiuc.org/api/v1/organizations?date=${date}`,
54+
);
55+
const json = (await data.json()) as RecursiveRecord[];
56+
const infraEntry = json.find((x) => x.id === "Infrastructure Committee");
57+
58+
expect(infraEntry).toBeDefined();
59+
expect(infraEntry?.description).toBe(`Populated by E2E tests on ${date}`);
60+
expect(infraEntry?.website).toBe(
61+
`https://infra.acm.illinois.edu?date=${date}`,
62+
);
63+
64+
const links = infraEntry?.links as RecursiveRecord[];
65+
expect(links).toBeDefined();
66+
const otherLink = links.find((link) => link.type === "OTHER");
67+
expect(otherLink).toBeDefined();
68+
expect(otherLink?.url).toBe(
69+
`https://infra.acm.illinois.edu/e2e?date=${date}`,
70+
);
71+
});
72+
});

0 commit comments

Comments
 (0)