Merge branch 'main' into 1130_store-and-return-scratch-data

Author: mwtrew

.clabot

@@ -1,5 +1,6 @@
 {
   "contributors": [
+    "abcampo-iry",
     "adrian-rpf",
     "ce-rpf",
     "chris.lowis@gofreerange.com",
@@ -12,6 +13,7 @@
     "cursoragent",
     "danhalson",
     "dependabot[bot]",
+    "DNR500",
     "fspeirs",
     "grega",
     "IzzySmillie",

app/controllers/api/lessons_controller.rb

@@ -2,6 +2,8 @@
 
 module Api
   class LessonsController < ApiController
+    include RemixSelection
+
     before_action :authorize_user, except: %i[index show]
     before_action :verify_school_class_belongs_to_school, only: :create
     load_and_authorize_resource :lesson
@@ -82,24 +84,17 @@ def verify_school_class_belongs_to_school
     end
 
     def user_remixes(lessons)
-      lessons.map do |lesson|
-        next nil unless lesson&.project&.remixes&.any?
-
-        user_remix(lesson)
-      end
+      lessons.map { |lesson| user_remix(lesson) }
     end
 
     def user_remix(lesson)
-      remixes = lesson&.project&.remixes
+      return nil unless lesson&.project&.remixes&.any?
 
-      remixes = remixes
-                .where(user_id: current_user.id)
-                .accessible_by(current_ability)
-                .order(created_at: :asc)
-
-      remixes = remixes.includes(school_project: :feedback) if current_user&.school_student?(school)
-
-      remixes.first
+      remix_for_user(
+        lesson.project,
+        current_user,
+        include_feedback: current_user&.school_student?(school)
+      )
     end
 
     def lesson_params

app/controllers/api/projects/remixes_controller.rb

@@ -3,6 +3,8 @@
 module Api
   module Projects
     class RemixesController < ApiController
+      include RemixSelection
+
       before_action :authorize_user
       load_and_authorize_resource :school, only: :index
       before_action :load_and_authorize_remix, only: %i[show show_identifier]
@@ -44,7 +46,9 @@ def project
       end
 
       def load_and_authorize_remix
-        @project = Project.find_by!(remixed_from_id: project.id, user_id: current_user&.id)
+        @project = remix_for_user(project, current_user)
+        raise ActiveRecord::RecordNotFound unless @project
+
         authorize! :show, @project
       end
 

app/controllers/concerns/remix_selection.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+module RemixSelection
+  extend ActiveSupport::Concern
+
+  def remix_for_user(project, user, include_feedback: false)
+    return nil if user.nil?
+
+    query = Project.where(remixed_from_id: project.id, user_id: user.id)
+                   .order(created_at: :asc)
+                   .accessible_by(current_ability)
+
+    query = query.includes(school_project: :feedback) if include_feedback
+
+    query.first
+  end
+end

lib/tasks/seeds_helper.rb

@@ -32,7 +32,7 @@ def create_school(creator_id, school_id = nil)
       # Country-specific required fields
       school.reference = format('%06d', rand(100_000..999_999)) if country_code == 'GB'
       if country_code == 'US'
-        school.district_nces_id = format('%012d', rand(10**12))
+        school.district_nces_id = format('%07d', rand(10**7))
         school.district_name = Faker::Address.community
       end
       school.school_roll_number = "#{rand(10_000..99_999)}#{('A'..'Z').to_a.sample}" if country_code == 'IE'

lib/tasks/test_seeds.rake

@@ -44,32 +44,31 @@ namespace :test_seeds do
 
   desc 'Create a school with lessons and students'
   task create: :environment do
-    if School.find_by(code: TEST_SCHOOL)
+    if School.exists?(id: TEST_SCHOOL)
       puts "Test school (#{TEST_SCHOOL}) already exists, run the destroy_seed_data task to start over)."
-      return
-    end
-
-    ActiveRecord::Base.transaction do
-      Rails.logger.info 'Attempting to seed data...'
-      creator_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe])
-      teacher_id = ENV.fetch('SEEDING_TEACHER_ID', TEST_USERS[:john_doe])
+    else
+      ActiveRecord::Base.transaction do
+        Rails.logger.info 'Attempting to seed data...'
+        creator_id = ENV.fetch('SEEDING_CREATOR_ID', TEST_USERS[:jane_doe])
+        teacher_id = ENV.fetch('SEEDING_TEACHER_ID', TEST_USERS[:john_doe])
 
-      school = create_school(creator_id, TEST_SCHOOL)
-      verify_school(school)
-      assign_a_teacher(teacher_id, school)
+        school = create_school(creator_id, TEST_SCHOOL)
+        verify_school(school)
+        assign_a_teacher(teacher_id, school)
 
-      # for each of the owner and teacher, create a class and assign students
-      [creator_id, teacher_id].each do |user_id|
-        teacher_name = user_id == creator_id ? 'Jane Doe' : 'John Doe'
-        school_class = create_school_class(user_id, school, "#{teacher_name}'s Class", "A class for #{teacher_name}'s students")
-        assign_students(school_class, school)
+        # for each of the owner and teacher, create a class and assign students
+        [creator_id, teacher_id].each do |user_id|
+          teacher_name = user_id == creator_id ? 'Jane Doe' : 'John Doe'
+          school_class = create_school_class(user_id, school, "#{teacher_name}'s Class", "A class for #{teacher_name}'s students")
+          assign_students(school_class, school)
 
-        lessons = create_lessons(user_id, school, school_class)
-        lessons.each do |lesson|
-          create_project(user_id, school, lesson, 'print("Hello World!")')
+          lessons = create_lessons(user_id, school, school_class)
+          lessons.each do |lesson|
+            create_project(user_id, school, lesson, 'print("Hello World!")')
+          end
         end
+        Rails.logger.info 'Done...'
       end
-      Rails.logger.info 'Done...'
     end
   end
 end

spec/lib/test_seeds_spec.rb

@@ -36,8 +36,10 @@
 
   describe ':seed_a_school_with_lessons_and_students' do
     let(:task) { Rake::Task['test_seeds:create'] }
+    let(:seed_country_code) { nil }
 
     before do
+      allow(Faker::Address).to receive(:country_code).and_return(seed_country_code) if seed_country_code
       task.reenable
       task.invoke
     end
@@ -82,6 +84,20 @@
       expect(ClassTeacher.where(teacher_id:).length).to eq(1)
     end
 
+    it 'is idempotent' do
+      school = School.find_by!(creator_id:)
+      owner_class = SchoolClass.joins(:teachers).find_by!(school_id: school.id, teachers: { teacher_id: creator_id })
+      teacher_class = SchoolClass.joins(:teachers).find_by!(school_id: school.id, teachers: { teacher_id: })
+
+      expect do
+        task.reenable
+        task.invoke
+      end.not_to change { [SchoolClass.where(school_id: school.id).count, Lesson.where(school_id: school.id).count, Project.where(school_id: school.id).count] }
+
+      expect(owner_class.reload.lessons.count).to eq(2)
+      expect(teacher_class.reload.lessons.count).to eq(2)
+    end
+
     it 'assigns students' do
       school_id = School.find_by(creator_id:).id
       school_class_id = SchoolClass.find_by(school_id:).id
@@ -90,5 +106,20 @@
       expect(Role.student.where(user_id: student_2, school_id:)).to exist
       expect(ClassStudent.where(student_id: student_2, school_class_id:)).to exist
     end
+
+    context 'when the seeded school is in the US' do
+      let(:seed_country_code) { 'US' }
+
+      it 'creates a valid school and owner lessons' do
+        school = School.find_by(creator_id:)
+        school_class = SchoolClass.joins(:teachers).find_by(school_id: school.id, teachers: { teacher_id: creator_id })
+
+        expect(school).to be_valid
+        expect(school.district_nces_id).to match(/\A\d{7}\z/)
+        expect(school.district_name).to be_present
+        expect(school_class).not_to be_nil
+        expect(Lesson.where(school_id: school.id, school_class_id: school_class.id).length).to eq(2)
+      end
+    end
   end
 end

spec/requests/projects/remix_spec.rb

@@ -71,6 +71,24 @@
 
         expect(response).to have_http_status(:not_found)
       end
+
+      context 'when multiple remixes exist for the same user and project' do
+        let!(:oldest_remix) do
+          create(:project, remixed_from_id: original_project.id, user_id: authenticated_user.id,
+                           created_at: 2.days.ago, updated_at: 2.days.ago)
+        end
+
+        before do
+          create(:project, remixed_from_id: original_project.id, user_id: authenticated_user.id,
+                           created_at: 1.hour.from_now, updated_at: 1.hour.from_now)
+        end
+
+        it 'returns the oldest created remix' do
+          get("/api/projects/#{original_project.identifier}/remix", headers:)
+
+          expect(response.parsed_body['identifier']).to eq(oldest_remix.identifier)
+        end
+      end
     end
 
     describe('#show_identifier') do
@@ -100,6 +118,23 @@
         get("/api/projects/#{original_project.identifier}/remix/identifier", headers:)
         expect(response).to have_http_status(:not_found)
       end
+
+      context 'when multiple remixes exist for the same user and project' do
+        let!(:oldest_remix) do
+          create(:project, remixed_from_id: original_project.id, user_id: authenticated_user.id,
+                           created_at: 2.days.ago, updated_at: 2.days.ago)
+        end
+
+        before do
+          create(:project, remixed_from_id: original_project.id, user_id: authenticated_user.id,
+                           created_at: 1.hour.from_now, updated_at: 1.hour.from_now)
+        end
+
+        it 'returns the identifier of the oldest created remix' do
+          get("/api/projects/#{original_project.identifier}/remix/identifier", headers:)
+          expect(response.parsed_body['identifier']).to eq(oldest_remix.identifier)
+        end
+      end
     end
 
     describe '#create' do