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

Merged
merged 34 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
34 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
a77c384
Merge upstream
brianjaustin Dec 27, 2024
0662741
Proxy requests so we can cache via CF
brianjaustin Dec 27, 2024
523f333
Cleanup imagemagick
brianjaustin Dec 28, 2024
178be24
Allow after task to skip 403s to fix later
brianjaustin Dec 30, 2024
6529a97
Rescue more errors
brianjaustin Dec 30, 2024
297e0ac
More generic error
brianjaustin Dec 30, 2024
60e2389
Fix pseuds task typo
brianjaustin Dec 30, 2024
99b4510
Rubocop
brianjaustin Dec 30, 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
12 changes: 12 additions & 0 deletions app/helpers/collections_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,16 @@ 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

# Wraps the collection's standard_icon_url in an image tag
def collection_icon_display(collection)
image_tag(standard_icon_url(collection), size: "100x100", alt: collection.icon_alt_text, class: "icon", skip_pipeline: true)
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
14 changes: 5 additions & 9 deletions app/helpers/users_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,17 @@ def print_pseuds(user)
end

# 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
def standard_icon(pseud = nil)
return "/images/skins/iconsets/default/icon_user.png" unless pseud&.icon&.attached?

pseud.icon.variant(:standard).processed.url
end

# no alt text if there isn't specific alt text
def icon_display(user = nil, pseud = nil)
path = user ? (pseud ? user_pseud_path(pseud.user, pseud) : user_path(user)) : nil
pseud ||= user.default_pseud if user
icon = standard_icon(user, pseud)
icon = standard_icon(pseud)
alt_text = pseud.try(:icon_alt_text) || nil

if path
Expand Down
23 changes: 13 additions & 10 deletions app/models/collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@ 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_limit: [100, 100])
end

validates_attachment_content_type :icon, content_type: /image\/\S+/, allow_nil: true
validates_attachment_size :icon, less_than: 500.kilobytes, allow_nil: true
# i18n-tasks-use t("errors.attributes.icon.invalid_format")
# i18n-tasks-use t("errors.attributes.icon.too_large")
validates :icon, attachment: {
allowed_formats: %r{image/\S+},
maximum_size: ArchiveConfig.ICON_SIZE_KB_MAX.kilobytes
}

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 +419,9 @@ def delete_icon
alias_method :delete_icon?, :delete_icon

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

self.icon.purge
self.icon_alt_text = nil
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
end
end
2 changes: 1 addition & 1 deletion app/models/comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def check_for_spam
includes(
pseud: { user: [:roles, :block_of_current_user, :block_by_current_user, :preference] },
parent: { work: [:pseuds, :users] }
)
).merge(Pseud.with_attached_icon)
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
}

# Gets methods and associations from acts_as_commentable plugin
Expand Down
30 changes: 11 additions & 19 deletions app/models/pseud.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,16 @@ 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"

validates_attachment_content_type :icon,
content_type: %w[image/gif image/jpeg image/png],
allow_nil: true
has_one_attached :icon do |attachable|
attachable.variant(:standard, resize_to_limit: [100, 100])
end

validates_attachment_size :icon, less_than: 500.kilobytes, allow_nil: true
# i18n-tasks-use t("errors.attributes.icon.invalid_format")
# i18n-tasks-use t("errors.attributes.icon.too_large")
validates :icon, attachment: {
allowed_formats: %w[image/gif image/jpeg image/png],
maximum_size: ArchiveConfig.ICON_SIZE_KB_MAX.kilobytes
}

NAME_LENGTH_MIN = 1
NAME_LENGTH_MAX = 40
Expand Down Expand Up @@ -419,8 +411,8 @@ def delete_icon

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

self.icon.purge
self.icon_alt_text = nil
end

Expand Down
30 changes: 15 additions & 15 deletions app/models/skin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,16 @@ 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_limit: [100, 100])
end

# i18n-tasks-use t("errors.attributes.icon.invalid_format")
# i18n-tasks-use t("errors.attributes.icon.too_large")
validates :icon, attachment: {
allowed_formats: %r{image/\S+},
maximum_size: ArchiveConfig.ICON_SIZE_KB_MAX.kilobytes
}

after_save :skin_invalidate_cache
def skin_invalidate_cache
Expand All @@ -70,8 +73,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 +103,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 +478,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 +492,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 +581,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
24 changes: 24 additions & 0 deletions app/validators/attachment_validator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# frozen_string_literal: true

# Custom validator to ensure that a field using ActiveStorage
# * matches the given formats, specified with regex or by a list (leave empty to allow any)
# * is less than the given maximum (if none is given, the default is 500kb)
class AttachmentValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
return unless value&.attached?

allowed_formats = options[:allowed_formats]
maximum_size = options[:maximum_size] || 500.kilobytes

case allowed_formats
when Regexp
record.errors.add(attribute, :invalid_format) unless allowed_formats.match?(value.content_type)
when Array
record.errors.add(attribute, :invalid_format) unless allowed_formats.include?(value.content_type)
end

record.errors.add(attribute, :too_large, size_limit_kb: size_limit_kb) unless value.blob.byte_size < maximum_size
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved

value.purge if record.errors[attribute].any?
end
end
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) %>
<%= collection_icon_display(claim.collection) %>
</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) %>
<%= collection_icon_display(collection_item.collection) %>
</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) %>
<%= collection_icon_display(collection) %>
</div>
<p class="datetime"><%= set_format_for_date(collection.updated_at) %></p>
<% if collection.all_moderators.length > 0%>
Expand Down
Loading
Loading