Skip to content

Commit

Permalink
Only pass crop values if cropping is enabled
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@tvdeyen
tvdeyen committed Jun 28, 2021
1 parent 52694b3 commit 5474ff4
Show file tree
Hide file tree
Showing 2 changed files with 142 additions and 81 deletions.
20 changes: 8 additions & 12 deletions app/models/concerns/alchemy/picture_thumbnails.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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",
}
Expand Down Expand Up @@ -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]

Expand Down
203 changes: 134 additions & 69 deletions lib/alchemy/test_support/having_picture_thumbnails_examples.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 5474ff4

Please sign in to comment.