From 5474ff48573c3783c6b87fa6e3af7a636c08552f Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Sun, 27 Jun 2021 13:41:00 +0200 Subject: [PATCH] Only pass crop values if cropping is enabled Before we enabled cropping even if cropping was disabled only because we had crop values in the database. This is confusing, inconsistent and error prone. Imagine an essence picture having crop values, but the image cropper is disabled because the element settings has been changed. The image was still be cropped in those cases. Now cropping has to be explicitely enabled in the element settings to crop an image, regardless of the database state. --- .../concerns/alchemy/picture_thumbnails.rb | 20 +- .../having_picture_thumbnails_examples.rb | 203 ++++++++++++------ 2 files changed, 142 insertions(+), 81 deletions(-) diff --git a/app/models/concerns/alchemy/picture_thumbnails.rb b/app/models/concerns/alchemy/picture_thumbnails.rb index c6fc6578eb..4c2b979a24 100644 --- a/app/models/concerns/alchemy/picture_thumbnails.rb +++ b/app/models/concerns/alchemy/picture_thumbnails.rb @@ -49,13 +49,13 @@ def picture_url(options = {}) def picture_url_options return {} if picture.nil? - crop = crop_values_present? || settings[:crop] + crop = !!settings[:crop] { format: picture.default_render_format, - crop: !!crop, - crop_from: crop_from.presence, - crop_size: crop_size.presence, + crop: crop, + crop_from: crop && crop_from.presence || nil, + crop_size: crop && crop_size.presence || nil, size: settings[:size], }.with_indifferent_access end @@ -76,13 +76,13 @@ def thumbnail_url # # @return [HashWithIndifferentAccess] def thumbnail_url_options - crop = crop_values_present? || settings[:crop] + crop = !!settings[:crop] { size: "160x120", - crop: !!crop, - crop_from: crop_from.presence || default_crop_from&.join("x"), - crop_size: crop_size.presence || default_crop_size&.join("x"), + crop: crop, + crop_from: crop && crop_from.presence || default_crop_from&.join("x"), + crop_size: crop && crop_size.presence || default_crop_size&.join("x"), flatten: true, format: picture&.image_file_format || "jpg", } @@ -111,10 +111,6 @@ def allow_image_cropping? private - def crop_values_present? - crop_from.present? && crop_size.present? - end - def default_crop_size return nil unless settings[:crop] && settings[:size] diff --git a/lib/alchemy/test_support/having_picture_thumbnails_examples.rb b/lib/alchemy/test_support/having_picture_thumbnails_examples.rb index c17a99f1c5..ff119e432f 100644 --- a/lib/alchemy/test_support/having_picture_thumbnails_examples.rb +++ b/lib/alchemy/test_support/having_picture_thumbnails_examples.rb @@ -51,24 +51,30 @@ allow(record).to receive(:crop_size) { "200x200" } end - it "passes these crop values to the picture's url method." do - expect(picture).to receive(:url).with( - hash_including(crop_from: "10x10", crop_size: "200x200"), - ) - picture_url - end - - context "but with crop values in the options" do - let(:options) do - { crop_from: "30x30", crop_size: "75x75" } + context "if cropping is enabled" do + before do + allow(record).to receive(:settings) { { crop: true } } end - it "passes these crop values instead." do + it "passes these crop values to the picture's url method." do expect(picture).to receive(:url).with( - hash_including(crop_from: "30x30", crop_size: "75x75"), + hash_including(crop_from: "10x10", crop_size: "200x200"), ) picture_url end + + context "but with crop values in the options" do + let(:options) do + { crop_from: "30x30", crop_size: "75x75" } + end + + it "passes these crop values instead." do + expect(picture).to receive(:url).with( + hash_including(crop_from: "30x30", crop_size: "75x75"), + ) + picture_url + end + end end end @@ -121,30 +127,39 @@ allow(record).to receive(:crop_size) { "200x200" } end - it "includes these crop values.", :aggregate_failures do - expect(picture_url_options[:crop_from]).to eq "10x10" - expect(picture_url_options[:crop_size]).to eq "200x200" - end + context "with cropping enabled" do + before do + allow(record).to receive(:settings) { { crop: true } } + end - it "includes {crop: true}" do - expect(picture_url_options[:crop]).to be true + it "includes these crop values.", :aggregate_failures do + expect(picture_url_options[:crop_from]).to eq "10x10" + expect(picture_url_options[:crop_size]).to eq "200x200" + end end - end - # Regression spec for issue #1279 - context "with crop values being empty strings" do - before do - allow(record).to receive(:crop_from) { "" } - allow(record).to receive(:crop_size) { "" } - end + context "with cropping disabled" do + before do + allow(record).to receive(:settings) { { crop: nil } } + end - it "does not include these crop values.", :aggregate_failures do - expect(picture_url_options[:crop_from]).to be_nil - expect(picture_url_options[:crop_size]).to be_nil + it "does not include these crop values.", :aggregate_failures do + expect(picture_url_options[:crop_from]).to be_nil + expect(picture_url_options[:crop_size]).to be_nil + end end - it "includes {crop: false}" do - expect(picture_url_options[:crop]).to be false + # Regression spec for issue #1279 + context "with crop values being empty strings" do + before do + allow(record).to receive(:crop_from) { "" } + allow(record).to receive(:crop_size) { "" } + end + + it "does not include these crop values.", :aggregate_failures do + expect(picture_url_options[:crop_from]).to be_nil + expect(picture_url_options[:crop_size]).to be_nil + end end end @@ -197,36 +212,61 @@ thumbnail_url end - context "when crop sizes are present" do - before do - allow(record).to receive(:crop_size).and_return("200x200") - allow(record).to receive(:crop_from).and_return("10x10") + context "when crop is enabled in the settings" do + let(:settings) do + { crop: true } end - it "passes these crop sizes to the picture's url method." do - expect(picture).to receive(:url).with( - hash_including(crop_from: "10x10", crop_size: "200x200", crop: true), - ) - thumbnail_url + context "and crop sizes are present" do + before do + allow(record).to receive(:crop_size).and_return("200x200") + allow(record).to receive(:crop_from).and_return("10x10") + end + + it "passes these crop sizes to the picture's url method." do + expect(picture).to receive(:url).with( + hash_including( + crop_from: "10x10", + crop_size: "200x200", + crop: true, + ), + ) + thumbnail_url + end + end + + context "when no crop sizes are present" do + it "it does not pass crop sizes to the picture's url method and enables center cropping." do + expect(picture).to receive(:url).with( + hash_including( + crop_from: nil, + crop_size: nil, + crop: true, + ), + ) + thumbnail_url + end end end - context "when no crop sizes are present" do - it "it does not pass crop sizes to the picture's url method and disables cropping." do - expect(picture).to receive(:url).with( - hash_including(crop_from: nil, crop_size: nil, crop: false), - ) - thumbnail_url + context "when cropping is disabled in the settings" do + let(:settings) do + { crop: false } end - context "when crop is explicitely enabled in the settings" do - let(:settings) do - { crop: true } + context "but crop sizes are present" do + before do + allow(record).to receive(:crop_size).and_return("200x200") + allow(record).to receive(:crop_from).and_return("10x10") end - it "it enables cropping." do + it "it disables cropping." do expect(picture).to receive(:url).with( - hash_including(crop: true), + hash_including( + crop_size: nil, + crop_from: nil, + crop: false, + ), ) thumbnail_url end @@ -274,34 +314,59 @@ end end - context "when crop values are present" do - before do - expect(record).to receive(:crop_size).at_least(:once) { "200x200" } - expect(record).to receive(:crop_from).at_least(:once) { "10x10" } + context "when cropping is enabled in settings" do + let(:settings) do + { crop: true } end - it "includes these crop values" do - expect(thumbnail_url_options).to match( - hash_including(crop_from: "10x10", crop_size: "200x200", crop: true) - ) + context "and crop values are present" do + before do + expect(record).to receive(:crop_size).at_least(:once) { "200x200" } + expect(record).to receive(:crop_from).at_least(:once) { "10x10" } + end + + it "includes these crop values" do + expect(thumbnail_url_options).to match( + hash_including( + crop_from: "10x10", + crop_size: "200x200", + crop: true, + ) + ) + end + end + + context "and no crop values are present" do + it "does not include crop values but enables center cropping" do + expect(thumbnail_url_options).to match( + hash_including( + crop_from: nil, + crop_size: nil, + crop: true, + ) + ) + end end end - context "when no crop values are present" do - it "does not include these crop values" do - expect(thumbnail_url_options).to_not match( - hash_including(crop_from: "10x10", crop_size: "200x200", crop: true) - ) + context "when cropping is disabled in settings" do + let(:settings) do + { crop: false } end - context "when crop is explicitely enabled in the settings" do - let(:settings) do - { crop: true } + context "but crop values are present" do + before do + allow(record).to receive(:crop_size) { "200x200" } + allow(record).to receive(:crop_from) { "10x10" } end - it "it enables cropping." do + it "does not include crop values" do expect(thumbnail_url_options).to match( - hash_including(crop: true), + hash_including( + crop_from: nil, + crop_size: nil, + crop: false, + ) ) end end @@ -367,7 +432,7 @@ end end - context "with no sizes in settngs" do + context "with no sizes in settings" do it "sets sizes to zero" do expect(subject[:min_size]).to eq([0, 0]) end