fix(marketplace): track adoptions through a course roll-forward

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
     )

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

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

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.