diff --git a/app/controllers/course/assessment/marketplace/listings_controller.rb b/app/controllers/course/assessment/marketplace/listings_controller.rb index dcbbf7bbd6..eec9f66166 100644 --- a/app/controllers/course/assessment/marketplace/listings_controller.rb +++ b/app/controllers/course/assessment/marketplace/listings_controller.rb @@ -31,6 +31,7 @@ def show @assessment = @listing.assessment authorize!(:preview_in_marketplace, @assessment) + @destination_tabs = destination_tabs render 'show' end end diff --git a/app/views/course/assessment/marketplace/listings/show.json.jbuilder b/app/views/course/assessment/marketplace/listings/show.json.jbuilder index 4a5724ab96..92d6b4f730 100644 --- a/app/views/course/assessment/marketplace/listings/show.json.jbuilder +++ b/app/views/course/assessment/marketplace/listings/show.json.jbuilder @@ -3,6 +3,15 @@ json.id @assessment.id json.title @assessment.title json.description format_ckeditor_rich_text(@assessment.description) +# The current course's category/tab structure, so the duplicate confirmation dialog can offer the +# destination tab picker from the listing detail page (the listing itself lives in another course). +json.destinationTabs @destination_tabs do |tab| + json.id tab[:id] + json.title tab[:title] + json.categoryId tab[:category_id] + json.categoryTitle tab[:category_title] +end + json.gradingMode @assessment.autograded? ? 'autograded' : 'manual' json.baseExp @assessment.base_exp if @assessment.base_exp > 0 json.bonusExp @assessment.time_bonus_exp if @assessment.time_bonus_exp > 0 diff --git a/client/app/bundles/course/duplication/components/TypeBadge/index.tsx b/client/app/bundles/course/duplication/components/TypeBadge/index.tsx index 1f4add68ec..d1eeb171df 100644 --- a/client/app/bundles/course/duplication/components/TypeBadge/index.tsx +++ b/client/app/bundles/course/duplication/components/TypeBadge/index.tsx @@ -45,15 +45,18 @@ const translations: Record = }, }); -const TypeBadge: FC<{ text?: string; itemType: DuplicableItemType }> = ({ - text, - itemType, -}) => { +const TypeBadge: FC<{ + text?: string; + itemType: DuplicableItemType; + dense?: boolean; +}> = ({ text, itemType, dense = false }) => { const { t } = useTranslation(); return ( diff --git a/client/app/bundles/course/marketplace/components/DestinationTabPicker.tsx b/client/app/bundles/course/marketplace/components/DestinationTabPicker.tsx new file mode 100644 index 0000000000..b0f90d854c --- /dev/null +++ b/client/app/bundles/course/marketplace/components/DestinationTabPicker.tsx @@ -0,0 +1,93 @@ +import { FC } from 'react'; +import { + Card, + CardContent, + FormControlLabel, + Radio, + RadioGroup, +} from '@mui/material'; + +import TypeBadge from 'course/duplication/components/TypeBadge'; + +import { DestinationTab } from '../types'; + +interface Group { + categoryId: number; + categoryTitle: string; + tabs: DestinationTab[]; +} + +interface DestinationTabPickerProps { + tabs: DestinationTab[]; + value: number | null; + onChange: (tabId: number) => void; +} + +// Group tabs by category in first-seen order (the controller already emits categories then their +// tabs in display order, so this preserves that without re-sorting). Robust to a category's tabs +// arriving non-contiguously. +const groupByCategory = (tabs: DestinationTab[]): Group[] => { + const groups: Group[] = []; + const indexByCategory = new Map(); + tabs.forEach((tab) => { + const existing = indexByCategory.get(tab.categoryId); + if (existing === undefined) { + indexByCategory.set(tab.categoryId, groups.length); + groups.push({ + categoryId: tab.categoryId, + categoryTitle: tab.categoryTitle, + tabs: [tab], + }); + } else { + groups[existing].tabs.push(tab); + } + }); + return groups; +}; + +const DestinationTabPicker: FC = ({ + tabs, + value, + onChange, +}) => { + const groups = groupByCategory(tabs); + + return ( + + + onChange(Number(e.target.value))} + value={value != null ? String(value) : ''} + > + {groups.map((group) => ( +
+
+ + {group.categoryTitle} +
+ {group.tabs.map((tab) => ( + } + label={ + + + {tab.title} + + } + value={String(tab.id)} + /> + ))} +
+ ))} +
+
+
+ ); +}; + +export default DestinationTabPicker; diff --git a/client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx b/client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx index abd1aee3f6..06ec2ea5c5 100644 --- a/client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx +++ b/client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx @@ -1,7 +1,9 @@ -import { useState } from 'react'; +import { useEffect, useState } from 'react'; +import { Tooltip } from 'react-tooltip'; import { Card, CardContent, ListSubheader } from '@mui/material'; -import DuplicationAssessmentTree from 'course/duplication/components/DuplicationAssessmentTree'; +import TypeBadge from 'course/duplication/components/TypeBadge'; +import UnpublishedIcon from 'course/duplication/components/UnpublishedIcon'; import Prompt from 'lib/components/core/dialogs/Prompt'; import Link from 'lib/components/core/Link'; import toast from 'lib/hooks/toast'; @@ -9,37 +11,81 @@ import useTranslation from 'lib/hooks/useTranslation'; import { duplicateListings } from '../operations'; import translations from '../translations'; -import { MarketplaceListing } from '../types'; +import { DestinationTab, MarketplaceListing } from '../types'; + +import DestinationTabPicker from './DestinationTabPicker'; interface Props { listings: Pick[]; - destinationTabId: number | null; + destinationTabs: DestinationTab[]; + initialDestinationTabId: number | null; destinationCourse: { title: string; url: string }; - destinationCategory: { id: number; title: string } | null; - destinationTab: { id: number; title: string } | null; open: boolean; onClose: () => void; } const DuplicateConfirmation = ({ listings, - destinationTabId, + destinationTabs, + initialDestinationTabId, destinationCourse, - destinationCategory, - destinationTab, open, onClose, }: Props): JSX.Element => { const { t } = useTranslation(); const [submitting, setSubmitting] = useState(false); + // The selected tab defaults to the `from_tab` the user launched from, but only when it names a + // real tab in this course; otherwise the course's first tab. So exactly one existing tab is + // always selected, and an unknown/absent from_tab yields null (backend then defaults) rather than + // a phantom id. + const resolveInitial = (): number | null => { + if ( + initialDestinationTabId != null && + destinationTabs.some((tab) => tab.id === initialDestinationTabId) + ) { + return initialDestinationTabId; + } + return destinationTabs[0]?.id ?? null; + }; + + const [selectedTabId, setSelectedTabId] = useState( + resolveInitial(), + ); + + // The pages keep this component mounted and only flip `open`, so `selectedTabId` outlives a close + // — re-seed it each time the dialog opens, or a tab the user picked and then walked away from + // would still be selected next time. + // + // Deps are `[open]` on purpose. Adding `destinationTabs` would compare it by identity, so any + // parent re-render passing a fresh array would re-fire this and reset the radio out from under a + // user mid-decision. Reopening is the only moment the selection should be re-seeded. + useEffect(() => { + if (!open) return; + setSelectedTabId(resolveInitial()); + }, [open]); + const confirm = async (): Promise => { setSubmitting(true); await duplicateListings( listings.map((l) => l.id), - destinationTabId, - () => { - toast.success(t(translations.duplicateStarted, { n: listings.length })); + selectedTabId, + // This is pollJob's *completion* callback — the job has finished by now. `redirectUrl` points + // at the destination tab; it is optional on JobCompleted, so the link is conditional. + (redirectUrl) => { + toast.success( + <> + {t(translations.duplicateCompleted, { n: listings.length })} + {redirectUrl && ( + <> + {' '} + + {t(translations.viewDuplicatedAssessment)} + + + )} + , + ); setSubmitting(false); onClose(); }, @@ -52,10 +98,12 @@ const DuplicateConfirmation = ({ return ( @@ -71,16 +119,32 @@ const DuplicateConfirmation = ({ - {t(translations.assessmentsHeading)} + {t(translations.pickDestinationTab)} - + + {t(translations.duplicating)} + + + {listings.map((listing) => ( +
+ + + {listing.title} +
+ ))} +
+
+ + {t(translations.itemUnpublished)} +
); }; diff --git a/client/app/bundles/course/marketplace/components/__test__/DestinationTabPicker.test.tsx b/client/app/bundles/course/marketplace/components/__test__/DestinationTabPicker.test.tsx new file mode 100644 index 0000000000..b78449abfb --- /dev/null +++ b/client/app/bundles/course/marketplace/components/__test__/DestinationTabPicker.test.tsx @@ -0,0 +1,106 @@ +import { fireEvent, render } from 'test-utils'; + +import DestinationTabPicker from '../DestinationTabPicker'; + +const tabs = [ + { id: 10, title: 'Tutorials', categoryId: 1, categoryTitle: 'Week 3' }, + { id: 11, title: 'Problem Sets', categoryId: 1, categoryTitle: 'Week 3' }, + { id: 20, title: 'Lab', categoryId: 2, categoryTitle: 'Week 4' }, +]; + +it('renders an empty radio group when there are no tabs', async () => { + const page = render( + , + ); + + expect(await page.findByRole('radiogroup')).toBeEmptyDOMElement(); +}); + +it('groups tabs under one header per category and renders a radio per tab', async () => { + const page = render( + , + ); + + // I18nProvider (TypeBadge uses it) async-loads messages, so await the first query. + expect(await page.findByText('Week 3')).toBeVisible(); + expect(page.getByText('Week 4')).toBeVisible(); + // The two Week 3 tabs share a single header. + expect(page.getAllByText('Week 3')).toHaveLength(1); + expect(page.getAllByRole('radio')).toHaveLength(3); + // Headers are badged as categories, radios as tabs. RTL's text matcher only sees an element's + // direct text-node children, so TypeBadge's Typography matches 'Category'/'Tab' on its own. + expect(page.getAllByText('Category')).toHaveLength(2); + expect(page.getAllByText('Tab')).toHaveLength(3); +}); + +// The fixture is interleaved AND in descending categoryId order, so first-seen order and any +// sorted order disagree — the flat `tabs` fixture above cannot tell them apart. +it('groups a category under its first-seen header when its tabs arrive non-contiguously', async () => { + const interleavedTabs = [ + { id: 20, title: 'Lab', categoryId: 2, categoryTitle: 'Week 4' }, + { id: 10, title: 'Tutorials', categoryId: 1, categoryTitle: 'Week 3' }, + { id: 21, title: 'Recitation', categoryId: 2, categoryTitle: 'Week 4' }, + ]; + + const page = render( + , + ); + + // Week 4's two tabs are split by a Week 3 tab, but still share one header. + expect(await page.findAllByText('Week 4')).toHaveLength(1); + expect(page.getAllByText('Week 3')).toHaveLength(1); + // Week 4 is seen first, so its group (and both its tabs) comes first. + expect( + page.getAllByRole('radio').map((radio) => radio.getAttribute('value')), + ).toEqual(['20', '21', '10']); +}); + +it('marks the tab whose id equals value as checked', async () => { + const page = render( + , + ); + + expect( + await page.findByRole('radio', { name: /Problem Sets/ }), + ).toBeChecked(); + expect(page.getByRole('radio', { name: /Tutorials/ })).not.toBeChecked(); + expect(page.getByRole('radio', { name: /Lab/ })).not.toBeChecked(); +}); + +it('checks no tab when value is null', async () => { + const page = render( + , + ); + + expect(await page.findAllByRole('radio')).toHaveLength(3); + page + .getAllByRole('radio') + .forEach((radio) => expect(radio).not.toBeChecked()); +}); + +it('fires onChange with the numeric tab id when another tab is chosen', async () => { + const onChange = jest.fn(); + const page = render( + , + ); + + fireEvent.click(await page.findByRole('radio', { name: /Lab/ })); + + expect(onChange).toHaveBeenCalledWith(20); +}); + +it('does not move the selection itself when a tab is clicked', async () => { + const page = render( + , + ); + + fireEvent.click(await page.findByRole('radio', { name: /Lab/ })); + + // Controlled: the parent still says 11, so the checkmark must not move. + expect(page.getByRole('radio', { name: /Problem Sets/ })).toBeChecked(); + expect(page.getByRole('radio', { name: /Lab/ })).not.toBeChecked(); +}); diff --git a/client/app/bundles/course/marketplace/components/__test__/DuplicationConfirmation.test.tsx b/client/app/bundles/course/marketplace/components/__test__/DuplicationConfirmation.test.tsx index 6ae21fb1c0..afbac3070a 100644 --- a/client/app/bundles/course/marketplace/components/__test__/DuplicationConfirmation.test.tsx +++ b/client/app/bundles/course/marketplace/components/__test__/DuplicationConfirmation.test.tsx @@ -1,67 +1,241 @@ import { createMockAdapter } from 'mocks/axiosMock'; import { fireEvent, render, waitFor } from 'test-utils'; +import TestApp from 'utilities/TestApp'; +import GlobalAPI from 'api'; import CourseAPI from 'api/course'; +import toast from 'lib/hooks/toast'; import DuplicateConfirmation from '../DuplicateConfirmation'; +// The toast message is a ReactNode (it carries a link), so capture it and render it rather than +// mounting a ToastContainer. +jest.mock('lib/hooks/toast', () => ({ success: jest.fn(), error: jest.fn() })); + const mock = createMockAdapter(CourseAPI.marketplace.client); -beforeEach(() => mock.reset()); +// pollJob polls the *jobs* endpoint, which lives on a different axios client to the marketplace API. +const jobsMock = createMockAdapter(GlobalAPI.jobs.client); + +beforeEach(() => { + mock.reset(); + jobsMock.reset(); + jest.clearAllMocks(); +}); -const listings = [{ id: 1, title: 'Recursion Drills' }] as never; +const LISTING_TITLE = 'Recursion Drills'; +const listings = [{ id: 1, title: LISTING_TITLE }]; const url = `/courses/${global.courseId}/marketplace/listings/duplicate`; const course = { title: 'Enrollable Course', url: '/courses/4' }; +const REDIRECT_URL = '/courses/4/assessments?category=5&tab=42'; +const destinationTabs = [ + { id: 41, title: 'Tutorials', categoryId: 5, categoryTitle: 'Missions' }, + { id: 42, title: 'Assignments', categoryId: 5, categoryTitle: 'Missions' }, +]; + +const props = { + destinationCourse: course, + destinationTabs, + initialDestinationTabId: 42, + listings, + onClose: jest.fn(), +}; + +const successToastTexts = (): string[] => + (toast.success as unknown as jest.Mock).mock.calls.map(([message]) => { + if (typeof message === 'string') return message; + + const children = (message as { props?: { children?: unknown } }).props + ?.children; + if (Array.isArray(children)) { + return children.filter((child) => typeof child === 'string').join(''); + } + + return typeof children === 'string' ? children : ''; + }); + +it('forgets an abandoned selection and re-seeds the initial tab when reopened', async () => { + const page = render(); + + expect(await page.findByRole('radio', { name: /Assignments/ })).toBeChecked(); + + // The user picks a different tab, then dismisses the dialog without confirming. + fireEvent.click(page.getByRole('radio', { name: /Tutorials/ })); + expect(page.getByRole('radio', { name: /Tutorials/ })).toBeChecked(); + + // The page keeps this component mounted and only flips `open`, so `selectedTabId` outlives the + // close — which is the entire reason the re-seeding effect exists. + // rerender bypasses test-utils' TestApp wrapper, so re-wrap to keep providers. + page.rerender( + + + , + ); -it('shows the destination course and assessment tree with real names', async () => { + page.rerender( + + + , + ); + + // Reopening starts from the tab the user launched from, not the choice they walked away from. + expect(await page.findByRole('radio', { name: /Assignments/ })).toBeChecked(); + expect(page.getByRole('radio', { name: /Tutorials/ })).not.toBeChecked(); +}); + +it('keeps the user’s selection across a re-render while the dialog stays open', async () => { + const page = render(); + + fireEvent.click(await page.findByRole('radio', { name: /Tutorials/ })); + expect(page.getByRole('radio', { name: /Tutorials/ })).toBeChecked(); + + // A parent re-render must not re-seed the selection out from under the user mid-decision. + page.rerender( + + + , + ); + + expect(await page.findByRole('radio', { name: /Tutorials/ })).toBeChecked(); + expect(page.getByRole('radio', { name: /Assignments/ })).not.toBeChecked(); +}); + +it('shows the destination course, the tab picker, and the duplicating list', async () => { const page = render( , ); - // I18nProvider shows a LoadingIndicator until locale messages async-load; - // await the first query to render past it, then the rest are synchronous. - expect(await page.findByText('Enrollable Course')).toBeVisible(); + // I18nProvider shows a LoadingIndicator until locale messages async-load; await the first query + // to render past it, then the rest are synchronous. + expect(await page.findByText('Duplicate items?')).toBeVisible(); + + expect(page.getByText('Destination Course')).toBeVisible(); + expect(page.getByRole('link', { name: 'Enrollable Course' })).toHaveAttribute( + 'href', + '/courses/4', + ); + + expect(page.getByText('Pick destination tab')).toBeVisible(); expect(page.getByText('Missions')).toBeVisible(); + expect(page.getByText('Tutorials')).toBeVisible(); expect(page.getByText('Assignments')).toBeVisible(); - expect(page.getByText('Recursion Drills')).toBeVisible(); - // The old raw-key bug must not recur. - expect( - page.queryByText('course.marketplace.duplicateTitle'), - ).not.toBeInTheDocument(); + + expect(page.getByText('Duplicating')).toBeVisible(); + expect(page.getByText(LISTING_TITLE)).toBeVisible(); +}); + +it('stacks destination tabs vertically with large category and tab text', async () => { + const page = render( + , + ); + + expect(await page.findByText('Duplicate items?')).toBeVisible(); + + expect(page.getByText('Missions').closest('div')).toHaveClass('text-xl'); + + const assignments = page.getByRole('radio', { name: /Assignments/ }); + const tutorials = page.getByRole('radio', { name: /Tutorials/ }); + + expect(assignments.closest('label')?.parentElement).toHaveClass( + 'flex', + 'flex-col', + 'items-start', + ); + expect(assignments.closest('label')).toHaveClass('text-xl'); + expect(tutorials.closest('label')).toHaveClass('text-xl'); +}, 10000); + +// Pins the ⊘ icon and its wiring to the tooltip. NOT the tooltip copy: react-tooltip v5 renders +// nothing until shown, and hovering the anchor does not mount its content under jsdom (verified — +// `fireEvent.mouseEnter` + `findByText` on the message times out). So assert the wiring, which is +// what a dropped `tooltipId` or a dropped would break. +it('badges each item as an assessment and marks it as arriving unpublished', async () => { + const page = render( + , + ); + + expect(await page.findByText(LISTING_TITLE)).toBeVisible(); + expect(page.getByText('Assessment')).toBeVisible(); + expect(page.getByTestId('BlockIcon')).toHaveAttribute( + 'data-tooltip-id', + 'itemUnpublished', + ); +}); + +it('pre-selects the tab the user came from', async () => { + const page = render( + , + ); + + expect(await page.findByRole('radio', { name: /Assignments/ })).toBeChecked(); + expect(page.getByRole('radio', { name: /Tutorials/ })).not.toBeChecked(); +}); + +it('falls back to the first tab when the initial id is not a real tab', async () => { + const page = render( + , + ); + + expect(await page.findByRole('radio', { name: /Tutorials/ })).toBeChecked(); + expect(page.getByRole('radio', { name: /Assignments/ })).not.toBeChecked(); }); -it('falls back to Default placeholders when entered without a tab', async () => { +it('falls back to the first tab when entered without a from_tab', async () => { const page = render( , ); - expect(await page.findByText('Default Category')).toBeVisible(); - expect(page.getByText('Default Tab')).toBeVisible(); + expect(await page.findByRole('radio', { name: /Tutorials/ })).toBeChecked(); }); -it('posts a duplication request with the destination tab on confirm', async () => { +it('posts the pre-selected destination tab on confirm', async () => { mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); const page = render( { +it('posts the newly chosen tab after the user changes the selection', async () => { mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); const page = render( , ); - fireEvent.click(await page.findByRole('button', { name: /Duplicate/ })); + + fireEvent.click(await page.findByRole('radio', { name: /Tutorials/ })); + fireEvent.click(page.getByRole('button', { name: /Duplicate/ })); + await waitFor(() => expect(mock.history.post).toHaveLength(1)); + expect(JSON.parse(mock.history.post[0].data)).toMatchObject({ + listing_ids: [1], + destination_tab_id: 41, + }); +}); + +it('omits the destination tab entirely when the course has no tabs to pick from', async () => { + mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); + + const page = render( + , + ); + + expect(await page.findByText('Recursion Drills')).toBeVisible(); + expect(page.queryAllByRole('radio')).toHaveLength(0); + + fireEvent.click(page.getByRole('button', { name: /Duplicate/ })); + + await waitFor(() => expect(mock.history.post).toHaveLength(1)); + const body = JSON.parse(mock.history.post[0].data); expect(body).toMatchObject({ listing_ids: [1] }); - expect(body).not.toHaveProperty('destination_tab_id'); // backend then defaults to the first tab + // There is no tab to name, so the key must be absent and the backend picks its own default. + expect(body).not.toHaveProperty('destination_tab_id'); }); + +it('duplicates every selected listing and pluralises the completion toast', async () => { + mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); + jobsMock.onGet('/jobs/9').reply(200, { status: 'completed' }); + + const page = render( + , + ); + + expect(await page.findByText(LISTING_TITLE)).toBeVisible(); + expect(page.getByText('Graph Traversals')).toBeVisible(); + + fireEvent.click(page.getByRole('button', { name: /Duplicate/ })); + + await waitFor(() => expect(mock.history.post).toHaveLength(1)); + expect(JSON.parse(mock.history.post[0].data)).toMatchObject({ + listing_ids: [1, 2], + }); + + await waitFor( + () => expect(successToastTexts()).toContain('Assessments duplicated.'), + { timeout: 6000 }, + ); +}, 10000); + +// The toast fires from pollJob's COMPLETION callback, so it must not claim the work has merely +// "started" — and it must surface the redirectUrl that callback receives, which the dialog used to +// throw away, leaving the user with no idea where the duplicate landed. +it('reports completion and links to where the assessment landed', async () => { + mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); + jobsMock + .onGet('/jobs/9') + .reply(200, { status: 'completed', redirectUrl: REDIRECT_URL }); + + const page = render( + , + ); + + fireEvent.click(await page.findByRole('button', { name: /Duplicate/ })); + + // pollJob polls every 2s — longer than waitFor's 1s default. + await waitFor(() => expect(toast.success).toHaveBeenCalled(), { + timeout: 6000, + }); + + const message = (toast.success as unknown as jest.Mock).mock.calls[0][0]; + const toasted = render(
{message}
); + + expect(await toasted.findByText(/Assessment duplicated\./)).toBeVisible(); + expect(toasted.queryByText(/started/i)).not.toBeInTheDocument(); + expect( + toasted.getByRole('link', { name: 'View assessment' }), + ).toHaveAttribute('href', REDIRECT_URL); +}, 10000); + +it('closes itself once the duplication completes', async () => { + mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); + jobsMock + .onGet('/jobs/9') + .reply(200, { status: 'completed', redirectUrl: REDIRECT_URL }); + const onClose = jest.fn(); + + const page = render( + , + ); + + fireEvent.click(await page.findByRole('button', { name: /Duplicate/ })); + + // The dialog must dismiss itself on completion — the toast (with its link) is what remains. + await waitFor(() => expect(onClose).toHaveBeenCalledTimes(1), { + timeout: 6000, + }); +}, 10000); + +it('omits the link when the job returns no redirect url', async () => { + mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); + jobsMock.onGet('/jobs/9').reply(200, { status: 'completed' }); + + const page = render( + , + ); + + fireEvent.click(await page.findByRole('button', { name: /Duplicate/ })); + + await waitFor(() => expect(toast.success).toHaveBeenCalled(), { + timeout: 6000, + }); + + const message = (toast.success as unknown as jest.Mock).mock.calls[0][0]; + const toasted = render(
{message}
); + + expect(await toasted.findByText(/Assessment duplicated\./)).toBeVisible(); + expect( + toasted.queryByRole('link', { name: 'View assessment' }), + ).not.toBeInTheDocument(); +}, 10000); + +// Guards the reworded failure copy — the old string was a malformed gerund +// ("Duplicating assessment failed."). +it('reports a failed duplication in plain language', async () => { + mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); + jobsMock.onGet('/jobs/9').reply(200, { status: 'errored' }); + + const page = render( + , + ); + + fireEvent.click(await page.findByRole('button', { name: /Duplicate/ })); + + await waitFor(() => expect(toast.error).toHaveBeenCalled(), { + timeout: 6000, + }); + + expect(toast.error).toHaveBeenCalledWith( + 'Could not duplicate the assessment.', + ); + expect(toast.success).not.toHaveBeenCalled(); +}, 10000); + +it('locks the dialog while the job runs, then unlocks it without closing if the job fails', async () => { + mock.onPost(url).reply(200, { status: 'submitted', jobUrl: '/jobs/9' }); + jobsMock.onGet('/jobs/9').reply(200, { status: 'errored' }); + const onClose = jest.fn(); + + const page = render( + , + ); + + const duplicate = await page.findByRole('button', { name: /Duplicate/ }); + fireEvent.click(duplicate); + + // `Prompt` applies `disabled` to the cancel button as well as the primary one, so an in-flight + // job can be neither double-submitted nor abandoned halfway. + expect(duplicate).toBeDisabled(); + expect(page.getByRole('button', { name: 'Cancel' })).toBeDisabled(); + + fireEvent.click(duplicate); + + await waitFor(() => expect(toast.error).toHaveBeenCalled(), { + timeout: 6000, + }); + + expect(mock.history.post).toHaveLength(1); + + // A failed job must leave the dialog open and usable, so the user can retry. + await waitFor(() => expect(duplicate).toBeEnabled()); + expect(page.getByRole('button', { name: 'Cancel' })).toBeEnabled(); + expect(onClose).not.toHaveBeenCalled(); +}, 10000); diff --git a/client/app/bundles/course/marketplace/pages/ListingPreview/__test__/index.test.tsx b/client/app/bundles/course/marketplace/pages/ListingPreview/__test__/index.test.tsx index e1ebd1fc02..c687b47d63 100644 --- a/client/app/bundles/course/marketplace/pages/ListingPreview/__test__/index.test.tsx +++ b/client/app/bundles/course/marketplace/pages/ListingPreview/__test__/index.test.tsx @@ -46,6 +46,7 @@ it('renders the read-only assessment config', async () => { mock.onGet(url).reply(200, { id: 70, title: LISTING_TITLE, + destinationTabs: [], description: '

Awesome description 5

', gradingMode: 'manual', baseExp: 1000, @@ -101,6 +102,7 @@ it('carries from_tab into the per-question detail links', async () => { mock.onGet(url).reply(200, { id: 70, title: LISTING_TITLE, + destinationTabs: [], description: '

desc

', gradingMode: 'manual', baseExp: 0, @@ -137,6 +139,7 @@ it('navigates back to the marketplace carrying from_tab', async () => { mock.onGet(url).reply(200, { id: 70, title: LISTING_TITLE, + destinationTabs: [], description: '

desc

', gradingMode: 'manual', baseExp: 0, @@ -162,6 +165,7 @@ it('renders a back button to the marketplace index', async () => { mock.onGet(url).reply(200, { id: 70, title: LISTING_TITLE, + destinationTabs: [], description: '

desc

', gradingMode: 'manual', baseExp: 0, @@ -179,3 +183,28 @@ it('renders a back button to the marketplace index', async () => { // Page renders the back affordance as an IconButton with this testid when `backTo` is set. expect(screen.getByTestId('ArrowBackIconButton')).toBeInTheDocument(); }); + +it('marks the page title as a preview', async () => { + const url = `/courses/${global.courseId}/marketplace/listings/7`; + mock.onGet(url).reply(200, { + id: 70, + title: LISTING_TITLE, + destinationTabs: [], + description: '

desc

', + gradingMode: 'manual', + baseExp: 0, + bonusExp: 0, + showMcqMrqSolution: false, + showRubricToStudents: false, + gradedTestCases: '', + typeCounts: {}, + questions: [], + }); + + render(, { at: [url] }); + + await waitFor(() => expect(screen.getByText(LISTING_TITLE)).toBeVisible()); + // A "Preview" chip sits beside the title so the read-only listing detail page is never mistaken + // for the real assessment it mirrors. + expect(screen.getByText('Preview')).toBeVisible(); +}); diff --git a/client/app/bundles/course/marketplace/pages/ListingPreview/index.tsx b/client/app/bundles/course/marketplace/pages/ListingPreview/index.tsx index 8907bc2d43..5e6c9a04ab 100644 --- a/client/app/bundles/course/marketplace/pages/ListingPreview/index.tsx +++ b/client/app/bundles/course/marketplace/pages/ListingPreview/index.tsx @@ -51,7 +51,16 @@ const ListingPreview = (): JSX.Element => { } backTo={withFromTab(`${courseUrl}/marketplace`, fromTab)} className="space-y-5" - title={listing.title} + title={ + + {listing.title} + + + } > {listing.description && ( @@ -80,10 +89,9 @@ const ListingPreview = (): JSX.Element => { setDuplicating(false)} open={duplicating} diff --git a/client/app/bundles/course/marketplace/pages/MarketplaceIndex/index.tsx b/client/app/bundles/course/marketplace/pages/MarketplaceIndex/index.tsx index 4b1f2850fc..346fe0fb91 100644 --- a/client/app/bundles/course/marketplace/pages/MarketplaceIndex/index.tsx +++ b/client/app/bundles/course/marketplace/pages/MarketplaceIndex/index.tsx @@ -9,7 +9,7 @@ import Preload from 'lib/components/wrappers/Preload'; import DuplicateConfirmation from '../../components/DuplicateConfirmation'; import { fetchListings } from '../../operations'; import translations from '../../translations'; -import { DestinationTab, MarketplaceListing } from '../../types'; +import { MarketplaceListing } from '../../types'; import MarketplaceTable from './MarketplaceTable'; @@ -21,43 +21,25 @@ const MarketplaceIndex = (): JSX.Element => { const destinationTabId = parseInt(fromTab ?? '', 10) || null; const [pending, setPending] = useState([]); - const resolveDestination = ( - tabs: DestinationTab[], - ): { - category: { id: number; title: string } | null; - tab: { id: number; title: string } | null; - } => { - const match = tabs.find((tab) => tab.id === destinationTabId); - if (!match) return { category: null, tab: null }; - return { - category: { id: match.categoryId, title: match.categoryTitle }, - tab: { id: match.id, title: match.title }, - }; - }; - return ( } while={fetchListings}> - {({ listings, destinationTabs }): JSX.Element => { - const destination = resolveDestination(destinationTabs); - return ( - - - setPending([])} - open={pending.length > 0} - /> - - ); - }} + {({ listings, destinationTabs }): JSX.Element => ( + + + setPending([])} + open={pending.length > 0} + /> + + )} ); }; diff --git a/client/app/bundles/course/marketplace/translations.ts b/client/app/bundles/course/marketplace/translations.ts index 34452679c8..3818d2785c 100644 --- a/client/app/bundles/course/marketplace/translations.ts +++ b/client/app/bundles/course/marketplace/translations.ts @@ -65,6 +65,10 @@ export default defineMessages({ id: 'course.marketplace.previewAction', defaultMessage: 'Preview', }, + previewBadge: { + id: 'course.marketplace.previewBadge', + defaultMessage: 'Preview', + }, duplicateAssessment: { id: 'course.marketplace.duplicateAssessment', defaultMessage: 'Duplicate Assessment', @@ -96,23 +100,40 @@ export default defineMessages({ id: 'course.marketplace.destinationCourse', defaultMessage: 'Destination Course', }, - assessmentsHeading: { - id: 'course.marketplace.assessmentsHeading', - defaultMessage: 'Assessments', + pickDestinationTab: { + id: 'course.marketplace.pickDestinationTab', + defaultMessage: 'Pick destination tab', + }, + duplicating: { + id: 'course.marketplace.duplicating', + defaultMessage: 'Duplicating', + }, + // Reuses the duplication bundle's existing id verbatim so formatjs extract dedupes rather than + // minting a marketplace-only duplicate; marketplace renders the ⊘ unpublished tooltip itself now. + itemUnpublished: { + id: 'course.duplication.Duplication.DuplicateItemsConfirmation.itemUnpublished', + defaultMessage: + 'Items are duplicated as unpublished when duplicating to an existing course.', }, duplicateConfirm: { id: 'course.marketplace.duplicateConfirm', defaultMessage: 'Duplicate', }, - duplicateStarted: { - id: 'course.marketplace.duplicateStarted', + // Fired from pollJob's completion callback, so this reports what already happened. The old copy + // said "started", which was both malformed ("Duplicating assessment started.") and untrue. + duplicateCompleted: { + id: 'course.marketplace.duplicateCompleted', defaultMessage: - '{n, plural, one {Duplicating assessment} other {Duplicating assessments}} started.', + '{n, plural, one {Assessment duplicated} other {Assessments duplicated}}.', }, duplicateFailed: { id: 'course.marketplace.duplicateFailed', defaultMessage: - '{n, plural, one {Duplicating assessment} other {Duplicating assessments}} failed.', + '{n, plural, one {Could not duplicate the assessment} other {Could not duplicate the assessments}}.', + }, + viewDuplicatedAssessment: { + id: 'course.marketplace.viewDuplicatedAssessment', + defaultMessage: 'View assessment', }, selectToDuplicate: { id: 'course.marketplace.selectToDuplicate', diff --git a/client/app/bundles/course/marketplace/types.ts b/client/app/bundles/course/marketplace/types.ts index d093a4fdbf..c4ac6667af 100644 --- a/client/app/bundles/course/marketplace/types.ts +++ b/client/app/bundles/course/marketplace/types.ts @@ -43,6 +43,9 @@ export interface ListingPreviewData { id: number; title: string; description: string; + // The previewer's own category/tab structure, so the duplicate dialog can offer the destination + // tab picker from the listing detail page (the listing itself lives in another course). + destinationTabs: DestinationTab[]; gradingMode: 'autograded' | 'manual'; baseExp: number | null; bonusExp: number | null; diff --git a/client/app/lib/hooks/toast/toast.tsx b/client/app/lib/hooks/toast/toast.tsx index a94f377d10..bafb0c50af 100644 --- a/client/app/lib/hooks/toast/toast.tsx +++ b/client/app/lib/hooks/toast/toast.tsx @@ -16,7 +16,9 @@ import { import { Typography } from '@mui/material'; import { produce } from 'immer'; -type Toaster = (message: string, options?: ToastOptions) => Id; +// `formattedMessage` already renders a ReactNode (and `PromisedToastMessages` already types its +// messages that way), so this only widens the type — nothing changes at runtime. +type Toaster = (message: ReactNode, options?: ToastOptions) => Id; interface PromisedToastMessages { pending?: ReactNode; diff --git a/client/app/routers/course/marketplace.tsx b/client/app/routers/course/marketplace.tsx index 9a7c3f7983..a3c54571dc 100644 --- a/client/app/routers/course/marketplace.tsx +++ b/client/app/routers/course/marketplace.tsx @@ -1,4 +1,4 @@ -import { RouteObject } from 'react-router-dom'; +import { Navigate, RouteObject } from 'react-router-dom'; import { WithRequired } from 'types'; import { Translated } from 'lib/hooks/useTranslation'; @@ -16,6 +16,12 @@ const marketplaceRouter: Translated = () => ({ .default, }), }, + { + // `listings` on its own (no id) is not a real page — send it back to the + // marketplace index so it lands in the same place as `marketplace/`. + path: 'listings', + element: , + }, { path: 'listings/:listingId', lazy: async () => ({ diff --git a/client/locales/en.json b/client/locales/en.json index b4b3e1569a..0073cd7ca7 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -6119,6 +6119,9 @@ "course.marketplace.previewAction": { "defaultMessage": "Preview" }, + "course.marketplace.previewBadge": { + "defaultMessage": "Preview" + }, "course.marketplace.searchPlaceholder": { "defaultMessage": "Search by title" }, @@ -6140,17 +6143,23 @@ "course.marketplace.destinationCourse": { "defaultMessage": "Destination Course" }, - "course.marketplace.assessmentsHeading": { - "defaultMessage": "Assessments" + "course.marketplace.pickDestinationTab": { + "defaultMessage": "Pick destination tab" + }, + "course.marketplace.duplicating": { + "defaultMessage": "Duplicating" }, "course.marketplace.duplicateConfirm": { "defaultMessage": "Duplicate" }, - "course.marketplace.duplicateStarted": { - "defaultMessage": "{n, plural, one {Duplicating assessment} other {Duplicating assessments}} started." + "course.marketplace.duplicateCompleted": { + "defaultMessage": "{n, plural, one {Assessment duplicated} other {Assessments duplicated}}." }, "course.marketplace.duplicateFailed": { - "defaultMessage": "{n, plural, one {Duplicating assessment} other {Duplicating assessments}} failed." + "defaultMessage": "{n, plural, one {Could not duplicate the assessment} other {Could not duplicate the assessments}}." + }, + "course.marketplace.viewDuplicatedAssessment": { + "defaultMessage": "View assessment" }, "course.marketplace.selectToDuplicate": { "defaultMessage": "Select to duplicate" diff --git a/spec/controllers/course/assessment/marketplace/listings_controller_spec.rb b/spec/controllers/course/assessment/marketplace/listings_controller_spec.rb index 008b73b548..9c02a8ac2d 100644 --- a/spec/controllers/course/assessment/marketplace/listings_controller_spec.rb +++ b/spec/controllers/course/assessment/marketplace/listings_controller_spec.rb @@ -139,6 +139,20 @@ expect(question['options']).to be_present end + it 'includes the current course destination tabs so the duplicate dialog can offer a picker' do + get :show, params: { course_id: course, id: listing.id, format: :json } + tabs = response.parsed_body['destinationTabs'] + expect(tabs).to be_present + default_tab = course.assessment_categories.first.tabs.first + row = tabs.find { |tab| tab['id'] == default_tab.id } + expect(row).to include( + 'id' => default_tab.id, + 'title' => default_tab.title, + 'categoryId' => default_tab.category.id, + 'categoryTitle' => default_tab.category.title + ) + end + context 'when the listing is unpublished' do let!(:listing) { create(:course_assessment_marketplace_listing, published: false) } it 'is forbidden' do