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 29 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
20 changes: 8 additions & 12 deletions .github/workflows/automated-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,22 +54,19 @@ jobs:
arguments: db:otwseed
- command: rspec
arguments: spec/controllers
imagemagick: true
- command: rspec
arguments: spec/models
imagemagick: true
- command: rspec
arguments: --exclude-pattern 'spec/{controllers,models}/**/*.rb'
imagemagick: true
libvips: true
- command: cucumber
arguments: features/admins
imagemagick: true
libvips: true
- command: cucumber
arguments: features/bookmarks
imagemagick: true
- command: cucumber
arguments: features/collections
imagemagick: true
libvips: true
- command: cucumber
arguments: features/comments_and_kudos
- command: cucumber
Expand All @@ -79,10 +76,10 @@ jobs:
vcr: true
- command: cucumber
arguments: features/other_a
imagemagick: true
libvips: true
- command: cucumber
arguments: features/other_b
imagemagick: true
libvips: true
- command: cucumber
arguments: features/prompt_memes_a
- command: cucumber
Expand All @@ -100,7 +97,6 @@ jobs:
- command: cucumber
arguments: features/works
ebook: true
imagemagick: true

steps:
- name: Check out code
Expand Down Expand Up @@ -138,9 +134,9 @@ jobs:
restore-keys: |
cassette-library-${{ hashFiles(matrix.tests.arguments) }}-

- name: Install imagemagick for image processing
if: ${{ matrix.tests.imagemagick }}
run: sudo apt-get install -y imagemagick
- 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
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 @@ -183,3 +182,5 @@ group :staging, :production do
gem "sentry-rails"
gem "sentry-resque"
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 @@ -320,15 +320,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 @@ -363,6 +360,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.8)
minitest (5.25.1)
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 @@ -683,8 +683,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
3 changes: 2 additions & 1 deletion app/controllers/blocked/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ class UsersController < ApplicationController
# GET /users/:user_id/blocked/users
def index
@blocks = @user.blocks_as_blocker
.joins(:blocked).includes(blocked: :default_pseud)
.joins(:blocked)
.includes(blocked: [:pseuds, { default_pseud: { icon_attachment: :blob } }])
.order(created_at: :desc).order(id: :desc).page(params[:page])

@pseuds = @blocks.map { |b| b.blocked.default_pseud }
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/collection_items_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def index
@collection_items.unreviewed_by_collection
end
elsif params[:user_id] && (@user = User.find_by(login: params[:user_id])) && @user == current_user
@collection_items = CollectionItem.for_user(@user).includes(:collection)
@collection_items = CollectionItem.for_user(@user).includes(:collection).merge(Collection.with_attached_icon)
@collection_items = case params[:status]
when "approved"
@collection_items.approved_by_both
Expand Down
15 changes: 12 additions & 3 deletions app/controllers/collections_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,20 @@ def load_collection_from_id

def index
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up deleting the N+1 spec for this method, as it was flagging something unrelated and we want to move collections to Elasticsearch anyways (also I'd like to keep the PR from getting too much out of hand 🙈)

if params[:work_id] && (@work = Work.find_by(id: params[:work_id]))
@collections = @work.approved_collections.by_title.includes(:parent, :moderators, :children, :collection_preference, owners: [:user]).paginate(page: params[:page])
@collections = @work.approved_collections
.by_title
.for_blurb
.paginate(page: params[:page])
elsif params[:collection_id] && (@collection = Collection.find_by(name: params[:collection_id]))
@collections = @collection.children.by_title.includes(:parent, :moderators, :children, :collection_preference, owners: [:user]).paginate(page: params[:page])
@collections = @collection.children
.by_title
.for_blurb
.paginate(page: params[:page])
elsif params[:user_id] && (@user = User.find_by(login: params[:user_id]))
@collections = @user.maintained_collections.by_title.includes(:parent, :moderators, :children, :collection_preference, owners: [:user]).paginate(page: params[:page])
@collections = @user.maintained_collections
.by_title
.for_blurb
.paginate(page: params[:page])
@page_subtitle = ts("%{username} - Collections", username: @user.login)
else
if params[:user_id]
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/muted/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ class UsersController < ApplicationController
# GET /users/:user_id/muted/users
def index
@mutes = @user.mutes_as_muter
.joins(:muted).includes(muted: :default_pseud)
.joins(:muted)
.includes(muted: [:pseuds, { default_pseud: { icon_attachment: :blob } }])
.order(created_at: :desc).order(id: :desc).page(params[:page])

@pseuds = @mutes.map { |b| b.muted.default_pseud }
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/people_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ def search
else
options = people_search_params.merge(page: params[:page])
@search = PseudSearchForm.new(options)
@people = @search.search_results
@people = @search.search_results.scope(:for_search)
flash_search_warnings(@people)
end
end

def index
if @collection.present?
@people = @collection.participants.order(:name).page(params[:page])
@people = @collection.participants.with_attached_icon.includes(:user).order(:name).page(params[:page])
@rec_counts = Pseud.rec_counts_for_pseuds(@people)
@work_counts = Pseud.work_counts_for_pseuds(@people)
else
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/pseuds_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def load_user
# GET /pseuds.xml
def index
if @user
@pseuds = @user.pseuds.alphabetical.paginate(page: params[:page])
@pseuds = @user.pseuds.with_attached_icon.alphabetical.paginate(page: params[:page])
@rec_counts = Pseud.rec_counts_for_pseuds(@pseuds)
@work_counts = Pseud.work_counts_for_pseuds(@pseuds)
@page_subtitle = @user.login
Expand Down
17 changes: 8 additions & 9 deletions app/controllers/skins_controller.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
class SkinsController < ApplicationController

before_action :users_only, only: [:new, :create, :destroy]
before_action :load_skin, except: [:index, :new, :create, :unset]
before_action :check_title, only: [:create, :update]
Expand All @@ -23,22 +22,22 @@ def index
redirect_to skins_path and return
end
if is_work_skin
@skins = @user.work_skins.sort_by_recent
@skins = @user.work_skins.sort_by_recent.includes(:author).with_attached_icon
@title = ts('My Work Skins')
else
@skins = @user.skins.site_skins.sort_by_recent
@skins = @user.skins.site_skins.sort_by_recent.includes(:author).with_attached_icon
@title = ts('My Site Skins')
end
else
if is_work_skin
@skins = WorkSkin.approved_skins.sort_by_recent_featured
@skins = WorkSkin.approved_skins.sort_by_recent_featured.includes(:author).with_attached_icon
@title = ts('Public Work Skins')
else
if logged_in?
@skins = Skin.approved_skins.usable.site_skins.sort_by_recent_featured
else
@skins = Skin.approved_skins.usable.site_skins.cached.sort_by_recent_featured
end
@skins = if logged_in?
Skin.approved_skins.usable.site_skins.sort_by_recent_featured.with_attached_icon
else
Skin.approved_skins.usable.site_skins.cached.sort_by_recent_featured.with_attached_icon
end
@title = ts('Public Site Skins')
end
end
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

# Fetches the current skin. This is determined by the following
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
29 changes: 17 additions & 12 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: %r{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 @@ -165,6 +165,7 @@ def title
scope :prompt_meme, -> { where(challenge_type: "PromptMeme") }
scope :name_only, -> { select("collections.name") }
scope :by_title, -> { order(:title) }
scope :for_blurb, -> { includes(:parent, :moderators, :children, :collection_preference, owners: [:user]).with_attached_icon }

def cleanup_url
self.header_image_url = Addressable::URI.heuristic_parse(self.header_image_url) if self.header_image_url
Expand Down Expand Up @@ -339,7 +340,7 @@ def maintainers_list

def collection_email
return self.email if self.email.present?
return parent.email if parent && parent.email.present?
return parent.email if parent && parent.email.present?
end

def notify_maintainers_assignments_sent
Expand Down Expand Up @@ -429,7 +430,7 @@ def self.sorted_and_filtered(sort, filters, page)
query = query.no_challenge
end
end
query = query.order(sort)
query = query.order(sort).for_blurb

if filters[:fandom].blank?
query.paginate(pagination_args)
Expand All @@ -454,6 +455,10 @@ def delete_icon
alias 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
self.icon_comment_text = nil
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
Loading
Loading