From e07f64960667180d3e5eeee317dfdbbfa3292dfa Mon Sep 17 00:00:00 2001 From: lws49 Date: Tue, 21 Jul 2026 02:05:48 +0800 Subject: [PATCH 1/5] fix(spec): make factory sequences unique per process Specs commit (use_transactional_fixtures is false), so a bare per-second timestamp collides whenever two rspec processes start within the same second, tripping unique constraints. Append a random suffix per process. --- spec/factories/instances.rb | 7 ++++--- spec/factories/user_emails.rb | 7 +++++-- spec/support/userstamp.rb | 23 ++++++++++++++++++++--- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/spec/factories/instances.rb b/spec/factories/instances.rb index c28441455dd..e4f4ce155ae 100644 --- a/spec/factories/instances.rb +++ b/spec/factories/instances.rb @@ -1,12 +1,13 @@ # frozen_string_literal: true FactoryBot.define do - base_time = Time.zone.now.to_i + # Unique per process — see the note in user_emails.rb; host and name are both unique-constrained. + run_id = "#{Time.zone.now.to_i}-#{SecureRandom.hex(3)}" sequence :host do |n| - "local-#{base_time}-#{n}.lvh.me" + "local-#{run_id}-#{n}.lvh.me" end factory :instance do - sequence(:name) { |n| "Instance-#{base_time}-#{n}" } + sequence(:name) { |n| "Instance-#{run_id}-#{n}" } host trait :with_learning_map_component_enabled do diff --git a/spec/factories/user_emails.rb b/spec/factories/user_emails.rb index a29f8d66e57..6801d742fca 100644 --- a/spec/factories/user_emails.rb +++ b/spec/factories/user_emails.rb @@ -1,8 +1,11 @@ # frozen_string_literal: true FactoryBot.define do - base_time = Time.zone.now.to_i + # Unique per process. Specs commit (use_transactional_fixtures is false), so a bare timestamp + # collides whenever two rspec processes start within the same second, and the second process then + # fails User::Email's uniqueness validation. The timestamp is kept for tracing leaked rows. + run_id = "#{Time.zone.now.to_i}-#{SecureRandom.hex(3)}" sequence :email do |n| - "user_#{n}@domain-#{base_time}-name.com" + "user_#{n}@domain-#{run_id}-name.com" end factory :user_email, class: User::Email.name do diff --git a/spec/support/userstamp.rb b/spec/support/userstamp.rb index 114d9431c29..03ae1c9c54d 100644 --- a/spec/support/userstamp.rb +++ b/spec/support/userstamp.rb @@ -1,5 +1,22 @@ # frozen_string_literal: true -ActsAsTenant.with_tenant(Instance.default) do - # Create a global stamper for this spec run - User.stamper = User.human_users.first +RSpec.configure do |config| + # Create a global stamper for this spec run. + # + # The stamper becomes the creator (and therefore the auto-built owner course_user) of courses + # created in specs, and mail-sending specs deliver to that owner — so the stamper MUST own a + # valid email. This suite commits without cleanup (use_transactional_fixtures is false, no + # DatabaseCleaner), so if any spec removes the seeded admin's email it stays removed; the next + # process's db:seed then recreates the admin as a *new* user, leaving the lowest-id human + # (User.human_users.first) permanently without an email. + # + # Resolve the stamper by the seeded admin email (matching db/seeds and seed.rake) so it always + # owns one, and do it in before(:suite) — this runs AFTER rails_helper's top-level db:seed, so + # the admin email is guaranteed present even after such a recreation. (Setting it at file-load + # time ran before db:seed and re-froze the stale, emailless user.) Fall back to the lowest-id + # human only if that email is somehow absent. + config.before(:suite) do + ActsAsTenant.with_tenant(Instance.default) do + User.stamper = User::Email.find_by_email('test@example.org')&.user || User.human_users.first + end + end end From ef0dc1bb0ad2af5a81c4ad1f682011686aa19cc4 Mon Sep 17 00:00:00 2001 From: lws49 Date: Wed, 29 Jul 2026 17:30:47 +0800 Subject: [PATCH 2/5] feat(marketplace): browse, preview and duplicate the served snapshot Browse, listing/question preview and duplication all read the current version's snapshot rather than the live source assessment, and a manager can cut a new version from the assessment page. --- .../marketplace/listings_controller.rb | 22 +- .../marketplace/questions_controller.rb | 8 +- .../marketplace_listings_controller.rb | 37 +- .../assessment/marketplace/duplication_job.rb | 105 +++++- app/models/course/assessment.rb | 6 + .../course/assessment/marketplace/listing.rb | 8 +- .../marketplace/listings/index.json.jbuilder | 2 +- .../marketplace/listings/show.json.jbuilder | 5 +- client/app/api/course/Marketplace.ts | 8 + .../AssessmentShow/AssessmentShowHeader.tsx | 17 +- .../__test__/AssessmentShowHeader.test.tsx | 64 +++- .../components/DuplicateConfirmation.tsx | 11 +- .../components/PublishToMarketplaceButton.tsx | 36 ++ .../__test__/DuplicationConfirmation.test.tsx | 47 ++- .../PublishToMarketplaceButton.test.tsx | 42 +++ .../ListingPreview/__test__/index.test.tsx | 44 ++- .../course/marketplace/translations.ts | 28 +- client/locales/en.json | 6 +- client/locales/ko.json | 2 +- client/locales/zh.json | 2 +- config/routes.rb | 4 +- .../marketplace/listings_controller_spec.rb | 54 ++- .../marketplace/questions_controller_spec.rb | 73 ++-- .../marketplace_listings_controller_spec.rb | 127 ++++++- .../marketplace/duplication_job_spec.rb | 333 +++++++++++++++++- 25 files changed, 943 insertions(+), 148 deletions(-) diff --git a/app/controllers/course/assessment/marketplace/listings_controller.rb b/app/controllers/course/assessment/marketplace/listings_controller.rb index 3ef8152e888..85bea0e9102 100644 --- a/app/controllers/course/assessment/marketplace/listings_controller.rb +++ b/app/controllers/course/assessment/marketplace/listings_controller.rb @@ -4,13 +4,15 @@ class Course::Assessment::Marketplace::ListingsController < Course::Assessment:: def index ActsAsTenant.without_tenant do - # Preload `lesson_plan_item` — `title` is not a column on Course::Assessment; it lives on - # the acting-as record. + # Preload `lesson_plan_item` — `title` is not a column on Course::Assessment; it lives on the + # acting-as record. Reads go through the current version snapshot, never the authoring copy: the + # marketplace serves what a duplicate would give you. `where.not(current_version_id: + # nil)` guards a published listing with no snapshot, whose nil `current_version` would 500 browse. @listings = Course::Assessment::Marketplace::Listing.published. - where.not(authoring_assessment_id: nil). - includes(authoring_assessment: :lesson_plan_item).to_a + where.not(current_version_id: nil). + includes(current_version: { assessment: :lesson_plan_item }).to_a @adoption_counts = adoption_counts(@listings.map(&:id)) - @question_counts = question_counts(@listings.map(&:authoring_assessment_id)) + @question_counts = question_counts(@listings.map { |listing| listing.current_version.assessment_id }) @destination_tabs = destination_tabs end end @@ -29,11 +31,11 @@ def duplicate def show ActsAsTenant.without_tenant do @listing = Course::Assessment::Marketplace::Listing.published. - includes(:authoring_assessment).find_by(id: params[:id]) + includes(current_version: :assessment).find_by(id: params[:id]) raise CanCan::AccessDenied unless @listing - @assessment = @listing.authoring_assessment - # This page renders the authoring copy, which an orphaned listing no longer has — see `index`. + # The SNAPSHOT, never the authoring copy (design §4.2). + @assessment = @listing.current_version&.assessment raise CanCan::AccessDenied unless @assessment authorize!(:preview_in_marketplace, @listing) @@ -73,10 +75,8 @@ def destination_tabs def authorized_listings listings = ActsAsTenant.without_tenant do - # Orphaned listings excluded for the reason `index` gives — the duplicate copies the - # authoring assessment, so there is nothing for it to read. Course::Assessment::Marketplace::Listing.published.where(id: duplicate_params[:listing_ids]). - where.not(authoring_assessment_id: nil).includes(:authoring_assessment) + includes(current_version: :assessment) end raise CanCan::AccessDenied if listings.empty? diff --git a/app/controllers/course/assessment/marketplace/questions_controller.rb b/app/controllers/course/assessment/marketplace/questions_controller.rb index ccbba11729b..22a6b21ffe3 100644 --- a/app/controllers/course/assessment/marketplace/questions_controller.rb +++ b/app/controllers/course/assessment/marketplace/questions_controller.rb @@ -4,12 +4,12 @@ class Course::Assessment::Marketplace::QuestionsController < Course::Assessment: def show ActsAsTenant.without_tenant do - listing = Course::Assessment::Marketplace::Listing.published.includes(:authoring_assessment). - find_by(id: params[:listing_id]) + listing = Course::Assessment::Marketplace::Listing.published. + includes(current_version: :assessment).find_by(id: params[:listing_id]) raise CanCan::AccessDenied unless listing - @assessment = listing.authoring_assessment - # An orphaned listing has nothing left to preview — see ListingsController#index. + # The SNAPSHOT, never the authoring copy. + @assessment = listing.current_version&.assessment raise CanCan::AccessDenied unless @assessment authorize!(:preview_in_marketplace, listing) diff --git a/app/controllers/course/assessment/marketplace_listings_controller.rb b/app/controllers/course/assessment/marketplace_listings_controller.rb index de9462916f7..e7e9c7c0cd3 100644 --- a/app/controllers/course/assessment/marketplace_listings_controller.rb +++ b/app/controllers/course/assessment/marketplace_listings_controller.rb @@ -2,20 +2,31 @@ class Course::Assessment::MarketplaceListingsController < Course::Assessment::Controller before_action :authorize_publish_to_marketplace! + # A published version of an existing listing is not a source assessment. Refused server-side and + # not only by withholding the button: the listing this would create has its source assessment + # frozen inside the container, so it could never be edited nor cut a further version. + SNAPSHOT_REJECTION = 'This is a published version of an existing listing, not a source assessment.' + def create - listing = Course::Assessment::Marketplace::Listing.find_or_initialize_by(authoring_assessment: @assessment) - now = Time.zone.now - listing.published = true - listing.first_published_at ||= now - listing.last_published_at = now - # `publisher` is an audit userstamp for the *latest* publish (design D29), so it moves with - # `last_published_at`. `creator` already retains whoever first created the row. - listing.publisher = current_user - if listing.save - render json: { published: true }, status: :ok - else - render json: { errors: listing.errors.full_messages }, status: :unprocessable_content - end + return render json: { errors: [SNAPSHOT_REJECTION] }, status: :unprocessable_content if + @assessment.marketplace_snapshot? + + listing = Course::Assessment::Marketplace::PublishService.publish(@assessment, current_user) + render json: { published: listing.published }, status: :ok + rescue ActiveRecord::RecordInvalid => e + render json: { errors: e.record.errors.full_messages }, status: :unprocessable_content + end + + # Cuts a new version from the authoring copy. Deliberately separate from `create`: re-listing an unlisted + # assessment reactivates the row but must NOT silently republish changed content. + def publish_version + listing = @assessment.marketplace_listing + return render json: { errors: ['Not listed on the marketplace.'] }, status: :unprocessable_content if listing.nil? + + version = Course::Assessment::Marketplace::PublishService.publish_new_version(listing, current_user) + render json: { published_at: version.published_at }, status: :ok + rescue ArgumentError => e + render json: { errors: [e.message] }, status: :unprocessable_content end def destroy diff --git a/app/jobs/course/assessment/marketplace/duplication_job.rb b/app/jobs/course/assessment/marketplace/duplication_job.rb index 14bfdbc0ac2..997863c563a 100644 --- a/app/jobs/course/assessment/marketplace/duplication_job.rb +++ b/app/jobs/course/assessment/marketplace/duplication_job.rb @@ -5,6 +5,10 @@ class Course::Assessment::Marketplace::DuplicationJob < ApplicationJob queue_as :duplication + # Mirrors `validates :title, length: { maximum: 255 }` on Course::LessonPlan::Item, which is where + # an assessment's title actually lives. + TITLE_LIMIT = 255 + protected def perform_tracked(listing_ids, destination_course, destination_tab_id, options = {}) @@ -12,15 +16,15 @@ def perform_tracked(listing_ids, destination_course, destination_tab_id, options 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`. - last_copy = duplicate_listing(listing, destination_course, current_user) - reparent_into_tab(last_copy, target_tab) + copies = listings.map do |listing| + copy = duplicate_listing(listing, destination_course, current_user) + reparent_into_tab(copy, target_tab) + resolve_title_collision(copy, listing, destination_course) + record_adoption(listing, destination_course, copy, current_user) + copy end - redirect_to assessments_url(destination_course, target_tab || last_copy&.tab) + landing_url = landing_url_for(copies, destination_course) + redirect_to landing_url if landing_url end end @@ -36,7 +40,7 @@ def find_tab(destination_course, destination_tab_id) end def duplicate_listing(listing, destination_course, current_user) - source = listing.authoring_assessment + source = listing.current_version.assessment Course::Duplication::ObjectDuplicationService.duplicate_objects( source.course, destination_course, source, current_user: current_user ) @@ -50,15 +54,78 @@ def reparent_into_tab(copy, target_tab) 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) + # Renames an imported copy whose title is already taken in the destination course. + # + # Fires on every import, not only on re-import of the same listing: a copy landing on top of an + # unrelated assessment of the same name collides just as badly, and previously landed silently. + # + # Escalates only as far as it has to: + # "Lab 3" -> "Lab 3 [12 Jun 2026]" -> "Lab 3 [12 Jun 2026] (2)" -> (3) ... + # + # @param [Course::Assessment] copy + # @param [Course::Assessment::Marketplace::Listing] listing + # @param [Course] destination_course + # @return [void] + def resolve_title_collision(copy, listing, destination_course) + taken = Course::Assessment.titles_in_course(destination_course, except_id: copy.id) + base = copy.title + return if taken.exclude?(base.downcase) + + published_at = ActsAsTenant.without_tenant { listing.current_version&.published_at } + # A listing with no recorded vintage has nothing to name, so it goes straight to the counter — + # stamping an empty "[]" would be worse than the collision it is trying to resolve. + dated = published_at ? "#{base} [#{published_at.strftime('%d %b %Y')}]" : base + candidate = truncate_to_limit(dated, base) + + suffix_number = 2 + while taken.include?(candidate.downcase) + candidate = truncate_to_limit("#{dated} (#{suffix_number})", base) + suffix_number += 1 + end + + copy.title = candidate + copy.save! + end + + # Truncate the base for an over-long title. + # + # @param [String] candidate + # @param [String] base + # @return [String] + def truncate_to_limit(candidate, base) + return candidate if candidate.length <= TITLE_LIMIT + + suffix = candidate.delete_prefix(base) + base.truncate(TITLE_LIMIT - suffix.length) + suffix + end + + # Where the completion toast's link sends the manager. + # + # @param [Array] copies + # @param [Course] destination_course + # @return [String, nil] nil when every listing was filtered out by `.published`, in which case + # nothing landed and there is nowhere to link to. + def landing_url_for(copies, destination_course) + return nil if copies.empty? + + host = destination_course.instance.host + return course_assessment_url(destination_course, copies.first, host: host) if copies.one? + + tab = copies.first.tab + course_assessments_url(destination_course, category: tab.category_id, tab: tab.id, host: host) + end + + # Written here rather than left to `Course::Duplication::BaseService#record_marketplace_adoptions`: + # that sweep keys off the SOURCE's own `marketplace_listing`, and the source here is the container + # snapshot, which authors no listing. This path is the only one that knows which listing it served. + def record_adoption(listing, destination_course, copy, current_user) + Course::Assessment::Marketplace::Adoption.create!( + listing: listing, + destination_course: destination_course, + duplicated_assessment: copy, + adopted_version_at: listing.current_version.published_at, + creator: current_user, + updater: current_user + ) end end diff --git a/app/models/course/assessment.rb b/app/models/course/assessment.rb index d5d66f8892c..4d3d1c4233f 100644 --- a/app/models/course/assessment.rb +++ b/app/models/course/assessment.rb @@ -307,11 +307,17 @@ def csv_downloadable? # @param [User] current_user The user who triggered the duplication. def record_marketplace_adoption(duplicate, destination_course, current_user) return unless marketplace_listing&.published? + # Publishing duplicates the source INTO the container to cut a snapshot. That is the listing + # growing a version, not a course adopting it, so the container is never an adopter. + return if destination_course.preview? Course::Assessment::Marketplace::Adoption.create!( listing: marketplace_listing, destination_course: destination_course, duplicated_assessment: duplicate, + # Stamped here rather than at the call site: this is the single writer of adoption rows, and the + # adopter's "your copy is behind" banner has nothing to compare against without it. + adopted_version_at: marketplace_listing.current_version&.published_at, creator: current_user, updater: current_user ) diff --git a/app/models/course/assessment/marketplace/listing.rb b/app/models/course/assessment/marketplace/listing.rb index 85d181a1f26..21faa61fa97 100644 --- a/app/models/course/assessment/marketplace/listing.rb +++ b/app/models/course/assessment/marketplace/listing.rb @@ -1,8 +1,7 @@ # frozen_string_literal: true class Course::Assessment::Marketplace::Listing < ApplicationRecord # The mutable authoring copy — the origin-course assessment. Nullable: the listing outlives - # deletion of its origin. Browse, preview and duplicate all still read this; the snapshots in - # `versions` are recorded here but not yet served. + # deletion of its origin. What the marketplace serves is `current_version.assessment`. belongs_to :authoring_assessment, class_name: 'Course::Assessment', inverse_of: :marketplace_listing, optional: true belongs_to :publisher, class_name: 'User', inverse_of: false @@ -43,9 +42,8 @@ def self.for_admin_index end end - # An orphaned listing lost its authoring copy (the origin assessment was deleted). Its snapshots - # survive, but every course-facing path reads the authoring copy, so the listing leaves the - # marketplace until the rebuild lands. Deliberately separate from `admin_state`, a display concern. + # An orphaned listing lost its authoring copy (the origin assessment was deleted) but still + # serves its last snapshot. Deliberately separate from `admin_state`, which is a display concern. # @return [Boolean] def orphaned? authoring_assessment_id.nil? diff --git a/app/views/course/assessment/marketplace/listings/index.json.jbuilder b/app/views/course/assessment/marketplace/listings/index.json.jbuilder index 5b67c876813..39c64621555 100644 --- a/app/views/course/assessment/marketplace/listings/index.json.jbuilder +++ b/app/views/course/assessment/marketplace/listings/index.json.jbuilder @@ -1,7 +1,7 @@ # frozen_string_literal: true json.canAccess true json.listings @listings do |listing| - assessment = listing.authoring_assessment + assessment = listing.current_version.assessment json.id listing.id json.assessmentId assessment.id json.title assessment.title diff --git a/app/views/course/assessment/marketplace/listings/show.json.jbuilder b/app/views/course/assessment/marketplace/listings/show.json.jbuilder index 92d6b4f7305..06cc7bddcca 100644 --- a/app/views/course/assessment/marketplace/listings/show.json.jbuilder +++ b/app/views/course/assessment/marketplace/listings/show.json.jbuilder @@ -1,5 +1,8 @@ # frozen_string_literal: true -json.id @assessment.id +# The LISTING's id, matching `index.json.jbuilder` — everything below is the snapshot assessment's +# content, but the resource this payload identifies is the listing. The duplicate dialog posts this +# id back as a `listing_ids` entry, so the snapshot assessment's id here 403s the duplicate. +json.id @listing.id json.title @assessment.title json.description format_ckeditor_rich_text(@assessment.description) diff --git a/client/app/api/course/Marketplace.ts b/client/app/api/course/Marketplace.ts index 344178c65d5..41f402c8d0f 100644 --- a/client/app/api/course/Marketplace.ts +++ b/client/app/api/course/Marketplace.ts @@ -22,6 +22,14 @@ export default class MarketplaceAPI extends BaseCourseAPI { ); } + publishNewVersion( + assessmentId: number, + ): Promise> { + return this.client.post( + `/courses/${this.courseId}/assessments/${assessmentId}/marketplace_listing/versions`, + ); + } + index(): Promise< AxiosResponse<{ listings: MarketplaceListing[]; diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowHeader.tsx b/client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowHeader.tsx index 0837787ab82..c81dcb970dc 100644 --- a/client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowHeader.tsx +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowHeader.tsx @@ -18,6 +18,7 @@ import marketplaceTranslations from 'course/marketplace/translations'; import DeleteButton from 'lib/components/core/buttons/DeleteButton'; import { PromptText } from 'lib/components/core/dialogs/Prompt'; import Link from 'lib/components/core/Link'; +import { SUPPORT_EMAIL } from 'lib/constants/sharedConstants'; import toast from 'lib/hooks/toast'; import useTranslation from 'lib/hooks/useTranslation'; @@ -68,7 +69,7 @@ const AssessmentShowHeader = ( }; return ( - <> +
{assessment.deleteUrl && ( {t(translations.deletingThisAssessment)} {assessment.title} - {t(translations.deleteAssessmentWarning)} {publishedToMarketplace && ( - {t(marketplaceTranslations.deleteWarning)} + + {t(marketplaceTranslations.deleteWarning, { + mailto: (chunk: string): JSX.Element => ( + + {chunk} + + ), + })} + )} + {t(translations.deleteAssessmentWarning)} )} @@ -178,7 +187,7 @@ const AssessmentShowHeader = ( )} - +
); }; diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowHeader.test.tsx b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowHeader.test.tsx index 8b8f3c42174..e5ead14331b 100644 --- a/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowHeader.test.tsx +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowHeader.test.tsx @@ -2,6 +2,7 @@ import { createMockAdapter } from 'mocks/axiosMock'; import { fireEvent, render, waitFor, within } from 'test-utils'; import CourseAPI from 'api/course'; +import { SUPPORT_EMAIL } from 'lib/constants/sharedConstants'; import AssessmentShowHeader from '../AssessmentShowHeader'; @@ -29,10 +30,11 @@ const baseAssessment = { // Test the conditional in the delete Prompt whose // message contains this phrase, rendered only when `isPublishedToMarketplace`. -const MARKETPLACE_WARNING = /removes it from the marketplace/i; +const MARKETPLACE_WARNING = /keeps serving its last published version/i; +const DELETE_ASSESSMENT_LABEL = 'Delete Assessment'; describe('', () => { - it('warns that deletion removes the marketplace listing when the assessment is listed', async () => { + it('explains that the marketplace listing survives deletion when the assessment is listed', async () => { const page = render( ', () => { ); // First query awaits the i18n LoadingIndicator; subsequent getBy* are sync. - fireEvent.click(await page.findByLabelText('Delete Assessment')); // opens the delete Prompt + fireEvent.click(await page.findByLabelText(DELETE_ASSESSMENT_LABEL)); // opens the delete Prompt expect(page.getByText(MARKETPLACE_WARNING)).toBeVisible(); }); + it('names the assessment right after the intro line, before the marketplace explanation', async () => { + const page = render( + , + ); + + fireEvent.click(await page.findByLabelText(DELETE_ASSESSMENT_LABEL)); + + const content = page.getByRole('dialog').textContent ?? ''; + const positions = [ + 'You are about to delete the following assessment:', + baseAssessment.title, + 'keeps serving its last published version', + 'This action cannot be undone!', + ].map((phrase) => content.indexOf(phrase)); + + // -1 would make the ascending check vacuously true, so require every phrase. + expect(positions).not.toContain(-1); + expect(positions).toEqual([...positions].sort((a, b) => a - b)); + }); + + it('links to support so the listing can be unlisted', async () => { + const page = render( + , + ); + + fireEvent.click(await page.findByLabelText(DELETE_ASSESSMENT_LABEL)); + + expect(page.getByRole('link', { name: /contact us/i })).toHaveAttribute( + 'href', + `mailto:${SUPPORT_EMAIL}`, + ); + }); + + // Internal vocabulary must not leak into the instructor-facing warning. + it('calls the lost object the source assessment', async () => { + const page = render( + , + ); + + fireEvent.click(await page.findByLabelText(DELETE_ASSESSMENT_LABEL)); + + const dialog = await page.findByRole('dialog'); + expect(within(dialog).getByText(/source assessment/)).toBeVisible(); + expect( + within(dialog).queryByText(/authoring\s+copy/), + ).not.toBeInTheDocument(); + }); + it('shows no marketplace warning when the assessment is not listed', async () => { const page = render( ', () => { />, ); - fireEvent.click(await page.findByLabelText('Delete Assessment')); // delete Prompt still opens + fireEvent.click(await page.findByLabelText(DELETE_ASSESSMENT_LABEL)); // delete Prompt still opens expect(page.queryByText(MARKETPLACE_WARNING)).not.toBeInTheDocument(); }); diff --git a/client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx b/client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx index 5b6518dfdce..d04a0f1a2cd 100644 --- a/client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx +++ b/client/app/bundles/course/marketplace/components/DuplicateConfirmation.tsx @@ -103,12 +103,11 @@ const DuplicateConfirmation = ({ <> {t(translations.duplicateCompleted, { n })} {redirectUrl && ( - <> - {' '} - - {t(translations.viewDuplicatedAssessment)} - - + + {t(translations.viewDuplicatedAssessment, { + n: listings.length, + })} + )} , ); diff --git a/client/app/bundles/course/marketplace/components/PublishToMarketplaceButton.tsx b/client/app/bundles/course/marketplace/components/PublishToMarketplaceButton.tsx index 56aaaf19fe5..dec05373d7b 100644 --- a/client/app/bundles/course/marketplace/components/PublishToMarketplaceButton.tsx +++ b/client/app/bundles/course/marketplace/components/PublishToMarketplaceButton.tsx @@ -23,6 +23,7 @@ const PublishToMarketplaceButton = ({ }: Props): JSX.Element | null => { const { t } = useTranslation(); const [open, setOpen] = useState(false); + const [versionOpen, setVersionOpen] = useState(false); const [submitting, setSubmitting] = useState(false); const listed = assessment.isPublishedToMarketplace; @@ -53,8 +54,31 @@ const PublishToMarketplaceButton = ({ } }; + const confirmNewVersion = async (): Promise => { + setSubmitting(true); + try { + await CourseAPI.marketplace.publishNewVersion(assessment.id); + toast.success(t(translations.newVersionPublished)); + setVersionOpen(false); + } catch { + toast.error(t(translations.newVersionFailed)); + } finally { + setSubmitting(false); + } + }; + return ( <> + {listed && ( + + )} + + ) : ( + + {t(translations.marketplaceUpdateBlocked)} + + )} + + + setConfirming(false)} + open={confirming} + primaryColor="primary" + primaryLabel={t(translations.marketplaceUpdateInPlace)} + title={t(translations.marketplaceUpdateConfirmTitle)} + > + + {t(translations.marketplaceUpdateConfirmBody, { + latest: latestDate, + })} + + + {update.testSubmissionCount > 0 && ( + + {t(translations.marketplaceUpdateConfirmDeletion, { + count: update.testSubmissionCount, + })} + + )} + + + ); +}; + +export default MarketplaceUpdateBanner; diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowPage.test.tsx b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowPage.test.tsx new file mode 100644 index 00000000000..31937d7c3bd --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowPage.test.tsx @@ -0,0 +1,83 @@ +import { render, RenderResult } from 'test-utils'; +import { AssessmentData } from 'types/course/assessment/assessments'; + +import AssessmentShowPage from '../AssessmentShowPage'; + +// Minimal AssessmentData: enough for the page to mount. Everything optional is left out so the +// assertions below can only be about the marketplace chip. +const baseAssessment = { + id: 1, + title: 'Sample Assessment', + tabTitle: 'Assessments: Default', + tabUrl: '/courses/1/assessments', + description: '', + autograded: false, + startAt: { isFixed: false, effectiveTime: null, referenceTime: null }, + hasAttempts: false, + status: 'open', + actionButtonUrl: null, + permissions: { + canAttempt: true, + canManage: true, + canObserve: false, + canInviteToKoditsu: false, + canPublishToMarketplace: false, + }, + isPublishedToMarketplace: false, + marketplaceListingUrl: '/courses/1/assessments/1/marketplace_listing', + marketplaceUpdate: null, + requirements: [], + indexUrl: '/courses/1/assessments', + isStudent: false, +} as unknown as AssessmentData; + +const renderWith = ( + marketplaceVersion?: AssessmentData['marketplaceVersion'], +): RenderResult => + render( + , + ); + +// '2026-07-24T07:04:00Z' rendered in Asia/Singapore (UTC+8), as in MarketplaceVersionChip's own test. +const PUBLISHED_AT_LABEL = '24 Jul 2026, 3:04pm'; + +describe('', () => { + // Every snapshot in the container carries the origin's title verbatim and shares one tab, so the + // page has to say which one this is — otherwise opening a container row loses the identity the + // index row showed. + it('dates a container snapshot and marks it live', async () => { + const page = renderWith({ + listingId: 7, + publishedAt: '2026-07-24T07:04:00Z', + source: 'MP Allowlist Source Course', + latest: true, + listed: true, + }); + + expect(await page.findByText(PUBLISHED_AT_LABEL)).toBeVisible(); + expect(page.getByText('Live')).toBeVisible(); + }); + + // The working copy is not a version at all — mistaking it for one would read as though the + // marketplace serves whatever an admin is midway through editing. + it("labels the listing's working copy as the source assessment", async () => { + const page = renderWith({ + listingId: 7, + publishedAt: null, + source: 'MP Allowlist Source Course', + latest: false, + listed: true, + }); + + expect(await page.findByText('Source Assessment')).toBeVisible(); + expect(page.queryByText('Live')).not.toBeInTheDocument(); + }); + + it('shows no marketplace chip outside the container', async () => { + const page = renderWith(undefined); + + expect(await page.findByText(baseAssessment.title)).toBeVisible(); + expect(page.queryByText(/2026/)).not.toBeInTheDocument(); + expect(page.queryByText('Source Assessment')).not.toBeInTheDocument(); + }); +}); diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/MarketplaceUpdateBanner.test.tsx b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/MarketplaceUpdateBanner.test.tsx new file mode 100644 index 00000000000..936545a3549 --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/MarketplaceUpdateBanner.test.tsx @@ -0,0 +1,321 @@ +import { createMockAdapter } from 'mocks/axiosMock'; +import { fireEvent, render, waitFor, within } from 'test-utils'; + +import GlobalAPI from 'api'; +import CourseAPI from 'api/course'; + +import MarketplaceUpdateBanner from '../MarketplaceUpdateBanner'; + +const mockUpdateToast = { + success: jest.fn(), + error: jest.fn(), +}; + +jest.mock('lib/hooks/toast', () => ({ + __esModule: true, + default: { success: jest.fn(), error: jest.fn() }, + loadingToast: jest.fn(() => mockUpdateToast), +})); + +const mock = createMockAdapter(CourseAPI.marketplace.client); +// 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(); +}); + +// Students have submitted work, so this copy can never be replaced in place. +const update = { + adoptedVersionAt: '2026-06-12T00:00:00Z', + latestVersionAt: '2026-07-24T00:00:00Z', + canUpdateInPlace: false, + testSubmissionCount: 0, +}; + +// No student has touched this copy, so the marketplace's newer content can replace it where it sits. +const updatableInPlace = { + ...update, + canUpdateInPlace: true, +}; + +// Two cuts on the same calendar day: the pair must escalate to include the time, or the banner +// would tell the manager their copy is from the same day it was superseded. +const sameDayUpdate = { + ...update, + adoptedVersionAt: '2026-07-24T01:00:00Z', + latestVersionAt: '2026-07-24T07:04:00Z', +}; + +const APPLY_URL = `/courses/${global.courseId}/assessments/5/marketplace_adoption/apply_latest_version`; +const JOB_URL = '/jobs/9'; +const REDIRECT_URL = `/courses/${global.courseId}/assessments/53`; +// What the apply endpoint answers with: the job is merely enqueued, and `jobUrl` is where its +// progress is reported. +const enqueued = { status: 'submitted', jobUrl: JOB_URL }; +const UPDATE = 'Update this assessment'; + +// Version numbers are not a user-facing concept: the manager who copied this assessment never saw +// "v1". The banner therefore dates both content vintages instead of numbering them. +// formatLongDate('2026-07-24T00:00:00Z') under TZ=Asia/Singapore → '24 Jul 2026'. +it('dates both content vintages without version numbers, sync or behind', async () => { + const page = render( + , + ); + + const alert = await page.findByRole('alert'); + expect(alert.textContent).toContain( + 'This assessment was updated in the marketplace on 24 Jul 2026. Your copy is from 12 Jun 2026.', + ); + expect(alert.textContent).not.toMatch(/\bv\d/); + expect(alert.textContent).not.toMatch(/sync/i); + expect(alert.textContent).not.toMatch(/behind/i); +}); + +it('escalates to the time when both vintages fall on one day', async () => { + const page = render( + , + ); + + const alert = await page.findByRole('alert'); + expect(alert.textContent).toContain( + 'This assessment was updated in the marketplace on 24 Jul 2026, 3:04pm. Your copy is from 24 Jul 2026, 9:00am.', + ); +}); + +// The notice is a statement of fact about the copy, not a notification, so nothing may silence it. +// MUI renders Alert's close × whenever `onClose` is passed, so counting the buttons is what keeps +// the banner un-closeable — the update is the only thing it may ever offer. +it('renders the update as its only button, with nothing to close it', async () => { + const page = render( + , + ); + + const alert = await page.findByRole('alert'); + expect(within(alert).getAllByRole('button')).toHaveLength(1); + expect( + within(alert).getByRole('button', { name: UPDATE }), + ).toBeInTheDocument(); +}); + +// Replacing the content would destroy the students' work, so there is nothing safe to offer. An +// action-less banner is only honest if it says why — otherwise the manager hunts for a button. +it('explains why it cannot update when students have submitted work', async () => { + const page = render( + , + ); + + const alert = await page.findByRole('alert'); + expect(within(alert).queryAllByRole('button')).toHaveLength(0); + expect(alert.textContent).toContain('can no longer be updated automatically'); + expect(alert.textContent).toContain('students have already submitted work'); + expect(alert.textContent).toMatch(/edits of your own/i); + expect(alert.textContent).toMatch(/import this assessment .* again/i); +}); + +it('offers to update in place when no student has submitted work', async () => { + const page = render( + , + ); + + expect(await page.findByRole('button', { name: UPDATE })).toBeInTheDocument(); + expect( + page.queryByText(/can no longer be updated automatically/), + ).not.toBeInTheDocument(); +}); + +it('names the test submissions the update will delete', async () => { + const page = render( + , + ); + + fireEvent.click(await page.findByText(UPDATE)); + + const dialog = await page.findByRole('dialog'); + expect(dialog.textContent).toContain('2 test submissions'); + expect(dialog.textContent).toContain('replaces'); +}); + +it('omits the deletion warning when there is nothing to delete', async () => { + const page = render( + , + ); + + fireEvent.click(await page.findByText(UPDATE)); + + const dialog = await page.findByRole('dialog'); + expect(dialog.textContent).not.toMatch(/test submission/i); +}); + +// The manager is about to overwrite their content, so the prompt has to name WHICH version it is +// about to bring in — the same vintage the banner is reporting. +it('names the incoming version in the confirmation', async () => { + const page = render( + , + ); + + fireEvent.click(await page.findByText(UPDATE)); + + const dialog = await page.findByRole('dialog'); + expect(dialog.textContent).toContain('published on 24 Jul 2026'); +}); + +it('posts the in-place update on confirm', async () => { + mock.onPost(APPLY_URL).reply(200, enqueued); + jobsMock.onGet(JOB_URL).reply(200, { status: 'errored' }); + + const page = render( + , + ); + + fireEvent.click(await page.findByText(UPDATE)); + const dialog = await page.findByRole('dialog'); + fireEvent.click( + within(dialog).getByRole('button', { name: new RegExp(UPDATE) }), + ); + + await waitFor(() => expect(mock.history.post).toHaveLength(1)); + expect(mock.history.post[0].url).toBe(APPLY_URL); + await waitFor(() => expect(mockUpdateToast.error).toHaveBeenCalled(), { + timeout: 6000, + }); +}); + +// `canUpdateInPlace` is advisory: the endpoint re-checks for student work and answers 422 if a +// student has submitted since the page loaded. The request never reaches pollJob, so nothing else +// can unlock the prompt or retract the loading toast. +it('reports a refused update and unlocks the prompt', async () => { + mock + .onPost(APPLY_URL) + .reply(422, { errors: ['Students have submitted work.'] }); + + const page = render( + , + ); + + fireEvent.click(await page.findByText(UPDATE)); + const dialog = await page.findByRole('dialog'); + const confirm = within(dialog).getByRole('button', { + name: new RegExp(UPDATE), + }); + fireEvent.click(confirm); + + await waitFor(() => + expect(mockUpdateToast.error).toHaveBeenCalledWith( + 'Could not update this assessment.', + ), + ); + expect(page.getByRole('dialog')).toBeInTheDocument(); + await waitFor(() => expect(confirm).toBeEnabled()); +}); + +// The one thing that retires the banner: the copy has stopped being behind. The page still holds +// the pre-update payload, so the banner is the only thing that can notice. +it('reports completion once the in-place update job finishes', async () => { + mock.onPost(APPLY_URL).reply(200, enqueued); + jobsMock + .onGet(JOB_URL) + .reply(200, { status: 'completed', redirectUrl: REDIRECT_URL }); + + const page = render( + , + ); + + fireEvent.click(await page.findByText(UPDATE)); + const dialog = await page.findByRole('dialog'); + fireEvent.click( + within(dialog).getByRole('button', { name: new RegExp(UPDATE) }), + ); + + await waitFor(() => expect(mockUpdateToast.success).toHaveBeenCalled(), { + timeout: 6000, + }); + await waitFor(() => + expect(page.queryByRole('alert')).not.toBeInTheDocument(), + ); +}, 10000); + +it('keeps the update locked while the job is still running', async () => { + mock.onPost(APPLY_URL).reply(200, enqueued); + jobsMock + .onGet(JOB_URL) + .replyOnce(200, { status: 'submitted' }) + .onGet(JOB_URL) + .reply(200, { status: 'errored' }); + + const page = render( + , + ); + + fireEvent.click(await page.findByText(UPDATE)); + const dialog = await page.findByRole('dialog'); + const confirm = within(dialog).getByRole('button', { + name: new RegExp(UPDATE), + }); + fireEvent.click(confirm); + + await waitFor(() => expect(mock.history.post).toHaveLength(1)); + + // The job has not reported back, so the dialog must stay open and un-resubmittable. + expect(confirm).toBeDisabled(); + expect(within(dialog).getByRole('button', { name: 'Cancel' })).toBeDisabled(); + + fireEvent.click(confirm); + expect(mock.history.post).toHaveLength(1); + + await waitFor(() => expect(mockUpdateToast.error).toHaveBeenCalled(), { + timeout: 6000, + }); +}, 10000); + +it('reports a failed job and unlocks the dialog for a retry', async () => { + mock.onPost(APPLY_URL).reply(200, enqueued); + jobsMock.onGet(JOB_URL).reply(200, { status: 'errored' }); + + const page = render( + , + ); + + fireEvent.click(await page.findByText(UPDATE)); + const dialog = await page.findByRole('dialog'); + const confirm = within(dialog).getByRole('button', { + name: new RegExp(UPDATE), + }); + fireEvent.click(confirm); + + await waitFor(() => expect(mockUpdateToast.error).toHaveBeenCalled(), { + timeout: 6000, + }); + + expect(mockUpdateToast.error).toHaveBeenCalledWith( + 'Could not update this assessment.', + ); + expect(page.getByRole('dialog')).toBeInTheDocument(); + // The banner is still there too: nothing was updated, so it is still telling the truth. Queried + // by text rather than by role — the open dialog `aria-hidden`s the rest of the body, so its + // `alert` role is unreachable while the retry prompt is up. + expect( + page.getByText(/This assessment was updated in the marketplace/), + ).toBeInTheDocument(); + await waitFor(() => expect(confirm).toBeEnabled()); +}, 10000); + +it('renders nothing when there is no update', async () => { + // The sentinel is what makes this assertion mean anything: `test-utils` mounts a translations + // Suspense, so the alert is absent on the first tick regardless. Awaiting a sibling proves the + // tree finished mounting; only then is the alert's absence evidence the component returned null. + const page = render( + <> + sentinel + + , + ); + + expect(await page.findByText('sentinel')).toBeInTheDocument(); + expect(page.queryByRole('alert')).not.toBeInTheDocument(); +}); diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/versionVintage.test.ts b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/versionVintage.test.ts new file mode 100644 index 00000000000..db97056d49a --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/versionVintage.test.ts @@ -0,0 +1,41 @@ +import { formatVintagePair } from '../versionVintage'; + +// Tests run under TZ=Asia/Singapore, so a UTC instant renders +8h. +describe('formatVintagePair', () => { + it('renders dates only when the two vintages fall on different days', () => { + expect( + formatVintagePair('2026-06-12T00:00:00Z', '2026-07-24T00:00:00Z'), + ).toEqual({ adopted: '12 Jun 2026', latest: '24 Jul 2026' }); + }); + + // A listing republished twice in one day would otherwise render "updated on 24 Jul 2026, your + // copy is from 24 Jul 2026" — self-contradicting, and the adopter cannot resolve it. + it('escalates BOTH vintages to include the time when they share a calendar day', () => { + expect( + formatVintagePair('2026-07-24T01:00:00Z', '2026-07-24T07:04:00Z'), + ).toEqual({ + adopted: '24 Jul 2026, 9:00am', + latest: '24 Jul 2026, 3:04pm', + }); + }); + + // Same calendar day is judged in the VIEWER's zone, which is what they read on screen. These two + // instants are different UTC days but the same Singapore day. + it('judges the shared day in the viewer timezone, not UTC', () => { + expect( + formatVintagePair('2026-07-23T17:00:00Z', '2026-07-24T02:00:00Z'), + ).toEqual({ + adopted: '24 Jul 2026, 1:00am', + latest: '24 Jul 2026, 10:00am', + }); + }); + + it('escalates when the two vintages are the identical instant', () => { + expect( + formatVintagePair('2026-07-24T07:04:00Z', '2026-07-24T07:04:00Z'), + ).toEqual({ + adopted: '24 Jul 2026, 3:04pm', + latest: '24 Jul 2026, 3:04pm', + }); + }); +}); diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/versionVintage.ts b/client/app/bundles/course/assessment/pages/AssessmentShow/versionVintage.ts new file mode 100644 index 00000000000..c24a25c2a6e --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/versionVintage.ts @@ -0,0 +1,24 @@ +import moment, { formatLongDate, formatLongDateTime } from 'lib/moment'; + +/** + * Formats an adopted vintage and the served vintage as a pair. + * + * A version is identified by when it was published, and adopters read that as a date — a time is + * false precision here. But a listing republished twice in one day would render "updated on 24 Jul + * 2026. Your copy is from 24 Jul 2026.", which the adopter cannot resolve. So precision escalates to + * include the time exactly when the two vintages would otherwise be indistinguishable. + * + * Both sides escalate together — one dated and one timestamped would read as a different kind of + * thing rather than as two points on one scale. + * + * The shared-day test is made in the viewer's timezone, because that is the rendering they compare. + */ +export const formatVintagePair = ( + adopted: string, + latest: string, +): { adopted: string; latest: string } => { + const sameDay = moment(adopted).isSame(moment(latest), 'day'); + const format = sameDay ? formatLongDateTime : formatLongDate; + + return { adopted: format(adopted), latest: format(latest) }; +}; diff --git a/client/app/bundles/course/assessment/pages/AssessmentsIndex/MarketplaceVersionChip.tsx b/client/app/bundles/course/assessment/pages/AssessmentsIndex/MarketplaceVersionChip.tsx new file mode 100644 index 00000000000..878aa37c1f3 --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentsIndex/MarketplaceVersionChip.tsx @@ -0,0 +1,102 @@ +import { FC } from 'react'; +import { Chip, Tooltip } from '@mui/material'; +import { MarketplaceVersionData } from 'types/course/assessment/assessments'; + +import useTranslation from 'lib/hooks/useTranslation'; +import { formatLongDateTime } from 'lib/moment'; + +import translations from '../../translations'; + +interface MarketplaceVersionChipProps { + for: MarketplaceVersionData; +} + +/** + * Tells apart the assessments in the marketplace container course, which all sit in one tab under + * their original titles: immutable published snapshots, chipped with their publication date, and the + * listing's editable working copy, chipped "Source Assessment". View-only — nothing here is ever + * retitled, because an adopter's duplicated copy reads that title. + */ +const MarketplaceVersionChip: FC = (props) => { + const { for: marketplaceVersion } = props; + const { t } = useTranslation(); + const publishedAt = marketplaceVersion.publishedAt; + + // A null vintage means the working copy, which is not a version at all — hence a different label + // and a different colour, so an admin never mistakes it for something the marketplace serves. + const isAuthoring = publishedAt === null; + + // Two different facts. `latest` is the newest cut; `listed` is whether the listing is on the + // marketplace. Only their conjunction means "this is what an adopter gets", and only that earns + // the strong label — so Live stands in for Latest rather than sitting beside it. + const isLive = marketplaceVersion.latest && marketplaceVersion.listed; + + const hint = ((): string => { + if (isAuthoring) { + return marketplaceVersion.source + ? t(translations.marketplaceAuthoringHintWithSource, { + listingId: marketplaceVersion.listingId, + source: marketplaceVersion.source, + }) + : t(translations.marketplaceAuthoringHint, { + listingId: marketplaceVersion.listingId, + }); + } + + return marketplaceVersion.source + ? t(translations.marketplaceVersionHintWithSource, { + listingId: marketplaceVersion.listingId, + source: marketplaceVersion.source, + }) + : t(translations.marketplaceVersionHint, { + listingId: marketplaceVersion.listingId, + }); + })(); + + // Date AND time: one container tab holds every snapshot of every listing, so same-day siblings + // sit next to each other and the time is the only thing separating them. + const label = isAuthoring + ? t(translations.marketplaceAuthoring) + : t(translations.marketplaceVersion, { + version: formatLongDateTime(publishedAt), + }); + + return ( +
+ + + + + {!isAuthoring && marketplaceVersion.latest && ( + + + + )} +
+ ); +}; + +export default MarketplaceVersionChip; diff --git a/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/MarketplaceVersionChip.test.tsx b/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/MarketplaceVersionChip.test.tsx new file mode 100644 index 00000000000..b0e9c2722cf --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/MarketplaceVersionChip.test.tsx @@ -0,0 +1,140 @@ +import userEvent from '@testing-library/user-event'; +import { render } from 'test-utils'; +import { MarketplaceVersionData } from 'types/course/assessment/assessments'; + +import MarketplaceVersionChip from '../MarketplaceVersionChip'; + +// '2026-07-24T07:04:00Z' rendered in Asia/Singapore (UTC+8). +const PUBLISHED_AT_LABEL = '24 Jul 2026, 3:04pm'; + +const snapshot = ( + overrides: Partial = {}, +): MarketplaceVersionData => ({ + listingId: 7, + publishedAt: '2026-07-24T07:04:00Z', + source: 'MP Allowlist Source Course', + latest: false, + listed: true, + ...overrides, +}); + +describe('', () => { + // One container tab holds every snapshot of every listing under identical titles, so siblings ARE + // side by side here — the time is what tells two same-day cuts apart. + it('labels a snapshot with its publish date and time', async () => { + const page = render(); + + expect(await page.findByText(PUBLISHED_AT_LABEL)).toBeInTheDocument(); + }); + + it('labels the working copy as the source assessment rather than a date', async () => { + const page = render( + , + ); + + expect(await page.findByText('Source Assessment')).toBeInTheDocument(); + expect(page.queryByText(/2026/)).not.toBeInTheDocument(); + }); + + it('marks the newest version of a published listing as Live, alongside its date', async () => { + const page = render( + , + ); + + expect(await page.findByText('Live')).toBeInTheDocument(); + // The date is not replaced by the status — an admin needs both. + expect(page.getByText(PUBLISHED_AT_LABEL)).toBeInTheDocument(); + // Live and Latest are mutually exclusive: Live is the stronger of the two and stands in for it. + expect(page.queryByText('Latest')).not.toBeInTheDocument(); + }); + + // An unlisted listing still HAS a newest version — it is what an admin re-publishing acts on — but + // nothing is being served, so it must not read Live. + it('marks the newest version of an unlisted listing as Latest, not Live', async () => { + const page = render( + , + ); + + expect(await page.findByText('Latest')).toBeInTheDocument(); + expect(page.queryByText('Live')).not.toBeInTheDocument(); + }); + + it('marks a superseded snapshot neither Live nor Latest', async () => { + const page = render( + , + ); + + expect(await page.findByText(PUBLISHED_AT_LABEL)).toBeInTheDocument(); + expect(page.queryByText('Live')).not.toBeInTheDocument(); + expect(page.queryByText('Latest')).not.toBeInTheDocument(); + }); + + // Unreachable today — the backend hardcodes `latest: false` for the working copy — but + // constructible here, and the two classifiers must not be able to disagree: the Version filter in + // AssessmentsTable already treats a null `publishedAt` as "Source Assessment" regardless of + // `latest`, so this chip must never render Live or Latest alongside it. + it('never marks the working copy Live or Latest, even if `latest` were true', async () => { + const page = render( + , + ); + + expect(await page.findByText('Source Assessment')).toBeInTheDocument(); + expect(page.queryByText('Live')).not.toBeInTheDocument(); + expect(page.queryByText('Latest')).not.toBeInTheDocument(); + }); + + it('identifies the listing by a stable id rather than an ordinal', async () => { + const user = userEvent.setup(); + const page = render( + , + ); + + await user.hover(await page.findByText(PUBLISHED_AT_LABEL)); + + const tooltip = await page.findByRole('tooltip'); + expect(tooltip).toHaveTextContent( + 'Listing ID 12 · from MP Allowlist Source Course', + ); + // "#12" reads as a position in a list, which is what made an admin expect it to renumber when a + // neighbouring listing was deleted. It is a primary key and never moves. + expect(tooltip).not.toHaveTextContent('#12'); + }); + + it('names the listing alone when the source course was never recorded', async () => { + const user = userEvent.setup(); + const page = render( + , + ); + + await user.hover(await page.findByText(PUBLISHED_AT_LABEL)); + + const tooltip = await page.findByRole('tooltip'); + expect(tooltip).toHaveTextContent('Listing ID 12'); + expect(tooltip).not.toHaveTextContent('from'); + }); + + it('says the working copy is not a published version', async () => { + const user = userEvent.setup(); + const page = render( + , + ); + + await user.hover(await page.findByText('Source Assessment')); + + expect(await page.findByRole('tooltip')).toHaveTextContent( + 'Listing ID 12 · editable working copy, not a published version', + ); + }); +}); diff --git a/client/app/bundles/course/assessment/translations.ts b/client/app/bundles/course/assessment/translations.ts index eddd80ac53b..c51cde451eb 100644 --- a/client/app/bundles/course/assessment/translations.ts +++ b/client/app/bundles/course/assessment/translations.ts @@ -1,6 +1,47 @@ import { defineMessages } from 'react-intl'; const translations = defineMessages({ + marketplaceUpdateAvailable: { + id: 'course.assessment.marketplaceUpdateAvailable', + defaultMessage: + 'This assessment was updated in the marketplace on {latest}. Your copy is from {adopted}.', + }, + marketplaceUpdateInPlace: { + id: 'course.assessment.marketplaceUpdateInPlace', + defaultMessage: 'Update this assessment', + }, + marketplaceUpdateBlocked: { + id: 'course.assessment.marketplaceUpdateBlocked', + defaultMessage: + 'This assessment can no longer be updated automatically: students have already submitted work for it, and it may carry edits of your own. Replacing its content would discard both. To use the new version, import this assessment from the marketplace again.', + }, + marketplaceUpdateConfirmTitle: { + id: 'course.assessment.marketplaceUpdateConfirmTitle', + defaultMessage: 'Update this assessment?', + }, + marketplaceUpdateConfirmBody: { + id: 'course.assessment.marketplaceUpdateConfirmBody', + defaultMessage: + "This replaces this assessment's questions and materials with the version published on {latest}. It keeps its place in your course, its deadlines, and whether it is published.", + }, + marketplaceUpdateConfirmDeletion: { + id: 'course.assessment.marketplaceUpdateConfirmDeletion', + defaultMessage: + '{count, plural, one {# test submission} other {# test submissions}} on this assessment will be deleted. No student has submitted work for it.', + }, + marketplaceUpdateStarted: { + id: 'course.assessment.marketplaceUpdateStarted', + defaultMessage: 'Updating this assessment…', + }, + marketplaceUpdateCompleted: { + id: 'course.assessment.marketplaceUpdateCompleted', + defaultMessage: + 'Assessment updated to the latest version. Refresh to see the latest version.', + }, + marketplaceUpdateFailed: { + id: 'course.assessment.marketplaceUpdateFailed', + defaultMessage: 'Could not update this assessment.', + }, updateAssessment: { id: 'course.assessment.edit.update', defaultMessage: 'Save', @@ -160,6 +201,54 @@ const translations = defineMessages({ id: 'course.assessments.index.seeAllRequirements', defaultMessage: 'See all requirements', }, + marketplaceVersion: { + id: 'course.assessments.index.marketplaceVersion', + defaultMessage: '{version}', + }, + // The label is renamed; the message id is not. "Source assessment" is already this codebase's + // user-facing name for the authoring copy (MarketplaceListingsTable's "Open source assessment", + // MarketplaceRestoreAuthoringButton's "Rebuild source assessment"). Renaming the id would orphan + // the key in all three locale files for a copy change; `authoring_assessment` is unaffected. + marketplaceAuthoring: { + id: 'course.assessments.index.marketplaceAuthoring', + defaultMessage: 'Source Assessment', + }, + marketplaceAuthoringHint: { + id: 'course.assessments.index.marketplaceAuthoringHint', + defaultMessage: + 'Listing ID {listingId} · editable working copy, not a published version', + }, + marketplaceAuthoringHintWithSource: { + id: 'course.assessments.index.marketplaceAuthoringHintWithSource', + defaultMessage: + 'Listing ID {listingId} · editable working copy, not a published version · from {source}', + }, + marketplaceVersionHint: { + id: 'course.assessments.index.marketplaceVersionHint', + defaultMessage: 'Listing ID {listingId}', + }, + marketplaceVersionHintWithSource: { + id: 'course.assessments.index.marketplaceVersionHintWithSource', + defaultMessage: 'Listing ID {listingId} · from {source}', + }, + marketplaceLive: { + id: 'course.assessments.index.marketplaceLive', + defaultMessage: 'Live', + }, + marketplaceLiveHint: { + id: 'course.assessments.index.marketplaceLiveHint', + defaultMessage: + 'The version the marketplace is serving right now. Adopting this listing copies this content.', + }, + marketplaceLatest: { + id: 'course.assessments.index.marketplaceLatest', + defaultMessage: 'Latest', + }, + marketplaceLatestHint: { + id: 'course.assessments.index.marketplaceLatestHint', + defaultMessage: + 'The newest version of this listing. Nothing is being served — the listing is off the marketplace.', + }, requirements: { id: 'course.assessment.show.requirements', defaultMessage: 'Requirements', diff --git a/client/app/types/course/assessment/assessments.ts b/client/app/types/course/assessment/assessments.ts index f23d634b075..0e901c93d61 100644 --- a/client/app/types/course/assessment/assessments.ts +++ b/client/app/types/course/assessment/assessments.ts @@ -26,6 +26,30 @@ export interface AchievementBadgeData { title: string; } +/** + * Which marketplace listing a container-course assessment belongs to. Present only for a system + * admin viewing the marketplace's container course — every assessment there keeps its original title + * verbatim, so this is the only thing telling them apart. + */ +export interface MarketplaceVersionData { + listingId: number; + /** Null for the listing's editable working copy, which is not a version at all. */ + publishedAt: string | null; + /** Denormalised at publish; survives deletion of the origin course, but may never have been set. */ + source: string | null; + /** + * Whether `Listing#current_version` points at this snapshot — the newest cut, not necessarily one + * anybody can adopt. Always false for the working copy, which is not a version. + */ + latest: boolean; + /** + * Whether the listing is on the marketplace (`Listing#published`), carried on every one of its + * rows including the working copy. Combined with `latest` this is what distinguishes a version + * being served from merely the newest one. True for a published orphan, which still serves. + */ + listed: boolean; +} + export interface AssessmentListData extends AssessmentActionsData { id: number; title: string; @@ -40,6 +64,7 @@ export interface AssessmentListData extends AssessmentActionsData { timeLimit?: number; isStartTimeBegin: boolean; isKoditsuAssessmentEnabled?: boolean; + marketplaceVersion?: MarketplaceVersionData; baseExp?: number; timeBonusExp?: number; @@ -69,6 +94,8 @@ export interface AssessmentsListData { tabTitle: string; tabUrl: string; canManageMonitor: boolean; + /** True only in the marketplace's snapshot container, viewed by a system admin. */ + isMarketplaceContainer: boolean; category: { id: number; title: string; @@ -92,6 +119,27 @@ interface GenerateQuestionBuilderData { url: string; } +export interface MarketplaceUpdateData { + /** + * When the content this copy was made from was published — its vintage, not the copy date. A + * version IS its publication datetime; there is no ordinal anywhere in this payload. + */ + adoptedVersionAt: string; + /** When the version the marketplace currently serves was published. */ + latestVersionAt: string; + /** + * Whether this copy may be replaced in place. False as soon as any non-phantom student of the + * course has a submission on it, in which case the banner offers no action at all. Advisory: the + * endpoint re-checks before destroying anything. + */ + canUpdateInPlace: boolean; + /** + * Staff and phantom test runs on this copy. They do not block the update, but it deletes them, so + * the confirmation prompt names the number first. + */ + testSubmissionCount: number; +} + export interface AssessmentData extends AssessmentActionsData { id: number; title: string; @@ -110,6 +158,13 @@ export interface AssessmentData extends AssessmentActionsData { }; isPublishedToMarketplace: boolean; marketplaceListingUrl: string; + /** Null unless a newer version of the adopted marketplace listing is available. */ + marketplaceUpdate: MarketplaceUpdateData | null; + /** + * Present only for a system admin viewing an assessment the marketplace owns inside its container + * course — a published snapshot or a listing's working copy. Same shape as the index row's badge. + */ + marketplaceVersion?: MarketplaceVersionData; requirements: { title: string; satisfied?: boolean; diff --git a/client/locales/en.json b/client/locales/en.json index 57e731ccf66..aa30c20c106 100644 --- a/client/locales/en.json +++ b/client/locales/en.json @@ -1571,6 +1571,27 @@ "course.assessment.generation.untitledQuestion": { "defaultMessage": "Untitled Question" }, + "course.assessment.marketplaceUpdateCompleted": { + "defaultMessage": "Assessment updated to the latest version. Refresh to see the latest version." + }, + "course.assessment.marketplaceUpdateConfirmBody": { + "defaultMessage": "This replaces this assessment's questions and materials with the version published on {latest}. It keeps its place in your course, its deadlines, and whether it is published." + }, + "course.assessment.marketplaceUpdateConfirmDeletion": { + "defaultMessage": "{count, plural, one {# test submission} other {# test submissions}} on this assessment will be deleted. No student has submitted work for it." + }, + "course.assessment.marketplaceUpdateConfirmTitle": { + "defaultMessage": "Update this assessment?" + }, + "course.assessment.marketplaceUpdateFailed": { + "defaultMessage": "Could not update this assessment." + }, + "course.assessment.marketplaceUpdateInPlace": { + "defaultMessage": "Update this assessment" + }, + "course.assessment.marketplaceUpdateStarted": { + "defaultMessage": "Updating this assessment…" + }, "course.assessment.question.multipleResponses.showOptions": { "defaultMessage": "Show Options" }, @@ -4271,6 +4292,15 @@ "course.assessments.index.hasTodo": { "defaultMessage": "Has TODO" }, + "course.assessments.index.marketplaceAuthoring": { + "defaultMessage": "Source Assessment" + }, + "course.assessments.index.marketplaceAuthoringHint": { + "defaultMessage": "Listing ID {listingId} · editable working copy, not a published version" + }, + "course.assessments.index.marketplaceAuthoringHintWithSource": { + "defaultMessage": "Listing ID {listingId} · editable working copy, not a published version · from {source}" + }, "course.assessments.index.neededFor": { "defaultMessage": "Needed for" }, diff --git a/client/locales/ko.json b/client/locales/ko.json index 720ee5aab9e..10e11ce6b7b 100644 --- a/client/locales/ko.json +++ b/client/locales/ko.json @@ -1571,6 +1571,27 @@ "course.assessment.generation.untitledQuestion": { "defaultMessage": "제목 없는 문항" }, + "course.assessment.marketplaceUpdateCompleted": { + "defaultMessage": "평가가 최신 버전으로 업데이트되었습니다." + }, + "course.assessment.marketplaceUpdateConfirmBody": { + "defaultMessage": "이 작업은 이 평가의 문항과 자료를 {latest}에 게시된 버전으로 대체합니다. 코스 내 위치, 마감일, 게시 여부는 유지됩니다." + }, + "course.assessment.marketplaceUpdateConfirmDeletion": { + "defaultMessage": "이 평가의 {count, plural, one {#개의 테스트 제출} other {#개의 테스트 제출}}이 삭제됩니다. 학생 제출물은 없습니다." + }, + "course.assessment.marketplaceUpdateConfirmTitle": { + "defaultMessage": "이 평가를 업데이트하시겠습니까?" + }, + "course.assessment.marketplaceUpdateFailed": { + "defaultMessage": "이 평가를 업데이트할 수 없습니다." + }, + "course.assessment.marketplaceUpdateInPlace": { + "defaultMessage": "이 평가 업데이트" + }, + "course.assessment.marketplaceUpdateStarted": { + "defaultMessage": "이 평가를 업데이트하는 중…" + }, "course.assessment.question.multipleResponses.showOptions": { "defaultMessage": "옵션 보기" }, @@ -4253,6 +4274,15 @@ "course.assessments.index.hasTodo": { "defaultMessage": "할 일 있음" }, + "course.assessments.index.marketplaceAuthoring": { + "defaultMessage": "원본 평가" + }, + "course.assessments.index.marketplaceAuthoringHint": { + "defaultMessage": "등록 항목 ID {listingId} · 수정 가능한 작업 사본이며, 발행된 버전이 아닙니다" + }, + "course.assessments.index.marketplaceAuthoringHintWithSource": { + "defaultMessage": "등록 항목 ID {listingId} · 수정 가능한 작업 사본이며, 발행된 버전이 아닙니다 · 출처: {source}" + }, "course.assessments.index.neededFor": { "defaultMessage": "필요한 경우" }, diff --git a/client/locales/zh.json b/client/locales/zh.json index 2fc6a90ea3d..7649ff1bab5 100644 --- a/client/locales/zh.json +++ b/client/locales/zh.json @@ -1562,6 +1562,27 @@ "course.assessment.generation.untitledQuestion": { "defaultMessage": "无标题题目" }, + "course.assessment.marketplaceUpdateCompleted": { + "defaultMessage": "评估已更新到最新版本。" + }, + "course.assessment.marketplaceUpdateConfirmBody": { + "defaultMessage": "这会将此评估的问题和资料替换为 {latest} 发布的版本。它会保留其在课程中的位置、截止日期以及发布状态。" + }, + "course.assessment.marketplaceUpdateConfirmDeletion": { + "defaultMessage": "此评估上的 {count, plural, one {# 个测试提交} other {# 个测试提交}} 将被删除。没有学生提交过作业。" + }, + "course.assessment.marketplaceUpdateConfirmTitle": { + "defaultMessage": "更新此评估?" + }, + "course.assessment.marketplaceUpdateFailed": { + "defaultMessage": "无法更新此评估。" + }, + "course.assessment.marketplaceUpdateInPlace": { + "defaultMessage": "更新此评估" + }, + "course.assessment.marketplaceUpdateStarted": { + "defaultMessage": "正在更新此评估…" + }, "course.assessment.question.multipleResponses.showOptions": { "defaultMessage": "显示选项" }, @@ -4247,6 +4268,15 @@ "course.assessments.index.hasTodo": { "defaultMessage": "显示待办事项" }, + "course.assessments.index.marketplaceAuthoring": { + "defaultMessage": "源评估" + }, + "course.assessments.index.marketplaceAuthoringHint": { + "defaultMessage": "市场条目 ID {listingId} · 可编辑的工作副本,非已发布版本" + }, + "course.assessments.index.marketplaceAuthoringHintWithSource": { + "defaultMessage": "市场条目 ID {listingId} · 可编辑的工作副本,非已发布版本 · 来自 {source}" + }, "course.assessments.index.neededFor": { "defaultMessage": "需要的" }, diff --git a/config/locales/en/course/assessment/assessments.yml b/config/locales/en/course/assessment/assessments.yml index 5af72a174a2..14477293b3b 100644 --- a/config/locales/en/course/assessment/assessments.yml +++ b/config/locales/en/course/assessment/assessments.yml @@ -1,6 +1,11 @@ en: course: assessment: + marketplace_adoptions: + apply_latest_version: + student_submissions_exist: >- + This assessment cannot be updated in place because students have already submitted + work for it. Import the latest version as a new assessment instead. assessments: invalid_questions_order: 'Invalid ordering for assessment questions' show: diff --git a/config/locales/ko/course/assessment/assessments.yml b/config/locales/ko/course/assessment/assessments.yml index aec245e5ec3..ad99f423c44 100644 --- a/config/locales/ko/course/assessment/assessments.yml +++ b/config/locales/ko/course/assessment/assessments.yml @@ -1,6 +1,11 @@ ko: course: assessment: + marketplace_adoptions: + apply_latest_version: + student_submissions_exist: >- + 학생들이 이미 이 평가에 제출한 작업이 있으므로 이 평가를 제자리에서 업데이트할 수 없습니다. + 대신 최신 버전을 새 평가로 가져오세요. assessments: invalid_questions_order: '평가 질문의 순서가 잘못되었습니다' show: diff --git a/config/locales/zh/course/assessment/assessments.yml b/config/locales/zh/course/assessment/assessments.yml index de0b696d652..6ce7a20cf36 100644 --- a/config/locales/zh/course/assessment/assessments.yml +++ b/config/locales/zh/course/assessment/assessments.yml @@ -1,6 +1,11 @@ zh: course: assessment: + marketplace_adoptions: + apply_latest_version: + student_submissions_exist: >- + 由于学生已经提交了此评估的作业,无法就地更新此评估。 + 请改为将最新版本导入为新的评估。 assessments: invalid_questions_order: '测验问题的权重无效' show: diff --git a/config/routes.rb b/config/routes.rb index 4938115aeb7..b43af999527 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -297,6 +297,9 @@ resource :marketplace_listing, only: [:create, :destroy] do post 'versions' => 'marketplace_listings#publish_version' end + resource :marketplace_adoption, only: [] do + post 'apply_latest_version' => 'marketplace_adoptions#apply_latest_version' + end namespace :question do resources :multiple_responses, only: [:new, :create, :edit, :update, :destroy] do diff --git a/spec/controllers/course/assessment/assessments_marketplace_spec.rb b/spec/controllers/course/assessment/assessments_marketplace_spec.rb index 98bc1d0c3d7..36c4cae751a 100644 --- a/spec/controllers/course/assessment/assessments_marketplace_spec.rb +++ b/spec/controllers/course/assessment/assessments_marketplace_spec.rb @@ -6,6 +6,12 @@ let!(:instance) { Instance.default } with_tenant(:instance) do + # The container is a per-instance singleton (`index_courses_on_instance_id_one_preview`), and this + # suite commits, so examples share one row instead of each minting a colliding preview course. + def preview_container + Course.find_by(preview: true) || create(:course, preview: true) + end + let(:course) { create(:course) } let(:assessment) { create(:assessment, course: course) } let(:admin) { create(:administrator) } @@ -29,6 +35,49 @@ end end + describe 'marketplaceUpdate' do + let(:destination_course) { create(:course) } + let(:manager) { create(:course_manager, course: destination_course).user } + let(:copy) { create(:assessment, course: destination_course) } + let(:v1_at) { 10.days.ago.change(usec: 0) } + let(:v2_at) { 1.day.ago.change(usec: 0) } + let(:listing) do + create(:course_assessment_marketplace_listing, :versioned, + published: true, first_published_at: v1_at) + end + + before { controller_sign_in(controller, manager) } + + subject do + get :show, params: { course_id: destination_course.id, id: copy.id, format: :json } + end + + it 'is null for an assessment that was never adopted' do + subject + + expect(response.parsed_body['marketplaceUpdate']).to be_nil + end + + it 'carries the notice when a newer version exists' do + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: destination_course, + duplicated_assessment: copy, adopted_version_at: v1_at) + v2 = create(:course_assessment_marketplace_listing_version, + listing: listing, + assessment: create(:assessment, course: listing.authoring_assessment.course), + published_at: v2_at, published_by: listing.publisher) + listing.update!(current_version: v2) + + subject + + notice = response.parsed_body['marketplaceUpdate'] + expect(notice.keys).to contain_exactly('adoptedVersionAt', 'latestVersionAt', + 'canUpdateInPlace', 'testSubmissionCount') + expect(Time.zone.parse(notice['adoptedVersionAt'])).to be_within(1.second).of(v1_at) + expect(Time.zone.parse(notice['latestVersionAt'])).to be_within(1.second).of(v2_at) + end + end + context 'as a course manager (non-admin)' do let(:manager) { create(:course_manager, course: course).user } before { controller_sign_in(controller, manager) } @@ -39,5 +88,200 @@ end end end + + # Opening a container assessment must carry the identity its index row carries. Without it the + # snapshot, the listing's working copy and an ordinary draft are three indistinguishable pages — + # and the snapshot's lone marketplace control invites republishing immutable content as a listing + # of its own, whose source assessment would then be frozen inside the container. + describe 'GET #show — marketplace container context' do + let(:container) { preview_container } + let(:listing) do + create(:course_assessment_marketplace_listing, course: container, + source_course_name: 'MP Allowlist Source Course') + end + let(:working_copy) { listing.authoring_assessment } + let(:snapshot) { create(:assessment, course: container) } + let(:published_at) { 3.days.ago.change(usec: 0) } + let!(:version) do + create(:course_assessment_marketplace_listing_version, + listing: listing, assessment: snapshot, published_at: published_at, + published_by: listing.publisher).tap { |cut| listing.update!(current_version: cut) } + end + + def show_for(target_course, target_assessment) + get :show, as: :json, params: { course_id: target_course.id, id: target_assessment.id } + end + + context 'as a system admin' do + before { controller_sign_in(controller, admin) } + + it 'dates a snapshot with the same fields as its index row' do + show_for(container, snapshot) + + label = response.parsed_body['marketplaceVersion'] + expect(label.keys).to contain_exactly('listingId', 'publishedAt', 'source', 'latest', + 'listed') + expect(label['listingId']).to eq(listing.id) + expect(label['source']).to eq('MP Allowlist Source Course') + expect(label['latest']).to be(true) + expect(label['listed']).to be(true) + expect(Time.zone.parse(label['publishedAt'])).to be_within(1.second).of(published_at) + end + + it 'reports the working copy as a non-version' do + show_for(container, working_copy) + + label = response.parsed_body['marketplaceVersion'] + expect(label['listingId']).to eq(listing.id) + expect(label['publishedAt']).to be_nil + expect(label['latest']).to be(false) + end + + it 'withholds publishing from a snapshot, which is already an existing listing content' do + show_for(container, snapshot) + + expect(response.parsed_body['permissions']).to include('canPublishToMarketplace' => false) + end + + it 'keeps publishing available on the working copy' do + show_for(container, working_copy) + + expect(response.parsed_body['permissions']).to include('canPublishToMarketplace' => true) + end + + # An assessment authored directly in the container is neither a snapshot nor a working copy. + # Publishing it is the supported way a marketplace-hosted listing comes to exist at all. + it 'keeps publishing available on an unlabelled container assessment' do + fresh = create(:assessment, course: container) + + show_for(container, fresh) + + expect(response.parsed_body).not_to have_key('marketplaceVersion') + expect(response.parsed_body['permissions']).to include('canPublishToMarketplace' => true) + end + + # The guard is the container's `preview` flag, mirroring the index: the same assessment + # outside the container must stay unlabelled. + it 'omits the context outside the container, even for a versioned assessment' do + in_normal_course = create(:assessment, course: course) + create(:course_assessment_marketplace_listing_version, + listing: listing, assessment: in_normal_course, published_at: 2.days.ago, + published_by: listing.publisher) + + show_for(course, in_normal_course) + + expect(response.parsed_body).not_to have_key('marketplaceVersion') + end + end + + # Previewers are enrolled into the container as managers. The context is admin-only navigation, + # exactly as on the index. + context 'as a non-admin manager of the container' do + before { controller_sign_in(controller, create(:course_manager, course: container).user) } + + it 'omits the context' do + show_for(container, snapshot) + + expect(response.parsed_body).not_to have_key('marketplaceVersion') + end + end + end + + describe 'version identity in the assessment payloads' do + render_views + + let(:destination_course) { create(:course) } + let(:manager) { create(:course_manager, course: destination_course).user } + let(:copy) { create(:assessment, course: destination_course) } + let(:v1_at) { 30.days.ago.change(usec: 0) } + let(:listing) do + create(:course_assessment_marketplace_listing, published: true, first_published_at: v1_at) + end + + before do + version = create(:course_assessment_marketplace_listing_version, + listing: listing, + assessment: create(:assessment, course: listing.authoring_assessment.course), + published_at: v1_at, published_by: listing.publisher) + listing.update!(current_version: version) + controller_sign_in(controller, manager) + end + + it 'dates both vintages on the update notice and carries no ordinal' do + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: destination_course, + duplicated_assessment: copy, adopted_version_at: v1_at) + latest = create(:course_assessment_marketplace_listing_version, + listing: listing, + assessment: create(:assessment, course: listing.authoring_assessment.course), + published_at: 1.day.ago.change(usec: 0), published_by: listing.publisher) + listing.update!(current_version: latest) + + get :show, as: :json, params: { course_id: destination_course, id: copy } + + update = response.parsed_body['marketplaceUpdate'] + expect(update.keys).to contain_exactly('adoptedVersionAt', 'latestVersionAt', + 'canUpdateInPlace', 'testSubmissionCount') + expect(Time.zone.parse(update['adoptedVersionAt'])).to be_within(1.second).of(v1_at) + expect(Time.zone.parse(update['latestVersionAt'])). + to be_within(1.second).of(latest.published_at) + end + + it 'emits a null update notice when the copy is current' do + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: destination_course, + duplicated_assessment: copy, adopted_version_at: v1_at) + + get :show, as: :json, params: { course_id: destination_course, id: copy } + + expect(response.parsed_body['marketplaceUpdate']).to be_nil + end + end + + describe 'the in-place update gate on the show payload' do + render_views + + let(:destination_course) { create(:course) } + let(:manager) { create(:course_manager, course: destination_course).user } + let(:copy) { create(:assessment, :with_mcq_question, course: destination_course) } + let(:v1_at) { 30.days.ago.change(usec: 0) } + let(:listing) do + create(:course_assessment_marketplace_listing, published: true, first_published_at: v1_at) + end + + before do + version = create(:course_assessment_marketplace_listing_version, + listing: listing, + assessment: create(:assessment, course: listing.authoring_assessment.course), + published_at: v1_at, published_by: listing.publisher) + listing.update!(current_version: version) + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: destination_course, + duplicated_assessment: copy, adopted_version_at: v1_at) + newer = create(:course_assessment_marketplace_listing_version, + listing: listing, + assessment: create(:assessment, course: listing.authoring_assessment.course), + published_at: 1.day.ago.change(usec: 0), published_by: listing.publisher) + listing.update!(current_version: newer) + controller_sign_in(controller, manager) + end + + it 'offers the in-place update on an unattempted copy' do + get :show, as: :json, params: { course_id: destination_course, id: copy } + + update = response.parsed_body['marketplaceUpdate'] + expect(update['canUpdateInPlace']).to be(true) + expect(update['testSubmissionCount']).to eq(0) + end + + it 'withholds the in-place update once a real student has attempted the copy' do + create(:submission, :attempting, assessment: copy, + creator: create(:course_student, course: destination_course).user) + + get :show, as: :json, params: { course_id: destination_course, id: copy } + + expect(response.parsed_body['marketplaceUpdate']['canUpdateInPlace']).to be(false) + end + end end end diff --git a/spec/controllers/course/assessment/marketplace_adoptions_controller_spec.rb b/spec/controllers/course/assessment/marketplace_adoptions_controller_spec.rb new file mode 100644 index 00000000000..8b970244698 --- /dev/null +++ b/spec/controllers/course/assessment/marketplace_adoptions_controller_spec.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::Assessment::MarketplaceAdoptionsController, type: :controller do + let!(:instance) { Instance.default } + with_tenant(:instance) do + let(:destination_course) { create(:course) } + let(:copy) { create(:assessment, :with_mcq_question, course: destination_course) } + let(:v1_at) { 30.days.ago.change(usec: 0) } + let(:listing) do + create(:course_assessment_marketplace_listing, published: true, first_published_at: v1_at) + end + let!(:v1) do + version = create(:course_assessment_marketplace_listing_version, + listing: listing, + assessment: create(:assessment, course: listing.authoring_assessment.course), + published_at: v1_at, published_by: listing.publisher) + listing.update!(current_version: version) + version + end + let!(:adoption) do + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: destination_course, + duplicated_assessment: copy, adopted_version_at: v1_at) + end + let(:manager) { create(:course_manager, course: destination_course).user } + + describe 'POST #apply_latest_version' do + render_views + + with_active_job_queue_adapter(:test) do + def apply + post :apply_latest_version, as: :json, + params: { course_id: destination_course.id, assessment_id: copy.id } + end + + context 'as a course manager' do + before { controller_sign_in(controller, manager) } + + it 'enqueues the update and answers with the job url' do + expect { apply }.to have_enqueued_job(Course::Assessment::Marketplace::ApplyVersionJob) + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['jobUrl']).to be_present + end + + # The client flag is advisory. A stale page must never be able to destroy student work. + it 'refuses when a real student has attempted the copy, whatever the client believed' do + create(:submission, :attempting, assessment: copy, + creator: create(:course_student, course: destination_course).user) + + expect { apply }.not_to have_enqueued_job(Course::Assessment::Marketplace::ApplyVersionJob) + expect(response).to have_http_status(:unprocessable_content) + key = 'course.assessment.marketplace_adoptions.apply_latest_version.student_submissions_exist' + expect(I18n.t(key)).to eq(key) + expect(response.parsed_body['errors'].first).to eq(I18n.t(key)) + end + + it 'still allows the update when only staff have test submissions' do + create(:submission, :attempting, assessment: copy, creator: manager) + + expect { apply }.to have_enqueued_job(Course::Assessment::Marketplace::ApplyVersionJob) + end + + it 'responds 404 when the assessment was never adopted' do + other = create(:assessment, course: destination_course) + + post :apply_latest_version, as: :json, + params: { course_id: destination_course.id, assessment_id: other.id } + + expect(response).to have_http_status(:not_found) + end + end + + context 'as a course student' do + before { controller_sign_in(controller, create(:course_student, course: destination_course).user) } + + it 'is denied' do + expect { apply }.to raise_exception(CanCan::AccessDenied) + end + end + end + end + end +end diff --git a/spec/jobs/course/assessment/marketplace/apply_version_job_spec.rb b/spec/jobs/course/assessment/marketplace/apply_version_job_spec.rb new file mode 100644 index 00000000000..da9569b084a --- /dev/null +++ b/spec/jobs/course/assessment/marketplace/apply_version_job_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::Assessment::Marketplace::ApplyVersionJob, type: :job do + let!(:instance) { Instance.default } + with_tenant(:instance) do + let(:user) { create(:administrator) } + let(:source_course) { create(:course) } + let(:source_assessment) do + create(:assessment, :with_mcq_question, course: source_course, title: 'Marketplace Lab') + end + let(:destination_course) { create(:course) } + let!(:listing) do + Course::Assessment::Marketplace::PublishService.publish(source_assessment, user) + end + let(:copy) do + create(:assessment, :with_mcq_question, course: destination_course, title: 'My Local Title') + end + let!(:adoption) do + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: destination_course, + duplicated_assessment: copy, + adopted_version_at: listing.current_version.published_at) + end + + before do + source_assessment.update!(title: 'Marketplace Lab v2') + Course::Assessment::Marketplace::PublishService.publish_new_version(listing.reload, user) + end + + def run_and_capture + job = described_class.new(copy, current_user: user) + job.perform_now + job.job + end + + it 'replaces the content and redirects back to the same assessment' do + job = run_and_capture + + expect(copy.reload.title).to eq('Marketplace Lab v2') + expect(job.redirect_to).to include("/courses/#{destination_course.id}/assessments/#{copy.id}") + end + + it 'reports the job as completed' do + job = run_and_capture + + expect(job.status).to eq('completed') + end + + it 'errors the job rather than raising when the listing serves nothing' do + listing.update!(current_version: nil) + + job = run_and_capture + + expect(job.status).to eq('errored') + end + + it 'errors instead of deleting work when a student attempt exists by execution time' do + create(:submission, :attempting, assessment: copy, + creator: create(:course_student, course: destination_course).user) + + job = run_and_capture + + expect(job.status).to eq('errored') + expect(copy.reload.title).to eq('My Local Title') + expect(copy.questions).not_to be_empty + end + end +end diff --git a/spec/models/course/assessment/marketplace/adoption_spec.rb b/spec/models/course/assessment/marketplace/adoption_spec.rb index 7b0e9c38488..f0c525eceb2 100644 --- a/spec/models/course/assessment/marketplace/adoption_spec.rb +++ b/spec/models/course/assessment/marketplace/adoption_spec.rb @@ -20,5 +20,171 @@ adoption.duplicated_assessment.destroy expect(described_class.exists?(adoption.id)).to be(false) end + + describe '.update_notice_for' do + let(:destination_course) { create(:course) } + let(:copy) { create(:assessment, :with_mcq_question, course: destination_course) } + let(:v1_at) { 30.days.ago.change(usec: 0) } + let(:listing) do + create(:course_assessment_marketplace_listing, published: true, first_published_at: v1_at) + end + let!(:v1) do + version = create(:course_assessment_marketplace_listing_version, + listing: listing, + assessment: create(:assessment, course: listing.authoring_assessment.course), + published_at: v1_at, + published_by: listing.publisher) + listing.update!(current_version: version) + version + end + + def cut_version(published_at) + version = create(:course_assessment_marketplace_listing_version, + listing: listing, + assessment: create(:assessment, course: listing.authoring_assessment.course), + published_at: published_at, + published_by: listing.publisher) + listing.update!(current_version: version) + version + end + + def adopt(adopted_version_at:) + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: destination_course, + duplicated_assessment: copy, adopted_version_at: adopted_version_at) + end + + it 'returns nil when the assessment was never adopted' do + expect(described_class.update_notice_for(copy.id)).to be_nil + end + + it 'returns nil when the adopted vintage is the current one' do + adopt(adopted_version_at: v1_at) + + expect(described_class.update_notice_for(copy.id)).to be_nil + end + + it 'returns the notice when a newer vintage exists' do + adopt(adopted_version_at: v1_at) + v2 = cut_version(2.days.ago.change(usec: 0)) + + notice = described_class.update_notice_for(copy.id) + + expect(notice[:adopted_version_at]).to be_within(1.second).of(v1_at) + expect(notice[:latest_version_at]).to be_within(1.second).of(v2.published_at) + end + + # The banner speaks in dates only — there is no ordinal anywhere in the payload. + it 'carries no version ordinal in the notice' do + adopt(adopted_version_at: v1_at) + cut_version(2.days.ago.change(usec: 0)) + + notice = described_class.update_notice_for(copy.id) + + expect(notice.keys).to contain_exactly(:adopted_version_at, :latest_version_at, + :can_update_in_place, :test_submission_count) + end + + it 'dates a mid-chain adopted vintage from the adoption row itself' do + v2 = cut_version(10.days.ago.change(usec: 0)) + adopt(adopted_version_at: v2.published_at) + cut_version(1.day.ago.change(usec: 0)) + + notice = described_class.update_notice_for(copy.id) + + expect(notice[:adopted_version_at]).to be_within(1.second).of(v2.published_at) + end + + # Fail toward silence: a false "an update is waiting" trains managers to ignore the banner. + it 'returns nil when the adopted vintage is unknown, rather than guessing' do + adopt(adopted_version_at: nil) + cut_version(2.days.ago.change(usec: 0)) + + expect(described_class.update_notice_for(copy.id)).to be_nil + end + + it 'returns nil when the listing has no current version at all' do + adoption = adopt(adopted_version_at: v1_at) + listing.update!(current_version: nil) + + expect(described_class.update_notice_for(adoption.duplicated_assessment_id)).to be_nil + end + + # An adopter whose copy is somehow NEWER than what the listing serves must not be told an + # update is waiting — the comparison is strictly greater-than, not merely different. + it 'returns nil when the adopted vintage is newer than the served one' do + adopt(adopted_version_at: 1.hour.ago.change(usec: 0)) + + expect(described_class.update_notice_for(copy.id)).to be_nil + end + + it 'resolves when the snapshot lives in another tenant, with no tenant escape' do + adopt(adopted_version_at: v1_at) + other_instance = create(:instance) + published = 1.day.ago.change(usec: 0) + ActsAsTenant.without_tenant do + snapshot = ActsAsTenant.with_tenant(other_instance) { create(:assessment) } + v2 = create(:course_assessment_marketplace_listing_version, + listing: listing, assessment: snapshot, published_at: published, + published_by: listing.publisher) + listing.update!(current_version: v2) + end + + expect(described_class.update_notice_for(copy.id)[:latest_version_at]). + to be_within(1.second).of(published) + end + + describe 'the in-place update gate' do + before do + adopt(adopted_version_at: v1_at) + cut_version(2.days.ago.change(usec: 0)) + end + + it 'allows the in-place update when nobody has attempted the copy' do + notice = described_class.update_notice_for(copy.id) + + expect(notice[:can_update_in_place]).to be(true) + expect(notice[:test_submission_count]).to eq(0) + end + + it 'reports the test submissions the update would delete' do + manager = create(:course_manager, course: destination_course) + create(:submission, :attempting, assessment: copy, creator: manager.user) + + notice = described_class.update_notice_for(copy.id) + + expect(notice[:can_update_in_place]).to be(true) + expect(notice[:test_submission_count]).to eq(1) + end + + it 'refuses the in-place update once a real student has attempted the copy' do + student = create(:course_student, course: destination_course) + create(:submission, :attempting, assessment: copy, creator: student.user) + + notice = described_class.update_notice_for(copy.id) + + expect(notice[:can_update_in_place]).to be(false) + end + end + end + + describe '#latest_version_at' do + let(:listing) { create(:course_assessment_marketplace_listing, :versioned, published: true) } + let(:adoption) do + create(:course_assessment_marketplace_adoption, listing: listing, + adopted_version_at: 1.day.ago) + end + + it 'reports the served version publish date' do + expect(adoption.latest_version_at). + to be_within(1.second).of(listing.current_version.published_at) + end + + it 'is nil for a listing with no current version' do + listing.update!(current_version: nil) + + expect(adoption.reload.latest_version_at).to be_nil + end + end end end diff --git a/spec/services/course/assessment/marketplace/apply_version_service_spec.rb b/spec/services/course/assessment/marketplace/apply_version_service_spec.rb new file mode 100644 index 00000000000..cdbba813600 --- /dev/null +++ b/spec/services/course/assessment/marketplace/apply_version_service_spec.rb @@ -0,0 +1,174 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Course::Assessment::Marketplace::ApplyVersionService, type: :service do + let!(:instance) { Instance.default } + with_tenant(:instance) do + let(:user) { create(:administrator) } + let(:source_course) { create(:course) } + let(:source_assessment) do + create(:assessment, :with_mcq_question, course: source_course, title: 'Marketplace Lab') + end + let(:destination_course) { create(:course) } + let!(:listing) do + Course::Assessment::Marketplace::PublishService.publish(source_assessment, user) + end + let(:copy) do + create(:assessment, :with_mcq_question, course: destination_course, title: 'My Local Title') + end + let!(:adoption) do + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: destination_course, + duplicated_assessment: copy, + adopted_version_at: listing.current_version.published_at) + end + + def cut_newer_version + source_assessment.update!(title: 'Marketplace Lab v2') + Course::Assessment::Marketplace::PublishService.publish_new_version(listing.reload, user) + end + + describe '.apply' do + it 'keeps the same assessment row rather than making a new one' do + cut_newer_version + + expect { described_class.apply(copy, user) }. + not_to(change { Course::Assessment.where(id: copy.id).count }) + + expect(copy.reload).to be_present + end + + it 'destroys the throwaway copy it duplicated to' do + cut_newer_version + + expect { described_class.apply(copy, user) }. + to change { destination_course.assessments.count }.by(0) + end + + # Restamping the vintage is the ONLY thing that retires the update banner — there is no + # dismissal state alongside it to clear. + it 'advances the adoption to the served vintage' do + version = cut_newer_version + + described_class.apply(copy, user) + + expect(adoption.reload.adopted_version_at).to be_within(1.second).of(version.published_at) + expect(adoption.reload).not_to be_update_pending + end + + it 'takes the title from the new version' do + cut_newer_version + + described_class.apply(copy, user) + + expect(copy.reload.title).to eq('Marketplace Lab v2') + end + + # Slice 3's rule, minus self-collision: the copy's own old title must not count against it. + it 'renames when the new title is already taken in the destination course' do + version = cut_newer_version + create(:assessment, course: destination_course, title: 'Marketplace Lab v2') + + described_class.apply(copy, user) + + expect(copy.reload.title).to eq("Marketplace Lab v2 [#{version.published_at.strftime('%d %b %Y')}]") + end + + it 'does not rename when only its own old title would collide' do + cut_newer_version + + described_class.apply(copy, user) + + expect(copy.reload.title).to eq('Marketplace Lab v2') + end + + it 'keeps the tab position the manager chose' do + cut_newer_version + original_tab_id = copy.tab_id + + described_class.apply(copy, user) + + expect(copy.reload.tab_id).to eq(original_tab_id) + end + + # Replacing content must not silently expose or hide an assessment. + it 'keeps the published state' do + cut_newer_version + copy.update!(published: true) + + described_class.apply(copy, user) + + expect(copy.reload.published).to be(true) + end + + it 'replaces the questions with the new version questions' do + old_question_ids = copy.questions.map(&:id) + cut_newer_version + + described_class.apply(copy, user) + + expect(copy.reload.questions.map(&:id)).not_to match_array(old_question_ids) + expect(copy.questions).not_to be_empty + expect(Course::Assessment::Question.where(id: old_question_ids)).to be_empty + end + + # Staff test runs do not block the update, but their answers point at questions that no longer + # exist, so they go with them. + it 'destroys the submissions that were on the copy' do + manager = create(:course_manager, course: destination_course) + create(:submission, :attempting, assessment: copy, creator: manager.user) + cut_newer_version + + expect { described_class.apply(copy, user) }. + to change { Course::Assessment::Submission.where(assessment_id: copy.id).count }.to(0) + end + + it 'refuses once a real student has attempted by execution time' do + create(:submission, :attempting, assessment: copy, + creator: create(:course_student, course: destination_course).user) + old_question_ids = copy.questions.map(&:id) + cut_newer_version + + expect { described_class.apply(copy, user) }. + to raise_error(ArgumentError, /students have already submitted/) + + expect(copy.reload.title).to eq('My Local Title') + expect(copy.questions.map(&:id)).to match_array(old_question_ids) + expect(adoption.reload.adopted_version_at).to be_within(1.second).of(listing.first_published_at) + end + + # They were computed against a schedule that no longer exists. `find_or_create_personal_time_for` + # rebuilds them on demand from the new reference times, so this is not data loss. + it 'destroys personal times anchored to the replaced schedule' do + student = create(:course_student, course: destination_course) + copy.lesson_plan_item.find_or_create_personal_time_for(student).save! + cut_newer_version + + expect { described_class.apply(copy, user) }. + to change { Course::PersonalTime.where(lesson_plan_item_id: copy.lesson_plan_item.id).count }.to(0) + end + + it 'refuses an assessment that was never adopted' do + plain = create(:assessment, course: destination_course) + + expect { described_class.apply(plain, user) }.to raise_error(ArgumentError) + end + + it 'refuses a listing with no current version' do + listing.update!(current_version: nil) + + expect { described_class.apply(copy, user) }.to raise_error(ArgumentError) + end + + # The whole point of one transaction: a half-replaced assessment has no questions and no way back. + it 'leaves the copy untouched when the transplant fails' do + cut_newer_version + allow_any_instance_of(described_class).to receive(:copy_attributes!).and_raise('boom') + + expect { described_class.apply(copy, user) }.to raise_error('boom') + expect(copy.reload.questions).not_to be_empty + expect(copy.title).to eq('My Local Title') + end + end + end +end From 94b4bf8a47c4b303db09eb3dc28330c52f9d2553 Mon Sep 17 00:00:00 2001 From: lws49 Date: Wed, 29 Jul 2026 17:31:21 +0800 Subject: [PATCH 4/5] feat(marketplace): badge container snapshots in the assessment index Inside the preview container every listing's snapshots share one title, so the index chip dates each one and links it to the listing it belongs to. --- .../assessments/index.json.jbuilder | 18 + .../AssessmentsIndex/AssessmentsTable.tsx | 128 +++++++ .../__test__/AssessmentsTable.test.tsx | 318 ++++++++++++++++++ .../__test__/StatusBadges.test.tsx | 41 +++ .../bundles/course/assessment/translations.ts | 28 ++ .../assessments_marketplace_spec.rb | 148 ++++++++ 6 files changed, 681 insertions(+) create mode 100644 client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/AssessmentsTable.test.tsx create mode 100644 client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/StatusBadges.test.tsx diff --git a/app/views/course/assessment/assessments/index.json.jbuilder b/app/views/course/assessment/assessments/index.json.jbuilder index a2fbca99c0a..14a964d7045 100644 --- a/app/views/course/assessment/assessments/index.json.jbuilder +++ b/app/views/course/assessment/assessments/index.json.jbuilder @@ -1,6 +1,8 @@ # frozen_string_literal: true achievements_enabled = !current_component_host[:course_achievements_component].nil? submissions_hash = @assessments.to_h { |assessment| [assessment.id, assessment.submissions] } +# Empty for every course except the marketplace's snapshot container viewed by a system admin. +marketplace_versions = defined?(@marketplace_versions) ? @marketplace_versions : {} json.display do json.isStudent current_course_user&.student? || false @@ -14,6 +16,11 @@ json.display do json.canCreateAssessments can?(:create, Course::Assessment.new(tab: @tab)) json.canManageMonitor @can_manage_monitor && @monitoring_component_enabled + # True only in the marketplace's snapshot container, viewed by a system admin. Switches on the + # container-only Listing/Version/Source columns and the search toolbar — every other course's + # assessments index must stay exactly as it was. + json.isMarketplaceContainer @marketplace_container || false + json.category do json.id @category.id json.title @category.title @@ -51,6 +58,17 @@ json.assessments @assessments do |assessment| json.isKoditsuAssessmentEnabled assessment.is_koditsu_enabled end + marketplace_version = marketplace_versions[assessment.id] + if marketplace_version + json.marketplaceVersion do + json.listingId marketplace_version[:listing_id] + json.publishedAt marketplace_version[:published_at] + json.source marketplace_version[:source] + json.latest marketplace_version[:latest] + json.listed marketplace_version[:listed] + end + end + assessment_with_loaded_timeline = @items_hash[assessment.id].actable # assessment_with_loaded_timeline is passed below since the timeline is already preloaded and will be checked can_attempt_assessment = can?(:attempt, assessment_with_loaded_timeline) diff --git a/client/app/bundles/course/assessment/pages/AssessmentsIndex/AssessmentsTable.tsx b/client/app/bundles/course/assessment/pages/AssessmentsIndex/AssessmentsTable.tsx index 85602e2722c..cb6ab8cbdad 100644 --- a/client/app/bundles/course/assessment/pages/AssessmentsIndex/AssessmentsTable.tsx +++ b/client/app/bundles/course/assessment/pages/AssessmentsIndex/AssessmentsTable.tsx @@ -1,3 +1,4 @@ +import { useMemo } from 'react'; import { AssessmentListData, AssessmentsListData, @@ -13,6 +14,7 @@ import useTranslation from 'lib/hooks/useTranslation'; import translations from '../../translations'; import ActionButtons from './ActionButtons'; +import MarketplaceVersionChip from './MarketplaceVersionChip'; import StatusBadges from './StatusBadges'; interface AssessmentsTableProps { @@ -23,10 +25,59 @@ const AssessmentsTable = (props: AssessmentsTableProps): JSX.Element => { const { display, assessments, totalStudentCount } = props.assessments; const { t } = useTranslation(); + const isContainer = display.isMarketplaceContainer; + + const listingLabels = useMemo((): Record => { + const newest: Record = {}; + + assessments.forEach((assessment) => { + const version = assessment.marketplaceVersion; + if (!version) return; + + const held = newest[version.listingId]; + const publishedAt = version.publishedAt ?? ''; + if (!held || publishedAt > held.publishedAt) + newest[version.listingId] = { title: assessment.title, publishedAt }; + }); + + return Object.fromEntries( + Object.entries(newest).map(([listingId, held]) => [ + listingId, + t(translations.marketplaceListingLabel, { + title: held.title, + listingId, + }), + ]), + ); + }, [assessments, t]); + + const listingLabelFor = (assessment: AssessmentListData): string => + assessment.marketplaceVersion + ? listingLabels[assessment.marketplaceVersion.listingId] + : ''; + + /** + * Live / Latest / Older version / Source Assessment — null for an assessment that belongs to no + * listing. Live and Latest are mutually exclusive: both mean "newest cut", and Live additionally + * means the listing is on the marketplace, so it stands in for the weaker label. + */ + const versionKindFor = (assessment: AssessmentListData): string | null => { + const version = assessment.marketplaceVersion; + if (!version) return null; + if (version.publishedAt === null) + return t(translations.marketplaceAuthoring); + if (!version.latest) return t(translations.marketplaceOlderVersion); + + return version.listed + ? t(translations.marketplaceLive) + : t(translations.marketplaceLatest); + }; + const columns: ColumnTemplate[] = [ { of: 'title', title: t(translations.title), + searchable: isContainer, cell: (assessment) => (
), }, + { + id: 'marketplaceListing', + title: t(translations.marketplaceListingColumn), + unless: !isContainer, + filterable: true, + filterProps: { + getValue: (assessment) => + assessment.marketplaceVersion ? [listingLabelFor(assessment)] : [], + shouldInclude: (assessment, filterValue?: string[]) => + !filterValue?.length || + filterValue.includes(listingLabelFor(assessment)), + }, + cell: (assessment) => + assessment.marketplaceVersion ? ( + + {listingLabelFor(assessment)} + + ) : ( + t(translations.marketplaceNotAVersion) + ), + }, + { + id: 'marketplaceVersion', + title: t(translations.marketplaceVersionColumn), + unless: !isContainer, + sortable: true, + filterable: true, + // Sorts on the publication instant, not the rendered label. The server orders by + // `ordered_by_date_and_title`, and every snapshot of a listing inherits the origin's identical + // start_at AND title — so siblings have no tiebreak and their order can differ between loads. + // This column is how an admin pins them down. + accessorFn: (assessment) => + assessment.marketplaceVersion?.publishedAt ?? '', + filterProps: { + getValue: (assessment): string[] => { + const kind = versionKindFor(assessment); + return kind ? [kind] : []; + }, + shouldInclude: (assessment, filterValue?: string[]) => + !filterValue?.length || + filterValue.includes(versionKindFor(assessment) ?? ''), + }, + cell: (assessment) => + assessment.marketplaceVersion ? ( + + ) : ( + t(translations.marketplaceNotAVersion) + ), + }, + { + id: 'marketplaceSource', + title: t(translations.marketplaceSourceColumn), + unless: !isContainer, + sortable: true, + searchable: true, + // Deliberately NOT filterable, mirroring MarketplaceListingsTable: source courses number in the + // hundreds, most contributing one or two listings, and the filter is client-side over loaded + // rows. + accessorFn: (assessment) => assessment.marketplaceVersion?.source ?? '', + cell: (assessment) => + assessment.marketplaceVersion?.source ?? + t(translations.marketplaceNotAVersion), + }, { of: 'baseExp', title: t(translations.exp), @@ -185,6 +302,17 @@ const AssessmentsTable = (props: AssessmentsTableProps): JSX.Element => { }` } getRowId={(assessment): string => assessment.id.toString()} + renderEmpty={ + isContainer ? ( + + ) : undefined + } + search={ + isContainer + ? { searchPlaceholder: t(translations.marketplaceSearchText) } + : undefined + } + toolbar={isContainer ? { show: true } : undefined} /> ); }; diff --git a/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/AssessmentsTable.test.tsx b/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/AssessmentsTable.test.tsx new file mode 100644 index 00000000000..73ef86ed8b4 --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/AssessmentsTable.test.tsx @@ -0,0 +1,318 @@ +import userEvent from '@testing-library/user-event'; +import { render, waitFor, within } from 'test-utils'; +import { + AssessmentListData, + AssessmentsListData, +} from 'types/course/assessment/assessments'; + +import AssessmentsTable from '../AssessmentsTable'; + +const SEARCH_PLACEHOLDER = 'Search by assessment title or source course'; +const NO_RESULTS_MESSAGE = "Whoops, there's nothing to see here, yet!"; + +const assessment = ( + overrides: Partial = {}, +): AssessmentListData => ({ + id: 1, + title: 'Recursion', + status: 'open', + actionButtonUrl: null, + passwordProtected: false, + published: true, + autograded: false, + hasPersonalTimes: false, + affectsPersonalTimes: false, + url: '/courses/1/assessments/1', + conditionSatisfied: true, + startAt: { isFixed: false, effectiveTime: null, referenceTime: null }, + isStartTimeBegin: true, + ...overrides, +}); + +const listData = ( + assessments: AssessmentListData[], + isMarketplaceContainer: boolean, +): AssessmentsListData => ({ + display: { + isStudent: false, + isGamified: false, + isKoditsuExamEnabled: false, + timelineAlgorithm: 'fixed', + allowRandomization: false, + isAchievementsEnabled: false, + isMonitoringEnabled: false, + bonusAttributes: false, + endTimes: false, + canCreateAssessments: true, + tabId: 1, + tabTitle: 'Assessments: Default', + tabUrl: '/courses/1/assessments', + canManageMonitor: false, + isMarketplaceContainer, + category: { + id: 1, + title: 'Assessments', + tabs: [{ id: 1, title: 'Default' }], + }, + }, + assessments, +}); + +/** + * Four rows across three listings, covering every version kind: two cuts of a published listing + * (one served, one superseded), the single served cut of another, and the newest cut of a listing + * that has been taken off the marketplace. + */ +const containerRows = (): AssessmentListData[] => [ + assessment({ + id: 1, + title: 'Publish me 2', + marketplaceVersion: { + listingId: 4, + publishedAt: '2026-07-29T01:01:00Z', + source: 'Marketplace Preview Fixtures', + latest: false, + listed: true, + }, + }), + assessment({ + id: 2, + title: 'Publish me 2', + marketplaceVersion: { + listingId: 4, + publishedAt: '2026-07-29T01:04:00Z', + source: 'Marketplace Preview Fixtures', + latest: true, + listed: true, + }, + }), + assessment({ + id: 3, + title: 'Listed MCQ', + marketplaceVersion: { + listingId: 2, + publishedAt: '2026-07-29T00:59:00Z', + source: 'Other Source Course', + latest: true, + listed: true, + }, + }), + assessment({ + id: 4, + title: 'Taken down', + marketplaceVersion: { + listingId: 5, + publishedAt: '2026-07-29T02:07:00Z', + source: 'Retired Source Course', + latest: true, + listed: false, + }, + }), +]; + +// Column headers are matched by REGEX, never by an exact string: a filterable column's header cell +// also contains the filter IconButton, whose tooltip contributes "Filter" to the cell's accessible +// name (MUI applies the tooltip title as `aria-label` on a child with no text of its own). +describe(' in the marketplace container', () => { + it('adds the Listing, Version and Source columns', async () => { + const page = render( + , + ); + + expect( + await page.findByRole('columnheader', { name: /Listing/ }), + ).toBeInTheDocument(); + expect( + page.getByRole('columnheader', { name: /Version/ }), + ).toBeInTheDocument(); + expect( + page.getByRole('columnheader', { name: /Source/ }), + ).toBeInTheDocument(); + }); + + // The container tab is the ONLY place these belong. Leaking them would rewrite the assessments + // index for every course in the deployment. + it('shows none of them, and no search box, in an ordinary course', async () => { + const page = render( + , + ); + + expect( + await page.findByRole('link', { name: 'Recursion' }), + ).toBeInTheDocument(); + expect( + page.queryByRole('columnheader', { name: /Listing/ }), + ).not.toBeInTheDocument(); + expect( + page.queryByRole('columnheader', { name: /Version/ }), + ).not.toBeInTheDocument(); + expect( + page.queryByRole('columnheader', { name: /Source/ }), + ).not.toBeInTheDocument(); + expect( + page.queryByPlaceholderText(SEARCH_PLACEHOLDER), + ).not.toBeInTheDocument(); + // Generic, rather than keyed off our placeholder text: `MuiTableToolbar`'s `SearchField` falls + // back to a generic "Search" placeholder whenever the toolbar renders but `search` is unset, so a + // toolbar leaking in unconditionally would still pass the placeholder-only check above. + expect(page.queryByRole('textbox')).not.toBeInTheDocument(); + }); + + it('offers a search box in the container', async () => { + const page = render( + , + ); + + expect( + await page.findByPlaceholderText(SEARCH_PLACEHOLDER), + ).toBeInTheDocument(); + }); + + // Source course is searchable rather than filterable, matching the decision already recorded on + // MarketplaceListingsTable: courses number in the hundreds and a menu would grow without bound. + it('narrows to one listing by searching its source course', async () => { + const user = userEvent.setup(); + const page = render( + , + ); + + await user.type( + await page.findByPlaceholderText(SEARCH_PLACEHOLDER), + 'Other Source', + ); + + expect( + await page.findByRole('link', { name: 'Listed MCQ' }), + ).toBeInTheDocument(); + expect( + page.queryByRole('link', { name: 'Publish me 2' }), + ).not.toBeInTheDocument(); + expect( + page.queryByRole('link', { name: 'Taken down' }), + ).not.toBeInTheDocument(); + }); + + // The reason the Listing axis is a filter and not a search: these two rows are textually + // identical, so no search string can separate them from the third. + it('labels every row of one listing identically, using its newest title', async () => { + const page = render( + , + ); + + expect( + await page.findAllByRole('link', { name: 'Publish me 2 · ID 4' }), + ).toHaveLength(2); + expect( + page.getAllByRole('link', { name: 'Listed MCQ · ID 2' }), + ).toHaveLength(1); + }); + + it('links a listing to its admin history page', async () => { + const page = render( + , + ); + + expect( + await page.findByRole('link', { name: 'Listed MCQ · ID 2' }), + ).toHaveAttribute('href', '/admin/marketplace_listings/2'); + }); + + it('shows the Live chip only on the served snapshot of each published listing', async () => { + const page = render( + , + ); + + // Two published listings, one served snapshot each. The superseded cut and the unlisted + // listing's newest cut are both excluded. + expect(await page.findAllByText('Live')).toHaveLength(2); + }); + + // An unlisted listing still has a newest version — the one an admin re-publishing acts on — but + // nothing is being served, so it must read Latest and never Live. + it('marks an unlisted listing’s newest version Latest rather than Live', async () => { + const page = render( + , + ); + + expect(await page.findByText('Latest')).toBeInTheDocument(); + + const takenDownRow = page + .getByRole('link', { name: 'Taken down' }) + .closest('tr') as HTMLElement; + expect(within(takenDownRow).getByText('Latest')).toBeInTheDocument(); + expect(within(takenDownRow).queryByText('Live')).not.toBeInTheDocument(); + }); + + // Selecting Live is "what is the marketplace serving right now" in one click. The filter button is + // addressed by its 'Filter' name because the header also holds a sort button. + it('isolates what the marketplace is serving through the Version filter', async () => { + const user = userEvent.setup(); + const page = render( + , + ); + + const versionHeader = await page.findByRole('columnheader', { + name: /Version/, + }); + await user.click( + within(versionHeader).getByRole('button', { name: 'Filter' }), + ); + await user.click(await page.findByRole('menuitem', { name: 'Live' })); + // An open MUI menu marks the rest of the page `aria-hidden`, so the table rows are unqueryable + // until it is closed — matching the established pattern in MarketplaceListingsIndex.test.tsx. + await user.keyboard('{Escape}'); + await waitFor(() => + expect(page.queryByRole('menu')).not.toBeInTheDocument(), + ); + + // Listing 4's 9:04 cut survives and its 9:01 sibling does not; listing 2's only cut survives; + // the unlisted listing's newest cut is excluded because nothing of it is being served. + expect(page.getAllByRole('link', { name: 'Publish me 2' })).toHaveLength(1); + expect(page.getByRole('link', { name: 'Listed MCQ' })).toBeInTheDocument(); + expect( + page.queryByRole('link', { name: 'Taken down' }), + ).not.toBeInTheDocument(); + }); + + // Unlike the all-or-nothing `assessments.length === 0` case (covered elsewhere), a search or + // filter that matches nothing is reached with rows still in the payload, so the empty note has to + // come from the table itself rather than a check before it. + it('shows an empty state when the search matches nothing, but not while rows still match', async () => { + const user = userEvent.setup(); + const page = render( + , + ); + + const search = await page.findByPlaceholderText(SEARCH_PLACEHOLDER); + + expect(page.queryByText(NO_RESULTS_MESSAGE)).not.toBeInTheDocument(); + + await user.type(search, 'No source course matches this string'); + + expect(await page.findByText(NO_RESULTS_MESSAGE)).toBeInTheDocument(); + }); + + it('leaves the Listing, Version and Source cells empty for an assessment authored in the container', async () => { + const page = render( + , + ); + + expect( + await page.findByRole('link', { name: 'Hand-made in the container' }), + ).toBeInTheDocument(); + + // Indexed off the row rather than counting em dashes across the whole table, so an unrelated + // column rendering one cannot silently satisfy this. + const cells = within(page.getAllByRole('row')[1]).getAllByRole('cell'); + expect(cells[1]).toHaveTextContent('—'); + expect(cells[2]).toHaveTextContent('—'); + expect(cells[3]).toHaveTextContent('—'); + }); +}); diff --git a/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/StatusBadges.test.tsx b/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/StatusBadges.test.tsx new file mode 100644 index 00000000000..1f24bb7f0e6 --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentsIndex/__test__/StatusBadges.test.tsx @@ -0,0 +1,41 @@ +import { render, screen } from 'test-utils'; +import { AssessmentListData } from 'types/course/assessment/assessments'; + +import StatusBadges from '../StatusBadges'; + +const assessment = ( + overrides: Partial = {}, +): AssessmentListData => ({ + id: 1, + title: 'Recursion', + status: 'open', + actionButtonUrl: null, + passwordProtected: false, + published: true, + autograded: false, + hasPersonalTimes: false, + affectsPersonalTimes: false, + url: '/courses/1/assessments/1', + conditionSatisfied: true, + startAt: { isFixed: false, effectiveTime: null, referenceTime: null }, + isStartTimeBegin: true, + ...overrides, +}); + +const renderBadges = (data: AssessmentListData): void => { + render( + , + ); +}; + +// The marketplace cases that used to live here moved with the chip: two to +// MarketplaceVersionChip.test.tsx in Task 3, and the rest to AssessmentsTable.test.tsx, which is +// where the chip now renders. They are deleted rather than inverted into absence assertions — a +// removed behaviour gets its tests removed, not rewritten to assert it is gone. +describe('', () => { + it('marks an unpublished assessment as a draft', async () => { + renderBadges(assessment({ published: false })); + + expect(await screen.findByText('Draft')).toBeVisible(); + }); +}); diff --git a/client/app/bundles/course/assessment/translations.ts b/client/app/bundles/course/assessment/translations.ts index c51cde451eb..35cb4e9bd45 100644 --- a/client/app/bundles/course/assessment/translations.ts +++ b/client/app/bundles/course/assessment/translations.ts @@ -249,6 +249,34 @@ const translations = defineMessages({ defaultMessage: 'The newest version of this listing. Nothing is being served — the listing is off the marketplace.', }, + marketplaceListingColumn: { + id: 'course.assessments.index.marketplaceListingColumn', + defaultMessage: 'Listing', + }, + marketplaceVersionColumn: { + id: 'course.assessments.index.marketplaceVersionColumn', + defaultMessage: 'Version', + }, + marketplaceSourceColumn: { + id: 'course.assessments.index.marketplaceSourceColumn', + defaultMessage: 'Source', + }, + marketplaceListingLabel: { + id: 'course.assessments.index.marketplaceListingLabel', + defaultMessage: '{title} · ID {listingId}', + }, + marketplaceOlderVersion: { + id: 'course.assessments.index.marketplaceOlderVersion', + defaultMessage: 'Older version', + }, + marketplaceNotAVersion: { + id: 'course.assessments.index.marketplaceNotAVersion', + defaultMessage: '—', + }, + marketplaceSearchText: { + id: 'course.assessments.index.marketplaceSearchText', + defaultMessage: 'Search by assessment title or source course', + }, requirements: { id: 'course.assessment.show.requirements', defaultMessage: 'Requirements', diff --git a/spec/controllers/course/assessment/assessments_marketplace_spec.rb b/spec/controllers/course/assessment/assessments_marketplace_spec.rb index 36c4cae751a..a679b2c8276 100644 --- a/spec/controllers/course/assessment/assessments_marketplace_spec.rb +++ b/spec/controllers/course/assessment/assessments_marketplace_spec.rb @@ -89,6 +89,139 @@ def preview_container end end + # Snapshots keep their original title and share one tab of the container course, so the badge is + # the only thing distinguishing them. It must stay off every normal course's index (hot path) and + # away from the previewers who are enrolled into the container as managers. + describe 'GET #index — marketplace version badge' do + let(:container) { preview_container } + let(:snapshot) { create(:assessment, course: container) } + let(:listing) do + create(:course_assessment_marketplace_listing, source_course_name: 'MP Allowlist Source Course') + end + let(:published_at) { 3.days.ago.change(usec: 0) } + let(:outside_published_at) { 2.days.ago.change(usec: 0) } + let!(:version) do + create(:course_assessment_marketplace_listing_version, + listing: listing, assessment: snapshot, published_at: published_at, + published_by: listing.publisher) + end + + def index_for(target_course) + get :index, as: :json, params: { course_id: target_course.id } + end + + def payload_for(target_assessment) + response.parsed_body['assessments'].find { |json| json['id'] == target_assessment.id } + end + + context 'as a system admin' do + before { controller_sign_in(controller, admin) } + + it 'labels a container snapshot with its published date and provenance' do + index_for(container) + + label = payload_for(snapshot)['marketplaceVersion'] + expect(label.keys).to contain_exactly('listingId', 'publishedAt', 'source', 'latest', + 'listed') + expect(label['listingId']).to eq(listing.id) + expect(label['source']).to eq('MP Allowlist Source Course') + expect(Time.zone.parse(label['publishedAt'])).to be_within(1.second).of(published_at) + end + + # The guard is the container's `preview` flag, not the mere existence of a version row: the + # same assessment id outside the container must stay unlabelled. + it 'omits the badge outside the container, even for a versioned assessment' do + in_normal_course = create(:assessment, course: course) + create(:course_assessment_marketplace_listing_version, + listing: listing, assessment: in_normal_course, published_at: outside_published_at, + published_by: listing.publisher) + + index_for(course) + + expect(payload_for(in_normal_course)).not_to have_key('marketplaceVersion') + end + + it 'does not query listing versions for a normal course' do + expect(Course::Assessment::Marketplace::ListingVersion).not_to receive(:labels_for_assessments) + + index_for(course) + end + + it 'marks the served snapshot as the latest' do + listing.update!(current_version: version) + + index_for(container) + + expect(payload_for(snapshot)['marketplaceVersion']['latest']).to be(true) + end + + it 'does not mark a superseded snapshot as the latest' do + pointed_at_snapshot = create(:assessment, course: container) + pointed_at = create(:course_assessment_marketplace_listing_version, + listing: listing, assessment: pointed_at_snapshot, published_at: 1.day.ago, + published_by: listing.publisher) + listing.update!(current_version: pointed_at) + + index_for(container) + + expect(payload_for(snapshot)['marketplaceVersion']['latest']).to be(false) + expect(payload_for(pointed_at_snapshot)['marketplaceVersion']['latest']).to be(true) + end + + it 'reports whether the listing is on the marketplace' do + index_for(container) + + expect(payload_for(snapshot)['marketplaceVersion']['listed']).to be(true) + end + + it 'reports an unlisted listing as not listed' do + listing.update!(published: false) + + index_for(container) + + expect(payload_for(snapshot)['marketplaceVersion']['listed']).to be(false) + end + + it 'flags the container so the client can show its own columns and toolbar' do + index_for(container) + + expect(response.parsed_body['display']).to include('isMarketplaceContainer' => true) + end + + # The flag drives a search toolbar and three extra columns. Leaking it into ordinary courses + # would change the assessments index for every course in the deployment. + it 'does not flag an ordinary course as the container' do + index_for(course) + + expect(response.parsed_body['display']).to include('isMarketplaceContainer' => false) + end + end + + context 'as a non-admin manager of the container' do + before { controller_sign_in(controller, create(:course_manager, course: container).user) } + + it 'omits the badge' do + index_for(container) + + expect(payload_for(snapshot)).not_to have_key('marketplaceVersion') + end + + it 'does not query listing versions' do + expect(Course::Assessment::Marketplace::ListingVersion).not_to receive(:labels_for_assessments) + + index_for(container) + end + + # Previewers are enrolled into the container as managers. They must see neither the badge nor + # the admin-only navigation the flag switches on. + it 'does not flag the container' do + index_for(container) + + expect(response.parsed_body['display']).to include('isMarketplaceContainer' => false) + end + end + end + # Opening a container assessment must carry the identity its index row carries. Without it the # snapshot, the listing's working copy and an ordinary draft are three indistinguishable pages — # and the snapshot's lone marketplace control invites republishing immutable content as a listing @@ -236,6 +369,21 @@ def show_for(target_course, target_assessment) expect(response.parsed_body['marketplaceUpdate']).to be_nil end + + it 'dates a container snapshot chip by publish date, with no ordinal' do + container_course = preview_container + snapshot = create(:assessment, course: container_course) + listing.current_version.update!(assessment: snapshot) + controller_sign_in(controller, admin) + + get :index, as: :json, params: { course_id: container_course } + + row = response.parsed_body['assessments'].find { |a| a['id'] == snapshot.id } + expect(row['marketplaceVersion']).to have_key('publishedAt') + expect(row['marketplaceVersion']).not_to have_key('version') + expect(Time.zone.parse(row['marketplaceVersion']['publishedAt'])). + to be_within(1.second).of(v1_at) + end end describe 'the in-place update gate on the show payload' do From 941d049b7f54e885fafe152a29a029bf9da2403d Mon Sep 17 00:00:00 2001 From: lws49 Date: Wed, 29 Jul 2026 21:15:09 +0800 Subject: [PATCH 5/5] feat(marketplace): warn against editing a published snapshot A snapshot in the container course is an ordinary assessment with every management affordance live, and editing one is silently destructive: it changes what future adopters copy for a version that was never published, and stops the version's publication date describing its content. A soft guard only. Nothing is disabled, because the surface is admin-only and the escape hatch for repairing served content without minting a version is deliberate. The banner names the risk and links straight at the source assessment to edit instead, on that assessment's own host since it may live on another instance. --- .../assessment/assessments_controller.rb | 30 ++++++- app/models/instance.rb | 15 ++++ .../assessment/assessments/show.json.jbuilder | 3 + .../AssessmentShow/AssessmentShowPage.tsx | 3 + .../MarketplaceSnapshotBanner.tsx | 56 +++++++++++++ .../__test__/AssessmentShowPage.test.tsx | 28 +++++++ .../MarketplaceSnapshotBanner.test.tsx | 81 +++++++++++++++++++ .../bundles/course/assessment/translations.ts | 14 ++++ .../types/course/assessment/assessments.ts | 7 ++ .../assessments_marketplace_spec.rb | 76 ++++++++++++++++- spec/models/instance_spec.rb | 29 +++++++ 11 files changed, 340 insertions(+), 2 deletions(-) create mode 100644 client/app/bundles/course/assessment/pages/AssessmentShow/MarketplaceSnapshotBanner.tsx create mode 100644 client/app/bundles/course/assessment/pages/AssessmentShow/__test__/MarketplaceSnapshotBanner.test.tsx diff --git a/app/controllers/course/assessment/assessments_controller.rb b/app/controllers/course/assessment/assessments_controller.rb index d7b81829b35..bc3d9207819 100644 --- a/app/controllers/course/assessment/assessments_controller.rb +++ b/app/controllers/course/assessment/assessments_controller.rb @@ -275,9 +275,37 @@ def marketplace_version_labels # The single-assessment reading of the same labels, for `show`. Nil for a container assessment that # is neither a snapshot nor a listing's working copy — one authored in the container directly. # + # A snapshot additionally carries where to edit the content it froze. Merged here rather than in + # `labels_for_assessments`, which the index shares and has no use for the field. + # # @return [Hash, nil] def marketplace_version_label - Course::Assessment::Marketplace::ListingVersion.labels_for_assessments([@assessment.id])[@assessment.id] + label = Course::Assessment::Marketplace::ListingVersion. + labels_for_assessments([@assessment.id])[@assessment.id] + return nil if label.nil? + # Skipped for the working copy: the source assessment is this page. + return label if label[:published_at].nil? + + label.merge(source_assessment_url: source_assessment_url(label[:listing_id])) + end + + # Absolute, and carrying the source assessment's own host: a course id only resolves on its + # instance's host, and a listing's source lives on whichever instance published it. Nil for an + # orphaned listing, whose source was deleted and whose rebuild has not landed. + # + # @param [Integer] listing_id + # @return [String, nil] + def source_assessment_url(listing_id) + ActsAsTenant.without_tenant do + listing = Course::Assessment::Marketplace::Listing. + includes(authoring_assessment: { lesson_plan_item: { course: :instance } }). + find_by(id: listing_id) + assessment = listing&.authoring_assessment + next nil if assessment.nil? + + course_assessment_url(assessment.course_id, assessment, + **assessment.course.instance.host_options) + end end def load_assessment_submission_counts diff --git a/app/models/instance.rb b/app/models/instance.rb index 38eb55e8654..31761d10685 100644 --- a/app/models/instance.rb +++ b/app/models/instance.rb @@ -139,6 +139,21 @@ def host read_attribute(:host).gsub('coursemology.org', default_host) end + # `#host` carries the port the app is publicly served on, and a url built from it must name that + # port separately: a controller's `url_options` always supplies `port: request.optional_port`, and + # Rails reads a port out of `host:` only when no `:port` key is present — so passing the host + # alone silently swaps in the port the request reached Rails on. + # + # The two differ whenever a proxy sits in front, i.e. every development setup, and the url then + # names a port the browser cannot reach. A host with no port yields `port: nil`, which is what + # production wants. Jobs and mailers escape this: no request, hence no `:port` key. + # + # @return [Hash] the `host:`/`port:` options for a url on this instance + def host_options + name, port = host.split(':', 2) + { host: name, port: port } + end + def redirect_uri protocol = if Rails.env.development? && ENV['RAILS_USE_HTTP'] 'http' diff --git a/app/views/course/assessment/assessments/show.json.jbuilder b/app/views/course/assessment/assessments/show.json.jbuilder index 3281ea7d26a..e7352701563 100644 --- a/app/views/course/assessment/assessments/show.json.jbuilder +++ b/app/views/course/assessment/assessments/show.json.jbuilder @@ -92,6 +92,9 @@ if @marketplace_version json.source @marketplace_version[:source] json.latest @marketplace_version[:latest] json.listed @marketplace_version[:listed] + if @marketplace_version.key?(:source_assessment_url) + json.sourceAssessmentUrl @marketplace_version[:source_assessment_url] + end end end diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowPage.tsx b/client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowPage.tsx index 3ccd8b428ee..452456a512a 100644 --- a/client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowPage.tsx +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/AssessmentShowPage.tsx @@ -27,6 +27,7 @@ import MarketplaceVersionChip from '../AssessmentsIndex/MarketplaceVersionChip'; import AssessmentDetails from './AssessmentDetails'; import AssessmentShowHeader from './AssessmentShowHeader'; import GenerateQuestionMenu from './GenerateQuestionMenu'; +import MarketplaceSnapshotBanner from './MarketplaceSnapshotBanner'; import MarketplaceUpdateBanner from './MarketplaceUpdateBanner'; import NewQuestionMenu from './NewQuestionMenu'; import QuestionsManager from './QuestionsManager'; @@ -82,6 +83,8 @@ const AssessmentShowPage = (props: AssessmentShowPageProps): JSX.Element => { )} + + { + const { t } = useTranslation(); + + // An assessment the marketplace does not own carries no version at all. + if (!version) return null; + // A null vintage is the listing's working copy, which is exactly what an admin is meant to edit. + // Kept as its own guard rather than an optional chain: `version?.publishedAt === null` is false + // for an absent version, so the two conditions do not collapse into one. + if (version.publishedAt === null) return null; + + return ( + + + {t(translations.marketplaceSnapshotWarning)} + + + {version.sourceAssessmentUrl ? ( + + {t(translations.marketplaceSnapshotSourceLink)} + + ) : ( + + {t(translations.marketplaceSnapshotSourceMissing)} + + )} + + ); +}; + +export default MarketplaceSnapshotBanner; diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowPage.test.tsx b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowPage.test.tsx index 31937d7c3bd..dce462b8c97 100644 --- a/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowPage.test.tsx +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/AssessmentShowPage.test.tsx @@ -71,6 +71,11 @@ describe('', () => { expect(await page.findByText('Source Assessment')).toBeVisible(); expect(page.queryByText('Live')).not.toBeInTheDocument(); + // Editing the working copy is the point, so it must not be warned against. The chip assertion + // above is the async gate: once it is up, the banner has had its chance to render. + expect( + page.queryByText(/frozen at its publication date/), + ).not.toBeInTheDocument(); }); it('shows no marketplace chip outside the container', async () => { @@ -79,5 +84,28 @@ describe('', () => { expect(await page.findByText(baseAssessment.title)).toBeVisible(); expect(page.queryByText(/2026/)).not.toBeInTheDocument(); expect(page.queryByText('Source Assessment')).not.toBeInTheDocument(); + expect( + page.queryByText(/frozen at its publication date/), + ).not.toBeInTheDocument(); + }); + + // The show page is the only route to a snapshot, so the warning has to reach it through the page, + // not merely render in isolation. + it('warns on the page when the assessment is a published snapshot', async () => { + const page = renderWith({ + listingId: 7, + publishedAt: '2026-07-24T07:04:00Z', + source: 'MP Allowlist Source Course', + latest: true, + listed: true, + sourceAssessmentUrl: 'http://origin.lvh.me/courses/3/assessments/9', + }); + + expect( + await page.findByText(/frozen at its publication date/), + ).toBeInTheDocument(); + expect( + page.getByRole('link', { name: 'Open source assessment' }), + ).toHaveAttribute('href', 'http://origin.lvh.me/courses/3/assessments/9'); }); }); diff --git a/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/MarketplaceSnapshotBanner.test.tsx b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/MarketplaceSnapshotBanner.test.tsx new file mode 100644 index 00000000000..7f261798028 --- /dev/null +++ b/client/app/bundles/course/assessment/pages/AssessmentShow/__test__/MarketplaceSnapshotBanner.test.tsx @@ -0,0 +1,81 @@ +import { render, RenderResult } from 'test-utils'; +import { MarketplaceVersionData } from 'types/course/assessment/assessments'; + +import MarketplaceSnapshotBanner from '../MarketplaceSnapshotBanner'; + +/** + * `NotificationPopup` mounts inside `I18nProvider` (see `Providers`), so its presence is the signal + * that the provider resolved its messages and the banner has had its chance to render. Without this + * gate an absence assertion passes vacuously, by running before anything has mounted at all. + */ +const settle = (page: RenderResult): Promise => + page.findByLabelText(/Notifications/); + +const SOURCE_URL = 'http://origin.lvh.me/courses/3/assessments/9'; + +const snapshot = ( + overrides: Partial = {}, +): MarketplaceVersionData => ({ + listingId: 7, + publishedAt: '2026-07-24T07:04:00Z', + source: 'MP Allowlist Source Course', + latest: true, + listed: true, + sourceAssessmentUrl: SOURCE_URL, + ...overrides, +}); + +describe('', () => { + it('warns that a snapshot is frozen and sends the admin to the source assessment', async () => { + const page = render(); + + expect( + await page.findByText(/frozen at its publication date/), + ).toBeInTheDocument(); + // `role="alert"` is what the two absence assertions below query on, so pin it here. + expect(page.getByRole('alert')).toBeInTheDocument(); + + // `href`, not `to`: a cross-instance absolute url must not be routed as an in-app path. + const link = page.getByRole('link', { name: 'Open source assessment' }); + expect(link).toHaveAttribute('href', SOURCE_URL); + }); + + // An orphaned listing has no source to open yet. Saying so beats a dead or absent link. + it('explains the missing source instead of linking when the listing is orphaned', async () => { + const page = render( + , + ); + + expect( + await page.findByText(/one is being rebuilt from this version/), + ).toBeInTheDocument(); + expect(page.queryByRole('link')).not.toBeInTheDocument(); + }); + + // `publishedAt === null` is the working copy, which is exactly what an admin is meant to edit — + // the same discriminator MarketplaceVersionChip uses to label it "Source Assessment". + it('renders nothing for the listing working copy', async () => { + const page = render( + , + ); + + await settle(page); + + expect(page.queryByRole('alert')).not.toBeInTheDocument(); + }); + + it('renders nothing for an assessment the marketplace does not own', async () => { + const page = render(); + + await settle(page); + + expect(page.queryByRole('alert')).not.toBeInTheDocument(); + }); +}); diff --git a/client/app/bundles/course/assessment/translations.ts b/client/app/bundles/course/assessment/translations.ts index 35cb4e9bd45..19ffbe54d0e 100644 --- a/client/app/bundles/course/assessment/translations.ts +++ b/client/app/bundles/course/assessment/translations.ts @@ -277,6 +277,20 @@ const translations = defineMessages({ id: 'course.assessments.index.marketplaceSearchText', defaultMessage: 'Search by assessment title or source course', }, + marketplaceSnapshotWarning: { + id: 'course.assessments.show.marketplaceSnapshotWarning', + defaultMessage: + 'This is a published version, frozen at its publication date. Editing it silently changes what courses copy from the marketplace without publishing a new version. Make changes on the source assessment instead, then publish a new version.', + }, + marketplaceSnapshotSourceLink: { + id: 'course.assessments.show.marketplaceSnapshotSourceLink', + defaultMessage: 'Open source assessment', + }, + marketplaceSnapshotSourceMissing: { + id: 'course.assessments.show.marketplaceSnapshotSourceMissing', + defaultMessage: + 'This listing has no source assessment right now — one is being rebuilt from this version.', + }, requirements: { id: 'course.assessment.show.requirements', defaultMessage: 'Requirements', diff --git a/client/app/types/course/assessment/assessments.ts b/client/app/types/course/assessment/assessments.ts index 0e901c93d61..669553a9522 100644 --- a/client/app/types/course/assessment/assessments.ts +++ b/client/app/types/course/assessment/assessments.ts @@ -48,6 +48,13 @@ export interface MarketplaceVersionData { * being served from merely the newest one. True for a published orphan, which still serves. */ listed: boolean; + /** + * Where to edit the content this snapshot froze. Present only on the show page, and only for a + * snapshot — on the working copy the source assessment is the page you are already on. Null when + * the listing is orphaned and its rebuilt copy has not landed. Absolute, because the source may + * live on another instance. + */ + sourceAssessmentUrl?: string | null; } export interface AssessmentListData extends AssessmentActionsData { diff --git a/spec/controllers/course/assessment/assessments_marketplace_spec.rb b/spec/controllers/course/assessment/assessments_marketplace_spec.rb index a679b2c8276..198502401b6 100644 --- a/spec/controllers/course/assessment/assessments_marketplace_spec.rb +++ b/spec/controllers/course/assessment/assessments_marketplace_spec.rb @@ -253,7 +253,7 @@ def show_for(target_course, target_assessment) label = response.parsed_body['marketplaceVersion'] expect(label.keys).to contain_exactly('listingId', 'publishedAt', 'source', 'latest', - 'listed') + 'listed', 'sourceAssessmentUrl') expect(label['listingId']).to eq(listing.id) expect(label['source']).to eq('MP Allowlist Source Course') expect(label['latest']).to be(true) @@ -270,6 +270,14 @@ def show_for(target_course, target_assessment) expect(label['latest']).to be(false) end + # The source assessment IS this page, so there is nothing to link to. Key absent, not null, + # so the banner's trigger never has to special-case it. + it 'omits the source link on the working copy, which is the page itself' do + show_for(container, working_copy) + + expect(response.parsed_body['marketplaceVersion']).not_to have_key('sourceAssessmentUrl') + end + it 'withholds publishing from a snapshot, which is already an existing listing content' do show_for(container, snapshot) @@ -305,6 +313,72 @@ def show_for(target_course, target_assessment) expect(response.parsed_body).not_to have_key('marketplaceVersion') end + + # The one field the index badge never carries. A snapshot is frozen, so the only useful + # action is to go edit the assessment it was cut from. + it 'points a snapshot at the assessment it was published from' do + show_for(container, snapshot) + + expect(response.parsed_body['marketplaceVersion']['sourceAssessmentUrl']). + to eq("http://#{instance.host}/courses/#{working_copy.course_id}/" \ + "assessments/#{working_copy.id}") + end + + # `Instance#host_options` exists for this: a controller's `url_options` always supplies the port the + # request arrived on, which behind any proxy is not the port the app is served on. + it 'builds the link on the instance host, not the port the request arrived on' do + request.host = 'localhost:3999' + + show_for(container, snapshot) + + expect(response.parsed_body['marketplaceVersion']['sourceAssessmentUrl']). + to start_with("http://#{instance.host}/") + end + + # The regression `without_tenant` exists for. Viewing a container snapshot means the request + # is tenanted to the container's instance, so a tenant-scoped `Course` lookup on a source + # published from elsewhere returns nil rather than raising - dropping the link silently. + it 'resolves a source assessment published from another instance' do + origin_instance = create(:instance) + origin_course = ActsAsTenant.with_tenant(origin_instance) { create(:course) } + origin_assessment = ActsAsTenant.with_tenant(origin_instance) do + create(:assessment, course: origin_course) + end + cross_listing = create(:course_assessment_marketplace_listing, + authoring_assessment: origin_assessment, + publisher: create(:user)) + cross_snapshot = create(:assessment, course: container) + create(:course_assessment_marketplace_listing_version, + listing: cross_listing, assessment: cross_snapshot, + published_at: 2.days.ago.change(usec: 0), + published_by: cross_listing.publisher). + tap { |cut| cross_listing.update!(current_version: cut) } + + show_for(container, cross_snapshot) + + expect(response.parsed_body['marketplaceVersion']['sourceAssessmentUrl']). + to eq("http://#{origin_instance.host}/courses/#{origin_course.id}/" \ + "assessments/#{origin_assessment.id}") + end + + # An orphaned listing has nothing to link at until its rebuild lands. The key is still + # emitted so the client can tell "no source" from "not a snapshot". + it 'emits a null link for an orphaned listing, whose source was deleted' do + orphan_listing = create(:course_assessment_marketplace_listing) + orphan_snapshot = create(:assessment, course: container) + create(:course_assessment_marketplace_listing_version, + listing: orphan_listing, assessment: orphan_snapshot, + published_at: 4.days.ago.change(usec: 0), + published_by: orphan_listing.publisher). + tap { |cut| orphan_listing.update!(current_version: cut) } + orphan_listing.update!(authoring_assessment: nil) + + show_for(container, orphan_snapshot) + + label = response.parsed_body['marketplaceVersion'] + expect(label).to have_key('sourceAssessmentUrl') + expect(label['sourceAssessmentUrl']).to be_nil + end end # Previewers are enrolled into the container as managers. The context is admin-only navigation, diff --git a/spec/models/instance_spec.rb b/spec/models/instance_spec.rb index ad5f4724f52..b8f4de25a36 100644 --- a/spec/models/instance_spec.rb +++ b/spec/models/instance_spec.rb @@ -210,6 +210,35 @@ end end + describe '#host_options' do + around do |example| + orig_default_host = Application::Application.config.x.default_host + example.run + ensure + Application::Application.config.x.default_host = orig_default_host + end + + subject(:instance) { build(:instance, host: 'tenant.coursemology.org') } + + context 'when the host carries no port' do + before { Application::Application.config.x.default_host = 'coursemology.org' } + + it 'names no port, leaving the default for the protocol' do + expect(instance.host_options).to eq(host: 'tenant.coursemology.org', port: nil) + end + end + + # The development shape: the served port arrives through `default_host`, and `#host` rewrites it + # onto every tenant. + context 'when the host carries a port' do + before { Application::Application.config.x.default_host = 'lvh.me:8080' } + + it 'names the port separately from the host' do + expect(instance.host_options).to eq(host: 'tenant.lvh.me', port: '8080') + end + end + end + let(:instance) { create(:instance) } with_tenant(:instance) do describe '.active_course_count' do