From 7215427e07709e47bfe2b424be5fcb7472b9ad22 Mon Sep 17 00:00:00 2001 From: Matthew Trew Date: Mon, 16 Mar 2026 15:38:45 +0000 Subject: [PATCH 1/4] Add asset save/loading --- Gemfile | 4 +- Gemfile.lock | 3 + .../api/scratch/assets_controller.rb | 12 +- app/models/scratch_asset.rb | 7 + config/application.rb | 7 + config/routes.rb | 2 +- .../20260310161646_create_scratch_assets.rb | 9 ++ db/schema.rb | 8 +- spec/factories/scratch_assets.rb | 16 ++ ...eating_and_showing_a_scratch_asset_spec.rb | 153 ++++++++++++++++++ .../scratch/showing_a_scratch_asset_spec.rb | 12 -- 11 files changed, 216 insertions(+), 17 deletions(-) create mode 100644 app/models/scratch_asset.rb create mode 100644 db/migrate/20260310161646_create_scratch_assets.rb create mode 100644 spec/factories/scratch_assets.rb create mode 100644 spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb delete mode 100644 spec/features/scratch/showing_a_scratch_asset_spec.rb diff --git a/Gemfile b/Gemfile index d30dd592b..d9644dbd5 100644 --- a/Gemfile +++ b/Gemfile @@ -18,6 +18,7 @@ gem 'faker' gem 'faraday' gem 'flipper', '~> 1.3' gem 'flipper-active_record', '~> 1.3' +gem 'flipper-ui', '~> 1.4' gem 'github_webhook', '~> 1.4' gem 'globalid' gem 'good_job', '~> 4.3' @@ -37,6 +38,7 @@ gem 'paper_trail' gem 'pg', '~> 1.6' gem 'postmark-rails' gem 'puma', '~> 7.2' +gem 'rack_content_type_default', '~> 1.1' gem 'rack-cors' gem 'rails', '~> 7.1' gem 'sentry-rails' @@ -79,5 +81,3 @@ group :test do gem 'webdrivers' gem 'webmock' end - -gem 'flipper-ui', '~> 1.4' diff --git a/Gemfile.lock b/Gemfile.lock index acd1dd848..bb360dcb0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -377,6 +377,8 @@ GEM rack (>= 3.0.0) rack-test (2.2.0) rack (>= 1.3) + rack_content_type_default (1.1.0) + rack rackup (2.3.1) rack (>= 3) rails (7.2.3) @@ -632,6 +634,7 @@ DEPENDENCIES pry-byebug puma (~> 7.2) rack-cors + rack_content_type_default (~> 1.1) rails (~> 7.1) rails-erd rspec diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index 08e6cf2c5..d9fadabf5 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -7,10 +7,20 @@ class AssetsController < ScratchController skip_before_action :check_scratch_feature, only: [:show] def show - render :show, formats: [:svg] + filename_with_extension = "#{params[:id]}.#{params[:format]}" + redirect_to url_for(ScratchAsset.find_by!(filename: filename_with_extension).file) 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) + end + rescue ActiveRecord::RecordNotUnique => e + logger.error(e) + end + render json: { status: 'ok', 'content-name': params[:id] }, status: :created end end diff --git a/app/models/scratch_asset.rb b/app/models/scratch_asset.rb new file mode 100644 index 000000000..04a9c3b65 --- /dev/null +++ b/app/models/scratch_asset.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class ScratchAsset < ApplicationRecord + validates :filename, presence: true + + has_one_attached :file +end diff --git a/config/application.rb b/config/application.rb index aea4254e0..5a1f3ee6d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -59,6 +59,13 @@ class Application < Rails::Application config.api_only = false config.middleware.insert_before 0, CorpMiddleware + + require 'rack/content_type_default' + config.middleware.insert_before( + Rack::Runtime, + Rack::ContentTypeDefault, :post, 'application/octet-stream', '/api/scratch/assets/.*' + ) + config.generators.system_tests = nil config.active_record.encryption.primary_key = ENV.fetch('ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY') diff --git a/config/routes.rb b/config/routes.rb index 6dfc482c3..059b29546 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -36,7 +36,7 @@ namespace :scratch do resources :projects, only: %i[show update] get '/assets/internalapi/asset/:id(.:format)/get/' => 'assets#show' - post '/assets/:id' => 'assets#create' + post '/assets/:id(.:format)' => 'assets#create' end resource :default_project, only: %i[show] do diff --git a/db/migrate/20260310161646_create_scratch_assets.rb b/db/migrate/20260310161646_create_scratch_assets.rb new file mode 100644 index 000000000..c5233ed4c --- /dev/null +++ b/db/migrate/20260310161646_create_scratch_assets.rb @@ -0,0 +1,9 @@ +class CreateScratchAssets < ActiveRecord::Migration[7.2] + def change + create_table :scratch_assets, id: :uuid do |t| + t.string :filename + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index afa3525a9..34567c449 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_09_104851) do +ActiveRecord::Schema[7.2].define(version: 2026_03_10_161646) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -331,6 +331,12 @@ t.index ["school_roll_number"], name: "index_schools_on_school_roll_number", unique: true, where: "(rejected_at IS NULL)" end + create_table "scratch_assets", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.string "filename" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "scratch_components", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.jsonb "content" t.uuid "project_id", null: false diff --git a/spec/factories/scratch_assets.rb b/spec/factories/scratch_assets.rb new file mode 100644 index 000000000..3603bd940 --- /dev/null +++ b/spec/factories/scratch_assets.rb @@ -0,0 +1,16 @@ +FactoryBot.define do + factory :scratch_asset do + sequence(:filename) { Random.hex } + + trait :with_file do + transient { asset_path { file_fixture('test_image_1.png') } } + + after(:build) do |asset, evaluator| + io = Rails.root.join(evaluator.asset_path).open + filename = File.basename(evaluator.asset_path) + content_type = Mime::Type.lookup_by_extension(filename) + asset.file.attach(io:, filename:, content_type:) + end + 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 new file mode 100644 index 000000000..f903b4149 --- /dev/null +++ b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Creating a Scratch asset', type: :request do + let(:basename) { 'test_image_1' } + let(:format) { 'png' } + let(:filename) { "#{basename}.#{format}" } + let(:school) { create(:school) } + let(:teacher) { create(:teacher, school:) } + let(:cookie_headers) { { 'Cookie' => "scratch_auth=#{UserProfileMock::TOKEN}" } } + + describe 'GET #show' do + let(:make_request) { get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/' } + + context 'when the asset exists' do + let!(:scratch_asset) { create(:scratch_asset, :with_file, filename:, asset_path: file_fixture(filename)) } + + it 'redirects to the asset file URL' do + make_request + + expect(response).to redirect_to(rails_blob_url(scratch_asset.file, only_path: true)) + end + end + end + + describe 'POST #create' do + let(:upload) { File.binread(file_fixture(filename)) } + let(:make_request) do + post '/api/scratch/assets/test_image_1.png', headers: { 'Content-Type' => 'application/octet-stream' }.merge(cookie_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 + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + expect { make_request }.to change(ScratchAsset, :count).by(1) + end + + it 'sets the filename on the asset' do + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + make_request + expect(ScratchAsset.last.filename).to eq(filename) + end + + it 'attaches the uploaded file to the asset' do + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + make_request + expect(ScratchAsset.last.file).to be_attached + end + + it 'stores the content of the file in the attachment' do + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + make_request + expect(ScratchAsset.last.file.download).to eq(upload) + end + + it 'responds with 201 Created' do + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + make_request + expect(response).to have_http_status(:created) + end + + it 'includes the status and filename (without extension) in the response' do + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + make_request + expect(response.parsed_body).to include( + 'status' => 'ok', + 'content-name' => basename + ) + end + + context 'when the asset already exists' 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)) + end + + it 'does not update the content of the file in the attachment' do + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + make_request + expect(ScratchAsset.last.file.download).to eq(original_upload) + end + + it 'responds with 201 Created' do + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + make_request + expect(response).to have_http_status(:created) + end + + it 'includes the status and filename (without extension) in the response' do + # Arrange + Flipper.enable_actor :cat_mode, school + + # Act & Assert + make_request + expect(response.parsed_body).to include( + 'status' => 'ok', + 'content-name' => basename + ) + end + end + end + + context 'when user is logged in a and cat_mode is disabled' do + before do + authenticated_in_hydra_as(teacher) + Flipper.disable :cat_mode + Flipper.disable_actor :cat_mode, school + end + + it 'responds 404 Not Found when cat_mode is not enabled' do + # Act + post '/api/scratch/assets/example.svg', headers: cookie_headers + + # Assert + expect(response).to have_http_status(:not_found) + end + end + end +end diff --git a/spec/features/scratch/showing_a_scratch_asset_spec.rb b/spec/features/scratch/showing_a_scratch_asset_spec.rb deleted file mode 100644 index 3afbc8993..000000000 --- a/spec/features/scratch/showing_a_scratch_asset_spec.rb +++ /dev/null @@ -1,12 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe 'Showing a Scratch asset', type: :request do - it 'returns scratch asset SVG' do - get '/api/scratch/assets/internalapi/asset/example.svg/get/' - - expect(response).to have_http_status(:ok) - expect(response.body).to include(' Date: Mon, 16 Mar 2026 15:48:35 +0000 Subject: [PATCH 2/4] Fix RuboCop violations --- spec/factories/scratch_assets.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/factories/scratch_assets.rb b/spec/factories/scratch_assets.rb index 3603bd940..dc3505058 100644 --- a/spec/factories/scratch_assets.rb +++ b/spec/factories/scratch_assets.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + FactoryBot.define do factory :scratch_asset do sequence(:filename) { Random.hex } From da54733f3c98599af6263abd6f7639307966a0b1 Mon Sep 17 00:00:00 2001 From: mwtrew <9936426+mwtrew@users.noreply.github.com> Date: Mon, 16 Mar 2026 16:13:47 +0000 Subject: [PATCH 3/4] Add non-null constraint and index to `filename` Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- db/migrate/20260310161646_create_scratch_assets.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/db/migrate/20260310161646_create_scratch_assets.rb b/db/migrate/20260310161646_create_scratch_assets.rb index c5233ed4c..bce4d80a2 100644 --- a/db/migrate/20260310161646_create_scratch_assets.rb +++ b/db/migrate/20260310161646_create_scratch_assets.rb @@ -1,7 +1,8 @@ class CreateScratchAssets < ActiveRecord::Migration[7.2] def change create_table :scratch_assets, id: :uuid do |t| - t.string :filename + t.string :filename, null: false + t.index :filename, unique: true t.timestamps end From a966aa87a0e955dd80b533945929f5a038323e2a Mon Sep 17 00:00:00 2001 From: mwtrew <9936426+mwtrew@users.noreply.github.com> Date: Mon, 16 Mar 2026 16:16:43 +0000 Subject: [PATCH 4/4] Fix typo Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../scratch/creating_and_showing_a_scratch_asset_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 f903b4149..eb84b8d7e 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 @@ -134,7 +134,7 @@ end end - context 'when user is logged in a and cat_mode is disabled' do + context 'when user is logged in and cat_mode is disabled' do before do authenticated_in_hydra_as(teacher) Flipper.disable :cat_mode