Forem Creation: Logo Upload & Resizing (#15499)

* wip - Got logo upload working

* Now have the logo rendering in the header and in the admin image config section.

* Small layout tweak for admin -> config -> images -> logo.

* feat: create a logo uploader with some tests

* feat: use the logoUploader instead of the ArticleImageUploader

* feat: return early because svg's do not contain exif or gps data

* chore: we can move the raise outside the transaction as the rest of the transaction won't execute if we raise an error

* feat: add a size range

* WIP: resize an image to a random number for now

* hid the logo behind a feature flag and kept logo_svg as is in the site header.

* Added the jpe file type to the logo uploader.

* Skipped the resizing of an image if it's an SVG in the logo uploader.

* Added content types to the content type logo uploader allow list.

* Synced logo validation with frontend and backend.

* Removed unnecessary ALLOWED_PARAMS elements.

* feat: update the logo upoader and tests

* chore: remove comments

* chore: remove comments

* feat: update the resizing for the images + add the correct content type

* spec: test the versions

* fix: update the Constant

* feat: add the versions of the logo

* feat: populate the settings correctly and consistently

* feat: add an random string to the file name to avoid caching issues

* feat: amend the logo layout

* chore: remove comments

* spec: update

* feat: image type whitelist

* feat: update the logo css and  also just use resized_logo and remove mobile resize

* feat: add a max-height

* only add site-logo if the feature flag is off

* Renamed IMAGE_TYPE_WHITELIST to IMAGE_TYPE_ALLOWLIST

* Update app/controllers/admin/creator_settings_controller.rb

Co-authored-by: Michael Kohl <citizen428@forem.com>

* Update app/uploaders/logo_uploader.rb

Co-authored-by: Michael Kohl <citizen428@forem.com>

* Update app/uploaders/logo_uploader.rb

Co-authored-by: Michael Kohl <citizen428@forem.com>

* Update app/uploaders/logo_uploader.rb

Co-authored-by: Michael Kohl <citizen428@forem.com>

* Update spec/uploaders/logo_uploader_spec.rb

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* Update spec/uploaders/logo_uploader_spec.rb

Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

* chore: revert admin change

* refactor: use a static value for directory

* feat: freeze constants

* feat: remove the logo requirement

* chore: spacing

* remove logo requirement

Co-authored-by: Ridhwana <ridhwana.khan16@gmail.com>
Co-authored-by: Michael Kohl <citizen428@forem.com>
Co-authored-by: Julianna Tetreault <32834804+juliannatetreault@users.noreply.github.com>

app/assets/stylesheets/base/main.scss

@@ -64,6 +64,7 @@ a {
 // Logo
 .site-logo {
   max-width: var(--max-width, 125px);
+  max-height: 40px;
   font-size: var(--font-size, var(--fs-base));
   font-weight: var(--font-weight, var(--fw-medium));
 

app/controllers/admin/creator_settings_controller.rb

@@ -1,18 +1,28 @@
 module Admin
   class CreatorSettingsController < Admin::ApplicationController
-    ALLOWED_PARAMS = %i[community_name logo_svg primary_brand_color_hex invite_only_mode public checked_code_of_conduct
-                        checked_terms_and_conditions].freeze
+    ALLOWED_PARAMS = %i[checked_code_of_conduct checked_terms_and_conditions community_name
+                        invite_only_mode logo primary_brand_color_hex public].freeze
 
-    def new; end
+    def new
+      @max_file_size = LogoUploader::MAX_FILE_SIZE
+      @logo_allowed_types = (LogoUploader::CONTENT_TYPE_ALLOWLIST +
+        LogoUploader::EXTENSION_ALLOWLIST.map { |extension| ".#{extension}" }).join(",")
+    end
 
     def create
       extra_authorization
       ActiveRecord::Base.transaction do
         ::Settings::Community.community_name = settings_params[:community_name]
-        ::Settings::General.logo_svg = settings_params[:logo_svg]
         ::Settings::UserExperience.primary_brand_color_hex = settings_params[:primary_brand_color_hex]
         ::Settings::Authentication.invite_only_mode = settings_params[:invite_only]
         ::Settings::UserExperience.public = settings_params[:public]
+
+        if settings_params[:logo]
+          logo_uploader = upload_logo(settings_params[:logo])
+          ::Settings::General.original_logo = logo_uploader.url
+          # An SVG will not be resized, hence we apply the OR statements below to populate SETTINGS consistently.
+          ::Settings::General.resized_logo = logo_uploader.resized_logo.url || logo_uploader.url
+        end
       end
       current_user.update!(
         saw_onboarding: true,
@@ -34,5 +44,11 @@ module Admin
     def settings_params
       params.permit(ALLOWED_PARAMS)
     end
+
+    def upload_logo(image)
+      LogoUploader.new.tap do |uploader|
+        uploader.store!(image)
+      end
+    end
   end
 end

app/models/settings/general.rb

@@ -37,6 +37,9 @@ module Settings
 
     setting :logo_svg, type: :string
 
+    setting :original_logo, type: :string
+    setting :resized_logo, type: :string
+
     setting :enable_video_upload, type: :boolean, default: false
 
     # Mascot

app/uploaders/base_uploader.rb

@@ -26,6 +26,9 @@ class BaseUploader < CarrierWave::Uploader::Base
 
   # strip EXIF (and GPS) data
   def strip_exif
+    # svg's do not contain exif or gps data
+    return if file.content_type.include?("svg")
+
     manipulate! do |image|
       image.strip unless image.frames.count > FRAME_STRIP_MAX
       image = yield(image) if block_given?

app/uploaders/logo_uploader.rb

@@ -0,0 +1,50 @@
+class LogoUploader < BaseUploader
+  MAX_FILE_SIZE = 3.megabytes
+  STORE_DIRECTORY = "uploads/logos/".freeze
+  EXTENSION_ALLOWLIST = %w[svg png jpg jpeg jpe].freeze
+  IMAGE_TYPE_ALLOWLIST = %i[svg png jpg jpeg jpe].freeze
+  CONTENT_TYPE_ALLOWLIST = %w[image/svg+xml image/png image/jpg image/jpeg].freeze
+
+  def store_dir
+    STORE_DIRECTORY
+  end
+
+  def extension_allowlist
+    EXTENSION_ALLOWLIST
+  end
+
+  def image_type_whitelist
+    # this is needed by CarrierWave::BombShelter
+    IMAGE_TYPE_ALLOWLIST
+  end
+
+  def size_range
+    1..MAX_FILE_SIZE
+  end
+
+  def content_type_allowlist
+    CONTENT_TYPE_ALLOWLIST
+  end
+
+  def filename
+    # random_string in the filename to avoid caching issues
+    "original_logo_#{random_string}.#{file.extension}" if original_filename
+  end
+
+  version :resized_logo, if: :not_svg? do
+    process resize_to_limit: [nil, 80]
+    def full_filename(_for_file = file)
+      "resized_logo_#{random_string}.#{file.extension}" if original_filename
+    end
+  end
+
+  private
+
+  def random_string
+    SecureRandom.alphanumeric(20)
+  end
+
+  def not_svg?(file)
+    file.content_type.exclude?("svg")
+  end
+end

app/views/admin/creator_settings/_form.html.erb

@@ -8,17 +8,14 @@
 </div>
 
 <div class="crayons-field mt-6 align-left">
-  <%= label_tag :logo_svg, class: "crayons-field__label" do %>
+  <%= label_tag :logo, class: "crayons-field__label" do %>
     Logo
     <span class="crayons-field__required crayons-tooltip" data-tooltip="This will set the logo for your Forem" aria-describedby="logo-subtitle"></span>
     <p id="logo-subtitle" class="crayons-field__description">Ideally SVG, but PNG or JPEG will work, too.</p>
   <% end %>
   <div class="flex flex-1 gap-4">
-    <%= file_field_tag :logo_svg, required: true, accept: ".svg,.png,.jpg,image/svg+xml,image/png,image/jpg", data: { "max-file-size-mb": "25", action: "change->creator-settings#previewLogo" }, aria: { describedby: "logo-subtitle" } %>
+    <%= file_field_tag :logo, accept: @logo_allowed_types.to_s, data: { "max-file-size-mb": @max_file_size.to_s, action: "change->creator-settings#previewLogo" }, aria: { describedby: "logo-subtitle" } %>
     <div data-creator-settings-target="previewLogo">
-      <% if ::Settings::General.logo_svg.present? %>
-        <%= ::Settings::General.logo_svg.html_safe %>
-      <% end %>
     </div>
   </div>
 </div>

app/views/admin/creator_settings/new.html.erb

@@ -12,7 +12,7 @@
       <% end %>
     </div>
 
-    <%= form_tag(admin_creator_settings_path, method: "post", class: "relative z-elevate p-4", "data-action": "submit->creator-settings#formValidations") do %>
+    <%= form_tag(admin_creator_settings_path, method: "post", multipart: true, class: "relative z-elevate p-4", "data-action": "submit->creator-settings#formValidations") do %>
       <% if defined?(resource) && resource&.errors&.any? %>
         <div class="crayons-card crayons-card--secondary crayons-notice crayons-notice--danger" role="alert" data-testid="signup-errors">
           <div class="crayons-card__header">

app/views/layouts/_logo.html.erb

@@ -1,6 +1,8 @@
-<a href="/" class="site-logo" aria-label="<%= t("views.main.aria_home") %>">
-  <% if Settings::General.logo_svg.present? %>
-    <%= logo_svg %>
+<a href="/" class="<%= "site-logo" unless FeatureFlag.enabled?(:creator_onboarding) %>" aria-label="<%= t("views.main.aria_home") %>">
+  <% if Settings::General.logo_svg %>
+      <%= Settings::General.logo_svg %>
+  <% elsif FeatureFlag.enabled?(:creator_onboarding) %>
+      <img class="site-logo" src="<%= Settings::General.resized_logo %>" alt="<%= community_name %>">
   <% else %>
     <span class="truncate-at-2">
       <%= community_name %>

cypress/integration/creatorOnboardingFlows/creatorSettings.spec.js

@@ -80,11 +80,6 @@ describe('Creator Settings Page', () => {
       'required',
     );
 
-    cy.findByLabelText(/logo/i, { selector: 'input' }).should(
-      'have.attr',
-      'required',
-    );
-
     // should not redirect the creator to the home page when the form is not completely filled out and 'Finish' is clicked
     cy.findByRole('button', { name: 'Finish' }).click();
     cy.url().should('equal', `${baseUrl}admin/creator_settings/new`);

spec/uploaders/logo_uploader_spec.rb

@@ -0,0 +1,97 @@
+require "rails_helper"
+require "carrierwave/test/matchers"
+require "exifr/jpeg"
+
+describe LogoUploader, type: :uploader do
+  include CarrierWave::Test::Matchers
+
+  let(:image_svg) { fixture_file_upload("300x100.svg") }
+  let(:image_jpg) { fixture_file_upload("800x600.jpg", "image/jpeg") }
+  let(:image_png) { fixture_file_upload("800x600.png", "image/png") }
+  let(:image_webp) { fixture_file_upload("800x600.webp", "image/webp") }
+  let(:image_with_gps) { fixture_file_upload("image_gps_data.jpg", "image/jpeg") }
+  let(:image_gif) { fixture_file_upload("high_frame_count.gif", "image/gif") }
+
+  # we need a new uploader before each test, and since the uploader is not a model
+  # we can recreate it quickly in memory with `let!`
+  let!(:uploader) { described_class.new }
+
+  before do
+    described_class.include CarrierWave::MiniMagick # needed for processing
+    described_class.enable_processing = true
+  end
+
+  after do
+    described_class.enable_processing = false
+    uploader.remove!
+  end
+
+  it "stores files in the correct directory" do
+    expect(uploader.store_dir).to eq("uploads/logos/")
+  end
+
+  describe "formats" do
+    it "permits a set of extensions" do
+      expect(uploader.extension_allowlist).to eq(%w[svg png jpg jpeg jpe])
+    end
+
+    it "permits jpegs" do
+      uploader.store!(image_jpg)
+      expect(uploader).to be_format("jpeg")
+    end
+
+    it "permits pngs" do
+      uploader.store!(image_png)
+      expect(uploader).to be_format("png")
+    end
+
+    it "permits svgs" do
+      uploader.store!(image_svg)
+      expect(uploader).to be_format("svg")
+    end
+
+    it "rejects unsupported formats like webp" do
+      expect { uploader.store!(image_webp) }.to raise_error(CarrierWave::IntegrityError)
+    end
+
+    it "rejects unsupported formats like gifs" do
+      expect { uploader.store!(image_gif) }.to raise_error(CarrierWave::IntegrityError)
+    end
+  end
+
+  describe "error handling" do
+    it "raises a CarrierWave error which can be parsed if MiniMagick timeout occurs" do
+      allow(MiniMagick::Image).to receive(:new).and_raise(Timeout::Error)
+
+      expect { uploader.store!(image_jpg) }.to raise_error(CarrierWave::IntegrityError, /Image processing timed out/)
+    end
+  end
+
+  describe "exif removal" do
+    it "removes EXIF and GPS data on single frame image upload", :aggregate_failures do
+      expect(EXIFR::JPEG.new(image_with_gps.path).exif?).to be(true)
+      expect(EXIFR::JPEG.new(image_with_gps.path).gps.present?).to be(true)
+      uploader.store!(image_with_gps)
+      expect(EXIFR::JPEG.new(uploader.file.path).exif?).to be(false)
+      expect(EXIFR::JPEG.new(uploader.file.path).gps.present?).to be(false)
+    end
+  end
+
+  describe "resize_image" do
+    it "creates versions of the image with different filenames", :aggregate_failures do
+      uploader.store!(image_jpg)
+      expect(uploader.filename).to match(/original_logo/)
+      expect(uploader.resized_logo.file.filename).to match(/resized_logo/)
+    end
+
+    it "contains the original file extension when a file is stored" do
+      uploader.store!(image_jpg)
+      expect(uploader.filename).to match(/\.jpg\z/)
+    end
+
+    it "creates versions of the image with different sizes" do
+      uploader.store!(image_jpg)
+      expect(uploader.resized_logo.size).to be <= uploader.size
+    end
+  end
+end