fix(marketplace): address review on the duplication flow

Polling:
- drop `pollJob` (marked DO NOT USE -- it orphans pollers on navigation) in
  favour of a `pollJobRequest` interval owned by DuplicateConfirmation and torn
  down by its effect cleanup
- handle a rejected duplicate request, which previously left the prompt
  disabled with no job to re-enable it

Destination tab:
- keep an omitted `destination_tab_id` nil instead of coercing it to 0, so the
  job falls back to the destination course's default tab
- redirect to the tab the copies actually landed in, and to that tab's own
  category, rather than to `tab=0` or a tab from another course

Adoptions:
- record them before the duplicates' `after_duplicate_save` callbacks run, so a
  failure rolls the transaction back before that work rather than undoing it
- move the per-copy rule onto `Course::Assessment#record_marketplace_adoption`,
  next to the association and `initialize_duplicate`; the service keeps only the
  sweep, which the `after_duplicate_save` hook cannot replace (it never runs
  during course duplication)

Messages:
- pass `n` to the pluralised `duplicateTitle` and `duplicateFailed`, which were
  formatted without it
- give `duplicateBody`'s `other` branch the count its locale strings already had
- drop an `as never` cast that disabled type-checking on the test's props

Author: LWS49

app/controllers/course/assessment/marketplace/listings_controller.rb

@@ -22,7 +22,9 @@ def index
   def duplicate
     listings = authorized_listings
     job = Course::Assessment::Marketplace::DuplicationJob.perform_later(
-      listings.map(&:id), current_course, duplicate_params[:destination_tab_id].to_i,
+      # `presence` first: an omitted tab (the sidebar entry point) must stay nil so the job lets the
+      # duplication fall back to the destination course's first tab, rather than looking for tab 0.
+      listings.map(&:id), current_course, duplicate_params[:destination_tab_id].presence&.to_i,
       current_user: current_user
     ).job
     render partial: 'jobs/submitted', locals: { job: job }

app/jobs/course/assessment/marketplace/duplication_job.rb

@@ -11,36 +11,54 @@ def perform_tracked(listing_ids, destination_course, destination_tab_id, options
     current_user = options[:current_user]
     ActsAsTenant.without_tenant do
       listings = Course::Assessment::Marketplace::Listing.published.where(id: listing_ids)
+      target_tab = find_tab(destination_course, destination_tab_id)
+      last_copy = nil
       listings.each do |listing|
         # The adoption row is written by the duplication service itself, which tracks every copy of a
         # listed assessment regardless of the path that produced it. See
         # `Course::Duplication::BaseService#record_marketplace_adoptions`.
-        copy = duplicate_listing(listing, destination_course, current_user)
-        reparent_into_tab(copy, destination_course, destination_tab_id)
+        last_copy = duplicate_listing(listing, destination_course, current_user)
+        reparent_into_tab(last_copy, target_tab)
       end
-      redirect_to course_assessments_url(destination_course,
-                                         category: destination_course.assessment_categories.first.id,
-                                         tab: destination_tab_id,
-                                         host: destination_course.instance.host)
+      redirect_to assessments_url(destination_course, target_tab || last_copy&.tab)
     end
   end
 
   private
 
+  # @return [Course::Assessment::Tab, nil] The requested tab, or nil when no tab was requested or
+  #   the requested one does not belong to the destination course.
+  def find_tab(destination_course, destination_tab_id)
+    return nil unless destination_tab_id
+
+    destination_course.assessment_categories.
+      flat_map(&:tabs).find { |tab| tab.id == destination_tab_id }
+  end
+
   def duplicate_listing(listing, destination_course, current_user)
     source = listing.assessment
     Course::Duplication::ObjectDuplicationService.duplicate_objects(
       source.course, destination_course, source, current_user: current_user
     )
   end
 
-  def reparent_into_tab(copy, destination_course, destination_tab_id)
-    target_tab = destination_course.assessment_categories.
-                 flat_map(&:tabs).find { |tab| tab.id == destination_tab_id }
+  def reparent_into_tab(copy, target_tab)
     return unless target_tab && copy.tab_id != target_tab.id
 
     copy.tab = target_tab
     copy.folder.parent = target_tab.category.folder
     copy.save!
   end
+
+  # Points at the tab the copies actually landed in. No tab is requested from the sidebar entry
+  # point, and a requested tab may not belong to the destination course -- in both cases the
+  # duplication picks the destination's default tab, and the redirect has to follow it there
+  # instead of naming a tab (and its category) that the user cannot open.
+  def assessments_url(destination_course, tab)
+    redirect_category_id = tab&.category_id || destination_course.assessment_categories.first.id
+    course_assessments_url(destination_course,
+                           category: redirect_category_id,
+                           tab: tab&.id,
+                           host: destination_course.instance.host)
+  end
 end

app/models/course/assessment.rb

@@ -250,6 +250,28 @@ def csv_downloadable?
     questions.any?(&:csv_downloadable?)
   end
 
+  # Records +duplicate+, a copy of this assessment, as an adoption of this assessment's marketplace
+  # listing. Every copy of a listed assessment is an adoption, whichever duplication path produced
+  # it, so this is called by the duplication services rather than by the marketplace's own job.
+  #
+  # The listing itself is never carried over -- +initialize_duplicate+ below does not duplicate the
+  # +marketplace_listing+ association -- so a copy always starts out unlisted.
+  #
+  # @param [Course::Assessment] duplicate The saved copy of this assessment.
+  # @param [Course] destination_course The course the copy was duplicated into.
+  # @param [User] current_user The user who triggered the duplication.
+  def record_marketplace_adoption(duplicate, destination_course, current_user)
+    return unless marketplace_listing&.published?
+
+    Course::Assessment::Marketplace::Adoption.create!(
+      listing: marketplace_listing,
+      destination_course: destination_course,
+      duplicated_assessment: duplicate,
+      creator: current_user,
+      updater: current_user
+    )
+  end
+
   def initialize_duplicate(duplicator, other) # rubocop:disable Metrics/AbcSize,Metrics/MethodLength
     copy_attributes(other, duplicator)
     target_tab = initialize_duplicate_tab(duplicator, other)

app/services/course/duplication/base_service.rb

@@ -30,21 +30,23 @@ def initialize_duplicator(*)
     raise NotImplementedError, 'To be implemented by specific duplication service.'
   end
 
-  # Records a marketplace adoption for every duplicated assessment whose source is published on the
-  # marketplace. Copies made outside +Course::Assessment::Marketplace::DuplicationJob+ -- selected
-  # object duplications and full course duplications that happen to carry a listed assessment along
-  # -- are adoptions too, and the listing has to know about them to reach every course holding a
-  # copy. The listing itself is never carried over: +Course::Assessment#initialize_duplicate+ does
-  # not duplicate the +marketplace_listing+ association, so a copy always starts out unlisted.
+  # Hands every duplicated assessment its own copy so it can record a marketplace adoption. Copies
+  # made outside +Course::Assessment::Marketplace::DuplicationJob+ -- selected object duplications
+  # and full course duplications that happen to carry a listed assessment along -- are adoptions
+  # too, and the listing has to know about them to reach every course holding a copy.
+  #
+  # This sweep lives in the duplication service rather than in a model's +after_duplicate_save+
+  # hook because that hook only runs for the top-level objects of an object duplication, and never
+  # at all during a course duplication -- both of which are paths this has to cover. The per-copy
+  # rule itself belongs to the assessment: see +Course::Assessment#record_marketplace_adoption+.
   #
   # Must be called inside the duplication transaction, after the duplicates have been saved.
   def record_marketplace_adoptions
     destination_course = @options[:destination_course] || duplicator.options[:destination_course]
     return unless destination_course
 
     duplicated_assessment_pairs.each do |source, duplicate|
-      listing = source.marketplace_listing
-      record_marketplace_adoption(listing, destination_course, duplicate) if listing&.published?
+      source.record_marketplace_adoption(duplicate, destination_course, @options[:current_user])
     end
   end
 
@@ -54,15 +56,4 @@ def duplicated_assessment_pairs
       source.is_a?(Course::Assessment) && duplicate&.persisted?
     end
   end
-
-  def record_marketplace_adoption(listing, destination_course, duplicate)
-    current_user = @options[:current_user]
-    Course::Assessment::Marketplace::Adoption.create!(
-      listing: listing,
-      destination_course: destination_course,
-      duplicated_assessment: duplicate,
-      creator: current_user,
-      updater: current_user
-    )
-  end
 end

app/services/course/duplication/object_duplication_service.rb

@@ -45,10 +45,12 @@ def duplicate_objects(objects)
       duplicated = duplicator.duplicate(objects)
       before_save(objects, duplicated)
       save_success = duplicated.respond_to?(:save) ? duplicated.save : duplicated.all?(&:save)
+      # Recorded before `after_save` so that a failure here rolls the transaction back before the
+      # models' post-duplication callbacks have run, rather than undoing their work afterwards.
+      record_marketplace_adoptions if save_success
       after_save_success = save_success && after_save(objects, duplicated)
       raise ActiveRecord::Rollback unless after_save_success
 
-      record_marketplace_adoptions
       duplicated
     end
   end

client/app/api/course/Marketplace.ts

@@ -1,4 +1,5 @@
 import { AxiosResponse } from 'axios';
+import { JobSubmitted } from 'types/jobs';
 
 import { MarketplaceListing } from 'course/marketplace/types';
 
@@ -30,7 +31,7 @@ export default class MarketplaceAPI extends BaseCourseAPI {
   duplicate(
     listingIds: number[],
     destinationTabId: number | null,
-  ): Promise<AxiosResponse> {
+  ): Promise<AxiosResponse<JobSubmitted>> {
     return this.client.post(`${this.#urlPrefix}/listings/duplicate`, {
       listing_ids: listingIds,
       ...(destinationTabId ? { destination_tab_id: destinationTabId } : {}),

client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx

@@ -1,13 +1,17 @@
-import { useState } from 'react';
+import { useEffect, useRef, useState } from 'react';
 import { useIntl } from 'react-intl';
+import { JobStatus } from 'types/jobs';
 
 import Prompt, { PromptText } from 'lib/components/core/dialogs/Prompt';
+import { pollJobRequest } from 'lib/helpers/jobHelpers';
 import toast from 'lib/hooks/toast';
 
 import { duplicateListings } from '../operations';
 import translations from '../translations';
 import { MarketplaceListing } from '../types';
 
+const JOB_POLL_INTERVAL_MS = 2000;
+
 interface Props {
   listings: Pick<MarketplaceListing, 'id' | 'title'>[];
   destinationTabId: number | null;
@@ -23,36 +27,70 @@ const DuplicateConfirmation = ({
 }: Props): JSX.Element => {
   const { formatMessage: t } = useIntl();
   const [submitting, setSubmitting] = useState(false);
+  const [jobUrl, setJobUrl] = useState<string | null>(null);
+  const pollingRef = useRef(false);
+
+  const n = listings.length;
 
   const confirm = async (): Promise<void> => {
     setSubmitting(true);
-    await duplicateListings(
-      listings.map((l) => l.id),
-      destinationTabId,
-      () => {
-        toast.success(t(translations.duplicateStarted, { n: listings.length }));
-        setSubmitting(false);
-        onClose();
-      },
-      () => {
-        toast.error(t(translations.duplicateFailed));
-        setSubmitting(false);
-      },
-    );
+    try {
+      const url = await duplicateListings(
+        listings.map((l) => l.id),
+        destinationTabId,
+      );
+      setJobUrl(url);
+    } catch {
+      // The request never reached the queue, so there is no job to poll. Releasing `submitting`
+      // here is what keeps the prompt usable for a retry instead of disabled for good.
+      toast.error(t(translations.duplicateFailed, { n }));
+      setSubmitting(false);
+    }
   };
 
+  // The poller lives with the component that started the job, so unmounting or navigating away
+  // tears it down. `pollingRef` stops a slow response from stacking up overlapping requests.
+  useEffect(() => {
+    if (!jobUrl) return undefined;
+
+    const finish = (succeeded: boolean): void => {
+      setJobUrl(null);
+      setSubmitting(false);
+      if (succeeded) {
+        toast.success(t(translations.duplicateStarted, { n }));
+        onClose();
+      } else {
+        toast.error(t(translations.duplicateFailed, { n }));
+      }
+    };
+
+    const interval = setInterval(() => {
+      if (pollingRef.current) return;
+      pollingRef.current = true;
+      pollJobRequest(jobUrl)
+        .then((response) => {
+          if (response.status === JobStatus.completed) finish(true);
+          else if (response.status === JobStatus.errored) finish(false);
+        })
+        .catch(() => finish(false))
+        .finally(() => {
+          pollingRef.current = false;
+        });
+    }, JOB_POLL_INTERVAL_MS);
+
+    return () => clearInterval(interval);
+  }, [jobUrl, n]);
+
   return (
     <Prompt
       disabled={submitting}
       onClickPrimary={confirm}
       onClose={onClose}
       open={open}
       primaryLabel={t(translations.duplicateConfirm)}
-      title={t(translations.duplicateTitle)}
+      title={t(translations.duplicateTitle, { n })}
     >
-      <PromptText>
-        {t(translations.duplicateBody, { n: listings.length })}
-      </PromptText>
+      <PromptText>{t(translations.duplicateBody, { n })}</PromptText>
     </Prompt>
   );
 };

client/app/bundles/course/marketplace/components/__test__/DuplicationConfirmation.test.tsx

@@ -8,7 +8,7 @@ import DuplicateConfirmation from '../DuplicateConfirmation';
 const mock = createMockAdapter(CourseAPI.marketplace.client);
 beforeEach(() => mock.reset());
 
-const listings = [{ id: 1, title: 'Recursion Drills' }] as never;
+const listings = [{ id: 1, title: 'Recursion Drills' }];
 const url = `/courses/${global.courseId}/marketplace/listings/duplicate`;
 
 it('posts a duplication request with the destination tab on confirm', async () => {
@@ -45,3 +45,21 @@ it('omits destination_tab_id when entered without a tab (sidebar entry)', async
   expect(body).toMatchObject({ listing_ids: [1] });
   expect(body).not.toHaveProperty('destination_tab_id'); // backend then defaults to the first tab
 });
+
+// A request that never reaches the queue leaves no job to poll, so nothing else can re-enable the
+// prompt: the confirm button has to come back by itself for the user to be able to retry.
+it('re-enables the prompt when the request itself fails', async () => {
+  mock.onPost(url).reply(500);
+  const page = render(
+    <DuplicateConfirmation
+      destinationTabId={42}
+      listings={listings}
+      onClose={jest.fn()}
+      open
+    />,
+  );
+  const button = await page.findByRole('button', { name: /Duplicate/ });
+  fireEvent.click(button);
+  await waitFor(() => expect(mock.history.post).toHaveLength(1));
+  await waitFor(() => expect(button).not.toBeDisabled());
+});

client/app/bundles/course/marketplace/operations.ts

@@ -1,5 +1,4 @@
 import CourseAPI from 'api/course';
-import pollJob from 'lib/helpers/jobHelpers';
 
 import { MarketplaceListing } from './types';
 
@@ -8,20 +7,16 @@ export const fetchListings = async (): Promise<MarketplaceListing[]> => {
   return response.data.listings as MarketplaceListing[];
 };
 
+// Returns the URL of the duplication job to poll. Polling is deliberately left to the caller: it
+// has to be started and torn down by the component that owns the flow, so that navigating away
+// cannot leave an orphaned poller behind.
 export const duplicateListings = async (
   listingIds: number[],
   destinationTabId: number | null,
-  onSuccess: (redirectUrl?: string) => void,
-  onFailure: () => void,
-): Promise<void> => {
+): Promise<string> => {
   const response = await CourseAPI.marketplace.duplicate(
     listingIds,
     destinationTabId,
   );
-  pollJob(
-    response.data.jobUrl,
-    (data) => onSuccess(data.redirectUrl),
-    onFailure,
-    2000,
-  );
+  return response.data.jobUrl;
 };

client/app/bundles/course/marketplace/translations.ts

@@ -92,7 +92,7 @@ export default defineMessages({
   duplicateBody: {
     id: 'course.marketplace.duplicateBody',
     defaultMessage:
-      '{n, plural, one {This assessment will be copied to your course.} other {These assessments will be copied to your course.}}',
+      '{n, plural, one {This assessment will be copied to your course.} other {These # assessments will be copied to your course.}}',
   },
   duplicateConfirm: {
     id: 'course.marketplace.duplicateConfirm',