Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 21 additions & 33 deletions app/assets/javascripts/components/image-cropper.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}
)
this.$targetWidth = parseInt(this.$imageCropper.dataset.width, 10)
this.$targetHeight = parseInt(this.$imageCropper.dataset.height, 10)
this.$croppingX = parseInt(this.$imageCropper.dataset.x, 10)
this.$croppingY = parseInt(this.$imageCropper.dataset.y, 10)
}

ImageCropper.prototype.init = function () {
Expand All @@ -31,13 +33,32 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}
function () {
this.initKeyboardControls()
this.updateAriaLabel()

const cropBoxData = this.cropper.getCropBoxData()

cropBoxData.left = this.$croppingX
cropBoxData.top = this.$croppingY

this.cropper.setCropBoxData(cropBoxData)
}.bind(this)
)

this.$image.addEventListener(
'crop',
function () {
this.updateAriaLabel()

const data = this.cropper.getData(true)

Object.keys(data).forEach((attribute) => {
const input = this.$imageCropper.querySelector(
`.js-cropped-image-input[name$="${attribute}]"]`
)

if (input) {
input.value = data[attribute]
}
})
}.bind(this)
)

Expand Down Expand Up @@ -76,7 +97,6 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}
rotatable: false,
scalable: false
})
this.setupFormListener()
}

ImageCropper.prototype.initKeyboardControls = function () {
Expand Down Expand Up @@ -162,37 +182,5 @@ window.GOVUK.Modules = window.GOVUK.Modules || {}
'is selected.'
}

ImageCropper.prototype.setupFormListener = function () {
const input = this.$imageCropper.querySelector('.js-cropped-image-input')
input.form.addEventListener(
'submit',
function (event) {
event.preventDefault()
this.cropper
.getCroppedCanvas({
width: this.$targetWidth,
height: this.$targetHeight
})
.toBlob(
function (blob) {
const file = new File(
[blob],
this.$imageCropper.dataset.filename,
{
type: this.$imageCropper.dataset.type,
lastModified: new Date().getTime()
}
)
const container = new DataTransfer()
container.items.add(file)
input.files = container.files
input.form.submit()
}.bind(this),
this.$imageCropper.dataset.type
)
}.bind(this)
)
}

Modules.ImageCropper = ImageCropper
})(window.GOVUK.Modules)
18 changes: 11 additions & 7 deletions app/components/admin/edition_images/image_component.html.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<li class="govuk-grid-row">
<div class="govuk-grid-column-one-third">
<% if image.image_data&.all_asset_variants_uploaded? %>
<img src="<%= image.url %>" alt="<%= preview_alt_text %>" class="app-view-edition-resource__preview">
<% if image.image_data&.original_uploaded? && image.thumbnail %>
<img src="<%= image.thumbnail %>" alt="<%= preview_alt_text %>" class="app-view-edition-resource__preview">
<% else %>
<span class="govuk-tag govuk-tag--green">Processing</span>
<% end %>
Expand All @@ -10,11 +10,15 @@
<div class="govuk-grid-column-two-thirds">
<p class="govuk-body"><strong>Caption: </strong><%= caption %></p>
<p class="govuk-body"><strong>Alt text: </strong><%= alt_text %></p>
<%= render "govuk_publishing_components/components/copy_to_clipboard", {
label: tag.strong("Markdown code:"),
copyable_content: image_markdown,
button_text: "Copy Markdown",
} %>
<% if image.image_data&.bitmap? && image.image_data&.requires_crop? %>
<span class="govuk-tag govuk-tag--red">Requires crop</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest, what would happen if the user constructed the markdown themselves and embedded it before cropping took place?

Copy link
Contributor Author

@patrickpatrickpatrick patrickpatrickpatrick Oct 23, 2025

Choose a reason for hiding this comment

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

I think an error would likely occur since there's logic which stops variants from being generated if the crop hasn't been provided. I'm wondering if I should perhaps remove that which although would generate unnecessary images would make edition validation significantly easier.

<% else %>
<%= render "govuk_publishing_components/components/copy_to_clipboard", {
label: tag.strong("Markdown code:"),
copyable_content: image_markdown,
button_text: "Copy Markdown",
} %>
<% end %>
</div>

<div class="app-view-edition-resource__actions govuk-grid-column-full govuk-button-group govuk-!-margin-top-4">
Expand Down
2 changes: 1 addition & 1 deletion app/components/admin/edition_images/image_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,6 @@ def find_image_index
end

def can_be_custom_lead_image?
edition.can_have_custom_lead_image? && !image.svg?
edition.can_have_custom_lead_image? && !image.svg? && !image.image_data.requires_crop?
Copy link
Contributor

Choose a reason for hiding this comment

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

@lauraghiorghisor-tw we'll need to add this to the lead image select block if this merges

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case please hold off on merging this PR until #10635 is done 🙏 I don't want to have to change the lead image setting (and do all the integration import testing) again if I can help it!

end
end
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<div class="govuk-grid-row">
<div class="govuk-grid-column-one-third">
<% if lead_image.image_data&.all_asset_variants_uploaded? %>
<img src="<%= lead_image.url %>" alt="Lead image" class="app-view-edition-resource__preview">
<img src="<%= lead_image.url(:s960) %>" alt="Lead image" class="app-view-edition-resource__preview">
<% else %>
<span class="govuk-tag govuk-tag--green">Processing</span>
<% end %>
Expand Down
90 changes: 62 additions & 28 deletions app/controllers/admin/edition_images_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,21 @@ def destroy
end

def update
if image.update(params.require(:image).permit(:caption, :alt_text))
image.assign_attributes(image_params)

if image_data_params["crop_data"].present?
image_data = image.image_data
new_image_data = ImageData.new
new_image_data.to_replace_id = image_data.id
new_image_data.assign_attributes(image_data_params)
new_image_data.file.download! image_data.file.url
new_image_data.save!
image.image_data = new_image_data
end

image.image_data.validate_on_image = image
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooofff, wish we didn't do this, I know it's existing code though. But yeah, we should really have a unique filename validation for images at the edition level, not set transitive attributes on the image data to walk back up the association chain. Grim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is unfortunately how we handle files in the bulk uploader too, I'd much prefer if it the filename was stored separately from the uploaded file within the AttachmentData/ImageData as it would make a lot of things much easier to validate and implement. It does feel like a separate task to rework this.


if image.save
PublishingApiDocumentRepublishingWorker.perform_async(@edition.document_id)
redirect_to admin_edition_images_path(@edition), notice: "#{image.image_data.carrierwave_image} details updated"
else
Expand All @@ -25,47 +39,59 @@ def update
end

def create
@new_image = @edition.images.build
@new_image.build_image_data(image_params["image_data"])

@new_image.image_data.validate_on_image = @new_image
# so that auth_bypass_id is discoverable by AssetManagerStorage
@new_image.image_data.images << @new_image
@images = images_params.map { |image| create_image(image) }

if @new_image.save
if @images.empty?
flash.now.alert = "No images selected. Choose a valid JPEG, PNG, SVG or GIF."
elsif @images.all?(&:valid?)
@images.each(&:save)
@edition.update_lead_image if @edition.can_have_custom_lead_image?
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
flash.now.notice = "Images successfully uploaded"
else
@new_image.errors.delete(:"image_data.file", :too_large)
# Remove @new_image from @edition.images array, otherwise the view will render it in the 'Uploaded images' list
@edition.images.delete(@new_image)
render :index
# Remove images from @edition.images array, otherwise the view will render it in the 'Uploaded images' list
@images.each { |image| @edition.images.delete(image) }
end

render :index
end

def create_image(image)
new_image = @edition.images.build

new_image.build_image_data(image["image_data"])

new_image.image_data.validate_on_image = new_image

# so that auth_bypass_id is discoverable by AssetManagerStorage
new_image.image_data.images << new_image

new_image
end

def edit
image = Image.find(params[:id])
flash.now.notice = "The image is being processed. Try refreshing the page." unless image&.image_data&.all_asset_variants_uploaded?
flash.now.notice = "The image is being processed. Try refreshing the page." unless image&.image_data&.original_uploaded?
end

private

def new_image_needs_cropping?
@new_image.errors.of_kind?(:"image_data.file", :too_large) && @new_image.errors.size == 1
def image_url
return unless image&.image_data&.original_uploaded?

image_data = image.image_data
unless image_data.file.cached?
image_data.file.download! image_data.file.url
end
img_data = Base64.strict_encode64(image_data.file.read)

"data:#{image_data.file.content_type};base64,#{img_data}"
end
helper_method :image_url

def image_data_url
file = @new_image.image_data.file
image_data = Base64.strict_encode64(file.read)
"data:#{file.content_type};base64,#{image_data}"
def image_kind_config
image.image_data.image_kind_config
end
helper_method :image_kind_config

def image
@image ||= find_image
Expand All @@ -92,7 +118,15 @@ def enforce_permissions!
end
end

def images_params
params.fetch(:images, []).map { |image| image.permit(image_data: %i[file]) }
end

def image_data_params
params.fetch(:image, {}).fetch(:image_data, {}).permit(:file, :image_kind, crop_data: %i[x y width height])
end

def image_params
params.fetch(:image, {}).permit(image_data: %i[file image_kind])
params.fetch(:image, {}).except(:image_data).permit(:caption, :alt_text, image_data: %i[crop_data file image_kind])
end
end
2 changes: 1 addition & 1 deletion app/helpers/govspeak_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def prepare_images(images)
image_data_id: image.image_data_id,
edition_id: image.edition_id,
alt_text: image.alt_text,
url: image.url,
url: image.embed_url,
caption: image.caption,
created_at: image.created_at,
updated_at: image.updated_at,
Expand Down
2 changes: 1 addition & 1 deletion app/models/concerns/edition/custom_lead_image.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def body_does_not_contain_lead_image
html = govspeak_edition_to_html(self)
doc = Nokogiri::HTML::DocumentFragment.parse(html)

if doc.css("img").any? { |img| img[:src] == edition_lead_image.image.url }
if doc.css("img[src*='#{edition_lead_image.image.filename}']").present?
errors.add(:body, "cannot have a reference to the lead image in the text")
end
end
Expand Down
22 changes: 21 additions & 1 deletion app/models/image.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,34 @@ class Image < ApplicationRecord

accepts_nested_attributes_for :image_data

delegate :filename, :content_type, :width, :height, :bitmap?, :svg?, to: :image_data
delegate :filename, :content_type, :width, :height, :bitmap?, :svg?, :can_be_cropped?, :requires_crop?, to: :image_data

default_scope -> { order(:id) }

def url(*args)
image_data.file_url(*args)
end

def embed_url
return unless image_data.respond_to?(:image_kind_config)

embed_version = image_data.image_kind_config.embed_version

return url if embed_version.blank? || !image_data.all_asset_variants_uploaded?

url(embed_version.to_sym) || url
end

def thumbnail
return image_data.file_url unless bitmap? && !requires_crop?

variant = image_data.assets.find { |asset| asset.variant != "original" }&.variant&.to_sym
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be looking for something specific here rather than any variant that isn't the original?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to present to the user a cropped version of the uploaded image. It might be a specific variant hasn't yet uploaded on page load, so this is why I've implemented it like this.


return if variant.blank?

url(variant)
end

private

def destroy_image_data_if_required
Expand Down
Loading