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 20 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
9 changes: 9 additions & 0 deletions .github/workflows/automated-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,15 @@ jobs:
arguments: spec/models
- command: rspec
arguments: --exclude-pattern 'spec/{controllers,models}/**/*.rb'
libvips: true
- 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 +76,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 +134,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
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: { default_pseud: { icon_attachment: :blob }, pseuds: { icon_attachment: :blob } })
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
.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
22 changes: 18 additions & 4 deletions app/controllers/collections_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,23 @@ 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
.merge(Collection.with_attached_icon)
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
.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
.merge(Collection.with_attached_icon)
.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
.merge(Collection.with_attached_icon)
.paginate(page: params[:page])
@page_subtitle = ts("%{username} - Collections", username: @user.login)
else
if params[:user_id]
Expand All @@ -44,7 +56,9 @@ def index
params[:sort_column] = "collections.created_at" if !valid_sort_column(params[:sort_column], 'collection')
params[:sort_direction] = 'DESC' if !valid_sort_direction(params[:sort_direction])
sort = params[:sort_column] + " " + params[:sort_direction]
@collections = Collection.sorted_and_filtered(sort, params[:collection_filters], params[:page])
@collections = Collection.for_blurb
.merge(Collection.with_attached_icon)
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
.sorted_and_filtered(sort, params[:collection_filters], params[:page])
end
end

Expand Down
4 changes: 3 additions & 1 deletion app/controllers/muted/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ 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: { default_pseud: { icon_attachment: :blob }, pseuds: { icon_attachment: :blob } })
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
.merge(Pseud.with_attached_icon)
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
.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.with_attached_icon.sort_by_recent.includes(:author)
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved
@title = ts('My Work Skins')
else
@skins = @user.skins.site_skins.sort_by_recent
@skins = @user.skins.site_skins.with_attached_icon.sort_by_recent.includes(:author)
@title = ts('My Site Skins')
end
else
if is_work_skin
@skins = WorkSkin.approved_skins.sort_by_recent_featured
@skins = WorkSkin.approved_skins.with_attached_icon.sort_by_recent_featured.includes(:author)
@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.with_attached_icon.sort_by_recent_featured
else
Skin.approved_skins.usable.site_skins.cached.with_attached_icon.sort_by_recent_featured
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

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
27 changes: 16 additions & 11 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 @@ -171,6 +171,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]) }
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved

before_validation :cleanup_url
def cleanup_url
Expand Down Expand Up @@ -373,7 +374,7 @@ def self.sorted_and_filtered(sort, filters, page)
pagination_args = {page: page}

# build up the query with scopes based on the options the user specifies
query = Collection.top_level
query = Collection.with_attached_icon.top_level
brianjaustin marked this conversation as resolved.
Show resolved Hide resolved

if !filters[:title].blank?
# we get the matching collections out of autocomplete and use their ids
Expand Down Expand Up @@ -419,6 +420,10 @@ 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
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