Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AO3-5578 Use ActiveStorage for existing image uploads #4807

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
58061f6
AO3-5578 Replace kt-paperclip with ActiveStorage
brianjaustin Apr 4, 2024
bb6c303
Task to migrate icons in S3
brianjaustin Apr 4, 2024
995d85a
Ah lockfiles, my constant nemesis
brianjaustin May 6, 2024
308e20c
Style and minor test fixes
brianjaustin May 13, 2024
f6e1af6
Install libvips for tests
brianjaustin May 13, 2024
1319ba0
Fix n+1 queries
brianjaustin May 24, 2024
fd1ffd4
Extract custom validator
brianjaustin May 24, 2024
b7b607d
Remove sneaky query and fix error
brianjaustin May 24, 2024
23baaa3
review feedback
brianjaustin May 26, 2024
606b9f2
revert safe nav removal
brianjaustin May 26, 2024
8306cac
Better image resizing
brianjaustin May 26, 2024
95def55
D'oh
brianjaustin May 26, 2024
d62a49a
Generic filename in rake task
brianjaustin May 26, 2024
4df7a59
Remove icon comment and update label
brianjaustin May 27, 2024
bc6e682
fix file size in error
brianjaustin Jun 17, 2024
0c82e49
Incomplete fixes (expected fail)
brianjaustin Jun 17, 2024
f5ec477
Partial n+1 fixes/workarounds
brianjaustin Jun 18, 2024
d88c733
Forgot ci update
brianjaustin Jun 27, 2024
3c15a46
More n+1 fixes (plus some rubocop)
brianjaustin Jun 28, 2024
ecaa09e
Fix some n+1 queries in search
brianjaustin Jun 28, 2024
6813dbe
Scope improvements
brianjaustin Jun 29, 2024
8c89db2
Try skeptical test
brianjaustin Jun 29, 2024
c90d782
Revert "Try skeptical test"
brianjaustin Jun 29, 2024
0c9781e
Fix test
brianjaustin Jun 29, 2024
6de3f11
General improvements
brianjaustin Jun 29, 2024
311c7a5
Appease the dog
brianjaustin Jun 29, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/automated-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,12 @@ jobs:
arguments: --exclude-pattern 'spec/{controllers,models}/**/*.rb'
- command: cucumber
arguments: features/admins
libvips: true
- command: cucumber
arguments: features/bookmarks
- command: cucumber
arguments: features/collections
libvips: true
- command: cucumber
arguments: features/comments_and_kudos
- command: cucumber
Expand All @@ -73,8 +75,10 @@ jobs:
vcr: true
- command: cucumber
arguments: features/other_a
libvips: true
- command: cucumber
arguments: features/other_b
libvips: true
- command: cucumber
arguments: features/prompt_memes_a
- command: cucumber
Expand Down Expand Up @@ -129,6 +133,10 @@ jobs:
restore-keys: |
cassette-library-${{ hashFiles(matrix.tests.arguments) }}-

- name: Install libvips for image processing
if: ${{ matrix.tests.libvips }}
run: sudo apt-get install -y libvips-dev

- name: Set up Ruby and run bundle install
uses: ruby/setup-ruby@v1
with:
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ public/system/test
# /tmp/
/tmp/*

# ActiveRecord storage path
storage/

# /vendor/
/vendor/gems

Expand Down
3 changes: 2 additions & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ gem "aws-sdk-s3"
gem 'css_parser'

gem "terrapin"
gem "kt-paperclip", ">= 5.2.0"

# for looking up image dimensions quickly
gem 'fastimage'
Expand Down Expand Up @@ -188,3 +187,5 @@ group :staging, :production do
# frameworks above it to be instrumented when the gem initializes.
gem "newrelic_rpm"
end

gem "image_processing", "~> 1.12"
14 changes: 7 additions & 7 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -321,15 +321,12 @@ GEM
rails-i18n
rainbow (>= 2.2.2, < 4.0)
terminal-table (>= 1.5.1)
image_processing (1.12.2)
mini_magick (>= 4.9.5, < 5)
ruby-vips (>= 2.0.17, < 3)
jmespath (1.6.2)
json (2.7.1)
kgio (2.10.0)
kt-paperclip (7.2.2)
activemodel (>= 4.2.0)
activesupport (>= 4.2.0)
marcel (~> 1.0.1)
mime-types
terrapin (>= 0.6.0, < 2.0)
launchy (2.5.2)
addressable (~> 2.8)
lograge (0.14.0)
Expand Down Expand Up @@ -364,6 +361,7 @@ GEM
mime-types (3.5.2)
mime-types-data (~> 3.2015)
mime-types-data (3.2024.0206)
mini_magick (4.12.0)
mini_mime (1.1.2)
mini_portile2 (2.8.5)
minitest (5.22.2)
Expand Down Expand Up @@ -538,6 +536,8 @@ GEM
rubocop-rspec (2.6.0)
rubocop (~> 1.19)
ruby-progressbar (1.13.0)
ruby-vips (2.2.1)
ffi (~> 1.12)
ruby2_keywords (0.0.5)
rubyntlm (0.6.3)
rubyzip (2.3.2)
Expand Down Expand Up @@ -673,8 +673,8 @@ DEPENDENCIES
htmlentities
httparty
i18n-tasks
image_processing (~> 1.12)
kgio (= 2.10.0)
kt-paperclip (>= 5.2.0)
launchy
lograge
mail (>= 2.8)
Expand Down
1 change: 1 addition & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
class ApplicationController < ActionController::Base
include ActiveStorage::SetCurrent
include Pundit::Authorization
protect_from_forgery with: :exception, prepend: true
rescue_from ActionController::InvalidAuthenticityToken, with: :display_auth_error
Expand Down
7 changes: 7 additions & 0 deletions app/helpers/collections_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,11 @@ def collection_item_approval_options(actor:, item_type:)
[t("#{key}.rejected"), :rejected]
]
end

# Fetches the icon URL for the given collection, using the standard (100x100>) variant.
def standard_icon_url(collection)
return "/images/skins/iconsets/default/icon_collection.png" unless collection.icon.attached?

collection.icon.variant(:standard).processed.url
end
end
10 changes: 7 additions & 3 deletions app/helpers/skins_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@ def skin_author_link(skin)

# we only actually display an image if there's a file
def skin_preview_display(skin)
if skin && skin.icon_file_name
link_to image_tag(skin.icon.url(:standard), alt: skin.icon_alt_text, class: "icon", skip_pipeline: true), skin.icon.url(:original)
end
return unless skin&.icon&.attached?
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved

link_to image_tag(skin.icon.variant(:standard).processed.url,
alt: skin.icon_alt_text,
class: "icon",
skip_pipeline: true),
skin.icon.url
end

def skin_tag
Expand Down
11 changes: 4 additions & 7 deletions app/helpers/users_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,10 @@ def print_pseuds(user)

# Determine which icon to show on user pages
def standard_icon(user = nil, pseud = nil)
if pseud && pseud.icon
pseud.icon.url(:standard).gsub(/^http:/, "https:")
elsif user && user.default_pseud && user.default_pseud.icon
user.default_pseud.icon.url(:standard).gsub(/^http:/, "https:")
else
'/images/skins/iconsets/default/icon_user.png'
end
return pseud.icon.variant(:standard).processed.url if pseud&.icon&.attached?
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
return user.default_pseud.icon.variant(:standard).processed.url if user&.default_pseud&.icon&.attached?

"/images/skins/iconsets/default/icon_user.png"
end

# no alt text if there isn't specific alt text
Expand Down
27 changes: 17 additions & 10 deletions app/models/collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,23 @@ class Collection < ApplicationRecord
include Filterable
include WorksOwner

has_attached_file :icon,
styles: { standard: "100x100>" },
url: "/system/:class/:attachment/:id/:style/:basename.:extension",
path: %w(staging production).include?(Rails.env) ? ":class/:attachment/:id/:style.:extension" : ":rails_root/public:url",
storage: %w(staging production).include?(Rails.env) ? :s3 : :filesystem,
s3_protocol: "https",
default_url: "/images/skins/iconsets/default/icon_collection.png"
has_one_attached :icon do |attachable|
attachable.variant(:standard, resize_to_fill: [100, nil])
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
end

validate :check_icon_properties
def check_icon_properties
return unless icon.attached?

# i18n-tasks-use t("errors.attributes.icon.invalid_format")
errors.add(:icon, :invalid_format) unless %r{image/\S+}.match?(icon.content_type)

validates_attachment_content_type :icon, content_type: /image\/\S+/, allow_nil: true
validates_attachment_size :icon, less_than: 500.kilobytes, allow_nil: true
size_limit_kb = 500
# i18n-tasks-use t("errors.attributes.icon.too_large")
errors.add(:icon, :too_large, size_limit_kb: size_limit_kb) unless icon.blob.byte_size < size_limit_kb.kilobytes

icon.purge if errors[:icon].any?
end

belongs_to :parent, class_name: "Collection", inverse_of: :children
has_many :children, class_name: "Collection", foreign_key: "parent_id", inverse_of: :parent
Expand Down Expand Up @@ -419,6 +426,6 @@ def delete_icon
alias_method :delete_icon?, :delete_icon

def clear_icon
self.icon = nil if delete_icon? && !icon.dirty?
self.icon.purge if delete_icon?
end
end
37 changes: 19 additions & 18 deletions app/models/pseud.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,24 @@ class Pseud < ApplicationRecord
include WorksOwner
include Justifiable

has_attached_file :icon,
styles: { standard: "100x100>" },
path: if Rails.env.production?
":attachment/:id/:style.:extension"
elsif Rails.env.staging?
":rails_env/:attachment/:id/:style.:extension"
else
":rails_root/public/system/:rails_env/:class/:attachment/:id_partition/:style/:filename"
end,
storage: %w(staging production).include?(Rails.env) ? :s3 : :filesystem,
s3_protocol: "https",
default_url: "/images/skins/iconsets/default/icon_user.png"
has_one_attached :icon do |attachable|
attachable.variant(:standard, resize_to_fill: [100, nil])
end

validate :check_icon_properties
def check_icon_properties
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
return unless icon.attached?

validates_attachment_content_type :icon,
content_type: %w[image/gif image/jpeg image/png],
allow_nil: true
allowed_formats = %w[image/gif image/jpeg image/png]
# i18n-tasks-use t("errors.attributes.icon.invalid_format")
errors.add(:icon, :invalid_format) unless allowed_formats.include?(icon.content_type)

validates_attachment_size :icon, less_than: 500.kilobytes, allow_nil: true
size_limit_kb = 500
# i18n-tasks-use t("errors.attributes.icon.too_large")
errors.add(:icon, :too_large, size_limit_kb: size_limit_kb) unless icon.blob.byte_size < size_limit_kb.kilobytes

icon.purge if errors[:icon].any?
end

NAME_LENGTH_MIN = 1
NAME_LENGTH_MAX = 40
Expand Down Expand Up @@ -419,9 +419,10 @@ def delete_icon

def clear_icon
return unless delete_icon?
self.icon = nil unless icon.dirty?

self.icon.purge
self.icon_alt_text = nil
self.icon = nil if delete_icon? && !icon.dirty?
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
end

#################################
Expand Down
37 changes: 22 additions & 15 deletions app/models/skin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,23 @@ class Skin < ApplicationRecord

accepts_nested_attributes_for :skin_parents, allow_destroy: true, reject_if: proc { |attrs| attrs[:position].blank? || (attrs[:parent_skin_title].blank? && attrs[:parent_skin_id].blank?) }

has_attached_file :icon,
styles: { standard: "100x100>" },
url: "/system/:class/:attachment/:id/:style/:basename.:extension",
path: %w(staging production).include?(Rails.env) ? ":class/:attachment/:id/:style.:extension" : ":rails_root/public:url",
storage: %w(staging production).include?(Rails.env) ? :s3 : :filesystem,
s3_protocol: "https",
default_url: "/images/skins/iconsets/default/icon_skins.png"
has_one_attached :icon do |attachable|
attachable.variant(:standard, resize_to_fill: [100, nil])
end

validate :check_icon_properties
def check_icon_properties
return unless icon.attached?

# i18n-tasks-use t("errors.attributes.icon.invalid_format")
errors.add(:icon, :invalid_format) unless %r{image/\S+}.match?(icon.content_type)

size_limit_kb = 500
# i18n-tasks-use t("errors.attributes.icon.too_large")
errors.add(:icon, :too_large, size_limit_kb: size_limit_kb) unless icon.blob.byte_size < size_limit_kb.kilobytes

icon.purge if errors[:icon].any?
end

after_save :skin_invalidate_cache
def skin_invalidate_cache
Expand All @@ -70,8 +80,6 @@ def skin_invalidate_cache
end
end

validates_attachment_content_type :icon, content_type: /image\/\S+/, allow_nil: true
validates_attachment_size :icon, less_than: 500.kilobytes, allow_nil: true
validates_length_of :icon_alt_text, allow_blank: true, maximum: ArchiveConfig.ICON_ALT_MAX,
too_long: ts("must be less than %{max} characters long.", max: ArchiveConfig.ICON_ALT_MAX)

Expand Down Expand Up @@ -102,7 +110,8 @@ def valid_media

validate :valid_public_preview
def valid_public_preview
return true if (self.official? || !self.public? || self.icon_file_name)
return true if self.official? || !self.public? || self.icon.attached?

errors.add(:base, ts("You need to upload a screencap if you want to share your skin."))
end

Expand Down Expand Up @@ -476,7 +485,7 @@ def self.load_site_css
skin.ie_condition = skin_ie
skin.unusable = true
skin.official = true
File.open(version_dir + 'preview.png', 'rb') {|preview_file| skin.icon = preview_file}
skin.icon.attach(io: File.open("#{version_dir}preview.png", "rb"), content_type: "image/png", filename: "preview.png")
Dismissed Show dismissed Hide dismissed
skin.save!
skins << skin
end
Expand All @@ -490,7 +499,7 @@ def self.load_site_css
top_skin = Skin.new(title: "Archive #{version}", css: "", description: "Version #{version} of the default Archive style.",
public: true, role: "site", media: ["screen"])
end
File.open(version_dir + 'preview.png', 'rb') {|preview_file| top_skin.icon = preview_file}
top_skin.icon.attach(io: File.open("#{version_dir}preview.png", "rb"), content_type: "image/png", filename: "preview.png")
Dismissed Show dismissed Hide dismissed
top_skin.official = true
top_skin.save!
skins.each_with_index do |skin, index|
Expand Down Expand Up @@ -579,8 +588,6 @@ def set_thumbnail_from_current_version
self.class.site_skins_dir + "preview.png"
end

File.open(icon_path) do |icon_file|
self.icon = icon_file
end
self.icon.attach(io: File.open(icon_path), content_type: "image/png", filename: "preview.png")
end
end
6 changes: 5 additions & 1 deletion app/models/work_skin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ def self.basic_formatting
def self.import_basic_formatting
css = File.read(File.join(Rails.public_path, "/stylesheets/work_skins/basic_formatting.css"))
skin = WorkSkin.find_or_create_by(title: "Basic Formatting", css: css, role: "user", public: true, official: true)
File.open(File.join(Rails.public_path, '/images/skins/previews/basic_formatting.png'), 'rb') {|preview_file| skin.icon = preview_file}
skin.icon.attach(
io: File.open(File.join(Rails.public_path, "/images/skins/previews/basic_formatting.png"), "rb"),
filename: "basic_formatting.png",
content_type: "image/png"
)
skin.official = true
skin.save!
skin
Expand Down
2 changes: 1 addition & 1 deletion app/views/challenge_claims/_unposted_claim_blurb.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
</p>

<div class="icon">
<%= image_tag(claim.collection.icon.url(:standard), :size => "100x100", :alt => claim.collection.icon_alt_text, :class => "icon", skip_pipeline: true) %>
<%= image_tag(standard_icon_url(claim.collection), size: "100x100", alt: claim.collection.icon_alt_text, class: "icon", skip_pipeline: true) %>
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
</div>
</div>

Expand Down
2 changes: 1 addition & 1 deletion app/views/collection_items/_item_fields.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
<p class="datetime"><%= collection_item.item_date.to_date %></p>
<% end %>
<div class="icon">
<%= image_tag(collection_item.collection.icon.url(:standard), size: "100x100", alt: collection_item.collection.icon_alt_text, class: "icon", skip_pipeline: true) %>
<%= image_tag(standard_icon_url(collection_item.collection), size: "100x100", alt: collection_item.collection.icon_alt_text, class: "icon", skip_pipeline: true) %>
</div>
</div>

Expand Down
2 changes: 1 addition & 1 deletion app/views/collections/_collection_blurb.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
</h4>
<!--collections header iconised header image-->
<div class="icon">
<%= image_tag(collection.icon.url(:standard), :size => "100x100", :alt => collection.icon_alt_text, :class => "icon", skip_pipeline: true) %>
<%= image_tag(standard_icon_url(collection), size: "100x100", alt: collection.icon_alt_text, class: "icon", skip_pipeline: true) %>
</div>
<p class="datetime"><%= set_format_for_date(collection.updated_at) %></p>
<% if collection.all_moderators.length > 0%>
Expand Down
4 changes: 2 additions & 2 deletions app/views/collections/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,15 @@
<ul class="notes">
<% unless @collection.new_record? %>
<li>
<%= image_tag(@collection.icon.url(:standard), size: "100x100", alt: @collection.icon_alt_text, skip_pipeline: true) %>
<%= image_tag(standard_icon_url(@collection), size: "100x100", alt: @collection.icon_alt_text, skip_pipeline: true) %>
<%= ts("This is the collection's icon.") %>
</li>
<% end %>
<li><%= ts("Each collection can have one icon") %></li>
<li><%= ts("Icons can be in png, jpeg or gif form") %></li>
<li><%= ts("Icons should be sized 100x100 pixels for best results") %></li>
</ul>
<% if @collection.icon_file_name %>
<% if @collection.icon.attached? %>
<%= collection_form.check_box :delete_icon, {:checked => false} %>
<%= collection_form.label :delete_icon, ts("Delete collection icon and revert to our default") %>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/collections/_header.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<div class="primary header module">
<h2 class="heading"><%= link_to_unless_current(@collection.title, @collection) %></h2>
<div class="icon">
<%= image_tag(@collection.icon.url(:standard), size: "100x100", alt: collection.icon_alt_text, skip_pipeline: true) %>
<%= image_tag(standard_icon_url(@collection), size: "100x100", alt: collection.icon_alt_text, skip_pipeline: true) %>
</div>
<!--/descriptions-->

Expand Down
Loading
Loading