From bf8690bf3336efbdf7ff1acdd9553e570a5430fe Mon Sep 17 00:00:00 2001 From: karol Date: Tue, 19 Aug 2025 22:14:59 +0200 Subject: [PATCH 1/5] initialize ActiveStorage --- config/application.rb | 6 +- db/cable_schema.rb | 2 +- ...te_active_storage_tables.active_storage.rb | 57 +++++++++++++++++++ db/queue_schema.rb | 2 +- db/schema.rb | 34 ++++++++++- 5 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20250827203150_create_active_storage_tables.active_storage.rb diff --git a/config/application.rb b/config/application.rb index e08867fb0..07b461733 100644 --- a/config/application.rb +++ b/config/application.rb @@ -56,9 +56,9 @@ class Application < Rails::Application config.action_mailer.preview_paths << Rails.root.join('spec/mailers/previews') # Specify default options for Rails generators - config.generators do |g| - g.orm :active_record, primary_key_type: :uuid - end + # config.generators do |g| + # g.orm :active_record, primary_key_type: :uuid + # end # Allow tables in addition to existing default tags config.action_view.sanitized_allowed_tags = ActionView::Base.sanitized_allowed_tags + %w[table thead tbody tfoot td tr details summary] diff --git a/db/cable_schema.rb b/db/cable_schema.rb index ad90fec73..2efdbf273 100644 --- a/db/cable_schema.rb +++ b/db/cable_schema.rb @@ -12,7 +12,7 @@ ActiveRecord::Schema[8.0].define(version: 2024_09_30_231316) do # These are extensions that must be enabled in order to support this database - enable_extension "plpgsql" + enable_extension "pg_catalog.plpgsql" create_table "solid_cable_messages", force: :cascade do |t| t.binary "channel", null: false diff --git a/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb b/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb new file mode 100644 index 000000000..6bd8bd082 --- /dev/null +++ b/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb @@ -0,0 +1,57 @@ +# This migration comes from active_storage (originally 20170806125915) +class CreateActiveStorageTables < ActiveRecord::Migration[7.0] + def change + # Use Active Record's configured type for primary and foreign keys + primary_key_type, foreign_key_type = primary_and_foreign_key_types + + create_table :active_storage_blobs, id: primary_key_type do |t| + t.string :key, null: false + t.string :filename, null: false + t.string :content_type + t.text :metadata + t.string :service_name, null: false + t.bigint :byte_size, null: false + t.string :checksum + + if connection.supports_datetime_with_precision? + t.datetime :created_at, precision: 6, null: false + else + t.datetime :created_at, null: false + end + + t.index [ :key ], unique: true + end + + create_table :active_storage_attachments, id: primary_key_type do |t| + t.string :name, null: false + t.references :record, null: false, polymorphic: true, index: false, type: foreign_key_type + t.references :blob, null: false, type: foreign_key_type + + if connection.supports_datetime_with_precision? + t.datetime :created_at, precision: 6, null: false + else + t.datetime :created_at, null: false + end + + t.index [ :record_type, :record_id, :name, :blob_id ], name: :index_active_storage_attachments_uniqueness, unique: true + t.foreign_key :active_storage_blobs, column: :blob_id + end + + create_table :active_storage_variant_records, id: primary_key_type do |t| + t.belongs_to :blob, null: false, index: false, type: foreign_key_type + t.string :variation_digest, null: false + + t.index [ :blob_id, :variation_digest ], name: :index_active_storage_variant_records_uniqueness, unique: true + t.foreign_key :active_storage_blobs, column: :blob_id + end + end + + private + def primary_and_foreign_key_types + config = Rails.configuration.generators + setting = config.options[config.orm][:primary_key_type] + primary_key_type = setting || :primary_key + foreign_key_type = setting || :bigint + [ primary_key_type, foreign_key_type ] + end +end diff --git a/db/queue_schema.rb b/db/queue_schema.rb index d6324fc1d..f42738170 100644 --- a/db/queue_schema.rb +++ b/db/queue_schema.rb @@ -12,7 +12,7 @@ ActiveRecord::Schema[8.0].define(version: 2024_09_04_193154) do # These are extensions that must be enabled in order to support this database - enable_extension "plpgsql" + enable_extension "pg_catalog.plpgsql" create_table "solid_queue_blocked_executions", force: :cascade do |t| t.bigint "job_id", null: false diff --git a/db/schema.rb b/db/schema.rb index f95ae3f2d..7a1b8aa32 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,11 +10,39 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2025_07_13_114125) do +ActiveRecord::Schema[8.0].define(version: 2025_08_27_203150) do # These are extensions that must be enabled in order to support this database + enable_extension "pg_catalog.plpgsql" enable_extension "pg_trgm" enable_extension "pgcrypto" - enable_extension "plpgsql" + + create_table "active_storage_attachments", force: :cascade do |t| + t.string "name", null: false + t.string "record_type", null: false + t.bigint "record_id", null: false + t.bigint "blob_id", null: false + t.datetime "created_at", null: false + t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id" + t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true + end + + create_table "active_storage_blobs", force: :cascade do |t| + t.string "key", null: false + t.string "filename", null: false + t.string "content_type" + t.text "metadata" + t.string "service_name", null: false + t.bigint "byte_size", null: false + t.string "checksum" + t.datetime "created_at", null: false + t.index ["key"], name: "index_active_storage_blobs_on_key", unique: true + end + + create_table "active_storage_variant_records", force: :cascade do |t| + t.bigint "blob_id", null: false + t.string "variation_digest", null: false + t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true + end create_table "anomaly_notifications", id: :serial, force: :cascade do |t| t.integer "contributor_id", null: false @@ -688,6 +716,8 @@ t.index ["user_type", "user_id"], name: "index_webauthn_credentials_on_user" end + add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" + add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" add_foreign_key "anomaly_notifications", "exercise_collections" add_foreign_key "anomaly_notifications", "exercises" add_foreign_key "authentication_tokens", "study_groups" From 871a2c3dce2c7ca28d16447bb9e9881bf5e204c0 Mon Sep 17 00:00:00 2001 From: karol Date: Tue, 19 Aug 2025 22:25:59 +0200 Subject: [PATCH 2/5] migrate ProFormA uploader from carrierwave to ActiveStorage --- app/controllers/exercises_controller.rb | 25 +++++++++---------- app/uploaders/proforma_zip_uploader.rb | 7 ------ spec/controllers/exercises_controller_spec.rb | 20 ++++++--------- spec/uploaders/proforma_zip_uploader_spec.rb | 23 ----------------- 4 files changed, 20 insertions(+), 55 deletions(-) delete mode 100644 app/uploaders/proforma_zip_uploader.rb delete mode 100644 spec/uploaders/proforma_zip_uploader_spec.rb diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index e19dd5cdd..fb1d7d750 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -161,9 +161,8 @@ def import_start uuid = ProformaService::UuidFromZip.call(zip: zip_file) exists, updatable = uuid_check(user: current_user, uuid:).values_at(:uuid_found, :update_right) - - uploader = ProformaZipUploader.new - uploader.cache!(params[:file]) + key = SecureRandom.hex + ActiveStorage::Blob.create_and_upload!(key:, io: zip_file, filename: zip_file.original_filename) message = if exists && updatable t('.exercise_exists_and_is_updatable') @@ -177,7 +176,7 @@ def import_start status: 'success', message:, actions: render_to_string(partial: 'import_actions', - locals: {exercise: @exercise, imported: false, exists:, updatable:, file_id: uploader.cache_name}), + locals: {exercise: @exercise, imported: false, exists:, updatable:, file_id: key}), } rescue ProformaXML::InvalidZip => e render json: { @@ -187,16 +186,16 @@ def import_start end def import_confirm - uploader = ProformaZipUploader.new - uploader.retrieve_from_cache!(params[:file_id]) - exercise = ::ProformaService::Import.call(zip: uploader.file, user: current_user) - exercise.save! + ActiveStorage::Blob.find_by(key: params[:file_id]).open do |zip| + exercise = ::ProformaService::Import.call(zip:, user: current_user) + exercise.save! - render json: { - status: 'success', - message: t('.success'), - actions: render_to_string(partial: 'import_actions', locals: {exercise:, imported: true}), - } + render json: { + status: 'success', + message: t('.success'), + actions: render_to_string(partial: 'import_actions', locals: {exercise:, imported: true}), + } + end rescue ProformaXML::ProformaError, ActiveRecord::RecordInvalid => e render json: { status: 'failure', diff --git a/app/uploaders/proforma_zip_uploader.rb b/app/uploaders/proforma_zip_uploader.rb deleted file mode 100644 index 51136e476..000000000 --- a/app/uploaders/proforma_zip_uploader.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -class ProformaZipUploader < CarrierWave::Uploader::Base - def filename - SecureRandom.uuid - end -end diff --git a/spec/controllers/exercises_controller_spec.rb b/spec/controllers/exercises_controller_spec.rb index 85328cc25..ef34c9a22 100644 --- a/spec/controllers/exercises_controller_spec.rb +++ b/spec/controllers/exercises_controller_spec.rb @@ -589,21 +589,21 @@ describe 'POST #import_start' do let(:valid_file) { fixture_file_upload('proforma_import/testfile.zip', 'application/zip') } let(:invalid_file) { 'invalid_file' } - let(:mock_uploader) { instance_double(ProformaZipUploader) } let(:uuid) { 'mocked-uuid' } let(:post_request) { post :import_start, params: {file: file} } let(:file) { valid_file } before do allow(controller).to receive(:current_user).and_return(user) - allow(ProformaZipUploader).to receive(:new).and_return(mock_uploader) end context 'when the file is valid' do before do allow(ProformaService::UuidFromZip).to receive(:call).and_return(uuid) - allow(mock_uploader).to receive(:cache!) - allow(mock_uploader).to receive(:cache_name).and_return('mocked-cache-name') + end + + it 'saves the file to the server' do + expect { post_request }.to change(ActiveStorage::Blob, :count).by(1) end context 'when the exercise exists and is updatable' do @@ -658,6 +658,10 @@ context 'when the file is invalid' do let(:file) { invalid_file } + it 'does not save the file to the server' do + expect { post_request }.not_to change(ActiveStorage::Blob, :count) + end + it 'renders failure JSON with correct error' do post_request @@ -685,16 +689,10 @@ describe 'POST #import_confirm' do let(:file_id) { 'file_id' } - let(:mock_uploader) { instance_double(ProformaZipUploader, file: 'mocked_file') } let(:post_request) { post :import_confirm, params: {file_id: file_id} } - before do - allow(ProformaZipUploader).to receive(:new).and_return(mock_uploader) - end - context 'when the import is successful' do before do - allow(mock_uploader).to receive(:retrieve_from_cache!).with(file_id) allow(ProformaService::Import).to receive(:call).with(zip: 'mocked_file', user: user).and_return(exercise) allow(exercise).to receive(:save!).and_return(true) end @@ -711,7 +709,6 @@ context 'when ProformaError or validation error occurs' do before do - allow(mock_uploader).to receive(:retrieve_from_cache!).with(file_id) allow(ProformaService::Import).to receive(:call).and_raise(ProformaXML::ProformaError, 'Proforma error') end @@ -727,7 +724,6 @@ context 'when StandardError occurs' do before do - allow(mock_uploader).to receive(:retrieve_from_cache!).and_raise(StandardError, 'Unexpected error') allow(Sentry).to receive(:capture_exception) end diff --git a/spec/uploaders/proforma_zip_uploader_spec.rb b/spec/uploaders/proforma_zip_uploader_spec.rb deleted file mode 100644 index 67700ca45..000000000 --- a/spec/uploaders/proforma_zip_uploader_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe ProformaZipUploader, type: :uploader do - subject(:uploader) { described_class.new } - - let(:file) { Rails.root.join('spec/fixtures/files/proforma_import/testfile.zip').open('r') } - - describe '#filename' do - before do - uploader.cache!(file) - end - - after do - uploader.remove! - end - - it 'generates a unique filename using SecureRandom.uuid' do - expect(uploader.filename).to match(/\b[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}\b/) - end - end -end From f028b3d2a7f2c0afeb85bb9d9556b3bea779e761 Mon Sep 17 00:00:00 2001 From: karol Date: Thu, 28 Aug 2025 22:45:59 +0200 Subject: [PATCH 3/5] migrate exercise show and update to ActiveStorage --- .../code_ocean/files_controller.rb | 14 ++++----- app/controllers/concerns/file_parameters.rb | 4 +-- app/controllers/exercises_controller.rb | 6 ++-- app/controllers/submissions_controller.rb | 4 +-- app/models/code_ocean/file.rb | 27 ++++------------ app/policies/code_ocean/file_policy.rb | 5 --- .../convert_exercise_to_task.rb | 2 +- .../convert_task_to_exercise.rb | 2 +- app/views/exercises/_code_field.html.slim | 4 +-- app/views/exercises/_editor_frame.html.slim | 2 +- app/views/exercises/_file_form.html.slim | 3 +- app/views/shared/_file.html.slim | 2 +- config/application.rb | 5 --- ...te_active_storage_tables.active_storage.rb | 23 ++++++++------ .../code_ocean/files_controller_spec.rb | 9 +++--- spec/controllers/exercises_controller_spec.rb | 19 ++++++++---- spec/factories/code_ocean/file.rb | 8 ++++- spec/factories/exercise.rb | 9 ++++-- .../strategy/docker_container_pool_spec.rb | 2 +- spec/models/code_ocean/file_spec.rb | 31 +------------------ .../convert_task_to_exercise_spec.rb | 2 +- spec/system/editor_system_spec.rb | 8 +++-- 22 files changed, 81 insertions(+), 110 deletions(-) diff --git a/app/controllers/code_ocean/files_controller.rb b/app/controllers/code_ocean/files_controller.rb index 884dfaf7f..f8102ccda 100644 --- a/app/controllers/code_ocean/files_controller.rb +++ b/app/controllers/code_ocean/files_controller.rb @@ -25,10 +25,10 @@ def show_protected_upload @file = CodeOcean::File.find(params[:id]) authorize! # The `@file.name_with_extension` is assembled based on the user-selected file type, not on the actual file name stored on disk. - raise Pundit::NotAuthorizedError if @embed_options[:disable_download] || @file.filepath != params[:filename] || @file.native_file.blank? + raise Pundit::NotAuthorizedError if @embed_options[:disable_download] || @file.filepath != params[:filename] || @file.attachment.blank? - real_location = Pathname(@file.native_file.current_path).realpath - send_file(real_location, type: 'application/octet-stream', filename: @file.name_with_extension, disposition: 'attachment') + url = rails_blob_path(@file.attachment, disposition: 'attachment', expires_in: 5.minutes) + redirect_to url, allow_other_host: true end def render_protected_upload @@ -36,12 +36,12 @@ def render_protected_upload @current_user = ExternalUser.new @file = authorize AuthenticatedUrlHelper.retrieve!(CodeOcean::File, request) - # The `@file.name_with_extension` is assembled based on the user-selected file type, not on the actual file name stored on disk. - raise Pundit::NotAuthorizedError unless @file.filepath == params[:filename] || @file.native_file.present? + raise Pundit::NotAuthorizedError unless @file.filepath == params[:filename] || @file.attachment.present? + + url = rails_blob_path(@file.attachment, disposition: 'inline', expires_in: 5.minutes) - real_location = Pathname(@file.native_file.current_path).realpath - send_file(real_location, type: @file.native_file.content_type, filename: @file.name_with_extension) + redirect_to url, allow_other_host: true end def create diff --git a/app/controllers/concerns/file_parameters.rb b/app/controllers/concerns/file_parameters.rb index 7b5191614..80bf75466 100644 --- a/app/controllers/concerns/file_parameters.rb +++ b/app/controllers/concerns/file_parameters.rb @@ -33,8 +33,8 @@ def reject_illegal_file_attributes(exercise, params) private :reject_illegal_file_attributes def file_attributes - %w[content context_id feedback_message file_id file_type_id hidden id name native_file path read_only role weight - file_template_id hidden_feedback] + %w[content context_id feedback_message file_id file_type_id hidden id name path read_only role weight + file_template_id hidden_feedback attachment] end private :file_attributes end diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index fb1d7d750..fe8f83d3c 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -304,12 +304,12 @@ def exercise_params_with_tags def handle_file_uploads if exercise_params exercise_params[:files_attributes].try(:each) do |_index, file_attributes| - if file_attributes[:content].respond_to?(:read) + if file_attributes[:attachment].respond_to?(:read) if FileType.find_by(id: file_attributes[:file_type_id]).try(:binary?) - file_attributes[:native_file] = file_attributes[:content] file_attributes[:content] = nil else - file_attributes[:content] = file_attributes[:content].read.detect_encoding!.encode.delete("\x00") + file_attributes[:content] = file_attributes[:attachment].read.detect_encoding!.encode.delete("\x00") + file_attributes[:attachment] = nil end end end diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 81f87af64..250bed6e1 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -65,7 +65,7 @@ def download def download_file raise Pundit::NotAuthorizedError if @embed_options[:disable_download] - if @file.native_file? + if @file.attachment.attached? redirect_to protected_upload_path(id: @file.id, filename: @file.filepath), status: :see_other else response.set_header('Content-Length', @file.size) @@ -96,7 +96,7 @@ def render_file end # Finally grant access and send the file - if @file.native_file? + if @file.attachment.attached? url = render_protected_upload_url(id: @file.id, filename: @file.filepath) redirect_to AuthenticatedUrlHelper.sign(url, @file), status: :see_other else diff --git a/app/models/code_ocean/file.rb b/app/models/code_ocean/file.rb index 0d9327531..a4fba9851 100644 --- a/app/models/code_ocean/file.rb +++ b/app/models/code_ocean/file.rb @@ -16,7 +16,6 @@ class File < ApplicationRecord after_initialize :set_default_values before_validation :clear_weight, unless: :teacher_defined_assessment? - before_validation :hash_content, if: :content_present? before_validation :set_ancestor_values, if: :incomplete_descendent? attr_writer :size @@ -36,7 +35,7 @@ class File < ApplicationRecord has_many :events_synchronized_editor, class_name: 'Event::SynchronizedEditor' alias descendants files - mount_uploader :native_file, FileUploader + has_one_attached :attachment scope :editable, -> { where(read_only: false) } scope :visible, -> { where(hidden: false) } @@ -50,7 +49,6 @@ class File < ApplicationRecord validates :feedback_message, if: :teacher_defined_assessment?, presence: true validates :feedback_message, absence: true, unless: :teacher_defined_assessment? - validates :hashed_content, if: :content_present?, presence: true validates :hidden, inclusion: [true, false] validates :hidden_feedback, inclusion: [true, false] validates :name, presence: true @@ -73,21 +71,13 @@ class File < ApplicationRecord end def read - if native_file? - return nil unless native_file_location_valid? - - native_file.read + if attachment.attached? + attachment.download else content end end - def native_file_location_valid? - real_location = Pathname(native_file.current_path).realpath - upload_location = Pathname(::File.join(native_file.root, 'uploads')).realpath - real_location.fnmatch? ::File.join(upload_location.to_s, '**') - end - def ancestor_id file_id || id end @@ -102,7 +92,7 @@ def teacher_defined_assessment? end def content_present? - content? || native_file? + content? || attachment.attached? end private :content_present? @@ -122,11 +112,6 @@ def filepath_without_extension end end - def hash_content - self.hashed_content = Digest::MD5.new.hexdigest(read || '') - end - private :hash_content - def incomplete_descendent? file_id.present? && file_type_id.blank? end @@ -158,8 +143,8 @@ def visible? end def size - @size ||= if native_file? - native_file.size + @size ||= if attachment.attached? + attachment.byte_size else content.size end diff --git a/app/policies/code_ocean/file_policy.rb b/app/policies/code_ocean/file_policy.rb index 1944a058c..ad0c1bfac 100644 --- a/app/policies/code_ocean/file_policy.rb +++ b/app/policies/code_ocean/file_policy.rb @@ -7,8 +7,6 @@ def author? end def show? - return no_one if @record.native_file? && !@record.native_file_location_valid? - if @record.context.is_a?(Exercise) admin? || author? || !@record.hidden else @@ -17,8 +15,6 @@ def show? end def show_protected_upload? - return no_one if @record.native_file? && !@record.native_file_location_valid? - if @record.context.is_a?(Exercise) admin? || author? || (!@record.context.unpublished && !@record.hidden) else @@ -27,7 +23,6 @@ def show_protected_upload? end def render_protected_upload? - return no_one if @record.native_file? && !@record.native_file_location_valid? return no_one if @record.context.is_a?(Exercise) && (@record.context.unpublished || @record.hidden) # The AuthenticatedUrlHelper will check for more details, but we cannot determine a specific user diff --git a/app/services/proforma_service/convert_exercise_to_task.rb b/app/services/proforma_service/convert_exercise_to_task.rb index 718914080..300d1f861 100644 --- a/app/services/proforma_service/convert_exercise_to_task.rb +++ b/app/services/proforma_service/convert_exercise_to_task.rb @@ -211,7 +211,7 @@ def filename(file) end def add_content_to_task_file(file, task_file) - if file.native_file.present? + if file.attachment.attached? file_content = file.read task_file.content = file_content task_file.used_by_grader = false diff --git a/app/services/proforma_service/convert_task_to_exercise.rb b/app/services/proforma_service/convert_task_to_exercise.rb index 238ce4eb3..6809ec962 100644 --- a/app/services/proforma_service/convert_task_to_exercise.rb +++ b/app/services/proforma_service/convert_task_to_exercise.rb @@ -119,7 +119,7 @@ def codeocean_file_from_task_file(file, parent_object = nil) xml_id_path: (parent_object.nil? ? [file.id] : [parent_object.id, file.id]).map(&:to_s) ) if file.binary - codeocean_file.native_file = FileIO.new(file.content.dup.force_encoding('UTF-8'), File.basename(file.filename)) + codeocean_file.attachment.attach(io: StringIO.new(file.content.dup.force_encoding('UTF-8')), filename: file.filename) else codeocean_file.content = file.content end diff --git a/app/views/exercises/_code_field.html.slim b/app/views/exercises/_code_field.html.slim index 6ffb350ca..0348d02c6 100644 --- a/app/views/exercises/_code_field.html.slim +++ b/app/views/exercises/_code_field.html.slim @@ -1,6 +1,6 @@ .mb-3 - = form.label(attribute, class: 'form-label') - = form.text_area(attribute, class: 'code-field form-control', rows: 16, style: 'display:none;') + = form.label('content', class: 'form-label') + = form.text_area('content', class: 'code-field form-control', rows: 16, style: 'display:none;') = render(partial: 'editor_edit', locals: {exercise: @exercise}) .card.border-warning.p-2.my-2 = form.file_field(attribute, class: 'form-control', style: 'display: inline-block;') diff --git a/app/views/exercises/_editor_frame.html.slim b/app/views/exercises/_editor_frame.html.slim index af9c98f5b..60af31e90 100644 --- a/app/views/exercises/_editor_frame.html.slim +++ b/app/views/exercises/_editor_frame.html.slim @@ -10,7 +10,7 @@ div class=(defined?(own_solution) ? 'own-frame' : 'frame') data-executable=file. - elsif file.file_type.video? = video_tag(file_path, controls: true) - else - = link_to(file.native_file.file.filename, file_path) + = link_to(file.attachment.filename, file_path) - else .editor-content.d-none data-file-id=file.ancestor_id = file.content div class=(defined?(own_solution) ? 'own-editor' : 'editor') data-file-id=file.ancestor_id data-indent-size=file.file_type.indent_size data-mode=file.file_type.editor_mode data-allow-auto-completion=exercise.allow_auto_completion.to_s data-id=file.id diff --git a/app/views/exercises/_file_form.html.slim b/app/views/exercises/_file_form.html.slim index 00a24021d..6dfa277a8 100644 --- a/app/views/exercises/_file_form.html.slim +++ b/app/views/exercises/_file_form.html.slim @@ -48,4 +48,5 @@ li.card.mt-2 .mb-3 = f.label(:weight, class: 'form-label') = f.number_field(:weight, class: 'form-control', min: 0, step: 'any') - = render('code_field', attribute: :content, form: f) + = render('code_field', attribute: :attachment, form: f) + / = render('code_field', attribute: :content, form: f) diff --git a/app/views/shared/_file.html.slim b/app/views/shared/_file.html.slim index d5f148de5..88324cc76 100644 --- a/app/views/shared/_file.html.slim +++ b/app/views/shared/_file.html.slim @@ -8,4 +8,4 @@ = row(label: 'code_ocean/file.hidden_feedback', value: file.hidden_feedback) = row(label: 'code_ocean/file.feedback_message', value: render_markdown(file.feedback_message), class: 'm-0') = row(label: 'code_ocean/file.weight', value: file.weight) -= row(label: 'code_ocean/file.content', value: file.native_file? ? link_to_if(policy(file).show?, file.native_file.file.filename, protected_upload_path(id: file.id, filename: file.filepath)) : code_tag(file.content, file.file_type.programming_language)) += row(label: 'code_ocean/file.content', value: file.attachment.attached? ? link_to_if(policy(file).show?, file.attachment.filename, protected_upload_path(id: file.id, filename: file.filepath)) : code_tag(file.content, file.file_type.programming_language)) diff --git a/config/application.rb b/config/application.rb index 07b461733..e0dcf69d6 100644 --- a/config/application.rb +++ b/config/application.rb @@ -55,11 +55,6 @@ class Application < Rails::Application config.action_mailer.preview_paths << Rails.root.join('spec/mailers/previews') - # Specify default options for Rails generators - # config.generators do |g| - # g.orm :active_record, primary_key_type: :uuid - # end - # Allow tables in addition to existing default tags config.action_view.sanitized_allowed_tags = ActionView::Base.sanitized_allowed_tags + %w[table thead tbody tfoot td tr details summary] diff --git a/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb b/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb index 6bd8bd082..f55e98358 100644 --- a/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb +++ b/db/migrate/20250827203150_create_active_storage_tables.active_storage.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # This migration comes from active_storage (originally 20170806125915) class CreateActiveStorageTables < ActiveRecord::Migration[7.0] def change @@ -19,7 +21,7 @@ def change t.datetime :created_at, null: false end - t.index [ :key ], unique: true + t.index [:key], unique: true end create_table :active_storage_attachments, id: primary_key_type do |t| @@ -33,7 +35,7 @@ def change t.datetime :created_at, null: false end - t.index [ :record_type, :record_id, :name, :blob_id ], name: :index_active_storage_attachments_uniqueness, unique: true + t.index %i[record_type record_id name blob_id], name: :index_active_storage_attachments_uniqueness, unique: true t.foreign_key :active_storage_blobs, column: :blob_id end @@ -41,17 +43,18 @@ def change t.belongs_to :blob, null: false, index: false, type: foreign_key_type t.string :variation_digest, null: false - t.index [ :blob_id, :variation_digest ], name: :index_active_storage_variant_records_uniqueness, unique: true + t.index %i[blob_id variation_digest], name: :index_active_storage_variant_records_uniqueness, unique: true t.foreign_key :active_storage_blobs, column: :blob_id end end private - def primary_and_foreign_key_types - config = Rails.configuration.generators - setting = config.options[config.orm][:primary_key_type] - primary_key_type = setting || :primary_key - foreign_key_type = setting || :bigint - [ primary_key_type, foreign_key_type ] - end + + def primary_and_foreign_key_types + config = Rails.configuration.generators + setting = config.options[config.orm][:primary_key_type] + primary_key_type = setting || :primary_key + foreign_key_type = setting || :bigint + [primary_key_type, foreign_key_type] + end end diff --git a/spec/controllers/code_ocean/files_controller_spec.rb b/spec/controllers/code_ocean/files_controller_spec.rb index a37ef7c68..544e2a9cd 100644 --- a/spec/controllers/code_ocean/files_controller_spec.rb +++ b/spec/controllers/code_ocean/files_controller_spec.rb @@ -19,11 +19,12 @@ let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.mp4' } } expect_assigns(file: :file) - expect_content_type('application/octet-stream') - expect_http_status(:ok) + expect_redirect - it 'sets the correct filename' do - expect(response.headers['Content-Disposition']).to include("attachment; filename=\"#{file.name_with_extension}\"") + it 'redirects to ActiveStorage blob with correct filename' do + location = response.headers['Location'] || response.location + expect(location).to include(file.name_with_extension) + expect(location).to include('disposition=attachment') end end end diff --git a/spec/controllers/exercises_controller_spec.rb b/spec/controllers/exercises_controller_spec.rb index ef34c9a22..df4ebcccf 100644 --- a/spec/controllers/exercises_controller_spec.rb +++ b/spec/controllers/exercises_controller_spec.rb @@ -91,7 +91,7 @@ let(:perform_request) { proc { post :create, params: {exercise: exercise_attributes.merge(files_attributes:)} } } context 'when specifying the file content within the form' do - let(:files_attributes) { {'0' => build(:file).attributes.except('context_id', 'context_type', 'created_at', 'hashed_content', 'updated_at')} } + let(:files_attributes) { {'0' => build(:file).attributes.except('context_id', 'context_type', 'created_at', 'hashed_content', 'updated_at', 'native_file')} } it 'creates the file' do expect { perform_request.call }.to change(CodeOcean::File, :count) @@ -99,7 +99,7 @@ end context 'when uploading a file' do - let(:files_attributes) { {'0' => build(:file, file_type:).attributes.except('context_id', 'context_type', 'created_at', 'hashed_content', 'updated_at').merge(content: uploaded_file)} } + let(:files_attributes) { {'0' => build(:file, file_type:).attributes.except('context_id', 'context_type', 'created_at', 'hashed_content', 'updated_at', 'native_file').merge(attachment: uploaded_file)} } context 'when uploading a binary file' do let(:file_path) { Rails.root.join('db/seeds/audio_video/devstories.mp4') } @@ -112,7 +112,7 @@ it 'assigns the native file' do perform_request.call - expect(Exercise.last.files.first.native_file).to be_a(FileUploader) + expect(assigns(:exercise).files.first.attachment).to be_attached end end @@ -127,7 +127,7 @@ it 'assigns the file content' do perform_request.call - expect(Exercise.last.files.first.content).to eq(File.read(file_path)) + expect(assigns(:exercise).files.first.content).to eq(File.read(file_path)) end end end @@ -690,10 +690,17 @@ describe 'POST #import_confirm' do let(:file_id) { 'file_id' } let(:post_request) { post :import_confirm, params: {file_id: file_id} } + let(:blob_double) { instance_double(ActiveStorage::Blob) } + let(:tempfile) { Tempfile.new } + + before do + allow(ActiveStorage::Blob).to receive(:find_by).and_return(blob_double) + allow(blob_double).to receive(:open).and_yield(tempfile) + end context 'when the import is successful' do before do - allow(ProformaService::Import).to receive(:call).with(zip: 'mocked_file', user: user).and_return(exercise) + allow(ProformaService::Import).to receive(:call).with(zip: tempfile, user: user).and_return(exercise) allow(exercise).to receive(:save!).and_return(true) end @@ -730,7 +737,7 @@ it 'logs the error and renders internal error JSON' do post_request - expect(Sentry).to have_received(:capture_exception).with(instance_of(StandardError)) + expect(Sentry).to have_received(:capture_exception).with(a_kind_of(StandardError)) expect(response).to have_http_status(:ok) parsed_response = response.parsed_body expect(parsed_response['status']).to eq('failure') diff --git a/spec/factories/code_ocean/file.rb b/spec/factories/code_ocean/file.rb index 5d8606fbd..cfa09a909 100644 --- a/spec/factories/code_ocean/file.rb +++ b/spec/factories/code_ocean/file.rb @@ -16,7 +16,13 @@ module CodeOcean trait(:image) do file_type factory: :dot_png name { 'poster' } - native_file { Rack::Test::UploadedFile.new(Rails.root.join('db/seeds/audio_video/poster.png'), 'image/png') } + after(:build) do |file| + file.attachment.attach( + io: Rails.root.join('db/seeds/audio_video/poster.png').open, + filename: 'poster.png', + content_type: 'image/png' + ) + end end end diff --git a/spec/factories/exercise.rb b/spec/factories/exercise.rb index fe23031fb..a027bb563 100644 --- a/spec/factories/exercise.rb +++ b/spec/factories/exercise.rb @@ -11,11 +11,16 @@ def create_seed_file(exercise, path, file_attributes = {}) name = File.basename(path).gsub(file_extension, '') file_attributes.merge!(file_type:, name:, path: path.split('/')[1..-2].join('/'), role: file_attributes[:role] || 'regular_file') if file_type.binary? - file_attributes[:native_file] = File.open(SeedsHelper.seed_file_path(path), 'r') + file_record = exercise.add_file!(file_attributes) + file_record.attachment.attach( + io: File.open(SeedsHelper.seed_file_path(path), 'rb'), + filename: File.basename(path) + ) + file_record else file_attributes[:content] = SeedsHelper.read_seed_file(path) + exercise.add_file!(file_attributes) end - exercise.add_file!(file_attributes) end FactoryBot.define do diff --git a/spec/lib/runner/strategy/docker_container_pool_spec.rb b/spec/lib/runner/strategy/docker_container_pool_spec.rb index b5bfc90c3..58eb5e501 100644 --- a/spec/lib/runner/strategy/docker_container_pool_spec.rb +++ b/spec/lib/runner/strategy/docker_container_pool_spec.rb @@ -157,7 +157,7 @@ end context 'when receiving a binary file' do - let(:files) { [build(:file, :image)] } + let(:files) { [create(:file, :image)] } it 'copies the file inside the workspace' do expect(File).to receive(:write).with(local_path.join(files.first.filepath), files.first.read) diff --git a/spec/models/code_ocean/file_spec.rb b/spec/models/code_ocean/file_spec.rb index e031da404..d47fa3e68 100644 --- a/spec/models/code_ocean/file_spec.rb +++ b/spec/models/code_ocean/file_spec.rb @@ -100,41 +100,12 @@ context 'with a native file' do let(:file) { create(:file, :image) } - after { file.native_file.remove! } + after { file.attachment.purge } context 'when the path has not been modified' do it 'reads the native file' do expect(file.read).to be_present end end - - context 'when the path has been modified' do - before do - file.update_column(:native_file, '../../../../database.yml') # rubocop:disable Rails/SkipsModelValidations - file.reload - end - - it 'does not read the native file' do - expect(file.read).not_to be_present - end - end - - context 'when a symlink is used' do - let(:fake_upload_location) { File.join(CarrierWave::Uploader::Base.new.root, 'uploads', 'files', 'database.yml') } - - before do - FileUtils.mkdir_p(File.dirname(fake_upload_location)) - FileUtils.touch Rails.root.join('config/database.yml') - File.symlink Rails.root.join('config/database.yml'), fake_upload_location - file.update_column(:native_file, '../database.yml') # rubocop:disable Rails/SkipsModelValidations - file.reload - end - - after { File.delete(fake_upload_location) } - - it 'does not read the native file' do - expect(file.read).not_to be_present - end - end end end diff --git a/spec/services/proforma_service/convert_task_to_exercise_spec.rb b/spec/services/proforma_service/convert_task_to_exercise_spec.rb index 89bca4099..0af39b674 100644 --- a/spec/services/proforma_service/convert_task_to_exercise_spec.rb +++ b/spec/services/proforma_service/convert_task_to_exercise_spec.rb @@ -259,7 +259,7 @@ let(:binary) { true } it 'creates an exercise with a file with attachment and the correct attributes' do - expect(convert_to_exercise_service.files.first.native_file).to be_present + expect(convert_to_exercise_service.files.first.attachment).to be_attached end end diff --git a/spec/system/editor_system_spec.rb b/spec/system/editor_system_spec.rb index b3bb25d7d..93bfbd495 100644 --- a/spec/system/editor_system_spec.rb +++ b/spec/system/editor_system_spec.rb @@ -61,11 +61,13 @@ end context 'when selecting a binary file' do + let(:upload_path) { protected_upload_path(id: file.id, filename: file.filepath) } + context 'when selecting an audio file' do let(:file) { exercise.files.detect {|file| file.file_type.audio? } } it 'contains an