diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index fa3fb391f..142845b6c 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -6,26 +6,60 @@ class AssetsController < ScratchController include ActiveStorage::SetCurrent skip_before_action :authorize_user, only: [:show] - skip_before_action :check_scratch_feature, only: [:show] + prepend_before_action :load_project_from_header, only: %i[show create] def show filename_with_extension = "#{params[:id]}.#{params[:format]}" - scratch_asset = ScratchAsset.find_by!(filename: filename_with_extension) - redirect_to scratch_asset.file.url(content_type: scratch_asset.file.content_type), allow_other_host: true + authorize! :show, @project_from_header + + scratch_asset = ScratchAsset.find_visible_to_project( + project: @project_from_header, + user: current_user, + filename: filename_with_extension + ) + raise ActiveRecord::RecordNotFound, 'Not Found' unless scratch_asset + + redirect_to scratch_asset.file.url(content_type: scratch_asset.response_content_type), allow_other_host: true end def create - begin - filename_with_extension = "#{params[:id]}.#{params[:format]}" - ScratchAsset.find_or_create_by!(filename: filename_with_extension) do |a| - a.file.attach(io: request.body, filename: filename_with_extension) + authorize! :show, @project_from_header + + filename_with_extension = "#{params[:id]}.#{params[:format]}" + scratch_asset = ScratchAsset.find_or_initialize_by( + project: @project_from_header, + uploaded_user_id: current_user.id, + filename: filename_with_extension + ) + + if scratch_asset.new_record? + begin + scratch_asset.save! + scratch_asset.file.attach(io: request.body, filename: filename_with_extension) + rescue ActiveRecord::RecordNotUnique + logger.info("Scratch asset already created during concurrent upload: #{filename_with_extension}") + ScratchAsset.find_by!( + project: @project_from_header, + uploaded_user_id: current_user.id, + filename: filename_with_extension + ) end - rescue ActiveRecord::RecordNotUnique => e - logger.error(e) end render json: { status: 'ok', 'content-name': params[:id] }, status: :created end + + private + + def load_project_from_header + identifier = request.headers['X-Project-ID'] + return render json: { error: 'X-Project-ID header is required' }, status: :bad_request if identifier.blank? + + @project_from_header = Project.find_by!( + identifier:, + project_type: Project::Types::CODE_EDITOR_SCRATCH + ) + end end end end diff --git a/app/controllers/api/scratch/projects_controller.rb b/app/controllers/api/scratch/projects_controller.rb index 4d90722ff..feaaf1dc9 100644 --- a/app/controllers/api/scratch/projects_controller.rb +++ b/app/controllers/api/scratch/projects_controller.rb @@ -3,6 +3,8 @@ module Api module Scratch class ProjectsController < ScratchController + include RemixSelection + skip_before_action :authorize_user, only: [:show] skip_before_action :check_scratch_feature, only: [:show] before_action :load_project, only: %i[show update] @@ -10,7 +12,7 @@ class ProjectsController < ScratchController before_action :ensure_create_is_a_remix, only: %i[create] def show - render json: @project.scratch_component.content + render json: scratch_project_content_with_stage_first(@project.scratch_component.content.to_h) end def create @@ -20,6 +22,16 @@ def create remix_params = create_params return render json: { error: I18n.t('errors.project.remixing.invalid_params') }, status: :bad_request if remix_params.dig(:scratch_component, :content).blank? + existing_remix = remix_for_user(original_project, current_user) + if existing_remix + scratch_component = existing_remix.scratch_component || existing_remix.build_scratch_component + scratch_component.content = scratch_content_params + existing_remix.save! + reassign_uploaded_scratch_assets(original_project:, remix_project: existing_remix) + + return render json: { status: 'ok', 'content-name': existing_remix.identifier }, status: :ok + end + remix_origin = request.origin || request.referer result = Project::CreateRemix.call( @@ -30,6 +42,7 @@ def create ) if result.success? + reassign_uploaded_scratch_assets(original_project:, remix_project: result[:project]) render json: { status: 'ok', 'content-name': result[:project].identifier }, status: :ok else render json: { error: result[:error] }, status: :bad_request @@ -68,6 +81,55 @@ def load_original_project(identifier) def scratch_content_params params.slice(:meta, :targets, :monitors, :extensions).to_unsafe_h end + + def scratch_project_content_with_stage_first(content) + targets = content['targets'] + return content unless targets.is_a?(Array) + + # Scratch expects the stage target first and can fail to load otherwise. + stage_targets, other_targets = targets.partition do |target| + target.is_a?(Hash) && (target['isStage'] || target[:isStage]) + end + return content if stage_targets.empty? + + content.merge('targets' => stage_targets + other_targets) + end + + def reassign_uploaded_scratch_assets(original_project:, remix_project:) + uploaded_user_id = current_user.id + return if skip_scratch_asset_reassignment?( + original_project:, + remix_project:, + uploaded_user_id: + ) + + ScratchAsset.where(project: original_project, uploaded_user_id:).find_each do |source_asset| + reassign_uploaded_scratch_asset( + source_asset:, + remix_project:, + uploaded_user_id: + ) + end + rescue StandardError => e + Sentry.capture_exception(e) + end + + def skip_scratch_asset_reassignment?(original_project:, remix_project:, uploaded_user_id:) + original_project.blank? || + remix_project.blank? || + uploaded_user_id.blank? || + original_project.id == remix_project.id + end + + def reassign_uploaded_scratch_asset(source_asset:, remix_project:, uploaded_user_id:) + if ScratchAsset.exists?(project: remix_project, uploaded_user_id:, filename: source_asset.filename) + source_asset.destroy! + else + source_asset.update!(project: remix_project) + end + rescue ActiveRecord::RecordNotUnique + source_asset.destroy! + end end end end diff --git a/app/models/project.rb b/app/models/project.rb index c28650ffb..db745bee2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -86,6 +86,20 @@ def scratch_project? project_type == Types::CODE_EDITOR_SCRATCH end + def self_and_ancestors + projects = [] + current_project = self + seen_ids = [] + + while current_project && seen_ids.exclude?(current_project.id) + projects << current_project + seen_ids << current_project.id + current_project = current_project.parent + end + + projects + end + private def check_unique_not_null diff --git a/app/models/scratch_asset.rb b/app/models/scratch_asset.rb index 431a10fde..e1347089a 100644 --- a/app/models/scratch_asset.rb +++ b/app/models/scratch_asset.rb @@ -1,7 +1,47 @@ # frozen_string_literal: true class ScratchAsset < ApplicationRecord - validates :filename, presence: true, uniqueness: true - + belongs_to :project, optional: true has_one_attached :file + + validates :filename, presence: true, uniqueness: { scope: %i[project_id uploaded_user_id] } + validates :uploaded_user_id, absence: true, if: :global? + validates :uploaded_user_id, presence: true, unless: :global? + validate :belongs_to_scratch_project + + scope :global_assets, -> { where(project_id: nil, uploaded_user_id: nil) } + + def self.find_visible_to_project(project:, user:, filename:) + lineage_projects = project.self_and_ancestors + current_user_id = user&.id + assets_by_project_id = where(filename:, project_id: lineage_projects.map(&:id)).group_by(&:project_id) + + lineage_projects.each do |lineage_project| + project_assets_by_user_id = assets_by_project_id.fetch(lineage_project.id, []).index_by(&:uploaded_user_id) + visible_asset = project_assets_by_user_id[current_user_id] || project_assets_by_user_id[lineage_project.user_id] + return find(visible_asset.id) if visible_asset + end + + global_assets.find_by(filename:) + end + + def global? + project_id.nil? + end + + def response_content_type + extension_is_svg = File.extname(filename).casecmp('.svg').zero? + return 'image/svg+xml' if global? && extension_is_svg + return 'application/octet-stream' if extension_is_svg + + Marcel::MimeType.for(name: filename) || 'application/octet-stream' + end + + private + + def belongs_to_scratch_project + return if project.blank? || project.scratch_project? + + errors.add(:project, 'must be a Scratch project') + end end diff --git a/db/migrate/20260410110000_scope_scratch_assets_to_projects.rb b/db/migrate/20260410110000_scope_scratch_assets_to_projects.rb new file mode 100644 index 000000000..46ecaa2fa --- /dev/null +++ b/db/migrate/20260410110000_scope_scratch_assets_to_projects.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +class ScopeScratchAssetsToProjects < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + def up + change_table :scratch_assets, bulk: true do |t| + t.uuid :project_id + t.uuid :uploaded_user_id + end + + add_foreign_key :scratch_assets, :projects, column: :project_id + + add_index :scratch_assets, :project_id, algorithm: :concurrently + + add_index :scratch_assets, + %i[project_id uploaded_user_id filename], + unique: true, + where: 'project_id IS NOT NULL', + name: 'index_scratch_assets_on_project_uploaded_user_and_filename', + algorithm: :concurrently + + add_index :scratch_assets, + :filename, + unique: true, + where: 'project_id IS NULL', + name: 'index_scratch_assets_on_global_filename', + algorithm: :concurrently + + remove_index :scratch_assets, name: 'index_scratch_assets_on_filename', algorithm: :concurrently + end + + def down + raise ActiveRecord::IrreversibleMigration, + 'Scratch assets are scoped by project and uploader and cannot be safely collapsed back into one global row per filename' + end +end diff --git a/db/schema.rb b/db/schema.rb index 1699e359f..eb22dba99 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_03_23_152328) do +ActiveRecord::Schema[7.2].define(version: 2026_04_10_110000) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -337,7 +337,11 @@ t.string "filename", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["filename"], name: "index_scratch_assets_on_filename", unique: true + t.uuid "project_id" + t.uuid "uploaded_user_id" + t.index ["filename"], name: "index_scratch_assets_on_global_filename", unique: true, where: "(project_id IS NULL)" + t.index ["project_id", "uploaded_user_id", "filename"], name: "index_scratch_assets_on_project_uploaded_user_and_filename", unique: true, where: "(project_id IS NOT NULL)" + t.index ["project_id"], name: "index_scratch_assets_on_project_id" end create_table "scratch_components", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| @@ -397,6 +401,7 @@ add_foreign_key "school_project_transitions", "school_projects" add_foreign_key "school_projects", "projects" add_foreign_key "school_projects", "schools" + add_foreign_key "scratch_assets", "projects" add_foreign_key "scratch_components", "projects" add_foreign_key "teacher_invitations", "schools" add_foreign_key "user_jobs", "good_jobs" diff --git a/lib/scratch_asset_importer.rb b/lib/scratch_asset_importer.rb index be70f1993..961ddb75c 100644 --- a/lib/scratch_asset_importer.rb +++ b/lib/scratch_asset_importer.rb @@ -29,11 +29,13 @@ def import private def import_asset(asset_name) - return if ScratchAsset.exists?(filename: asset_name) + return if ScratchAsset.global_assets.exists?(filename: asset_name) sleep(ASSET_FETCHING_DELAY) asset = connection.get("#{asset_name}/get/") - ScratchAsset.create!(filename: asset_name).file.attach(io: StringIO.new(asset.body), filename: asset_name) + ScratchAsset.create!(filename: asset_name, project_id: nil, uploaded_user_id: nil) + .file + .attach(io: StringIO.new(asset.body), filename: asset_name) rescue StandardError => e Rails.logger.error("Failed to import asset #{asset_name}: #{e.message}") end diff --git a/spec/factories/scratch_assets.rb b/spec/factories/scratch_assets.rb index dc3505058..58f67bb89 100644 --- a/spec/factories/scratch_assets.rb +++ b/spec/factories/scratch_assets.rb @@ -3,6 +3,8 @@ FactoryBot.define do factory :scratch_asset do sequence(:filename) { Random.hex } + project { nil } + uploaded_user_id { project&.user_id } trait :with_file do transient { asset_path { file_fixture('test_image_1.png') } } diff --git a/spec/features/scratch/creating_a_scratch_asset_spec.rb b/spec/features/scratch/creating_a_scratch_asset_spec.rb index 8b4e386a4..dedc1d182 100644 --- a/spec/features/scratch/creating_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_a_scratch_asset_spec.rb @@ -5,32 +5,38 @@ RSpec.describe 'Creating a Scratch asset', type: :request do let(:school) { create(:school) } let(:teacher) { create(:teacher, school:) } + let(:project) do + create(:project, project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil, user_id: teacher.id).tap do |scratch_project| + create(:scratch_component, project: scratch_project) + end + end let(:auth_headers) { { 'Authorization' => UserProfileMock::TOKEN } } + let(:project_headers) { auth_headers.merge('X-Project-ID' => project.identifier) } before do - Flipper.disable :cat_mode - Flipper.disable_actor :cat_mode, school + Flipper.enable_actor :cat_mode, school end it 'responds 401 Unauthorized when no Authorization header is provided' do - post '/api/scratch/assets/example.svg' + post '/api/scratch/assets/example.svg', headers: { 'X-Project-ID' => project.identifier } expect(response).to have_http_status(:unauthorized) end it 'responds 404 Not Found when cat_mode is not enabled' do authenticated_in_hydra_as(teacher) + Flipper.disable :cat_mode + Flipper.disable_actor :cat_mode, school - post '/api/scratch/assets/example.svg', headers: auth_headers + post '/api/scratch/assets/example.svg', headers: project_headers expect(response).to have_http_status(:not_found) end it 'creates an asset when cat_mode is enabled and the required headers are provided' do authenticated_in_hydra_as(teacher) - Flipper.enable_actor :cat_mode, school - post '/api/scratch/assets/example.svg', headers: auth_headers + post '/api/scratch/assets/example.svg', headers: project_headers expect(response).to have_http_status(:created) diff --git a/spec/features/scratch/creating_a_scratch_project_spec.rb b/spec/features/scratch/creating_a_scratch_project_spec.rb index 8a72c8757..7df984067 100644 --- a/spec/features/scratch/creating_a_scratch_project_spec.rb +++ b/spec/features/scratch/creating_a_scratch_project_spec.rb @@ -3,6 +3,7 @@ require 'rails_helper' RSpec.describe 'Creating a Scratch project (remixing)', type: :request do + let(:pending_asset_filename) { 'pending.png' } let(:school) { create(:school) } let(:teacher) { create(:teacher, school:) } let(:headers) do @@ -125,5 +126,109 @@ def make_request(query: request_query, request_headers: headers, request_params: expect(remixed_project.lesson_id).to be_nil expect(remixed_project.scratch_component.content.to_h).to eq(scratch_project.deep_stringify_keys) end + + it 'moves the current user pending original-project assets onto the new remix' do + student = create(:student, school:) + school_class = create(:school_class, school:, teacher_ids: [teacher.id]) + create(:class_student, school_class:, student_id: student.id) + original_project.update!(lesson: create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students')) + create_uploaded_scratch_asset( + filename: pending_asset_filename, + project: original_project, + uploaded_user_id: student.id, + body: 'pending-body' + ) + authenticated_in_hydra_as(student) + + expect { make_request }.to change(Project, :count).by(1) + + remixed_project = Project.find_by!(identifier: 'new-project-id') + expect( + ScratchAsset.find_by!(filename: pending_asset_filename, project: remixed_project, uploaded_user_id: student.id).file.download + ).to eq('pending-body') + expect( + ScratchAsset.find_by(filename: pending_asset_filename, project: original_project, uploaded_user_id: student.id) + ).to be_nil + end + + it 'updates the existing remix instead of creating a duplicate' do + student = create(:student, school:) + school_class = create(:school_class, school:, teacher_ids: [teacher.id]) + create(:class_student, school_class:, student_id: student.id) + original_project.update!(lesson: create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students')) + existing_remix = create( + :project, + school:, + user_id: student.id, + remixed_from_id: original_project.id, + project_type: Project::Types::CODE_EDITOR_SCRATCH, + locale: nil + ) + create(:scratch_component, project: existing_remix, content: { targets: ['old target'], monitors: [], extensions: [], meta: {} }) + create_uploaded_scratch_asset( + filename: pending_asset_filename, + project: original_project, + uploaded_user_id: student.id, + body: 'pending-body' + ) + authenticated_in_hydra_as(student) + + expect { make_request }.not_to change(Project, :count) + + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to eq( + 'status' => 'ok', + 'content-name' => existing_remix.identifier + ) + expect(existing_remix.reload.scratch_component.content.to_h).to eq(scratch_project.deep_stringify_keys) + expect( + ScratchAsset.find_by!(filename: pending_asset_filename, project: existing_remix, uploaded_user_id: student.id).file.download + ).to eq('pending-body') + expect( + ScratchAsset.find_by(filename: pending_asset_filename, project: original_project, uploaded_user_id: student.id) + ).to be_nil + end + + it 'keeps the existing remix asset when reassignment finds the same filename there already' do + student = create(:student, school:) + school_class = create(:school_class, school:, teacher_ids: [teacher.id]) + create(:class_student, school_class:, student_id: student.id) + original_project.update!(lesson: create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students')) + existing_remix = create( + :project, + school:, + user_id: student.id, + remixed_from_id: original_project.id, + project_type: Project::Types::CODE_EDITOR_SCRATCH, + locale: nil + ) + create(:scratch_component, project: existing_remix, content: { targets: ['old target'], monitors: [], extensions: [], meta: {} }) + create_uploaded_scratch_asset( + filename: pending_asset_filename, + project: original_project, + uploaded_user_id: student.id, + body: 'source-body' + ) + destination_asset = create_uploaded_scratch_asset( + filename: pending_asset_filename, + project: existing_remix, + uploaded_user_id: student.id, + body: 'destination-body' + ) + authenticated_in_hydra_as(student) + + expect { make_request }.not_to change(Project, :count) + + expect(destination_asset.reload.file.download).to eq('destination-body') + expect( + ScratchAsset.find_by(filename: pending_asset_filename, project: original_project, uploaded_user_id: student.id) + ).to be_nil + end + end + + def create_uploaded_scratch_asset(filename:, project:, uploaded_user_id:, body:, content_type: 'image/png') + ScratchAsset.create!(filename:, project:, uploaded_user_id:).tap do |asset| + asset.file.attach(io: StringIO.new(body), filename:, content_type:) + end end end diff --git a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb index ce3e2ef74..03f05ac7e 100644 --- a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb @@ -9,88 +9,251 @@ let(:filename) { "#{basename}.#{format}" } let(:school) { create(:school) } let(:teacher) { create(:teacher, school:) } + let(:project) { create_scratch_project(user_id: teacher.id) } let(:auth_headers) { { 'Authorization' => UserProfileMock::TOKEN } } + let(:project_headers) { auth_headers.merge('X-Project-ID' => project.identifier) } + + before do + Flipper.enable_actor :cat_mode, school + end describe 'GET #show' do - context 'when the asset is PNG' do + it 'responds 400 Bad Request when X-Project-ID is not provided' do + get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: auth_headers + + expect(response).to have_http_status(:bad_request) + end + + context 'when the user can view the project' do before do - create(:scratch_asset, :with_file, filename:, asset_path: file_fixture(filename)) + authenticated_in_hydra_as(teacher) + end + + context 'when the asset is PNG' do + before do + create(:scratch_asset, :with_file, filename:, project:, asset_path: file_fixture(filename)) + end + + it 'serves the file with png content type' do + get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: project_headers + + follow_redirect! while response.redirect? + + expect(response.media_type).to eq('image/png') + end end - it 'serves the file with png content type' do - get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/' + context 'when the asset is SVG' do + # rubocop:disable RSpec/NestedGroups + context 'when the asset belongs to the project' do + before do + create(:scratch_asset, :with_file, filename: svg_filename, project:, asset_path: file_fixture(svg_filename)) + end + + it 'serves the file with application/octet-stream content type' do + get '/api/scratch/assets/internalapi/asset/test_svg_image.svg/get/', headers: project_headers + + follow_redirect! while response.redirect? + + expect(response.media_type).to eq('application/octet-stream') + end + end + + context 'when the asset is global' do + before do + create(:scratch_asset, :with_file, filename: svg_filename, project: nil, asset_path: file_fixture(svg_filename)) + end + + it 'serves the file with image/svg+xml content type' do + get '/api/scratch/assets/internalapi/asset/test_svg_image.svg/get/', headers: project_headers + + follow_redirect! while response.redirect? + + expect(response.media_type).to eq('image/svg+xml') + end + end + # rubocop:enable RSpec/NestedGroups + end + + it 'falls back to a global asset when the project has no matching asset' do + create_uploaded_scratch_asset(filename: 'library.png', project: nil, body: 'global-body') + + get '/api/scratch/assets/internalapi/asset/library.png/get/', headers: project_headers follow_redirect! while response.redirect? - expect(response.media_type).to eq('image/png') + expect(response.body).to eq('global-body') + end + + it 'does not expose assets from another project' do + other_project = create_scratch_project(user_id: SecureRandom.uuid) + create_uploaded_scratch_asset(filename: 'hidden.png', project: other_project, body: 'other-project-body') + + get '/api/scratch/assets/internalapi/asset/hidden.png/get/', headers: project_headers + + expect(response).to have_http_status(:not_found) end end - context 'when the asset is SVG' do + context 'when the viewer is a student on the original project' do + let(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id]) } + let(:student) { create(:student, school:) } + let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') } + let(:teacher_project) { create_scratch_project(school:, lesson:, user_id: teacher.id) } + let(:project_headers) { auth_headers.merge('X-Project-ID' => teacher_project.identifier) } + before do - create(:scratch_asset, :with_file, filename: svg_filename, asset_path: file_fixture(svg_filename)) + authenticated_in_hydra_as(student) + create(:class_student, school_class:, student_id: student.id) + end + + it 'prefers the current student asset on the original project over the shared teacher asset' do + create_uploaded_scratch_asset(filename: 'shared.png', project: teacher_project, body: 'teacher-body') + create_uploaded_scratch_asset(filename: 'shared.png', project: teacher_project, uploaded_user_id: student.id, body: 'student-body') + + get '/api/scratch/assets/internalapi/asset/shared.png/get/', headers: project_headers + + follow_redirect! while response.redirect? + + expect(response.body).to eq('student-body') + end + + it "does not expose another student's private asset on the original project" do + other_student = create(:student, school:) + create(:class_student, school_class:, student_id: other_student.id) + create_uploaded_scratch_asset(filename: 'shared.png', project: teacher_project, body: 'teacher-body') + create_uploaded_scratch_asset( + filename: 'shared.png', + project: teacher_project, + uploaded_user_id: other_student.id, + body: 'other-student-body' + ) + + get '/api/scratch/assets/internalapi/asset/shared.png/get/', headers: project_headers + + follow_redirect! while response.redirect? + + expect(response.body).to eq('teacher-body') + end + + it 'does not prefer assets from an existing remix when reading through the original project' do + student_remix = create_scratch_project(school:, user_id: student.id, remixed_from_id: teacher_project.id) + create_uploaded_scratch_asset(filename: 'shared.png', project: teacher_project, body: 'teacher-body') + create_uploaded_scratch_asset(filename: 'shared.png', project: student_remix, uploaded_user_id: student.id, body: 'remix-body') + + get '/api/scratch/assets/internalapi/asset/shared.png/get/', headers: project_headers + + follow_redirect! while response.redirect? + + expect(response.body).to eq('teacher-body') + end + end + + context 'when the project is a remix' do + let(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id]) } + let(:student) { create(:student, school:) } + let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') } + let(:teacher_project) { create_scratch_project(school:, lesson:, user_id: teacher.id) } + let(:project) { create_scratch_project(school:, user_id: student.id, remixed_from_id: teacher_project.id) } + + before do + authenticated_in_hydra_as(student) + create(:class_student, school_class:, student_id: student.id) + end + + it 'serves assets from an ancestor project' do + create_uploaded_scratch_asset(filename: 'teacher_asset.png', project: teacher_project, body: 'ancestor-body') + + get '/api/scratch/assets/internalapi/asset/teacher_asset.png/get/', headers: project_headers + + follow_redirect! while response.redirect? + + expect(response.body).to eq('ancestor-body') + end + + it 'serves assets from the current remix before ancestor or global assets' do + create_uploaded_scratch_asset(filename: 'shared.png', project: teacher_project, body: 'ancestor-body') + create_uploaded_scratch_asset(filename: 'shared.png', project:, body: 'current-body') + create_uploaded_scratch_asset(filename: 'shared.png', project: nil, body: 'global-body') + + get '/api/scratch/assets/internalapi/asset/shared.png/get/', headers: project_headers + + follow_redirect! while response.redirect? + + expect(response.body).to eq('current-body') + expect(response.media_type).to eq('image/png') end - it 'serves the file with image/svg+xml content type' do - get '/api/scratch/assets/internalapi/asset/test_svg_image.svg/get/' + it 'still sees the current student asset on an ancestor project before the shared ancestor asset' do + create_uploaded_scratch_asset(filename: 'shared.png', project: teacher_project, body: 'teacher-body') + create_uploaded_scratch_asset(filename: 'shared.png', project: teacher_project, uploaded_user_id: student.id, body: 'student-body') + + get '/api/scratch/assets/internalapi/asset/shared.png/get/', headers: project_headers follow_redirect! while response.redirect? - expect(response.media_type).to eq('image/svg+xml') + expect(response.body).to eq('student-body') end end end describe 'POST #create' do let(:upload) { File.binread(file_fixture(filename)) } + let(:request_headers) do + { 'Content-Type' => 'application/octet-stream', 'X-Project-ID' => project.identifier }.merge(auth_headers) + end let(:make_request) do - post '/api/scratch/assets/test_image_1.png', headers: { 'Content-Type' => 'application/octet-stream' }.merge(auth_headers), params: upload + post '/api/scratch/assets/test_image_1.png', headers: request_headers, params: upload end context 'when user is logged in and cat_mode is enabled' do before do authenticated_in_hydra_as(teacher) - Flipper.disable :cat_mode - Flipper.disable_actor :cat_mode, school end - it 'creates a new asset' do - Flipper.enable_actor :cat_mode, school + it 'responds 400 Bad Request when X-Project-ID is not provided' do + post '/api/scratch/assets/test_image_1.png', + headers: { 'Content-Type' => 'application/octet-stream' }.merge(auth_headers), + params: upload + + expect(response).to have_http_status(:bad_request) + end + it 'creates a new asset' do expect { make_request }.to change(ScratchAsset, :count).by(1) end it 'sets the filename on the asset' do - Flipper.enable_actor :cat_mode, school + make_request + expect(ScratchAsset.find_by!(filename:, project:).filename).to eq(filename) + end + it 'links the asset to the project' do make_request - expect(ScratchAsset.last.filename).to eq(filename) + expect(ScratchAsset.find_by!(filename:, project:).project_id).to eq(project.id) end - it 'attaches the uploaded file to the asset' do - Flipper.enable_actor :cat_mode, school + it 'links the asset to the uploading user' do + make_request + expect(ScratchAsset.find_by!(filename:, project:).uploaded_user_id).to eq(teacher.id) + end + it 'attaches the uploaded file to the asset' do make_request - expect(ScratchAsset.last.file).to be_attached + expect(ScratchAsset.find_by!(filename:, project:).file).to be_attached end it 'stores the content of the file in the attachment' do - Flipper.enable_actor :cat_mode, school - make_request - expect(ScratchAsset.last.file.download).to eq(upload) + expect(ScratchAsset.find_by!(filename:, project:).file.download).to eq(upload) end it 'responds with 201 Created' do - Flipper.enable_actor :cat_mode, school - make_request expect(response).to have_http_status(:created) end it 'includes the status and filename (without extension) in the response' do - Flipper.enable_actor :cat_mode, school - make_request expect(response.parsed_body).to include( 'status' => 'ok', @@ -98,32 +261,26 @@ ) end - context 'when the asset already exists' do + context 'when the asset already exists in the same project' do let(:another_upload_path) { file_fixture('test_image_2.jpeg') } let(:upload) { File.binread(another_upload_path) } let(:original_upload) { File.binread(file_fixture(filename)) } before do - create(:scratch_asset, :with_file, filename:, asset_path: file_fixture(filename)) + create(:scratch_asset, :with_file, filename:, project:, asset_path: file_fixture(filename)) end it 'does not update the content of the file in the attachment' do - Flipper.enable_actor :cat_mode, school - make_request - expect(ScratchAsset.last.file.download).to eq(original_upload) + expect(ScratchAsset.find_by!(filename:, project:).file.download).to eq(original_upload) end it 'responds with 201 Created' do - Flipper.enable_actor :cat_mode, school - make_request expect(response).to have_http_status(:created) end it 'includes the status and filename (without extension) in the response' do - Flipper.enable_actor :cat_mode, school - make_request expect(response.parsed_body).to include( 'status' => 'ok', @@ -131,6 +288,100 @@ ) end end + + it 'allows another user to upload the same filename to the same project' do + make_request + + student = create(:student, school:) + school_class = create(:school_class, school:, teacher_ids: [teacher.id]) + create(:class_student, school_class:, student_id: student.id) + lesson = create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') + project.update!(school:, lesson:) + authenticated_in_hydra_as(student) + + expect do + post '/api/scratch/assets/test_image_1.png', + headers: request_headers, + params: upload + end.to change(ScratchAsset, :count).by(1) + expect( + ScratchAsset.where(filename:, project: project).pluck(:uploaded_user_id) + ).to contain_exactly(student.id, teacher.id) + end + + it 'remains idempotent when another request creates the same project asset first' do + existing_asset = create_uploaded_scratch_asset(filename:, project:, body: 'winner-body') + racing_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: teacher.id) + + allow(ScratchAsset).to receive(:find_or_initialize_by).and_wrap_original do |original, *args| + attributes = args.first + if attributes[:filename] == filename && + attributes[:project]&.id == project.id && + attributes[:uploaded_user_id] == teacher.id + racing_asset + else + original.call(*args) + end + end + allow(racing_asset).to receive(:save!).and_raise(ActiveRecord::RecordNotUnique) + + blob_count = ActiveStorage::Blob.count + + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:created) + expect(existing_asset.reload.file.download).to eq('winner-body') + expect(ActiveStorage::Blob.count).to eq(blob_count) + end + + context 'when the current project can be viewed but not updated' do + let(:student) { create(:student, school:) } + let(:teacher_project) do + school_class = create(:school_class, school:, teacher_ids: [teacher.id]) + create(:class_student, school_class:, student_id: student.id) + lesson = create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') + + create_scratch_project(school:, lesson:, user_id: teacher.id) + end + let(:request_headers) do + { + 'Authorization' => UserProfileMock::TOKEN, + 'Content-Type' => 'application/octet-stream', + 'Origin' => 'editor.com', + 'X-Project-ID' => teacher_project.identifier + } + end + + before do + teacher_project + authenticated_in_hydra_as(student) + end + + it 'creates the asset on the original project for the current user without creating a remix' do + expect { make_request }.not_to change(Project, :count) + expect(ScratchAsset.count).to eq(1) + + uploaded_asset = ScratchAsset.find_by!( + filename:, + project: teacher_project, + uploaded_user_id: student.id + ) + expect(uploaded_asset.file.download).to eq(upload) + expect(response).to have_http_status(:created) + end + + it 'keeps the upload on the original project even when the student already has a remix' do + remix = create_scratch_project(school:, user_id: student.id, remixed_from_id: teacher_project.id) + + expect { make_request }.to change(ScratchAsset, :count).by(1) + + expect( + ScratchAsset.find_by!(filename:, project: teacher_project, uploaded_user_id: student.id).file.download + ).to eq(upload) + expect(ScratchAsset.find_by(project: remix, uploaded_user_id: student.id, filename:)).to be_nil + expect(Project.where(remixed_from_id: teacher_project.id, user_id: student.id).count).to eq(1) + end + end end context 'when user is logged in and cat_mode is disabled' do @@ -141,10 +392,22 @@ end it 'responds 404 Not Found when cat_mode is not enabled' do - post '/api/scratch/assets/example.svg', headers: auth_headers + post '/api/scratch/assets/example.svg', headers: request_headers expect(response).to have_http_status(:not_found) end end end + + def create_scratch_project(**attributes) + create(:project, { project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil }.merge(attributes)).tap do |scratch_project| + create(:scratch_component, project: scratch_project) + end + end + + def create_uploaded_scratch_asset(filename:, project:, body:, uploaded_user_id: project&.user_id, content_type: 'image/png') + ScratchAsset.create!(filename:, project:, uploaded_user_id:).tap do |asset| + asset.file.attach(io: StringIO.new(body), filename:, content_type:) + end + end end diff --git a/spec/features/scratch/showing_a_scratch_project_spec.rb b/spec/features/scratch/showing_a_scratch_project_spec.rb index dcd760ca6..2ab60579a 100644 --- a/spec/features/scratch/showing_a_scratch_project_spec.rb +++ b/spec/features/scratch/showing_a_scratch_project_spec.rb @@ -19,6 +19,30 @@ expect(data).to have_key(:targets) end + it 'returns the stage target first when stored targets are out of order' do + project = create( + :project, + project_type: Project::Types::CODE_EDITOR_SCRATCH, + locale: 'en' + ) + create( + :scratch_component, + project:, + content: { + targets: [ + { name: 'Sprite1', isStage: false }, + { name: 'Stage', isStage: true }, + { name: 'Sprite2', isStage: false } + ] + } + ) + + get "/api/scratch/projects/#{project.identifier}" + + expect(response).to have_http_status(:ok) + expect(response.parsed_body.fetch('targets').pluck('name')).to eq(%w[Stage Sprite1 Sprite2]) + end + it 'returns a 404 if project does not exist' do get '/api/scratch/projects/non_existent_project' diff --git a/spec/lib/scratch_asset_importer_spec.rb b/spec/lib/scratch_asset_importer_spec.rb index 3058b3754..48f858fd5 100644 --- a/spec/lib/scratch_asset_importer_spec.rb +++ b/spec/lib/scratch_asset_importer_spec.rb @@ -13,6 +13,7 @@ scratch_asset = ScratchAsset.find_by(filename: '123abc.png') expect(scratch_asset).to be_present + expect(scratch_asset).to be_global expect(scratch_asset.file.download).to eq(image) end @@ -35,6 +36,19 @@ expect(ScratchAsset.find_by(filename: '456xyz.png')).to be_present end + it 'still imports a global asset when a project asset already uses the filename' do + project = create(:project, project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil, user_id: SecureRandom.uuid) + create(:scratch_component, project:) + create(:scratch_asset, :with_file, filename: '123abc.png', project:) + image = Rails.root.join('spec/fixtures/files/test_image_1.png').read + + stub_request(:get, 'https://example.net/internalapi/asset/123abc.png/get/').to_return(status: 200, body: image) + + expect do + described_class.import(['123abc.png'], 'https://example.net/internalapi/asset/') + end.to change { ScratchAsset.global_assets.where(filename: '123abc.png').count }.by(1) + end + it 'skips assets that fail to import' do image = Rails.root.join('spec/fixtures/files/test_image_1.png').read