From da055edb372e06d81adbf2f32c0e8aef0fe20c75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 02:33:44 +0200 Subject: [PATCH 1/5] Simplify rendering proposal image in dashboard So it's now similar to the way we render images in other places. --- app/views/dashboard/mailing/index.html.erb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/views/dashboard/mailing/index.html.erb b/app/views/dashboard/mailing/index.html.erb index 850fee3fa..1886d07c8 100644 --- a/app/views/dashboard/mailing/index.html.erb +++ b/app/views/dashboard/mailing/index.html.erb @@ -9,11 +9,7 @@
- <% if proposal.image.present? %> - <%= image_tag proposal.image.attachment.url(:large) %> - <% else %> - <%= image_tag "default_mailing.jpg" %> - <% end %> + <%= image_tag(proposal.image_url(:large).presence || "default_mailing.jpg") %>
From bc18a6e10e75c97305c2299ab431e9e2ed2fbcb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 02:48:44 +0200 Subject: [PATCH 2/5] Remove unused url method in poll question answers This method would never work because it relies on the `image` association, instead of the `images` association defined in the `Galleryable` module. --- app/models/concerns/galleryable.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/models/concerns/galleryable.rb b/app/models/concerns/galleryable.rb index b50e65675..8ba3c48d8 100644 --- a/app/models/concerns/galleryable.rb +++ b/app/models/concerns/galleryable.rb @@ -4,9 +4,5 @@ module Galleryable included do has_many :images, as: :imageable, inverse_of: :imageable, dependent: :destroy accepts_nested_attributes_for :images, allow_destroy: true, update_only: true - - def image_url(style) - image&.attachment&.url(style) || "" - end end end From 04585d289ceea6af54d2bb0346e9c3bd79ea1154 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 02:56:31 +0200 Subject: [PATCH 3/5] Remove not-so-precise attachments test We were testing the URL of the image changes to `missing.png`, but actually that's confusing because the image record is now invalid and so its changes can't be saved. That means that, when rendered in the browser, the image won't render the `missing.png` image but will try to render the destroyed one. If we want to render the `missing.png` image when the attachment has been destroyed, we need to remove the attachment presence validation or change the `url` method so it detects when an attachment is missing. --- spec/shared/models/acts_as_imageable.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/spec/shared/models/acts_as_imageable.rb b/spec/shared/models/acts_as_imageable.rb index 3b18e6930..b4624372d 100644 --- a/spec/shared/models/acts_as_imageable.rb +++ b/spec/shared/models/acts_as_imageable.rb @@ -58,12 +58,4 @@ shared_examples "acts as imageable" do |imageable_factory| expect(image).not_to be_valid end end - - it "image destroy should remove image from file storage" do - image.save! - image_url = image.attachment.url - new_url = "/attachments/original/missing.png" - - expect { image.attachment.destroy }.to change { image.attachment.url }.from(image_url).to(new_url) - end end From 6f219beff0e67aa45bf0de0d7c1abbe5b347a560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 19:32:36 +0200 Subject: [PATCH 4/5] Remove unused parameter in imageable tests method --- spec/shared/system/nested_imageable.rb | 44 ++++--------------- spec/system/admin/budget_phases_spec.rb | 5 +-- .../admin/legislation/processes_spec.rb | 2 +- .../questions/answers/images/images_spec.rb | 3 +- spec/system/legislation/proposals_spec.rb | 2 +- 5 files changed, 13 insertions(+), 43 deletions(-) diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index 4ab48c6a1..57c2329ec 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -47,10 +47,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file( - imageable_factory_name, - Rails.root.join("spec/fixtures/files/clippy.jpg") - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) expect_image_has_title("clippy.jpg") end @@ -75,10 +72,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file( - imageable_factory_name, - Rails.root.join("spec/fixtures/files/clippy.jpg") - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) expect(page).to have_selector ".loading-bar.complete" end @@ -87,11 +81,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file( - imageable_factory_name, - Rails.root.join("spec/fixtures/files/logo_header.png"), - false - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/logo_header.png"), false) expect(page).to have_selector ".loading-bar.errors" end @@ -100,10 +90,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file( - imageable_factory_name, - Rails.root.join("spec/fixtures/files/clippy.jpg") - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) expect_image_has_cached_attachment(".jpg") end @@ -112,11 +99,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file( - imageable_factory_name, - Rails.root.join("spec/fixtures/files/logo_header.png"), - false - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/logo_header.png"), false) expect_image_has_cached_attachment("") end @@ -141,10 +124,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file( - imageable_factory_name, - Rails.root.join("spec/fixtures/files/clippy.jpg") - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) within "#nested-image .image" do click_link "Remove image" @@ -171,10 +151,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) send(fill_resource_method_name) if fill_resource_method_name - imageable_attach_new_file( - imageable_factory_name, - Rails.root.join("spec/fixtures/files/clippy.jpg") - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) expect(page).to have_selector ".loading-bar.complete" @@ -188,10 +165,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) send(fill_resource_method_name) if fill_resource_method_name - imageable_attach_new_file( - imageable_factory_name, - Rails.root.join("spec/fixtures/files/clippy.jpg") - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) expect(page).to have_selector ".loading-bar.complete" @@ -255,7 +229,7 @@ def imageable_redirected_to_resource_show_or_navigate_to end end -def imageable_attach_new_file(_imageable_factory_name, path, success = true) +def imageable_attach_new_file(path, success = true) click_link "Add image" within "#nested-image" do image = find(".image") diff --git a/spec/system/admin/budget_phases_spec.rb b/spec/system/admin/budget_phases_spec.rb index b4b7f4a7a..5893a8aea 100644 --- a/spec/system/admin/budget_phases_spec.rb +++ b/spec/system/admin/budget_phases_spec.rb @@ -46,10 +46,7 @@ describe "Admin budget phases" do scenario "shows successful notice when updating the phase with a valid image" do visit edit_admin_budget_budget_phase_path(budget, budget.current_phase) - imageable_attach_new_file( - "budget_phase_image", - Rails.root.join("spec/fixtures/files/clippy.jpg") - ) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) click_on "Save changes" diff --git a/spec/system/admin/legislation/processes_spec.rb b/spec/system/admin/legislation/processes_spec.rb index b16a7e5c5..d19a3a16c 100644 --- a/spec/system/admin/legislation/processes_spec.rb +++ b/spec/system/admin/legislation/processes_spec.rb @@ -175,7 +175,7 @@ describe "Admin collaborative legislation", :admin do fill_in "End", with: base_date + 5.days end - imageable_attach_new_file(create(:image), Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) click_button "Create process" diff --git a/spec/system/admin/poll/questions/answers/images/images_spec.rb b/spec/system/admin/poll/questions/answers/images/images_spec.rb index 398680c2c..a31c4af8a 100644 --- a/spec/system/admin/poll/questions/answers/images/images_spec.rb +++ b/spec/system/admin/poll/questions/answers/images/images_spec.rb @@ -32,14 +32,13 @@ describe "Images", :admin do scenario "Add image to answer" do answer = create(:poll_question_answer) - image = create(:image) visit admin_answer_images_path(answer) expect(page).not_to have_css("img[title='clippy.jpg']") expect(page).not_to have_content("clippy.jpg") visit new_admin_answer_image_path(answer) - imageable_attach_new_file(image, Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) click_button "Save image" expect(page).to have_css("img[title='clippy.jpg']") diff --git a/spec/system/legislation/proposals_spec.rb b/spec/system/legislation/proposals_spec.rb index d615cd8fc..cfeac71fa 100644 --- a/spec/system/legislation/proposals_spec.rb +++ b/spec/system/legislation/proposals_spec.rb @@ -146,7 +146,7 @@ describe "Legislation Proposals" do fill_in "Proposal title", with: "Legislation proposal with image" fill_in "Proposal summary", with: "Including an image on a legislation proposal" - imageable_attach_new_file(create(:image), Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) check "legislation_proposal_terms_of_service" click_button "Create proposal" From c3b3bd4502375740dd32c918f0eaa0dcb3cdecd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 20:05:23 +0200 Subject: [PATCH 5/5] Test attachments from the user's point of view These tests were checking the URLs of documents and images pointed to the URL generated by the `attachment.url` method. In order to do so, we were running database queries after starting the process running the browser, which is sometimes causing database inconsistencies when running the tests. So I'm simply removing the URL check. The tests are slightly less useful now, but it isn't like they were 100% right in the first place. After all, if the `attachment.url` method wasn't working properly, the tests were still passing. --- spec/system/admin/site_customization/documents_spec.rb | 2 +- spec/system/admin/widgets/cards_spec.rb | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/system/admin/site_customization/documents_spec.rb b/spec/system/admin/site_customization/documents_spec.rb index ce7c236b8..92d2e543a 100644 --- a/spec/system/admin/site_customization/documents_spec.rb +++ b/spec/system/admin/site_customization/documents_spec.rb @@ -58,7 +58,7 @@ describe "Documents", :admin do click_button "Upload" expect(page).to have_content "Document uploaded succesfully" - expect(page).to have_link "logo.pdf", href: Document.last.attachment.url + expect(page).to have_link "logo.pdf" end scenario "Errors on create" do diff --git a/spec/system/admin/widgets/cards_spec.rb b/spec/system/admin/widgets/cards_spec.rb index 51e37f965..fae155352 100644 --- a/spec/system/admin/widgets/cards_spec.rb +++ b/spec/system/admin/widgets/cards_spec.rb @@ -40,19 +40,18 @@ describe "Cards", :admin do end scenario "Index" do - 3.times { create(:widget_card) } + cards = Array.new(3) { create(:widget_card) } visit admin_homepage_path expect(page).to have_css(".homepage-card", count: 3) - cards = Widget::Card.all cards.each do |card| expect(page).to have_content card.title expect(page).to have_content card.description expect(page).to have_content card.link_text expect(page).to have_content card.link_url - expect(page).to have_link("Show image", href: card.image_url(:large)) + expect(page).to have_link "Show image" end end