From 9c447b4d10956030d9777071be3ff57406428618 Mon Sep 17 00:00:00 2001 From: Damian Ho Date: Thu, 7 Dec 2023 23:09:07 -0500 Subject: [PATCH 1/6] 1810 Use ActiveStorage for attachments --- .github/workflows/rubyonrails.yml | 2 +- .gitignore | 1 + README.md | 2 +- app/controllers/attachments_controller.rb | 28 +++++++++++-------- app/models/attachment.rb | 19 ++----------- config/environments/development.rb | 2 +- config/environments/production.rb.template | 5 +++- config/environments/test.rb | 3 ++ ...te_active_storage_tables.active_storage.rb | 27 ++++++++++++++++++ db/schema.rb | 23 ++++++++++++++- 10 files changed, 80 insertions(+), 32 deletions(-) create mode 100644 db/migrate/20231208034215_create_active_storage_tables.active_storage.rb diff --git a/.github/workflows/rubyonrails.yml b/.github/workflows/rubyonrails.yml index 85d03a68c..ebe20ce11 100644 --- a/.github/workflows/rubyonrails.yml +++ b/.github/workflows/rubyonrails.yml @@ -56,7 +56,7 @@ jobs: cp config/school.yml.template config/school.yml cp config/autogradeConfig.rb.template config/autogradeConfig.rb cp .env.template .env - mkdir attachments/ tmp/ + mkdir tmp/ - name: Set up database run: | diff --git a/.gitignore b/.gitignore index 9228e0db3..7f74d28bd 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,7 @@ config/lti_platform_jwk.json app/views/home/_topannounce.html.erb attachments/ doc/ +storage/ out.txt # compiled assets diff --git a/README.md b/README.md index 60042da1c..8e46ad65f 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ We released new documentation! Check it out [here](https://docs.autolabproject.c 3. Create necessary directories. ``` - mkdir attachments/ tmp/ + mkdir tmp/ ``` ### Running Tests diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 2ad927453..525c42049 100755 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -1,3 +1,5 @@ +require "archive" + ## # Attachments can be either assessment or course-specific. # This controller handles both types, setting @is_assessment to distinguish the two @@ -57,23 +59,27 @@ def create action_auth_level :show, :student def show - filename = Rails.root.join("attachments", @attachment.filename) - unless File.exist?(filename) - COURSE_LOGGER.log("Cannot find the file '#{@attachment.filename}' for"\ - " attachment #{@attachment.name}") + attached_file = @attachment.attachment_file + unless attached_file.attached? + COURSE_LOGGER.log("No file attached to attachment '#{@attachment.name}'") - flash[:error] = "Error loading #{@attachment.name} from #{@attachment.filename}" - redirect_to([@course, :attachments]) && return + flash[:error] = "No file attached to attachment '#{@attachment.name}'" + redirect_to course_attachments_path(@course) && return end if @cud.instructor? || @attachment.released? - # Set to application/octet-stream to force download - send_file(filename, disposition: "inline", - type: "application/octet-stream", - filename: @attachment.filename) && return + begin + send_data attached_file.download, filename: @attachment.filename, + type: @attachment.mime_type + rescue StandardError + COURSE_LOGGER.log("Error viewing attachment '#{@attachment.name}'") + flash[:error] = "Error viewing attachment '#{@attachment.name}'" + redirect_to course_assessment_path(@course, @assessment) + end + return end flash[:error] = "You are unauthorized to view this attachment" - redirect_to([@course, @assessment]) + redirect_to course_assessment_path(@course, @assessment) end action_auth_level :edit, :instructor diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 8fefb2698..470a0d2f5 100755 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -9,6 +9,7 @@ class Attachment < ApplicationRecord validates :category_name, presence: true validates :filename, presence: true validates :release_at, presence: true + has_one_attached :attachment_file belongs_to :course belongs_to :assessment @@ -30,22 +31,8 @@ def released? end def file=(upload) - directory = "attachments" - filename = File.basename(upload.original_filename) - dir_path = Rails.root.join(directory) - FileUtils.mkdir_p(dir_path) unless File.exist?(dir_path) - - path = Rails.root.join(directory, filename) - addendum = 1 - - # Deal with duplicate filenames on disk - while File.exist?(path) - path = Rails.root.join(directory, "#{filename}.#{addendum}") - addendum += 1 - end - self.filename = File.basename(path) - File.open(path, "wb") { |f| f.write(upload.read) } - self.mime_type = upload.content_type + self.filename = File.basename(upload.original_filename) + attachment_file.attach(upload) end def after_create diff --git a/config/environments/development.rb b/config/environments/development.rb index f9b2dbac9..ad5318b4d 100755 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -61,7 +61,7 @@ # config.file_watcher = ActiveSupport::EventedFileUpdateChecker # OAuth2 Application Configuration for Github - # See (TODO replace with doc link) + # See https://docs.autolabproject.com/installation/github_integration/ config.x.github.client_id = ENV['GITHUB_CLIENT_ID'] config.x.github.client_secret = ENV['GITHUB_CLIENT_SECRET'] diff --git a/config/environments/production.rb.template b/config/environments/production.rb.template index b202a13de..d99e5782b 100755 --- a/config/environments/production.rb.template +++ b/config/environments/production.rb.template @@ -55,6 +55,9 @@ Autolab3::Application.configure do # Enable serving of images, stylesheets, and JavaScripts from an asset server # config.action_controller.asset_host = "http://assets.example.com" + # Store uploaded files on the local file system (see config/storage.yml for options). + config.active_storage.service = :local + # Disable delivery errors, bad email addresses will be ignored # config.action_mailer.raise_delivery_errors = false @@ -113,7 +116,7 @@ Autolab3::Application.configure do config.x.google_analytics_id = nil # OAuth2 Application Configuration for Github - # See (TODO replace with doc link) + # See https://docs.autolabproject.com/installation/github_integration/ config.x.github.client_id = ENV['GITHUB_CLIENT_ID'] config.x.github.client_secret = ENV['GITHUB_CLIENT_SECRET'] # LTI configuration file storage location. Needs to be in a private folder diff --git a/config/environments/test.rb b/config/environments/test.rb index b78b1a2cc..accb55b53 100755 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -34,6 +34,9 @@ # Store uploaded files on the local file system in a temporary directory. config.active_storage.service = :test + # Perform ActiveStorage jobs immediately. + config.active_job.queue_adapter = :inline + # Don't care if the mailer can't send. config.action_mailer.raise_delivery_errors = false diff --git a/db/migrate/20231208034215_create_active_storage_tables.active_storage.rb b/db/migrate/20231208034215_create_active_storage_tables.active_storage.rb new file mode 100644 index 000000000..0b2ce257c --- /dev/null +++ b/db/migrate/20231208034215_create_active_storage_tables.active_storage.rb @@ -0,0 +1,27 @@ +# This migration comes from active_storage (originally 20170806125915) +class CreateActiveStorageTables < ActiveRecord::Migration[5.2] + def change + create_table :active_storage_blobs do |t| + t.string :key, null: false + t.string :filename, null: false + t.string :content_type + t.text :metadata + t.bigint :byte_size, null: false + t.string :checksum, null: false + t.datetime :created_at, null: false + + t.index [ :key ], unique: true + end + + create_table :active_storage_attachments do |t| + t.string :name, null: false + t.references :record, null: false, polymorphic: true, index: false + t.references :blob, null: false + + t.datetime :created_at, null: false + + 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 + end +end diff --git a/db/schema.rb b/db/schema.rb index 44a20250d..4130ad5cd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,28 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2023_09_24_163059) do +ActiveRecord::Schema.define(version: 2023_12_08_034215) do + + 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.bigint "byte_size", null: false + t.string "checksum", null: false + t.datetime "created_at", null: false + t.index ["key"], name: "index_active_storage_blobs_on_key", unique: true + end create_table "annotations", force: :cascade do |t| t.integer "submission_id" From 602bf072716dc2a1e53d328b6517a67642b399ff Mon Sep 17 00:00:00 2001 From: Damian Ho Date: Thu, 7 Dec 2023 23:13:20 -0500 Subject: [PATCH 2/6] 1864 Add backwards compatibility to ActiveStorage Attachments --- app/controllers/attachments_controller.rb | 28 ++++++++++++++--------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 525c42049..549196207 100755 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -59,23 +59,29 @@ def create action_auth_level :show, :student def show - attached_file = @attachment.attachment_file - unless attached_file.attached? - COURSE_LOGGER.log("No file attached to attachment '#{@attachment.name}'") - - flash[:error] = "No file attached to attachment '#{@attachment.name}'" - redirect_to course_attachments_path(@course) && return - end if @cud.instructor? || @attachment.released? begin - send_data attached_file.download, filename: @attachment.filename, - type: @attachment.mime_type + attached_file = @attachment.attachment_file + if attached_file.attached? + send_data attached_file.download, filename: @attachment.filename, + type: @attachment.mime_type + return + end + + old_attachment_path = Rails.root.join("attachments", @attachment.filename) + if File.exist?(old_attachment_path) + send_file old_attachment_path, filename: @attachment.filename, type: @attachment.mime_type + else + COURSE_LOGGER.log("No file attached to attachment '#{@attachment.name}'") + flash[:error] = "No file attached to attachment '#{@attachment.name}'" + redirect_to course_attachments_path(@course) + end + return rescue StandardError COURSE_LOGGER.log("Error viewing attachment '#{@attachment.name}'") flash[:error] = "Error viewing attachment '#{@attachment.name}'" - redirect_to course_assessment_path(@course, @assessment) + redirect_to course_assessment_path(@course, @assessment) && return end - return end flash[:error] = "You are unauthorized to view this attachment" From fd0b294a71f5fd53a83bd6ee042839edca07a7ea Mon Sep 17 00:00:00 2001 From: Damian Ho Date: Thu, 7 Dec 2023 23:16:44 -0500 Subject: [PATCH 3/6] 1872 Add size limit to attachments --- app/models/attachment.rb | 7 +++++++ app/views/attachments/_form.html.erb | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 470a0d2f5..19ad48ab0 100755 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -9,11 +9,18 @@ class Attachment < ApplicationRecord validates :category_name, presence: true validates :filename, presence: true validates :release_at, presence: true + validate :file_size_limit has_one_attached :attachment_file belongs_to :course belongs_to :assessment + def file_size_limit + return unless attachment_file.attached? && attachment_file.byte_size > 1.gigabyte + + errors.add(:attachment_file, "must be less than 1GB") + end + # Constants ORDERING = "release_at ASC, name ASC".freeze diff --git a/app/views/attachments/_form.html.erb b/app/views/attachments/_form.html.erb index 6776dba80..ef6f70903 100644 --- a/app/views/attachments/_form.html.erb +++ b/app/views/attachments/_form.html.erb @@ -32,7 +32,7 @@ <% end %>
  • <% if @attachment.new_record? %> - <%= f.file_field :file, button_text: "Upload Attachment" %> + <%= f.file_field :file, button_text: "Upload Attachment", help_text: "Max Size: 1GB" %> <% else %> <%= f.text_field :mime_type, help_text: "Note: If you change the Mime type of a file, you might need to clear your browser's cache in order for you to see the change." %> From 63b4a931fbbfc4e81daf1ca5c2eef8f733fa7344 Mon Sep 17 00:00:00 2001 From: Damian Ho Date: Thu, 7 Dec 2023 23:33:11 -0500 Subject: [PATCH 4/6] Set mime_type --- app/models/attachment.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 19ad48ab0..73729b9ee 100755 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -40,6 +40,7 @@ def released? def file=(upload) self.filename = File.basename(upload.original_filename) attachment_file.attach(upload) + self.mime_type = upload.content_type end def after_create From 1b464a311bbe3ab35e3e896d9f9789191d8a7e0b Mon Sep 17 00:00:00 2001 From: Damian Ho Date: Thu, 7 Dec 2023 23:35:59 -0500 Subject: [PATCH 5/6] Remove require --- app/controllers/attachments_controller.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index 549196207..d4492f7b1 100755 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -1,5 +1,3 @@ -require "archive" - ## # Attachments can be either assessment or course-specific. # This controller handles both types, setting @is_assessment to distinguish the two From dc7c4d51ec7bc995edd85d68286f76487736791e Mon Sep 17 00:00:00 2001 From: Damian Ho Date: Fri, 8 Dec 2023 00:15:34 -0500 Subject: [PATCH 6/6] redirect to index on error --- app/controllers/attachments_controller.rb | 24 +++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/app/controllers/attachments_controller.rb b/app/controllers/attachments_controller.rb index d4492f7b1..ffb5f3d0b 100755 --- a/app/controllers/attachments_controller.rb +++ b/app/controllers/attachments_controller.rb @@ -33,7 +33,7 @@ def create if @attachment.update(attachment_params) flash[:success] = "Attachment created" - redirect_to_attachment_list + redirect_to_index else error_msg = "Attachment create failed:" if !@attachment.valid? @@ -69,21 +69,21 @@ def show old_attachment_path = Rails.root.join("attachments", @attachment.filename) if File.exist?(old_attachment_path) send_file old_attachment_path, filename: @attachment.filename, type: @attachment.mime_type + return else COURSE_LOGGER.log("No file attached to attachment '#{@attachment.name}'") flash[:error] = "No file attached to attachment '#{@attachment.name}'" - redirect_to course_attachments_path(@course) + redirect_to_index && return end - return rescue StandardError COURSE_LOGGER.log("Error viewing attachment '#{@attachment.name}'") flash[:error] = "Error viewing attachment '#{@attachment.name}'" - redirect_to course_assessment_path(@course, @assessment) && return + redirect_to_index && return end end flash[:error] = "You are unauthorized to view this attachment" - redirect_to course_assessment_path(@course, @assessment) + redirect_to_index end action_auth_level :edit, :instructor @@ -93,7 +93,7 @@ def edit; end def update if @attachment.update(attachment_params) flash[:success] = "Attachment updated" - redirect_to_attachment_list + redirect_to_index else error_msg = "Attachment update failed:" if !@attachment.valid? @@ -119,7 +119,7 @@ def update def destroy @attachment.destroy flash[:success] = "Attachment deleted" - redirect_to_attachment_list + redirect_to_index end private @@ -139,10 +139,10 @@ def set_attachment COURSE_LOGGER.log("Cannot find attachment with id: #{params[:id]}") flash[:error] = "Could not find Attachment \##{params[:id]}" - redirect_to_attachment_list + redirect_to_index end - def redirect_to_attachment_list + def redirect_to_index if @is_assessment redirect_to course_assessment_path(@course, @assessment) else @@ -152,10 +152,10 @@ def redirect_to_attachment_list def add_attachments_breadcrumb @breadcrumbs << if @is_assessment - (view_context.link_to "Assessment Attachments", - [@course, @assessment, :attachments]) + view_context.link_to "Assessment Attachments", + course_assessment_attachments_path(@course, @assessment) else - (view_context.link_to "Course Attachments", [@course, :attachments]) + view_context.link_to "Course Attachments", course_attachments_path(@course) end end