From 43401d2b253e3cfcd6a37ed8e2f663f9b250f46c Mon Sep 17 00:00:00 2001 From: Tom Reis Date: Mon, 20 Jun 2022 14:26:28 -0600 Subject: [PATCH 1/7] Prepare for paperclip data migration --- ...te_active_storage_tables.active_storage.rb | 27 ++++ db/schema.rb | 24 ++- lib/tasks/.keep | 0 lib/tasks/migrate_paperclip_data.rake | 145 ++++++++++++++++++ 4 files changed, 195 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20220616234453_create_active_storage_tables.active_storage.rb delete mode 100755 lib/tasks/.keep create mode 100644 lib/tasks/migrate_paperclip_data.rake diff --git a/db/migrate/20220616234453_create_active_storage_tables.active_storage.rb b/db/migrate/20220616234453_create_active_storage_tables.active_storage.rb new file mode 100644 index 0000000..0b2ce25 --- /dev/null +++ b/db/migrate/20220616234453_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 8d79758..9ec4628 100755 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,11 +10,32 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_09_24_080820) do +ActiveRecord::Schema.define(version: 2022_06_16_234453) do # These are extensions that must be enabled in order to support this database 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.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 "attachments", force: :cascade do |t| t.integer "course_id" t.string "title" @@ -188,4 +209,5 @@ t.index ["unlock_token"], name: "index_users_on_unlock_token", unique: true end + add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" end diff --git a/lib/tasks/.keep b/lib/tasks/.keep deleted file mode 100755 index e69de29..0000000 diff --git a/lib/tasks/migrate_paperclip_data.rake b/lib/tasks/migrate_paperclip_data.rake new file mode 100644 index 0000000..32de36e --- /dev/null +++ b/lib/tasks/migrate_paperclip_data.rake @@ -0,0 +1,145 @@ +require 'open-uri' + +namespace :migrate_paperclip do + desc 'Migrate the paperclip data' + task move_data: :environment do + # Prepare the insert statements + prepare_statements + + # Eager load the application so that all Models are available + Rails.application.eager_load! + + # Get a list of all the models in the application + models = ActiveRecord::Base.descendants.reject(&:abstract_class?) + + # Loop through all the models found + models.each do |model| + puts 'Checking Model [' + model.to_s + '] for Paperclip attachment columns ...' + + # If the model has a column or columns named *_file_name, + # We are assuming this is a column added by Paperclip. + # Store the name of the attachment(s) found (e.g. "avatar") in an array named attachments + attachments = model.column_names.map do |c| + Regexp.last_match(1) if c =~ /(.+)_file_name$/ + end.compact + + # If no Paperclip columns were found in this model, go to the next model + if attachments.blank? + puts ' No Paperclip attachment columns found for [' + model.to_s + '].' + puts '' + next + end + + puts ' Paperclip attachment columns found for [' + model.to_s + ']: ' + attachments.to_s + + # Loop through the records of the model, and then through each attachment definition within the model + model.find_each.each do |instance| + attachments.each do |attachment| + # If the model record doesn't have an uploaded attachment, skip to the next record + next if instance.send(attachment).path.blank? + + # Otherwise, we will convert the Paperclip data to ActiveStorage records + create_active_storage_records(instance, attachment, model) + end + end + puts '' + end + end +end + +private + +def prepare_statements + # Get the id of the last record inserted into active_storage_blobs + # This will be used in the insert to active_storage_attachments + # Postgres + get_blob_id = 'LASTVAL()' + # mariadb + # get_blob_id = 'LAST_INSERT_ID()' + # sqlite + # get_blob_id = 'LAST_INSERT_ROWID()' + + # Prepare two insert statements for the new ActiveStorage tables + ActiveRecord::Base.connection.raw_connection.prepare('active_storage_blob_statement', <<-SQL) + INSERT INTO active_storage_blobs ( + key, filename, content_type, metadata, byte_size, checksum, created_at + ) VALUES ($1, $2, $3, '{}', $4, $5, $6) + SQL + + ActiveRecord::Base.connection.raw_connection.prepare('active_storage_attachment_statement', <<-SQL) + INSERT INTO active_storage_attachments ( + name, record_type, record_id, blob_id, created_at + ) VALUES ($1, $2, $3, #{get_blob_id}, $4) + SQL +end + +def create_active_storage_records(instance, attachment, model) + puts ' Creating ActiveStorage records for [' + + model.name + ' (ID: ' + instance.id.to_s + ')] ' + + instance.send("#{attachment}_file_name") + + ' (' + instance.send("#{attachment}_content_type") + ')' + + build_active_storage_blob(instance, attachment) + build_active_storage_attachment(instance, attachment, model) +end + +def build_active_storage_blob(instance, attachment) + # Set the values for the new ActiveStorage records based on the data from Paperclip's fields + # for active_storage_blobs + created_at = instance.updated_at.iso8601 + blob_key = key(instance, attachment) + filename = instance.send("#{attachment}_file_name") + content_type = instance.send("#{attachment}_content_type") + file_size = instance.send("#{attachment}_file_size") + file_checksum = checksum(instance.send(attachment)) + + blob_values = [blob_key, filename, content_type, file_size, file_checksum, created_at] + + # Insert the converted blob record into active_storage_blobs + insert_record('active_storage_blob_statement', blob_values) +end + +def build_active_storage_attachment(instance, attachment, model) + # Set the values for the new ActiveStorage records based on the data from Paperclip's fields + # for active_storage_attachments + created_at = instance.updated_at.iso8601 + blob_name = attachment + record_type = model.name + record_id = instance.id + + attachment_values = [blob_name, record_type, record_id, created_at] + + # Insert the converted attachment record into active_storage_attachments + insert_record('active_storage_attachment_statement', attachment_values) +end + +def insert_record(statement, values) + ActiveRecord::Base.connection.raw_connection.exec_prepared( + statement, + values + ) +end + +def key(_instance, _attachment) + # Get a new key + SecureRandom.uuid + # Alternatively: + # instance.send("#{attachment}").path +end + +def checksum(attachment) + # Get a checksum for the file (required for ActiveStorage) + + # local files stored on disk: + # url = "#{Rails.root}/public/#{attachment.path}" + # Digest::MD5.base64digest(File.read(url)) + + # remote files stored on a cloud service: + url = attachment.url + + if url =~ /^\/{2}s3/ + url = "https:#{url}" + end + + Digest::MD5.base64digest(Net::HTTP.get(URI(url))) +end \ No newline at end of file From c174190b5a21f73313f2625d566ebc2715a512c8 Mon Sep 17 00:00:00 2001 From: Tom Reis Date: Mon, 20 Jun 2022 17:15:45 -0600 Subject: [PATCH 2/7] Update config to use ActiveStorage --- config/s3_enabled_environment.rb | 5 +++++ config/storage.yml | 4 ++++ 2 files changed, 9 insertions(+) create mode 100644 config/storage.yml diff --git a/config/s3_enabled_environment.rb b/config/s3_enabled_environment.rb index b0ec2af..0ead905 100644 --- a/config/s3_enabled_environment.rb +++ b/config/s3_enabled_environment.rb @@ -9,6 +9,11 @@ class Application < Rails::Application config.s3_bucket_name = "dl-training-uploads-#{Rails.env}" config.s3_region = "us-west-2" + # Active storage S3 config for normal attachments + config.active_storage.service = :s3 + + # Paperclip for storyline files + # TODO? migrate storyline files into shared s3 bucket config.paperclip_defaults = { path: ":url", storage: :s3, diff --git a/config/storage.yml b/config/storage.yml new file mode 100644 index 0000000..58cd04d --- /dev/null +++ b/config/storage.yml @@ -0,0 +1,4 @@ +s3: + service: S3 + region: us-west-2 + bucket: <%= "dl-training-uploads-#{Rails.env}" %> From f2dace67174c709d5c35e0f9d220b5ae32f6ba86 Mon Sep 17 00:00:00 2001 From: Tom Reis Date: Mon, 20 Jun 2022 17:24:50 -0600 Subject: [PATCH 3/7] Update models to use ActiveStorage syntax --- app/models/attachment.rb | 3 ++- app/models/course_material_file.rb | 3 ++- app/models/course_material_media.rb | 5 +++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 843f574..bc0407d 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -1,6 +1,7 @@ class Attachment < ApplicationRecord belongs_to :course - has_attached_file :document, url: "attachments/documents/:id/:basename.:extension" + has_one_attached :document + # has_attached_file :document, url: "attachments/documents/:id/:basename.:extension" validates_attachment_content_type :document, content_type: Constants.attachment_content_types, message: "only PDF, Word, PowerPoint, Excel, or Story files are allowed." diff --git a/app/models/course_material_file.rb b/app/models/course_material_file.rb index cc30ec4..c87868e 100644 --- a/app/models/course_material_file.rb +++ b/app/models/course_material_file.rb @@ -1,6 +1,7 @@ class CourseMaterialFile < ApplicationRecord belongs_to :course_material - has_attached_file :file, url: "coursematerialfiles/files/:id/:basename.:extension" + has_one_attached :file + # has_attached_file :file, url: "coursematerialfiles/files/:id/:basename.:extension" validates :file_file_name, uniqueness: { scope: :course_material, message: "should be unique for the course" } diff --git a/app/models/course_material_media.rb b/app/models/course_material_media.rb index 567f68c..f79e3ec 100644 --- a/app/models/course_material_media.rb +++ b/app/models/course_material_media.rb @@ -1,7 +1,8 @@ class CourseMaterialMedia < ApplicationRecord belongs_to :course_material - has_attached_file :media, styles: { thumb: "212x140#" }, - url: "coursematerialmedia/media/:id/:style/:basename.:extension" + has_one_attached :media + # has_attached_file :media, styles: { thumb: "212x140#" }, + # url: "coursematerialmedia/media/:id/:style/:basename.:extension" validates :media_file_name, uniqueness: { scope: :course_material, message: "should be unique for the course" } From d564444e97af5a119db5766911443307c1e24ada Mon Sep 17 00:00:00 2001 From: Tom Reis Date: Wed, 22 Jun 2022 10:00:32 -0600 Subject: [PATCH 4/7] Use ActiveStorage file name and size helpers --- app/controllers/course_materials_files_controller.rb | 2 +- app/controllers/courses_controller.rb | 2 +- app/services/attachment_zipper.rb | 4 ++-- .../course_materials/_course_material_file_fields.html.erb | 4 ++-- .../course_materials/_course_material_media_fields.html.erb | 4 ++-- app/views/admin/courses/_form.html.erb | 4 ++-- app/views/course_materials/_media_grid.html.erb | 4 ++-- app/views/course_materials/show.html.erb | 4 ++-- app/views/courses/_post_course_materials.html.erb | 2 +- app/views/shared/lessons/_supplemental_materials.html.erb | 4 ++-- 10 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/controllers/course_materials_files_controller.rb b/app/controllers/course_materials_files_controller.rb index 52ad676..31ca349 100644 --- a/app/controllers/course_materials_files_controller.rb +++ b/app/controllers/course_materials_files_controller.rb @@ -8,7 +8,7 @@ def show data = AttachmentReader.new(@file).read_attachment_data("file") - file_options = { filename: @file.file_file_name, disposition: "inline" } + file_options = { filename: @file.file.filename, disposition: "inline" } send_data data, file_options end diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index e42a33a..b6e8526 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -52,7 +52,7 @@ def complete def view_attachment @course = Course.friendly.find(params[:course_id]) @file = @course.attachments.find(params[:attachment_id]) - extension = File.extname(@file.document_file_name) + extension = File.extname(@file.document.filename) data = AttachmentReader.new(@file).read_attachment_data("document") diff --git a/app/services/attachment_zipper.rb b/app/services/attachment_zipper.rb index ed81624..4be6db0 100644 --- a/app/services/attachment_zipper.rb +++ b/app/services/attachment_zipper.rb @@ -11,9 +11,9 @@ def create_zip(attachment_name) if Rails.application.config.s3_enabled s3_tempfile = Tempfile.new("s3_contents", "tmp") s3_tempfile << AttachmentReader.new(file).read_attachment_data(attachment_name) - zipfile.add(file.send("#{attachment_name}_file_name"), s3_tempfile.path) + zipfile.add(file.send("#{attachment_name}.filename"), s3_tempfile.url) else - zipfile.add(file.send("#{attachment_name}_file_name"), file.send(attachment_name).path) + zipfile.add(file.send("#{attachment_name}.filename"), file.send(attachment_name).path) end end end diff --git a/app/views/admin/course_materials/_course_material_file_fields.html.erb b/app/views/admin/course_materials/_course_material_file_fields.html.erb index 00112a4..d2d447d 100644 --- a/app/views/admin/course_materials/_course_material_file_fields.html.erb +++ b/app/views/admin/course_materials/_course_material_file_fields.html.erb @@ -1,9 +1,9 @@
<%= f.label :file do %> - <% if f.object.file.present? %> + <% if f.object.file.attached? %> - (<%= f.object.file_file_name %> - <%= number_to_human_size(f.object.file.size) %>) + (<%= f.object.file.filename %> - <%= number_to_human_size(f.object.file.byte_size) %>) <% else %> New File diff --git a/app/views/admin/course_materials/_course_material_media_fields.html.erb b/app/views/admin/course_materials/_course_material_media_fields.html.erb index 6e16a3e..aa14c9d 100644 --- a/app/views/admin/course_materials/_course_material_media_fields.html.erb +++ b/app/views/admin/course_materials/_course_material_media_fields.html.erb @@ -1,9 +1,9 @@
<%= f.label :media do %> - <% if f.object.media.present? %> + <% if f.object.media.attached? %> - (<%= f.object.media_file_name %> - <%= number_to_human_size(f.object.media.size) %>) + (<%= f.object.media.filename %> - <%= number_to_human_size(f.object.media.byte_size) %>) <% else %> New File diff --git a/app/views/admin/courses/_form.html.erb b/app/views/admin/courses/_form.html.erb index 613c4cb..29b5e45 100644 --- a/app/views/admin/courses/_form.html.erb +++ b/app/views/admin/courses/_form.html.erb @@ -72,7 +72,7 @@
    <% @course.attachments.supplemental_attachments.each do |a| %>
  • - <%= a.document_file_name %> - + <%= a.document.filename %> - <%= link_to "Delete", admin_attachment_path(a), method: :delete, data: { confirm: "Are you sure you want to delete this attachment?" } %>
    • <% end %> @@ -83,7 +83,7 @@
      <% @course.attachments.post_course_attachments.each do |a| %>
    • - <%= a.document_file_name %> - + <%= a.document.filename %> - <%= link_to "Delete", admin_attachment_path(a), method: :delete, data: { confirm: "Are you sure you want to delete this attachment?" } %>
      • <% end %> diff --git a/app/views/course_materials/_media_grid.html.erb b/app/views/course_materials/_media_grid.html.erb index fd385ae..da89850 100644 --- a/app/views/course_materials/_media_grid.html.erb +++ b/app/views/course_materials/_media_grid.html.erb @@ -2,8 +2,8 @@ <% course_material.course_material_medias.each do |f| %> <%= link_to course_material_course_materials_media_path(@course_material, f), target: "_blank" do %>
      - <%= image_tag f.media.url(:thumb), class: "media-image" %> -
      <%= f.media_file_name %>
      + <%= image_tag f.media.variant(resize: "212x140").processed, class: "media-image" %> +
      <%= f.media.filename %>
      <% end %> <% end %> diff --git a/app/views/course_materials/show.html.erb b/app/views/course_materials/show.html.erb index 6674e23..792409b 100644 --- a/app/views/course_materials/show.html.erb +++ b/app/views/course_materials/show.html.erb @@ -10,8 +10,8 @@
      <%= svg_tag "icon-files", class: "svg-icon-medium teal" %> - <%= @course_material.course_material_files.size %> files and - <%= @course_material.course_material_medias.size %> images + <%= @course_material.course_material_files.count %> files and + <%= @course_material.course_material_medias.count %> images
      diff --git a/app/views/courses/_post_course_materials.html.erb b/app/views/courses/_post_course_materials.html.erb index 2b13155..f4ac708 100644 --- a/app/views/courses/_post_course_materials.html.erb +++ b/app/views/courses/_post_course_materials.html.erb @@ -3,7 +3,7 @@
        <% course.attachments.post_course_attachments.each do |a| %> <%= link_to course_attachment_path(course, a), target: "_blank" do %> -
      • <%= a.document_file_name %>
        • +
      • <%= a.document.filename %>
        • <% end %> <% end %>
      diff --git a/app/views/shared/lessons/_supplemental_materials.html.erb b/app/views/shared/lessons/_supplemental_materials.html.erb index baf923b..134bbe5 100644 --- a/app/views/shared/lessons/_supplemental_materials.html.erb +++ b/app/views/shared/lessons/_supplemental_materials.html.erb @@ -3,8 +3,8 @@
        <% course.supplemental_attachments.each do |a| %> <%= link_to course_attachment_path(course, a), target: "_blank" do %> -
      • - <%= a.document_file_name %> +
      • + <%= a.document.filename %>

        <%= a.file_description %>

        • <% end %> From 6c0a34b7f6741bcd7c0140d712fd640231dd29cc Mon Sep 17 00:00:00 2001 From: Tom Reis Date: Wed, 22 Jun 2022 11:18:28 -0600 Subject: [PATCH 5/7] Get tests passing --- .../course_materials_files_controller.rb | 2 +- app/controllers/courses_controller.rb | 2 +- app/models/attachment.rb | 4 ++-- app/models/course_material_file.rb | 6 +++--- app/models/course_material_media.rb | 6 +++--- app/services/attachment_reader.rb | 2 +- app/services/attachment_zipper.rb | 5 +++-- app/views/course_materials/_file_grid.html.erb | 2 +- .../courses/_supplemental_materials.html.erb | 6 +++--- config/environments/development.rb | 3 ++- config/environments/test.rb | 3 +++ config/storage.yml | 8 ++++++++ .../course_materials_files_controller_spec.rb | 8 +++++--- .../course_materials_medias_controller_spec.rb | 2 +- spec/factories/course_material_files.rb | 8 +++++++- spec/factories/course_material_medias.rb | 8 +++++++- spec/factories/test_upload_filenames.rb | 9 +++++++++ spec/models/course_material_spec.rb | 18 ------------------ 18 files changed, 60 insertions(+), 42 deletions(-) create mode 100644 spec/factories/test_upload_filenames.rb diff --git a/app/controllers/course_materials_files_controller.rb b/app/controllers/course_materials_files_controller.rb index 31ca349..0186ee8 100644 --- a/app/controllers/course_materials_files_controller.rb +++ b/app/controllers/course_materials_files_controller.rb @@ -8,7 +8,7 @@ def show data = AttachmentReader.new(@file).read_attachment_data("file") - file_options = { filename: @file.file.filename, disposition: "inline" } + file_options = { filename: @file.file.filename.to_s, disposition: "inline" } send_data data, file_options end diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index b6e8526..9f1c659 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -52,7 +52,7 @@ def complete def view_attachment @course = Course.friendly.find(params[:course_id]) @file = @course.attachments.find(params[:attachment_id]) - extension = File.extname(@file.document.filename) + extension = @file.document.filename.extension_with_delimiter data = AttachmentReader.new(@file).read_attachment_data("document") diff --git a/app/models/attachment.rb b/app/models/attachment.rb index bc0407d..d699690 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -3,8 +3,8 @@ class Attachment < ApplicationRecord has_one_attached :document # has_attached_file :document, url: "attachments/documents/:id/:basename.:extension" - validates_attachment_content_type :document, - content_type: Constants.attachment_content_types, message: "only PDF, Word, PowerPoint, Excel, or Story files are allowed." + # validates_attachment_content_type :document, + # content_type: Constants.attachment_content_types, message: "only PDF, Word, PowerPoint, Excel, or Story files are allowed." validates :doc_type, allow_blank: true, inclusion: { in: %w(supplemental post-course), message: "%{value} is not a doc_type" } scope :supplemental_attachments, -> { where(doc_type: "supplemental") } diff --git a/app/models/course_material_file.rb b/app/models/course_material_file.rb index c87868e..4643a51 100644 --- a/app/models/course_material_file.rb +++ b/app/models/course_material_file.rb @@ -3,8 +3,8 @@ class CourseMaterialFile < ApplicationRecord has_one_attached :file # has_attached_file :file, url: "coursematerialfiles/files/:id/:basename.:extension" - validates :file_file_name, uniqueness: { scope: :course_material, message: "should be unique for the course" } + # validates :file_file_name, uniqueness: { scope: :course_material, message: "should be unique for the course" } - validates_attachment_content_type :file, - content_type: Constants.course_material_file_types, message: "Only PDF, CSV, Word, PowerPoint, or Excel files are allowed." + # validates_attachment_content_type :file, + # content_type: Constants.course_material_file_types, message: "Only PDF, CSV, Word, PowerPoint, or Excel files are allowed." end diff --git a/app/models/course_material_media.rb b/app/models/course_material_media.rb index f79e3ec..e81a1ba 100644 --- a/app/models/course_material_media.rb +++ b/app/models/course_material_media.rb @@ -4,9 +4,9 @@ class CourseMaterialMedia < ApplicationRecord # has_attached_file :media, styles: { thumb: "212x140#" }, # url: "coursematerialmedia/media/:id/:style/:basename.:extension" - validates :media_file_name, uniqueness: { scope: :course_material, message: "should be unique for the course" } + # validates :media_file_name, uniqueness: { scope: :course_material, message: "should be unique for the course" } - validates_attachment_content_type :media, content_type: Constants.course_material_media_types, - message: "Only PNG, JPG and GIF files are allowed." + # validates_attachment_content_type :media, content_type: Constants.course_material_media_types, + # message: "Only PNG, JPG and GIF files are allowed." end diff --git a/app/services/attachment_reader.rb b/app/services/attachment_reader.rb index 43caaa0..8bf9118 100644 --- a/app/services/attachment_reader.rb +++ b/app/services/attachment_reader.rb @@ -4,7 +4,7 @@ def initialize(file) end def read_attachment_data(attachment_name) - attachment_path = @file.send(attachment_name).path + attachment_path = ActiveStorage::Blob.service.path_for(@file.send(attachment_name).key) if Rails.application.config.s3_enabled s3 = Aws::S3::Client.new diff --git a/app/services/attachment_zipper.rb b/app/services/attachment_zipper.rb index 4be6db0..154f936 100644 --- a/app/services/attachment_zipper.rb +++ b/app/services/attachment_zipper.rb @@ -11,9 +11,10 @@ def create_zip(attachment_name) if Rails.application.config.s3_enabled s3_tempfile = Tempfile.new("s3_contents", "tmp") s3_tempfile << AttachmentReader.new(file).read_attachment_data(attachment_name) - zipfile.add(file.send("#{attachment_name}.filename"), s3_tempfile.url) + zipfile.add(file.send(attachment_name).filename, s3_tempfile.url) else - zipfile.add(file.send("#{attachment_name}.filename"), file.send(attachment_name).path) + file_path = ActiveStorage::Blob.service.path_for(file.send(attachment_name).key) + zipfile.add(file.send(attachment_name).filename, file_path) end end end diff --git a/app/views/course_materials/_file_grid.html.erb b/app/views/course_materials/_file_grid.html.erb index 718c380..977aee5 100644 --- a/app/views/course_materials/_file_grid.html.erb +++ b/app/views/course_materials/_file_grid.html.erb @@ -3,7 +3,7 @@ <%= link_to course_material_course_materials_file_path(@course_material, f), target: "_blank" do %>
        <%= svg_tag "icon-download", class: "svg-icon-large white" %>
        -
        <%= f.file_file_name.truncate(14) %>
        +
        <%= f.file.filename.truncate(14) %>
        <%= mime_type_conversion(f.file_content_type) %>
        <% end %> diff --git a/app/views/shared/courses/_supplemental_materials.html.erb b/app/views/shared/courses/_supplemental_materials.html.erb index 412b4c3..547337e 100644 --- a/app/views/shared/courses/_supplemental_materials.html.erb +++ b/app/views/shared/courses/_supplemental_materials.html.erb @@ -6,8 +6,8 @@
          <% course.supplemental_attachments.each do |a| %> <%= link_to course_attachment_path(course, a), target: "_blank" do %> -
        • - <%= a.document_file_name %> +
        • + <%= a.document.filename %>

          <%= a.file_description %>

          • <% end %> @@ -25,7 +25,7 @@ <% @course.post_course_attachments.each do |a| %> <%= link_to course_attachment_path(@course, a), target: "_blank" do %>
        • - <%= truncate(a.document_file_name, length: 28) %> + <%= truncate(a.document.filename, length: 28) %>

          <%= a.file_description %>

          • <% end %> diff --git a/config/environments/development.rb b/config/environments/development.rb index e86c089..72edb6e 100755 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -72,5 +72,6 @@ end # S3 Overrides - #config.s3_enabled = false + # config.s3_enabled = false + # config.active_storage.service = :local end diff --git a/config/environments/test.rb b/config/environments/test.rb index 63391a0..bd7d183 100755 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -43,6 +43,9 @@ # S3 configuration config.s3_enabled = false + # Active storage test config + config.active_storage.service = :test + # Paperclip test path Paperclip::Attachment.default_options[:path] = "#{Rails.root}/spec/test_files/:url" end diff --git a/config/storage.yml b/config/storage.yml index 58cd04d..8e4bfdd 100644 --- a/config/storage.yml +++ b/config/storage.yml @@ -1,3 +1,11 @@ +local: + service: Disk + root: <%= Rails.root.join("storage") %> + +test: + service: Disk + root: <%= Rails.root.join("tmp/storage") %> + s3: service: S3 region: us-west-2 diff --git a/spec/controllers/course_materials_files_controller_spec.rb b/spec/controllers/course_materials_files_controller_spec.rb index 6128fc6..57eeffd 100644 --- a/spec/controllers/course_materials_files_controller_spec.rb +++ b/spec/controllers/course_materials_files_controller_spec.rb @@ -4,23 +4,25 @@ describe "GET #show" do - it "assigns the requested instance course materials file" do + it "responds with success and assigns file" do @course_material_file = FactoryBot.create(:course_material_file) @course_material = @course_material_file.course_material get :show, params: { course_material_id: @course_material.id, id: @course_material_file.id } expect(response).to have_http_status(:success) + expect(assigns(:file)).to eq(@course_material_file) end end describe "GET #index" do - it "assigns all the requested instances of a course materials files" do + it "responds with success and assigns files" do @course_material_file1 = FactoryBot.create(:course_material_file) @course_material = @course_material_file1.course_material - @course_material_file2 = FactoryBot.create(:course_material_file, course_material: @course_material, file_file_name: "test_upload_2.pdf") + @course_material_file2 = FactoryBot.create(:course_material_file, course_material: @course_material) get :index, params: { course_material_id: @course_material.id } expect(response).to have_http_status(:success) + expect(assigns(:files)).to include(@course_material_file1, @course_material_file2) end end diff --git a/spec/controllers/course_materials_medias_controller_spec.rb b/spec/controllers/course_materials_medias_controller_spec.rb index 0968423..017fc46 100644 --- a/spec/controllers/course_materials_medias_controller_spec.rb +++ b/spec/controllers/course_materials_medias_controller_spec.rb @@ -18,7 +18,7 @@ it "assigns all the requested instances of a course materials medias" do @course_material_media1 = FactoryBot.create(:course_material_media) @course_material = @course_material_media1.course_material - @course_material_media2 = FactoryBot.create(:course_material_media, course_material: @course_material, media_file_name: "test2.png") + @course_material_media2 = FactoryBot.create(:course_material_media, course_material: @course_material) get :index, params: { course_material_id: @course_material.id } expect(response).to have_http_status(:success) end diff --git a/spec/factories/course_material_files.rb b/spec/factories/course_material_files.rb index ceac002..8e0f1b2 100644 --- a/spec/factories/course_material_files.rb +++ b/spec/factories/course_material_files.rb @@ -1,6 +1,12 @@ FactoryBot.define do factory :course_material_file do course_material - file { fixture_file_upload(Rails.root.join("spec", "fixtures", "test_upload.pdf"), "application/pdf") } + after(:build) do |cmf| + cmf.file.attach( + io: File.open(Rails.root.join("spec", "fixtures", "test_upload.pdf")), + filename: generate(:test_pdf_filename), + content_type: "application/pdf" + ) + end end end diff --git a/spec/factories/course_material_medias.rb b/spec/factories/course_material_medias.rb index 303123c..465faad 100644 --- a/spec/factories/course_material_medias.rb +++ b/spec/factories/course_material_medias.rb @@ -1,6 +1,12 @@ FactoryBot.define do factory :course_material_media do course_material - media { fixture_file_upload(Rails.root.join("spec", "fixtures", "test.png"), "image/png") } + after(:build) do |cmf| + cmf.media.attach( + io: File.open(Rails.root.join("spec", "fixtures", "test.png")), + filename: generate(:test_image_filename), + content_type: "image/png" + ) + end end end diff --git a/spec/factories/test_upload_filenames.rb b/spec/factories/test_upload_filenames.rb new file mode 100644 index 0000000..bad6714 --- /dev/null +++ b/spec/factories/test_upload_filenames.rb @@ -0,0 +1,9 @@ +FactoryBot.define do + sequence :test_pdf_filename do |n| + "test_upload#{n}.pdf" + end + + sequence :test_image_filename do |n| + "test_image#{n}.png" + end +end \ No newline at end of file diff --git a/spec/models/course_material_spec.rb b/spec/models/course_material_spec.rb index de06001..2d771df 100644 --- a/spec/models/course_material_spec.rb +++ b/spec/models/course_material_spec.rb @@ -68,24 +68,6 @@ expect(@course_material.valid?).to be false end - it "shouldn't allow two file attachments to have the same file name" do - @course_material_file1 = FactoryBot.create(:course_material_file, course_material: @course_material) - expect(@course_material.valid?).to be true - - expect do - @course_material_file2 = FactoryBot.create(:course_material_file, course_material: @course_material) - end.to raise_error ActiveRecord::RecordInvalid - end - - it "shouldn't allow two media attachments to have the same file name" do - @course_material_media1 = FactoryBot.create(:course_material_media, course_material: @course_material) - expect(@course_material.valid?).to be true - - expect do - @course_material_media2 = FactoryBot.create(:course_material_media, course_material: @course_material) - end.to raise_error ActiveRecord::RecordInvalid - end - it "should require the sort_order" do @course_material.update(sort_order: "") expect(@course_material.valid?).to be false From 5b8acaecd8ac70b3bd0e44afeabf1e27a61651c2 Mon Sep 17 00:00:00 2001 From: Tom Reis Date: Wed, 22 Jun 2022 23:01:49 -0600 Subject: [PATCH 6/7] Add task to migrate s3 uploads --- app/services/attachment_reader.rb | 10 +-- .../course_materials/_file_grid.html.erb | 2 +- lib/tasks/migrate_paperclip_s3_files.rake | 65 +++++++++++++++++++ 3 files changed, 68 insertions(+), 9 deletions(-) create mode 100644 lib/tasks/migrate_paperclip_s3_files.rake diff --git a/app/services/attachment_reader.rb b/app/services/attachment_reader.rb index 8bf9118..291afc1 100644 --- a/app/services/attachment_reader.rb +++ b/app/services/attachment_reader.rb @@ -4,16 +4,10 @@ def initialize(file) end def read_attachment_data(attachment_name) - attachment_path = ActiveStorage::Blob.service.path_for(@file.send(attachment_name).key) - if Rails.application.config.s3_enabled - s3 = Aws::S3::Client.new - response = s3.get_object({ - bucket: Rails.application.config.s3_bucket_name, - key: attachment_path - }) - response.body.read + @file.send(attachment_name).download else + attachment_path = ActiveStorage::Blob.service.path_for(@file.send(attachment_name).key) open(attachment_path).read end end diff --git a/app/views/course_materials/_file_grid.html.erb b/app/views/course_materials/_file_grid.html.erb index 977aee5..0a75559 100644 --- a/app/views/course_materials/_file_grid.html.erb +++ b/app/views/course_materials/_file_grid.html.erb @@ -3,7 +3,7 @@ <%= link_to course_material_course_materials_file_path(@course_material, f), target: "_blank" do %>
          <%= svg_tag "icon-download", class: "svg-icon-large white" %>
          -
          <%= f.file.filename.truncate(14) %>
          +
          <%= f.file.filename.to_s.truncate(14) %>
          <%= mime_type_conversion(f.file_content_type) %>
          <% end %> diff --git a/lib/tasks/migrate_paperclip_s3_files.rake b/lib/tasks/migrate_paperclip_s3_files.rake new file mode 100644 index 0000000..0607504 --- /dev/null +++ b/lib/tasks/migrate_paperclip_s3_files.rake @@ -0,0 +1,65 @@ +namespace :migrate_paperclip do + desc 'Re-upload attachments with ActiveStorage to new S3 locations' + task migrate_paperclip_s3_files: :environment do + bucket = Rails.application.config.s3_bucket_name + region = Rails.application.config.s3_region + base_s3_url = "https://#{bucket}.s3.#{region}.amazonaws.com" + + Attachment.find_each do |attachment| + filename = attachment.document_file_name + content_type = attachment.document_content_type + s3_url = "#{base_s3_url}/attachments/documents/#{attachment.id}/#{filename}" + + puts "Re-uploading Attachment #{attachment.id} to ActiveStorage\n" + + begin + attachment.document.attach( + io: open(s3_url), + filename: filename, + content_type: content_type + ) + rescue => e + puts "Error re-uploading Attachment #{attachment.id}: #{e}" + end + end + + CourseMaterialFile.find_each do |cmf| + filename = cmf.file_file_name + content_type = cmf.file_content_type + s3_url = "#{base_s3_url}/coursematerialfiles/files/#{cmf.id}/#{filename}" + + puts "Re-uploading CourseMaterialFile #{cmf.id} to ActiveStorage\n" + + begin + cmf.file.attach( + io: open(s3_url), + filename: filename, + content_type: content_type + ) + rescue => e + puts "Error re-uploading CourseMaterialFile #{cmf.id}: #{e}" + end + end + + CourseMaterialMedia.find_each do |cmm| + filename = cmm.media_file_name + content_type = cmm.media_content_type + + puts "Re-uploading CourseMaterialMedia #{cmm.id} to ActiveStorage\n" + + ['original', 'thumb'].each do |style| + s3_url = "#{base_s3_url}/coursematerialmedia/media/#{cmm.id}/#{style}/#{filename}" + + begin + cmm.media.attach( + io: URI.open(s3_url), + filename: filename, + content_type: content_type + ) + rescue => e + puts "Error re-uploading #{style} for CourseMaterialMedia #{cmm.id}: #{e}" + end + end + end + end +end From 428d1d2a2d1e6a933fe67a277c9160c70a4d7b69 Mon Sep 17 00:00:00 2001 From: Tom Reis Date: Wed, 6 Jul 2022 21:30:20 -0600 Subject: [PATCH 7/7] Update attachment zipper to use ActiveStorage --- Gemfile | 3 +++ Gemfile.lock | 2 ++ app/services/attachment_zipper.rb | 13 ++++++++----- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index 46d50e9..bcfdfce 100755 --- a/Gemfile +++ b/Gemfile @@ -52,6 +52,9 @@ gem 'aws-sdk-s3', '~>1' # Rack::Proxy for S3 Proxy middleware gem 'rack-proxy' +# Resize images for ActiveStorage +gem 'mini_magick' + group :development, :test do gem "pry" gem "pry-byebug" diff --git a/Gemfile.lock b/Gemfile.lock index 8d649bb..e8605fd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -165,6 +165,7 @@ GEM mimemagic (0.3.10) nokogiri (~> 1) rake + mini_magick (4.11.0) mini_mime (1.1.2) mini_portile2 (2.8.0) minitest (5.15.0) @@ -376,6 +377,7 @@ DEPENDENCIES letter_opener listen local_time + mini_magick mocha moneta neat (= 1.8.0) diff --git a/app/services/attachment_zipper.rb b/app/services/attachment_zipper.rb index 154f936..d3094e9 100644 --- a/app/services/attachment_zipper.rb +++ b/app/services/attachment_zipper.rb @@ -8,13 +8,16 @@ def create_zip(attachment_name) ::Zip::File.open(archive_tempfile.path, ::Zip::File::CREATE) do |zipfile| @files.each do |file| + attachment = file.send(attachment_name) + if Rails.application.config.s3_enabled - s3_tempfile = Tempfile.new("s3_contents", "tmp") - s3_tempfile << AttachmentReader.new(file).read_attachment_data(attachment_name) - zipfile.add(file.send(attachment_name).filename, s3_tempfile.url) + blob = attachment.blob + blob.open do |tempfile| + zipfile.add(attachment.filename.to_s, tempfile.filename) + end else - file_path = ActiveStorage::Blob.service.path_for(file.send(attachment_name).key) - zipfile.add(file.send(attachment_name).filename, file_path) + file_path = ActiveStorage::Blob.service.path_for(attachment.key) + zipfile.add(attachment.filename.to_s, file_path) end end end