diff --git a/app/models/course/assessment.rb b/app/models/course/assessment.rb index 97991da327..95db684079 100644 --- a/app/models/course/assessment.rb +++ b/app/models/course/assessment.rb @@ -295,29 +295,40 @@ def csv_downloadable? questions.any?(&:csv_downloadable?) end - # Records +duplicate+, a copy of this assessment, as an adoption of this assessment's marketplace - # listing. Every copy of a listed assessment is an adoption, whichever duplication path produced - # it, so this is called by the duplication services rather than by the marketplace's own job. + # Records +duplicate+, a copy of this assessment, against the marketplace listing its content came + # from. Called by the duplication services rather than by the marketplace's own job, because a copy + # of an adopted assessment is made by ordinary duplication -- rolling a course forward for the new + # semester, copying a selection of objects across -- and a copy the listing cannot reach is a copy + # it can never send a version reminder to. + # + # Keyed off this assessment's own ADOPTION ROW, so only content that came through the marketplace + # propagates. An assessment that AUTHORS a listing is deliberately not a source here: copies of it + # are the publisher's own -- their course rolled forward, or the assessment handed to a colleague + # directly -- made without anyone choosing the listing, so counting them would let a listing nobody + # adopted show a rising adoption count. # # The listing itself is never carried over -- +initialize_duplicate+ below does not duplicate the - # +marketplace_listing+ association -- so a copy always starts out unlisted. + # +marketplace_listing+ association -- so a copy always starts out unlisted, which is exactly why a + # copy of a copy has to find its listing through the source's adoption row rather than its own. # # @param [Course::Assessment] duplicate The saved copy of this assessment. # @param [Course] destination_course The course the copy was duplicated into. # @param [User] current_user The user who triggered the duplication. def record_marketplace_adoption(duplicate, destination_course, current_user) - return unless marketplace_listing&.published? # 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? + source_adoption = Course::Assessment::Marketplace::Adoption.find_by(duplicated_assessment_id: id) + return if source_adoption.nil? + Course::Assessment::Marketplace::Adoption.create!( - listing: marketplace_listing, + listing: source_adoption.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, + # The vintage the SOURCE holds, not what the listing currently serves: crediting a rolled-forward + # copy with the latest version would silently mark stale content as up to date. + adopted_version_at: source_adoption.adopted_version_at, creator: current_user, updater: current_user ) diff --git a/app/services/course/duplication/base_service.rb b/app/services/course/duplication/base_service.rb index 0e32a99a25..c57d090aa1 100644 --- a/app/services/course/duplication/base_service.rb +++ b/app/services/course/duplication/base_service.rb @@ -32,8 +32,9 @@ def initialize_duplicator(*) # Hands every duplicated assessment its own copy so it can record a marketplace adoption. Copies # made outside +Course::Assessment::Marketplace::DuplicationJob+ -- selected object duplications - # and full course duplications that happen to carry a listed assessment along -- are adoptions - # too, and the listing has to know about them to reach every course holding a copy. + # and full course duplications that happen to carry an ADOPTED assessment along, most often a + # course rolled forward for a new batch of students -- are adoptions too, and the listing has to know + # about them to reach every course holding a copy. # # This sweep lives in the duplication service rather than in a model's +after_duplicate_save+ # hook because that hook only runs for the top-level objects of an object duplication, and never diff --git a/spec/jobs/course/assessment/marketplace/duplication_job_spec.rb b/spec/jobs/course/assessment/marketplace/duplication_job_spec.rb index 8328fd7a36..2a613aee4d 100644 --- a/spec/jobs/course/assessment/marketplace/duplication_job_spec.rb +++ b/spec/jobs/course/assessment/marketplace/duplication_job_spec.rb @@ -362,19 +362,6 @@ def backdate_current_version(record = listing) end end - # Grandchildren-excluded: an adoption is written for copies of a *listed* assessment, and a copy - # is never itself listed, so duplicating an already-adopted copy writes no second-generation row. - it 'does not write an adoption for an ordinary ObjectDuplicationService copy' do - run - copy = destination_course.assessments.order(:created_at).last - third_course = create(:course) - expect do - Course::Duplication::ObjectDuplicationService.duplicate_objects( - destination_course, third_course, copy, current_user: user - ) - end.not_to change(Course::Assessment::Marketplace::Adoption, :count) - end - # The sidebar entry point sends no tab, and a tab from another course can be sent by an # out-of-date URL. Neither may leave the redirect pointing at a tab the user cannot open. describe 'when the requested tab is absent or foreign' do @@ -429,11 +416,11 @@ def run_with_tab(tab_id) end end - # A listed assessment can leave its course by paths that do not go through this job: an - # instructor duplicating selected objects, or a full course duplication that carries the - # listed assessment along. Those copies must obey the same two rules as the job's copies -- - # the listing stays singular, and the destination course is recorded as an adopter. - describe 'manual duplication of a listed assessment' do + # Marketplace content leaves a course by paths that do not go through this job: an instructor + # duplicating selected objects, or a full course duplication carrying the assessment along. Both + # must keep the listing singular, and both must record the destination as an adopter -- but only + # for content that came THROUGH the marketplace, which is what separates these two describes. + describe 'manual duplication of the assessment that authors a listing' do let(:manual_destination) { create(:course) } before { listing } @@ -460,14 +447,11 @@ def duplicate_whole_course expect(copy.marketplace_listing).to be_nil end - it 'records the destination course as an adopter' do - copy = nil - expect { copy = duplicate_selected_objects }. - to change(Course::Assessment::Marketplace::Adoption, :count).by(1) - adoption = Course::Assessment::Marketplace::Adoption.order(:id).last - expect(adoption.listing).to eq(listing) - expect(adoption.destination_course).to eq(manual_destination) - expect(adoption.duplicated_assessment).to eq(copy) + # The publisher handing their own assessment to somebody directly bypassed the marketplace + # entirely, so the marketplace has no adoption to record. + it 'records no adoption' do + expect { duplicate_selected_objects }. + not_to change(Course::Assessment::Marketplace::Adoption, :count) end end @@ -481,13 +465,65 @@ def duplicate_whole_course expect(new_course.assessments.map(&:marketplace_listing)).to all(be_nil) end - it 'records the new course as an adopter' do + # The publisher rolling their own course forward. Recorded, this would let a listing nobody + # has adopted show an adoption count that climbs by one every semester its author re-runs. + it 'records no adoption' do + expect { duplicate_whole_course }. + not_to change(Course::Assessment::Marketplace::Adoption, :count) + end + end + end + + # The other half: an ADOPTED copy carried along by ordinary duplication. This is the semester + # roll-forward, and the copy it makes both counts as a course using the listing and stays + # reachable by the listing's version reminders. + describe 'manual duplication of an adopted copy' do + let(:adopting_course) { create(:course) } + + # The real import path, so the copy carries a genuine adoption row rather than a hand-built one. + def adopt + described_class.perform_now([listing.id], adopting_course, + adopting_course.assessment_categories.first.tabs.first.id, + current_user: user) + adopting_course.assessments.order(:created_at).last + end + + context 'when duplicating selected objects' do + it 'records the destination course as an adopter of the same listing' do + adopted = adopt + onward_destination = create(:course) + copy = nil + + expect do + copy = Course::Duplication::ObjectDuplicationService.duplicate_objects( + adopting_course, onward_destination, adopted, current_user: user + ) + end.to change { listing.reload.adoption_count }.from(1).to(2) + + adoption = Course::Assessment::Marketplace::Adoption. + find_by(duplicated_assessment_id: copy.id) + expect(adoption.listing).to eq(listing) + expect(adoption.destination_course).to eq(onward_destination) + # The vintage its source held, so the copy is told it is behind once a newer version lands. + expect(adoption.adopted_version_at). + to be_within(1.second).of(listing.current_version.published_at) + end + end + + context 'when duplicating the whole course' do + it 'records the new course as an adopter of the same listing' do + adopt + new_course = nil - expect { new_course = duplicate_whole_course }. - to change(Course::Assessment::Marketplace::Adoption, :count).by(1) - adoption = Course::Assessment::Marketplace::Adoption.order(:id).last + expect do + new_course = Course::Duplication::CourseDuplicationService.duplicate_course( + adopting_course, current_user: user, new_title: "#{adopting_course.title} copy" + ) + end.to change { listing.reload.adoption_count }.from(1).to(2) + + adoption = Course::Assessment::Marketplace::Adoption. + find_by(destination_course_id: new_course.id) expect(adoption.listing).to eq(listing) - expect(adoption.destination_course).to eq(new_course) end end end diff --git a/spec/models/course/assessment_spec.rb b/spec/models/course/assessment_spec.rb index 8fa97e000d..21eb341585 100644 --- a/spec/models/course/assessment_spec.rb +++ b/spec/models/course/assessment_spec.rb @@ -547,6 +547,79 @@ end end + # Only content that came THROUGH the marketplace propagates: a copy descended from an adoption + # stays reachable by version reminders and counts as a course using the listing, while copies of + # the publisher's own authoring assessment are not adoptions at all. + describe '#record_marketplace_adoption' do + let(:destination_course) { create(:course) } + let(:copy) { create(:assessment, course: destination_course) } + let(:duplicating_user) { create(:user) } + let(:listing) { create(:course_assessment_marketplace_listing, :versioned, course: course) } + + def record(source, destination = destination_course) + source.record_marketplace_adoption(copy, destination, duplicating_user) + Course::Assessment::Marketplace::Adoption.find_by(duplicated_assessment_id: copy.id) + end + + # Nobody chose the listing here — the publisher is duplicating their own assessment, whether + # into next semester's course or a colleague's. Recording it would let a listing with no + # adopters at all show an adoption count that climbs every term. + it 'records nothing for a copy of the assessment that authors the listing' do + expect(record(listing.authoring_assessment)).to be_nil + end + + it 'records nothing for an assessment with neither a listing nor an adoption' do + expect(record(create(:assessment, course: course))).to be_nil + end + + # The roll-forward case: next semester's course carries a copy of a copy. That source holds no + # listing of its own, so the chain runs through its adoption row -- without it the new copy + # drops out of the listing's reach and never sees a version reminder again. + context 'when the source is itself an adopted copy' do + let(:adopted) { create(:assessment, course: create(:course)) } + let!(:source_adoption) do + create(:course_assessment_marketplace_adoption, + listing: listing, destination_course: adopted.course, + duplicated_assessment: adopted, adopted_version_at: 30.days.ago.change(usec: 0)) + end + + it 'records the copy against the same listing' do + adoption = record(adopted) + + expect(adoption).to be_present + expect(adoption.listing).to eq(listing) + expect(adoption.destination_course).to eq(destination_course) + end + + # The copy holds whatever vintage its source held, NOT the listing's latest -- crediting it + # with the served version would silently mark a stale copy as up to date. + it 'inherits the vintage its source holds rather than the served one' do + adoption = record(adopted) + + expect(adoption.adopted_version_at). + to be_within(1.second).of(source_adoption.adopted_version_at) + end + + # Unlisting is a visibility decision. Severing the chain there would strand copies that + # already exist and can still be updated. + it 'records the row even once the listing is off the marketplace' do + listing.update!(published: false) + + expect(record(adopted)).to be_present + end + + # Publishing duplicates the source INTO the container to cut a snapshot. That is the listing + # growing a version, not a course adopting it. + it 'records nothing when the copy lands in the marketplace container' do + container = ActsAsTenant.without_tenant do + Course::Assessment::Marketplace::PreviewContainerService.container_course + end + + expect(record(adopted, container)).to be_nil + end + end + end + describe 'in-transaction marketplace authoring re-point' do # The re-point enqueues nothing, but the env default is `:background_thread` — a real thread # sharing this example's connection — and these examples assert on row counts in the container.