From c2332d7505379c30e11d295d90f5224385736993 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 1 Jun 2023 10:00:01 +1000 Subject: [PATCH] FEATURE: reduce avatar sizes to 6 from 20 (#21319) * FEATURE: reduce avatar sizes to 6 from 20 This PR introduces 3 changes: 1. SiteSetting.avatar_sizes, now does what is says on the tin. previously it would introduce a large number of extra sizes, to allow for various DPIs. Instead we now trust the admin with the size list. 2. When `avatar_sizes` changes, we ensure consistency and remove resized avatars that are not longer allowed per site setting. This happens on the 12 hourly job and limited out of the box to 20k cleanups per cycle, given this may reach out to AWS 20k times to remove things. 3.Our default avatar sizes are now "24|48|72|96|144|288" these sizes were very specifically picked to limit amount of bluriness introduced by webkit. Our avatars are already blurry due to 1px border, so this corrects old blur. This change heavily reduces storage required by forums which simplifies site moves and more. Co-authored-by: David Taylor --- .../discourse/app/lib/utilities.js | 34 +++++++++++++++---- .../tests/unit/lib/utilities-test.js | 27 ++++++++++----- .../tests/unit/models/report-test.js | 4 +-- app/models/user_avatar.rb | 25 +++++++++++++- config/site_settings.yml | 3 +- lib/discourse.rb | 11 ++---- lib/pretty_text.rb | 1 + lib/pretty_text/shims.js | 5 +-- .../spec/integration/post_chat_quote_spec.rb | 4 +-- plugins/chat/spec/models/chat/message_spec.rb | 12 +++---- spec/lib/discourse_spec.rb | 24 ++----------- spec/lib/oneboxer_spec.rb | 2 +- spec/lib/pretty_text_spec.rb | 8 ++--- spec/models/badge_spec.rb | 10 ++++-- spec/models/user_avatar_spec.rb | 31 +++++++++++++++-- spec/models/user_spec.rb | 23 +++++++------ spec/requests/api/users_spec.rb | 2 +- spec/requests/user_avatars_controller_spec.rb | 4 +-- spec/services/user_anonymizer_spec.rb | 4 +-- spec/services/username_changer_spec.rb | 20 +++++------ 20 files changed, 159 insertions(+), 95 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/utilities.js b/app/assets/javascripts/discourse/app/lib/utilities.js index 75ba7f02364..4b6c57aa573 100644 --- a/app/assets/javascripts/discourse/app/lib/utilities.js +++ b/app/assets/javascripts/discourse/app/lib/utilities.js @@ -20,17 +20,17 @@ export function splitString(str, separator = ",") { export function translateSize(size) { switch (size) { case "tiny": - return 20; + return 24; case "small": - return 25; + return 24; case "medium": - return 32; + return 48; case "large": - return 45; + return 48; case "extra_large": - return 60; + return 96; case "huge": - return 120; + return 144; } return size; } @@ -62,11 +62,31 @@ export function avatarUrl(template, size, { customGetURL } = {}) { if (!template) { return ""; } - const rawSize = getRawSize(translateSize(size)); + const rawSize = getRawAvatarSize(translateSize(size)); const templatedPath = template.replace(/\{size\}/g, rawSize); return (customGetURL || getURLWithCDN)(templatedPath); } +let allowedSizes = null; + +export function getRawAvatarSize(size) { + allowedSizes ??= + helperContext() + .siteSettings["avatar_sizes"].split("|") + .map((s) => parseInt(s, 10)) + .sort((a, b) => a - b); + + size = getRawSize(size); + + for (let i = 0; i < allowedSizes.length; i++) { + if (allowedSizes[i] >= size) { + return allowedSizes[i]; + } + } + + return allowedSizes[allowedSizes.length - 1]; +} + export function getRawSize(size) { const pixelRatio = window.devicePixelRatio || 1; let rawSize = 1; diff --git a/app/assets/javascripts/discourse/tests/unit/lib/utilities-test.js b/app/assets/javascripts/discourse/tests/unit/lib/utilities-test.js index 281ae9b189f..9da560adb2a 100644 --- a/app/assets/javascripts/discourse/tests/unit/lib/utilities-test.js +++ b/app/assets/javascripts/discourse/tests/unit/lib/utilities-test.js @@ -8,7 +8,7 @@ import { escapeExpression, extractDomainFromUrl, fillMissingDates, - getRawSize, + getRawAvatarSize, inCodeBlock, initializeDefaultHomepage, mergeSortedLists, @@ -86,17 +86,26 @@ module("Unit | Utilities", function (hooks) { ); }); + test("getRawAvatarSize avoids redirects", function (assert) { + assert.strictEqual( + getRawAvatarSize(1), + 24, + "returns the first size larger on the menu" + ); + + assert.strictEqual(getRawAvatarSize(2000), 288, "caps at highest"); + }); + test("avatarUrl", function (assert) { - let rawSize = getRawSize; assert.blank(avatarUrl("", "tiny"), "no template returns blank"); assert.strictEqual( avatarUrl("/fake/template/{size}.png", "tiny"), - "/fake/template/" + rawSize(20) + ".png", + "/fake/template/" + getRawAvatarSize(24) + ".png", "simple avatar url" ); assert.strictEqual( avatarUrl("/fake/template/{size}.png", "large"), - "/fake/template/" + rawSize(45) + ".png", + "/fake/template/" + getRawAvatarSize(48) + ".png", "different size" ); @@ -104,7 +113,9 @@ module("Unit | Utilities", function (hooks) { assert.strictEqual( avatarUrl("/fake/template/{size}.png", "large"), - "https://app-cdn.example.com/fake/template/" + rawSize(45) + ".png", + "https://app-cdn.example.com/fake/template/" + + getRawAvatarSize(48) + + ".png", "uses CDN if present" ); }); @@ -124,7 +135,7 @@ module("Unit | Utilities", function (hooks) { let avatarTemplate = "/path/to/avatar/{size}.png"; assert.strictEqual( avatarImg({ avatarTemplate, size: "tiny" }), - "", + "", "it returns the avatar html" ); @@ -134,7 +145,7 @@ module("Unit | Utilities", function (hooks) { size: "tiny", title: "evilest trout", }), - "", + "", "it adds a title if supplied" ); @@ -144,7 +155,7 @@ module("Unit | Utilities", function (hooks) { size: "tiny", extraClasses: "evil fish", }), - "", + "", "it adds extra classes if supplied" ); diff --git a/app/assets/javascripts/discourse/tests/unit/models/report-test.js b/app/assets/javascripts/discourse/tests/unit/models/report-test.js index c626a5a50b1..8e01d05762d 100644 --- a/app/assets/javascripts/discourse/tests/unit/models/report-test.js +++ b/app/assets/javascripts/discourse/tests/unit/models/report-test.js @@ -370,7 +370,7 @@ module("Unit | Model | report", function (hooks) { const computedUsernameLabel = usernameLabel.compute(row); assert.strictEqual( computedUsernameLabel.formattedValue, - "joffrey" + "joffrey" ); assert.strictEqual(computedUsernameLabel.value, "joffrey"); @@ -459,7 +459,7 @@ module("Unit | Model | report", function (hooks) { const userLink = computedLabels[0].compute(row).formattedValue; assert.strictEqual( userLink, - "joffrey" + "joffrey" ); }); }); diff --git a/app/models/user_avatar.rb b/app/models/user_avatar.rb index 838b5cd182e..fa830b3fa46 100644 --- a/app/models/user_avatar.rb +++ b/app/models/user_avatar.rb @@ -150,7 +150,7 @@ class UserAvatar < ActiveRecord::Base tempfile.close! if tempfile && tempfile.respond_to?(:close!) end - def self.ensure_consistency! + def self.ensure_consistency!(max_optimized_avatars_to_remove: 20_000) DB.exec <<~SQL UPDATE user_avatars SET gravatar_upload_id = NULL @@ -174,6 +174,29 @@ class UserAvatar < ActiveRecord::Base up.id IS NULL ) SQL + + ids = + DB.query_single(<<~SQL, sizes: Discourse.avatar_sizes, limit: max_optimized_avatars_to_remove) + SELECT oi.id FROM user_avatars a + JOIN optimized_images oi ON oi.upload_id = a.custom_upload_id + LEFT JOIN upload_references ur ON ur.upload_id = a.custom_upload_id and ur.target_type <> 'UserAvatar' + WHERE oi.width not in (:sizes) AND oi.height not in (:sizes) AND ur.upload_id IS NULL + LIMIT :limit + SQL + + warnings_reported = 0 + + ids.each do |id| + begin + OptimizedImage.find(id).destroy! + rescue ActiveRecord::RecordNotFound + rescue => e + if warnings_reported < 10 + Discourse.warn_exception(e, message: "Failed to remove optimized image") + warnings_reported += 1 + end + end + end end end diff --git a/config/site_settings.yml b/config/site_settings.yml index 98ff694b03c..5cc215c0e27 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1467,9 +1467,10 @@ files: type: url_list client: true avatar_sizes: - default: "20|25|32|45|60|120" + default: "24|48|72|96|144|288" type: list list_type: compact + client: true external_system_avatars_enabled: default: true client: true diff --git a/lib/discourse.rb b/lib/discourse.rb index 156812a31b2..97c1954e1b8 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -327,19 +327,12 @@ module Discourse @anonymous_top_menu_items ||= Discourse.anonymous_filters + %i[categories top] end + # list of pixel ratios Discourse tries to optimize for PIXEL_RATIOS ||= [1, 1.5, 2, 3] def self.avatar_sizes # TODO: should cache these when we get a notification system for site settings - set = Set.new - - SiteSetting - .avatar_sizes - .split("|") - .map(&:to_i) - .each { |size| PIXEL_RATIOS.each { |pixel_ratio| set << (size * pixel_ratio).to_i } } - - set + Set.new(SiteSetting.avatar_sizes.split("|").map(&:to_i)) end def self.activate_plugins! diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index 91d9b304791..4a668a30bfc 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -208,6 +208,7 @@ module PrettyText __optInput.watchedWordsReplace = #{WordWatcher.word_matcher_regexps(:replace, engine: :js).to_json}; __optInput.watchedWordsLink = #{WordWatcher.word_matcher_regexps(:link, engine: :js).to_json}; __optInput.additionalOptions = #{Site.markdown_additional_options.to_json}; + __optInput.avatar_sizes = #{SiteSetting.avatar_sizes.to_json}; JS buffer << "__optInput.topicId = #{opts[:topic_id].to_i};\n" if opts[:topic_id] diff --git a/lib/pretty_text/shims.js b/lib/pretty_text/shims.js index 793a3992eb5..37f3e492382 100644 --- a/lib/pretty_text/shims.js +++ b/lib/pretty_text/shims.js @@ -16,10 +16,11 @@ define("I18n", ["exports"], function (exports) { exports.default = I18n; }); -// Formatting doesn't currently need any helper context define("discourse-common/lib/helpers", ["exports"], function (exports) { exports.helperContext = function () { - return {}; + return { + siteSettings: { avatar_sizes: __optInput.avatar_sizes }, + }; }; }); diff --git a/plugins/chat/spec/integration/post_chat_quote_spec.rb b/plugins/chat/spec/integration/post_chat_quote_spec.rb index 2ffe97df0b5..c3343d1c199 100644 --- a/plugins/chat/spec/integration/post_chat_quote_spec.rb +++ b/plugins/chat/spec/integration/post_chat_quote_spec.rb @@ -237,7 +237,7 @@ martin
- +
#{message1.user.username}
@@ -251,7 +251,7 @@ martin
- +
#{message2.user.username}
diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index 561e9256dea..5ddfb92adaa 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/plugins/chat/spec/models/chat/message_spec.rb @@ -75,7 +75,7 @@ describe Chat::Message do post = Fabricate(:post, topic: topic) SiteSetting.external_system_avatars_enabled = false avatar_src = - "//test.localhost#{User.system_avatar_template(post.user.username).gsub("{size}", "40")}" + "//test.localhost#{User.system_avatar_template(post.user.username).gsub("{size}", "48")}" cooked = described_class.cook(<<~RAW) [quote="#{post.user.username}, post:#{post.post_number}, topic:#{topic.id}"] @@ -87,7 +87,7 @@ describe Chat::Message do
- +
chatbbcodeuser
@@ -150,7 +150,7 @@ describe Chat::Message do
- +
otherbbcodeuser
diff --git a/spec/lib/discourse_spec.rb b/spec/lib/discourse_spec.rb index f07eaee1071..2504a12169a 100644 --- a/spec/lib/discourse_spec.rb +++ b/spec/lib/discourse_spec.rb @@ -13,28 +13,8 @@ RSpec.describe Discourse do describe "avatar_sizes" do it "returns a list of integers" do - expect(Discourse.avatar_sizes).to contain_exactly( - 20, - 25, - 30, - 32, - 37, - 40, - 45, - 48, - 50, - 60, - 64, - 67, - 75, - 90, - 96, - 120, - 135, - 180, - 240, - 360, - ) + SiteSetting.avatar_sizes = "10|20|30" + expect(Discourse.avatar_sizes).to contain_exactly(10, 20, 30) end end diff --git a/spec/lib/oneboxer_spec.rb b/spec/lib/oneboxer_spec.rb index ed7cf50c2e9..4cc6472a6f5 100644 --- a/spec/lib/oneboxer_spec.rb +++ b/spec/lib/oneboxer_spec.rb @@ -94,7 +94,7 @@ RSpec.describe Oneboxer do onebox = preview(public_reply.url, user, public_category, public_topic) expect(onebox).not_to include(public_topic.title) - expect(onebox).to include(replier.avatar_template_url.sub("{size}", "40")) + expect(onebox).to include(replier.avatar_template_url.sub("{size}", "48")) expect(preview(public_hidden.url, user, public_category)).to match_html( link(public_hidden.url), diff --git a/spec/lib/pretty_text_spec.rb b/spec/lib/pretty_text_spec.rb index 3dc294aca2d..2848fca3a82 100644 --- a/spec/lib/pretty_text_spec.rb +++ b/spec/lib/pretty_text_spec.rb @@ -251,7 +251,7 @@ RSpec.describe PrettyText do

ddd

@@ -273,7 +273,7 @@ RSpec.describe PrettyText do

ddd

@@ -294,7 +294,7 @@ RSpec.describe PrettyText do

ddd

@@ -320,7 +320,7 @@ RSpec.describe PrettyText do

quoted post

@@ -477,7 +477,7 @@ RSpec.describe UsernameChanger do

quoted post

@@ -485,7 +485,7 @@ RSpec.describe UsernameChanger do