From 9a3d966608a5c6917a56d54ba0c0065760b31ba3 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Thu, 26 Feb 2026 10:57:45 +0000 Subject: [PATCH 1/2] Make load_and_authorize_remix deterministic. This commit changes RemixesController#load_and_authorize_remix to deterministically always return the most recent Project record based on created_at and, secondarily, updated_at dates. While, in principle, there should not be multiple records where (remixed_from_id:, user_id:) are the same, in practice there are and there is no database constraint to prevent there being. The previous code would nondeterministically return one of possibly many records, causing downstream errors in Experience CS since each would have a different identifier, not matching the one stored in Experience CS. Overarching epic: https://github.com/RaspberryPiFoundation/experience-cs/issues/1826 --- .../api/projects/remixes_controller.rb | 4 ++- spec/requests/projects/remix_spec.rb | 35 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb index 18bdb3867..3b7a70d68 100644 --- a/app/controllers/api/projects/remixes_controller.rb +++ b/app/controllers/api/projects/remixes_controller.rb @@ -44,7 +44,9 @@ def project end def load_and_authorize_remix - @project = Project.find_by!(remixed_from_id: project.id, user_id: current_user&.id) + @project = Project.where(remixed_from_id: project.id, user_id: current_user&.id) + .order(created_at: :desc, updated_at: :desc) + .first! authorize! :show, @project end diff --git a/spec/requests/projects/remix_spec.rb b/spec/requests/projects/remix_spec.rb index 50edde286..923ecd7f4 100644 --- a/spec/requests/projects/remix_spec.rb +++ b/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 + before 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 + + let!(:newer_remix) 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 most recently created remix' do + get("/api/projects/#{original_project.identifier}/remix", headers:) + + expect(response.parsed_body['identifier']).to eq(newer_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 + before 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 + + let!(:newer_remix) 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 most recently created remix' do + get("/api/projects/#{original_project.identifier}/remix/identifier", headers:) + expect(response.parsed_body['identifier']).to eq(newer_remix.identifier) + end + end end describe '#create' do From 4cf245de2a91cda7e2a0b5e77bc54ddb3fa2ada7 Mon Sep 17 00:00:00 2001 From: Fraser Speirs Date: Fri, 13 Mar 2026 14:26:02 +0000 Subject: [PATCH 2/2] Factor out Remix selection into a concern. This commit factors out the remix selection logic that was common between LessonsController#user_remix and RemixesController#load_and_authorize_remix into a new concern called RemixSelection that is used in both controllers. RemixSelection instantiates the rule that, for any user, the canonical remix of a project is the remix which was created first. Any subsequent remixes will not be accessible. --- app/controllers/api/lessons_controller.rb | 23 ++++++++----------- .../api/projects/remixes_controller.rb | 8 ++++--- app/controllers/concerns/remix_selection.rb | 17 ++++++++++++++ spec/requests/projects/remix_spec.rb | 16 ++++++------- 4 files changed, 39 insertions(+), 25 deletions(-) create mode 100644 app/controllers/concerns/remix_selection.rb diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 0d381e2ef..0f21d1260 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/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 diff --git a/app/controllers/api/projects/remixes_controller.rb b/app/controllers/api/projects/remixes_controller.rb index 3b7a70d68..e61a19667 100644 --- a/app/controllers/api/projects/remixes_controller.rb +++ b/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,9 +46,9 @@ def project end def load_and_authorize_remix - @project = Project.where(remixed_from_id: project.id, user_id: current_user&.id) - .order(created_at: :desc, updated_at: :desc) - .first! + @project = remix_for_user(project, current_user) + raise ActiveRecord::RecordNotFound unless @project + authorize! :show, @project end diff --git a/app/controllers/concerns/remix_selection.rb b/app/controllers/concerns/remix_selection.rb new file mode 100644 index 000000000..6375e63bf --- /dev/null +++ b/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 diff --git a/spec/requests/projects/remix_spec.rb b/spec/requests/projects/remix_spec.rb index 923ecd7f4..7be77aaa0 100644 --- a/spec/requests/projects/remix_spec.rb +++ b/spec/requests/projects/remix_spec.rb @@ -73,20 +73,20 @@ end context 'when multiple remixes exist for the same user and project' do - before 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 - let!(:newer_remix) do + 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 most recently created remix' do + it 'returns the oldest created remix' do get("/api/projects/#{original_project.identifier}/remix", headers:) - expect(response.parsed_body['identifier']).to eq(newer_remix.identifier) + expect(response.parsed_body['identifier']).to eq(oldest_remix.identifier) end end end @@ -120,19 +120,19 @@ end context 'when multiple remixes exist for the same user and project' do - before 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 - let!(:newer_remix) do + 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 most recently created remix' do + 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(newer_remix.identifier) + expect(response.parsed_body['identifier']).to eq(oldest_remix.identifier) end end end