From 1c55691319b2eb24c6ba152a3c6e967294fc0eef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 18:38:59 +0200 Subject: [PATCH 1/3] Merge similar edit nested imageable tests This way we save a couple of database insertions and browser requests. --- spec/shared/system/nested_imageable.rb | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index 1560db273..27eeeff3e 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -207,37 +207,24 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p end if path.include? "edit" - scenario "Should show persisted image" do + scenario "show persisted image" do create(:image, imageable: imageable) do_login_for user + visit send(path, arguments) expect(page).to have_css ".image", count: 1 - end - - scenario "Should not show add image button when image already exists" do - create(:image, imageable: imageable) - do_login_for user - visit send(path, arguments) - expect(page).not_to have_css "a#new_image_link" end - scenario "Should remove nested field after remove image" do + scenario "remove nested field after removing the image" do create(:image, imageable: imageable) do_login_for user + visit send(path, arguments) - click_on "Remove image" + click_link "Remove image" expect(page).not_to have_css ".image" - end - - scenario "Should show add image button after remove image" do - create(:image, imageable: imageable) - do_login_for user - visit send(path, arguments) - click_on "Remove image" - expect(page).to have_css "a#new_image_link" end end From 6c63aaee7667d8a1b8f1d90e00f7a243c9a8b22d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 19:14:02 +0200 Subject: [PATCH 2/3] Fix removing existing image and adding a new one We introduced a bug in commit acbd1b023. When editing a record and removing an existing image, we don't remove the HTML fields associated with that image but simply hide them, and then we add fields to create a new image when clicking on "Add image". This is standard cocoon behavior. However, since in the case of images there's a `has_one` relation, cocoon doesn't add unique identifiers to the new fields, generating duplicate IDs, which is invalid HTML. Since there's a duplicate file input ID, clicking on the "Choose image" label we aren't clicking on the new input but on the old one. This means we aren't correctly attaching an image. The tests passed because Capybara uses the equivalent of a keyboard to select the field, and in this case everything worked properly. So we need to delete the existing elements before inserting new ones. We're adding a test to check there aren't duplicate IDs. --- app/assets/javascripts/imageable.js | 3 +++ spec/shared/system/nested_imageable.rb | 11 +++++++++++ 2 files changed, 14 insertions(+) diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index c2ee3e882..a6e658796 100644 --- a/app/assets/javascripts/imageable.js +++ b/app/assets/javascripts/imageable.js @@ -8,6 +8,9 @@ $("#nested-image").on("cocoon:after-remove", function() { $("#new_image_link").removeClass("hide"); }); + $("#nested-image").on("cocoon:before-insert", function() { + $(".js-image-attachment").closest(".image").remove(); + }); $("#nested-image").on("cocoon:after-insert", function(e, nested_image) { var input; $("#new_image_link").addClass("hide"); diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index 27eeeff3e..9de787867 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -227,6 +227,17 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p expect(page).not_to have_css ".image" expect(page).to have_css "a#new_image_link" end + + scenario "don't duplicate fields after removing and adding an image" do + create(:image, imageable: imageable) + do_login_for user + + visit send(path, arguments) + click_link "Remove image" + click_link "Add image" + + expect(page).to have_css ".image", count: 1, visible: :all + end end end end From c0a6bf54fc14a6fd82e41726e8c58bb91b8c0ea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 23:42:52 +0200 Subject: [PATCH 3/3] Fix invisible error message in attachments In commit cc6f9391f we made the images and documents file inputs invisible (instead of using `display: none`) in order to make it possible to attach images and documents using the keyboard. However, since the error messages associated to these inputs has the same HTML class as the inputs, we were also hiding them (the `display: none` didn't affect the error messages because they've also got the `is-visible` class). Using the `[type=file]` selector we make it more explicit that we only want to style these inputs. I'm not adding a test for this scenario because technically the text is there and I'm not sure how to test for the presence of invisible elements. --- app/assets/stylesheets/mixins/uploads.scss | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/assets/stylesheets/mixins/uploads.scss b/app/assets/stylesheets/mixins/uploads.scss index 570b65326..692ec9b05 100644 --- a/app/assets/stylesheets/mixins/uploads.scss +++ b/app/assets/stylesheets/mixins/uploads.scss @@ -28,8 +28,7 @@ .attachment-errors { - > .js-image-attachment, - > .js-document-attachment { + > [type=file] { @include element-invisible; ~ .error {