Merge pull request #9575 from alphagov/different-image-sizes

Refactor image sizes so they can be made configurable

app/assets/javascripts/components/image-cropper.js

@@ -8,8 +8,8 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}
     this.$image = this.$imageCropper.querySelector(
       '.app-c-image-cropper__image'
     )
-    this.$targetWidth = 960
-    this.$targetHeight = 640
+    this.$targetWidth = parseInt(this.$imageCropper.dataset.width, 10)
+    this.$targetHeight = parseInt(this.$imageCropper.dataset.height, 10)
   }
 
   ImageCropper.prototype.init = function () {
@@ -65,7 +65,7 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}
     this.cropper = new window.Cropper(this.$image, {
       // eslint-disable-line
       viewMode: 2,
-      aspectRatio: 3 / 2,
+      aspectRatio: this.$targetWidth / this.$targetHeight,
       autoCrop: true,
       autoCropArea: 1,
       guides: false,

app/controllers/admin/edition_images_controller.rb

@@ -37,6 +37,9 @@ def create
       PublishingApiDocumentRepublishingWorker.perform_async(@edition.document_id)
       redirect_to edit_admin_edition_image_path(@edition, @new_image.id), notice: "#{@new_image.filename} successfully uploaded"
     elsif new_image_needs_cropping?
+      image_kind_config = @new_image.image_data.image_kind_config
+      @valid_width = image_kind_config.valid_width
+      @valid_height = image_kind_config.valid_height
       @data_url = image_data_url
       render :crop
     else
@@ -90,6 +93,6 @@ def enforce_permissions!
   end
 
   def image_params
-    params.fetch(:image, {}).permit(image_data: [:file])
+    params.fetch(:image, {}).permit(image_data: %i[file image_kind])
   end
 end

app/helpers/govspeak_helper.rb

@@ -48,6 +48,7 @@ def govspeak_html_attachment_to_html(html_attachment)
 
   def prepare_images(images)
     images
+      .select { |image| image.image_data&.image_kind_config&.permits? "govspeak_embed" }
       .select { |image| image.image_data&.all_asset_variants_uploaded? }
       .map do |image|
       {

app/models/asset.rb

@@ -19,11 +19,9 @@ def unique_variant_for_each_assetable
   enum variant: {
     original: "original".freeze,
     thumbnail: "thumbnail".freeze,
-    s960: "s960".freeze,
-    s712: "s712".freeze,
-    s630: "s630".freeze,
-    s465: "s465".freeze,
-    s300: "s300".freeze,
-    s216: "s216".freeze,
-  }
+  }.merge(
+    Whitehall.image_kinds.values
+             .flat_map(&:version_names)
+             .to_h { |version_name| [version_name.to_sym, version_name.freeze] },
+  )
 end

app/models/concerns/edition/images.rb

@@ -39,6 +39,10 @@ def allows_image_attachments?
     true
   end
 
+  def permitted_image_kinds
+    Whitehall.image_kinds.values_at("default")
+  end
+
 private
 
   def no_substantive_attributes?(attrs)

app/models/concerns/image_kind.rb

@@ -0,0 +1,25 @@
+module ImageKind
+  extend ActiveSupport::Concern
+
+  included do
+    def assign_attributes(attributes)
+      # It's important that the image_kind attribute is assigned first, as it is intended to be used by
+      # carrierwave uploaders when they are mounted to work out which image versions to use.
+      image_kind = attributes.delete(:image_kind)
+      ordered_attributes = if image_kind.present?
+                             { image_kind:, **attributes }
+                           else
+                             attributes
+                           end
+      super ordered_attributes
+    end
+
+    def image_kind
+      attributes.fetch("image_kind", "default")
+    end
+
+    def image_kind_config
+      @image_kind_config ||= Whitehall.image_kinds.fetch(image_kind)
+    end
+  end
+end

app/models/featured_image_data.rb

@@ -1,5 +1,6 @@
 class FeaturedImageData < ApplicationRecord
   mount_uploader :file, FeaturedImageUploader, mount_on: :carrierwave_image
+  include ImageKind
 
   belongs_to :featured_imageable, polymorphic: true
 
@@ -10,7 +11,7 @@ class FeaturedImageData < ApplicationRecord
   validates :file, presence: true
   validates :featured_imageable, presence: true
 
-  validates_with ImageValidator, size: [960, 640]
+  validates_with ImageValidator
 
   delegate :url, to: :file
 

app/models/image_data.rb

@@ -3,8 +3,8 @@
 class ImageData < ApplicationRecord
   attr_accessor :validate_on_image
 
-  VALID_WIDTH = 960
-  VALID_HEIGHT = 640
+  include ImageKind
+
   SVG_CONTENT_TYPE = "image/svg+xml".freeze
 
   has_many :images
@@ -15,7 +15,7 @@ class ImageData < ApplicationRecord
   mount_uploader :file, ImageUploader, mount_on: :carrierwave_image
 
   validates :file, presence: { message: "cannot be uploaded. Choose a valid JPEG, PNG, SVG or GIF." }
-  validates_with ImageValidator, size: [VALID_WIDTH, VALID_HEIGHT], if: :image_changed?
+  validates_with ImageValidator, if: :image_changed?
   validate :filename_is_unique
 
   delegate :width, :height, to: :dimensions
@@ -38,7 +38,7 @@ def bitmap?
 
   def all_asset_variants_uploaded?
     asset_variants = assets.map(&:variant).map(&:to_sym)
-    required_variants = bitmap? ? ImageUploader.versions.keys.push(:original) : [Asset.variants[:original].to_sym]
+    required_variants = file.active_version_names + [:original]
 
     (required_variants - asset_variants).empty?
   end
@@ -55,10 +55,10 @@ def dimensions
     @dimensions ||= if valid?
                       # Whitehall doesn't store local copies of original images. Once they've been
                       # uploaded to Asset Manager, we can't expect them to exist locally again.
-                      # But since every uploaded image has to be these exact dimensions, we can
+                      # But since every uploaded image has to have valid dimensions, we can
                       # be confident a valid image (either freshly uploaded, or already persisted)
-                      # will be these exact dimensions.
-                      Dimensions.new(VALID_WIDTH, VALID_HEIGHT)
+                      # will have valid dimensions.
+                      Dimensions.new(image_kind_config.valid_width, image_kind_config.valid_height)
                     else
                       image = MiniMagick::Image.open file.path
                       Dimensions.new(image[:width], image[:height])

app/models/promotional_feature_item.rb

@@ -1,6 +1,8 @@
 class PromotionalFeatureItem < ApplicationRecord
   VALID_YOUTUBE_URL_FORMAT = /\A(?:(?:https:\/\/youtu\.be\/)(.+)|(?:https:\/\/www\.youtube\.com\/watch\?v=)(.*?)(?:&|#|$).*)\Z/
 
+  include ImageKind
+
   belongs_to :promotional_feature, inverse_of: :promotional_feature_items
   has_one :organisation, through: :promotional_feature
   has_many :links, class_name: "PromotionalFeatureLink", dependent: :destroy, inverse_of: :promotional_feature_item
@@ -10,7 +12,7 @@ class PromotionalFeatureItem < ApplicationRecord
            inverse_of: :assetable
 
   validates :summary, presence: true, length: { maximum: 500 }
-  validates_with ImageValidator, method: :image, size: [960, 640], if: :image_changed?
+  validates_with ImageValidator, method: :image, if: :image_changed?
   validates :title_url, uri: true, allow_blank: true
   validate :image_or_youtube_url_is_present
   validates :youtube_video_url, format: {

app/models/topical_event_featuring_image_data.rb

@@ -1,14 +1,16 @@
 class TopicalEventFeaturingImageData < ApplicationRecord
   mount_uploader :file, FeaturedImageUploader, mount_on: :carrierwave_image
 
+  include ImageKind
+
   has_one :topical_event_featuring, inverse_of: :image
   has_many :assets,
            as: :assetable,
            inverse_of: :assetable
 
   validates :file, presence: true
 
-  validates_with ImageValidator, size: [960, 640]
+  validates_with ImageValidator
 
   delegate :url, to: :file
 

app/uploaders/featured_image_uploader.rb

@@ -16,12 +16,11 @@ def extension_allowlist
     config.validate_integrity = true
   end
 
-  version(:s960) { process resize_to_fill: [960, 640] }
-  version(:s712, from_version: :s960) { process resize_to_fill: [712, 480] }
-  version(:s630, from_version: :s960) { process resize_to_fill: [630, 420] }
-  version(:s465, from_version: :s960) { process resize_to_fill: [465, 310] }
-  version(:s300, from_version: :s960) { process resize_to_fill: [300, 195] }
-  version(:s216, from_version: :s960) { process resize_to_fill: [216, 140] }
+  Whitehall.image_kinds.fetch("default").versions.each do |v|
+    version v.name, from_version: v.from_version&.to_sym do
+      process resize_to_fill: v.resize_to_fill
+    end
+  end
 
   def image_cache
     file.file.gsub("/govuk/whitehall/carrierwave-tmp/", "") if send("cache_id").present?

app/uploaders/image_uploader.rb

@@ -10,23 +10,16 @@ def extension_allowlist
     %w[jpg jpeg gif png svg]
   end
 
-  version :s960, if: :bitmap? do
-    process resize_to_fill: [960, 640]
-  end
-  version :s712, from_version: :s960, if: :bitmap? do
-    process resize_to_fill: [712, 480]
-  end
-  version :s630, from_version: :s960, if: :bitmap? do
-    process resize_to_fill: [630, 420]
-  end
-  version :s465, from_version: :s960, if: :bitmap? do
-    process resize_to_fill: [465, 310]
-  end
-  version :s300, from_version: :s960, if: :bitmap? do
-    process resize_to_fill: [300, 195]
-  end
-  version :s216, from_version: :s960, if: :bitmap? do
-    process resize_to_fill: [216, 140]
+  Whitehall.image_kinds.each do |image_kind, image_kind_config|
+    use_versions_for_this_image_kind_proc = lambda do |uploader, opts|
+      uploader.model.image_kind == image_kind && uploader.bitmap?(opts[:file])
+    end
+
+    image_kind_config.versions.each do |v|
+      version v.name, from_version: v.from_version&.to_sym, if: use_versions_for_this_image_kind_proc do
+        process resize_to_fill: v.resize_to_fill
+      end
+    end
   end
 
   def bitmap?(new_file)
@@ -40,4 +33,10 @@ def image_cache
       file.file.gsub("/govuk/whitehall/carrierwave-tmp/", "")
     end
   end
+
+  def active_version_names
+    # active_versions is protected, so it can only be called by subclasses
+    # it returns an array of [key, value] pairs, and we want the keys
+    active_versions.map(&:first)
+  end
 end

app/validators/image_validator.rb

@@ -8,7 +8,6 @@ class ImageValidator < ActiveModel::Validator
   def initialize(options = {})
     super
     @method     = options[:method] || :file
-    @size       = options[:size] || nil
     @mime_types = options[:mime_types] || DEFAULT_MIME_TYPES
   end
 
@@ -39,12 +38,12 @@ def validate_mime_type(record, file_path)
   end
 
   def validate_size(record, image)
-    return unless @size
+    return unless record.respond_to?(:image_kind_config)
 
     actual_width = image[:width]
     actual_height = image[:height]
-    target_width = @size[0]
-    target_height = @size[1]
+    target_width = record.image_kind_config.valid_width
+    target_height = record.image_kind_config.valid_height
 
     too_small = actual_width < target_width || actual_height < target_height
     too_large = actual_width > target_width || actual_height > target_height

app/views/admin/edition_images/_image_upload.html.erb

@@ -15,6 +15,21 @@
     accept: "image/png, image/jpeg, image/gif, image/svg+xml",
   } %>
 
+  <% if @edition.permitted_image_kinds.size == 1 %>
+    <%= hidden_field_tag("image[image_data][image_kind]", @edition.permitted_image_kinds.first.name) %>
+  <% else %>
+    <%= render "govuk_publishing_components/components/radio", {
+      heading: "What kind of image is this?",
+      name: "image[image_data][image_kind]",
+      items: @edition.permitted_image_kinds.map do |image_kind|
+        {
+          value: image_kind.name,
+          text: image_kind.display_name,
+        }
+      end,
+    } %>
+  <% end %>
+
    <%= render "govuk_publishing_components/components/details", {
       title: "You must use an SVG for charts and diagrams",
     } do %>

app/views/admin/edition_images/crop.html.erb

@@ -14,11 +14,15 @@
       <p>If you need to crop your image, use your cursor or the arrow keys and “<kbd>+</kbd>” and “<kbd>-</kbd>” to select a part of the image. The part you select will be resized for GOV.UK. The shape is fixed.</p>
      </div>
 
+      <%= hidden_field_tag("image[image_data][image_kind]", @new_image.image_data.image_kind) %>
+
       <%= render "components/image_cropper", {
         name: "image[image_data][file]",
         src: @data_url,
         filename: @new_image.filename,
         type: @new_image.content_type,
+        width: @valid_width,
+        height: @valid_height,
       } %>
 
       <div class="govuk-button-group govuk-!-margin-bottom-6">

app/views/components/_image_cropper.html.erb

@@ -1,4 +1,4 @@
-<%= tag.div class: "app-c-image-cropper", tabindex: "0", data: {module: "image-cropper", filename:, type:}, "aria-live" => "polite" do %>
+<%= tag.div class: "app-c-image-cropper", tabindex: "0", data: { module: "image-cropper", filename:, type:, width:, height: }, "aria-live" => "polite" do %>
   <input type="file" class="js-cropped-image-input" name="<%= name %>" hidden>
   <%= tag.div class: "app-c-image-cropper__container" do %>
     <%= tag.img class: "app-c-image-cropper__image",

config/image_kinds.yml

@@ -0,0 +1,31 @@
+---
+default:
+  display_name: "Default (960 by 640)"
+  valid_width: 960
+  valid_height: 640
+  permitted_uses:
+    - govspeak_embed
+  versions:
+    - name: s960
+      width: 960
+      height: 640
+    - name: s712
+      width: 712
+      height: 480
+      from_version: s960
+    - name: s630
+      width: 630
+      height: 420
+      from_version: s960
+    - name: s465
+      width: 465
+      height: 310
+      from_version: s960
+    - name: s300
+      width: 300
+      height: 195
+      from_version: s960
+    - name: s216
+      width: 216
+      height: 140
+      from_version: s960

db/migrate/20241105092100_add_image_kind_to_image_data.rb

@@ -0,0 +1,5 @@
+class AddImageKindToImageData < ActiveRecord::Migration[7.1]
+  def change
+    add_column :image_data, :image_kind, :string, default: "default", null: false
+  end
+end

db/schema.rb

@@ -667,6 +667,7 @@
     t.string "carrierwave_image"
     t.datetime "created_at", precision: nil
     t.datetime "updated_at", precision: nil
+    t.string "image_kind", default: "default", null: false
   end
 
   create_table "images", id: :integer, charset: "utf8mb3", collation: "utf8mb3_unicode_ci", force: :cascade do |t|

lib/whitehall.rb

@@ -195,6 +195,10 @@ def self.worldwide_tagging_organisations
       YAML.load_file(Rails.root.join("config/worldwide_tagging_organisations.yml"))
   end
 
+  def self.image_kinds
+    @image_kinds ||= ImageKinds.build_image_kinds(YAML.load_file(Rails.root.join("config/image_kinds.yml")))
+  end
+
   def self.integration_or_staging?
     website_root = ENV.fetch("GOVUK_WEBSITE_ROOT", "")
     %w[integration staging].any? { |environment| website_root.include?(environment) }