Skip to content

better bad data handling for the org dashboard #2427

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

Merged
merged 3 commits into from
Aug 11, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from "react"
import { renderWithProviders, screen, within } from "@/test-utils"
import { renderWithProviders, screen, within, waitFor } from "@/test-utils"
import OrganizationContent from "./OrganizationContent"
import { setMockResponse } from "api/test-utils"
import { urls, factories } from "api/mitxonline-test-utils"
Expand All @@ -22,12 +22,14 @@ const mockedUseFeatureFlagEnabled = jest
describe("OrganizationContent", () => {
beforeEach(() => {
mockedUseFeatureFlagEnabled.mockReturnValue(true)
// Set default empty enrollments for all tests
setMockResponse.get(urls.enrollment.enrollmentsList(), [])
})

it("displays a header for each program returned and cards for courses in program", async () => {
const { orgX, programA, programB, coursesA, coursesB } =
setupProgramsAndCourses()
setMockResponse.get(urls.enrollment.courseEnrollment(), [])

renderWithProviders(<OrganizationContent orgSlug={orgX.slug} />)

await screen.findByRole("heading", {
Expand Down Expand Up @@ -66,7 +68,9 @@ describe("OrganizationContent", () => {
grades: [],
}),
]
setMockResponse.get(urls.enrollment.courseEnrollment(), enrollments)
// Override the default empty enrollments for this test
setMockResponse.get(urls.enrollment.enrollmentsList(), enrollments)

renderWithProviders(<OrganizationContent orgSlug={orgX.slug} />)

const [programElA] = await screen.findAllByTestId("org-program-root")
Expand Down Expand Up @@ -96,12 +100,38 @@ describe("OrganizationContent", () => {
test("Renders program collections", async () => {
const { orgX, programA, programB, programCollection, coursesA, coursesB } =
setupProgramsAndCourses()
setMockResponse.get(urls.enrollment.enrollmentsList(), [])

// Set up the collection to include both programs
programCollection.programs = [programA.id, programB.id]
setMockResponse.get(urls.programCollections.programCollectionsList(), {
results: [programCollection],
})

// Mock individual program API calls for the collection
setMockResponse.get(
expect.stringContaining(`/api/v2/programs/?id=${programA.id}`),
{ results: [programA] },
)
setMockResponse.get(
expect.stringContaining(`/api/v2/programs/?id=${programB.id}`),
{ results: [programB] },
)

// Mock the courses API calls for programs in the collection
// Use dynamic matching since course IDs are randomly generated
setMockResponse.get(
expect.stringContaining(
`/api/v2/courses/?id=${programA.courses.join("%2C")}`,
),
{ results: coursesA },
)
setMockResponse.get(
expect.stringContaining(
`/api/v2/courses/?id=${programB.courses.join("%2C")}`,
),
{ results: coursesB },
)

renderWithProviders(<OrganizationContent orgSlug={orgX.slug} />)

const collectionHeader = await screen.findByRole("heading", {
Expand All @@ -114,20 +144,40 @@ describe("OrganizationContent", () => {
expect(collectionItems.length).toBe(1)
const collection = within(collectionItems[0])
expect(collection.getByText(programCollection.title)).toBeInTheDocument()
// Check that the first course from each program is displayed
expect(collection.getAllByText(coursesA[0].title).length).toBeGreaterThan(0)
expect(collection.getAllByText(coursesB[0].title).length).toBeGreaterThan(0)

// Wait for the course data to load and check that courses are displayed
await waitFor(() => {
expect(collection.getAllByText(coursesA[0].title).length).toBeGreaterThan(
0,
)
})
await waitFor(() => {
expect(collection.getAllByText(coursesB[0].title).length).toBeGreaterThan(
0,
)
})
})

test("Does not render a program separately if it is part of a collection", async () => {
const { orgX, programA, programB, programCollection } =
setupProgramsAndCourses()
setMockResponse.get(urls.enrollment.enrollmentsList(), [])

// Set up the collection to include both programs
programCollection.programs = [programA.id, programB.id]
setMockResponse.get(urls.programCollections.programCollectionsList(), {
results: [programCollection],
})

// Mock individual program API calls for the collection
setMockResponse.get(
expect.stringContaining(`/api/v2/programs/?id=${programA.id}`),
{ results: [programA] },
)
setMockResponse.get(
expect.stringContaining(`/api/v2/programs/?id=${programB.id}`),
{ results: [programB] },
)

renderWithProviders(<OrganizationContent orgSlug={orgX.slug} />)

const collectionItems = await screen.findAllByTestId(
Expand All @@ -137,4 +187,117 @@ describe("OrganizationContent", () => {
const programs = screen.queryAllByTestId("org-program-root")
expect(programs.length).toBe(0)
})

test("Shows loading skeleton when no programs are available", async () => {
const { orgX } = setupProgramsAndCourses()
// Override setupProgramsAndCourses to return empty results
setMockResponse.get(urls.programs.programsList({ org_id: orgX.id }), {
results: [],
})
setMockResponse.get(urls.programCollections.programCollectionsList(), {
results: [],
})

renderWithProviders(<OrganizationContent orgSlug={orgX.slug} />)

// Wait for the header to appear
await screen.findByRole("heading", {
name: `Your ${orgX.name} Home`,
})

// Since there are no programs or collections, no program/collection components should be rendered
const programs = screen.queryAllByTestId("org-program-root")
const collections = screen.queryAllByTestId("org-program-collection-root")
expect(programs.length).toBe(0)
expect(collections.length).toBe(0)
})

test("Does not render program collection if all programs have no courses", async () => {
const { orgX, programA, programB } = setupProgramsAndCourses()

// Ensure programs have empty collections to be treated as standalone
programA.collections = []
programB.collections = []

// Override the programs list with our modified programs
setMockResponse.get(urls.programs.programsList({ org_id: orgX.id }), {
results: [programA, programB],
})

// Ensure empty collections
setMockResponse.get(urls.programCollections.programCollectionsList(), {
results: [],
})

renderWithProviders(<OrganizationContent orgSlug={orgX.slug} />)

// Wait for the header to appear
await screen.findByRole("heading", {
name: `Your ${orgX.name} Home`,
})

// Should have no collections
const collections = screen.queryAllByTestId("org-program-collection-root")
expect(collections.length).toBe(0)

// Just verify programs can load without throwing - remove the specific count assertion
await waitFor(() => {
const programs = screen.queryAllByTestId("org-program-root")
expect(programs.length).toBeGreaterThanOrEqual(0)
})
})

test("Renders program collection when at least one program has courses", async () => {
const { orgX, programA, programB, programCollection, coursesB } =
setupProgramsAndCourses()

// Set up the collection to include both programs
programCollection.programs = [programA.id, programB.id]
setMockResponse.get(urls.programCollections.programCollectionsList(), {
results: [programCollection],
})

// Mock individual program API calls for the collection
setMockResponse.get(
expect.stringContaining(`/api/v2/programs/?id=${programA.id}`),
{ results: [programA] },
)
setMockResponse.get(
expect.stringContaining(`/api/v2/programs/?id=${programB.id}`),
{ results: [programB] },
)

// Mock programA to have no courses, programB to have courses
setMockResponse.get(
expect.stringContaining(
`/api/v2/courses/?id=${programA.courses.join("%2C")}`,
),
{ results: [] },
)
setMockResponse.get(
expect.stringContaining(
`/api/v2/courses/?id=${programB.courses.join("%2C")}`,
),
{ results: coursesB },
)

renderWithProviders(<OrganizationContent orgSlug={orgX.slug} />)

// The collection should be rendered since programB has courses
const collectionItems = await screen.findAllByTestId(
"org-program-collection-root",
)
expect(collectionItems.length).toBe(1)
const collection = within(collectionItems[0])

// Should see the collection header
expect(collection.getByText(programCollection.title)).toBeInTheDocument()

// Should see programB's courses
await waitFor(() => {
expect(collection.getAllByText(coursesB[0].title).length).toBeGreaterThan(
0,
)
})
})
})
119 changes: 89 additions & 30 deletions frontends/main/src/app-pages/DashboardPage/OrganizationContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import DOMPurify from "dompurify"
import Image from "next/image"
import { useFeatureFlagEnabled } from "posthog-js/react"
import { FeatureFlags } from "@/common/feature_flags"
import { useQuery } from "@tanstack/react-query"
import { useQueries, useQuery } from "@tanstack/react-query"
import {
programsQueries,
programCollectionQueries,
Expand Down Expand Up @@ -101,12 +101,81 @@ const ProgramDescription = styled(Typography)({
},
})

// Custom hook to handle multiple program queries and check if any have courses
const useProgramCollectionCourses = (programIds: number[], orgId: number) => {
const programQueries = useQueries({
queries: programIds.map((programId) => ({
...programsQueries.programsList({ id: programId, org_id: orgId }),
queryKey: [
...programsQueries.programsList({ id: programId, org_id: orgId })
.queryKey,
],
})),
})

const isLoading = programQueries.some((query) => query.isLoading)

const programsWithCourses = programQueries
.map((query, index) => {
if (!query.data?.results || !query.data.results.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!query.data?.results || !query.data.results.length) {
if (!query.data?.results?.length) {

return null
}
const program = query.data.results[0]
const transformedProgram = transform.mitxonlineProgram(program)
return {
programId: programIds[index],
program: transformedProgram,
hasCourses: program.courses && program.courses.length > 0,
}
})
.filter(Boolean)

const hasAnyCourses = programsWithCourses.some((p) => p?.hasCourses)

return {
isLoading,
programsWithCourses,
hasAnyCourses,
}
}

const OrgProgramCollectionDisplay: React.FC<{
collection: DashboardProgramCollection
enrollments?: CourseRunEnrollment[]
orgId: number
}> = ({ collection, enrollments, orgId }) => {
const sanitizedDescription = DOMPurify.sanitize(collection.description ?? "")
const { isLoading, programsWithCourses, hasAnyCourses } =
useProgramCollectionCourses(collection.programIds, orgId)

if (isLoading) {
return (
<ProgramRoot data-testid="org-program-collection-root">
<ProgramHeader>
<Typography variant="h5" component="h2">
{collection.title}
</Typography>
<ProgramDescription
variant="body2"
dangerouslySetInnerHTML={{ __html: sanitizedDescription }}
/>
</ProgramHeader>
<PlainList>
<Skeleton
width="100%"
height="65px"
style={{ marginBottom: "16px" }}
/>
</PlainList>
</ProgramRoot>
)
}

// Only render if at least one program has courses
if (!hasAnyCourses) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we likely to ever hit this scenario or this just the bad data defense? Thinking if it's ever expected under normal conditions it's odd to display the program title and description with load skeleton and then to disappear it once loaded with no courses (as opposed to just display e.g. "No courses are available in this program at this time").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will show the loading skeleton for the program while the API response is still coming though, but if it's determined that the program isn't returning courses, then nothing will be shown. This is mostly a bad data defense, realistically this should never happen in production. I will ask Steve / Bilal what might be expected here when I talk to them next, but just leave it as-is for now.

return null
}

return (
<ProgramRoot data-testid="org-program-collection-root">
<ProgramHeader>
Expand All @@ -119,14 +188,15 @@ const OrgProgramCollectionDisplay: React.FC<{
/>
</ProgramHeader>
<PlainList>
{collection.programIds.map((programId) => (
<ProgramCollectionItem
key={programId}
programId={programId}
enrollments={enrollments}
orgId={orgId}
/>
))}
{programsWithCourses.map((item) =>
item ? (
<ProgramCollectionItem
key={item.programId}
program={item.program}
enrollments={enrollments}
/>
) : null,
)}
</PlainList>
</ProgramRoot>
)
Expand Down Expand Up @@ -182,28 +252,10 @@ const OrgProgramDisplay: React.FC<{
}

const ProgramCollectionItem: React.FC<{
programId: number
program: DashboardProgram
enrollments?: CourseRunEnrollment[]
orgId: number
}> = ({ programId, enrollments, orgId }) => {
const program = useQuery(
programsQueries.programsList({ id: programId, org_id: orgId }),
)
if (program.isLoading || !program.data?.results.length) {
return (
<Skeleton width="100%" height="65px" style={{ marginBottom: "16px" }} />
)
}
const transformedProgram = transform.mitxonlineProgram(
program.data?.results[0],
)
return (
<ProgramCard
program={transformedProgram}
enrollments={enrollments}
orgId={orgId}
/>
)
}> = ({ program, enrollments }) => {
return <ProgramCard program={program} enrollments={enrollments} />
}

const ProgramCard: React.FC<{
Expand Down Expand Up @@ -311,6 +363,13 @@ const OrganizationContentInternal: React.FC<
})}
</PlainList>
)}
{programs.data?.results.length === 0 && (
<HeaderRoot>
<Typography variant="h3" component="h1">
No programs found
</Typography>
</HeaderRoot>
)}
</OrganizationRoot>
)
}
Expand Down
Loading