From 7491f44d5456f07a2f390b643647050abe6689e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 16 Apr 2020 00:00:34 +0200 Subject: [PATCH 1/4] Remove unused shared specs These tests aren't used since commit 4db54092. --- spec/shared/features/imageable_destroy.rb | 69 ----------------------- 1 file changed, 69 deletions(-) delete mode 100644 spec/shared/features/imageable_destroy.rb diff --git a/spec/shared/features/imageable_destroy.rb b/spec/shared/features/imageable_destroy.rb deleted file mode 100644 index 539f0fe8b..000000000 --- a/spec/shared/features/imageable_destroy.rb +++ /dev/null @@ -1,69 +0,0 @@ -shared_examples "imageable destroy" do |imageable_factory_name, imageable_path, imageable_path_arguments| - include ActionView::Helpers - include ImagesHelper - include ImageablesHelper - - let!(:administrator) { create(:user) } - let!(:user) { create(:user) } - let!(:imageable_arguments) { {} } - let!(:imageables_arguments) { {} } - let!(:imageable) { create(imageable_factory_name, author: user) } - let!(:imageable_dom_name) { imageable_factory_name.parameterize } - - before do - create(:administrator, user: administrator) - - imageable_path_arguments.each do |argument_name, path_to_value| - imageable_arguments.merge!("#{argument_name}": imageable.send(path_to_value)) - end - end - - context "Destroy" do - before do - create(:image, imageable: imageable, user: imageable.author) - end - - scenario "Administrators cannot destroy imageables they have not authored" do - login_as(administrator) - - visit send(imageable_path, imageable_arguments) - expect(page).not_to have_link "Remove image" - end - - scenario "Users cannot destroy imageables they have not authored" do - login_as(create(:user)) - - visit send(imageable_path, imageable_arguments) - expect(page).not_to have_link "Remove image" - end - - scenario "Should show success notice after successful deletion" do - login_as imageable.author - - visit send(imageable_path, imageable_arguments) - click_on "Remove image" - - expect(page).to have_content "Image was deleted successfully." - end - - scenario "Should not show image after successful deletion" do - login_as imageable.author - - visit send(imageable_path, imageable_arguments) - click_on "Remove image" - - expect(page).not_to have_selector "figure img" - end - - scenario "Should redirect to imageable path after successful deletion" do - login_as imageable.author - - visit send(imageable_path, imageable_arguments) - click_on "Remove image" - - within "##{dom_id(imageable)}" do - expect(page).to have_selector "h1", text: imageable.title - end - end - end -end From 2cd4696244e09c1c691cd9d2110fd16e7691d01e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 15 Apr 2020 21:02:58 +0200 Subject: [PATCH 2/4] Don't include unneeded helpers in tests Including them might lead to conflicts since two methods might have the same name. For example, we're getting some exceptions when taking screenshots of a failing test, because the method `image_path` from `ActionView::Helpers::AssetUrlHelper` has the same name as a method used to save the screenshot. Besides, we were including all helpers in places were only the `dom_id` method is used, and in other places where no helper methods were used at all. So we can just invoke `ActionView::RecordIdentifier.dom_id` directly. --- spec/features/comments/budget_investments_spec.rb | 1 - spec/features/comments/debates_spec.rb | 1 - spec/features/comments/legislation_annotations_spec.rb | 1 - spec/features/comments/legislation_questions_spec.rb | 1 - spec/features/comments/polls_spec.rb | 1 - spec/features/comments/proposals_spec.rb | 1 - spec/features/comments/topics_spec.rb | 1 - spec/shared/features/documentable.rb | 4 +--- spec/shared/features/followable.rb | 6 ++++-- spec/shared/features/imageable.rb | 4 ---- spec/shared/features/mappable.rb | 2 -- spec/shared/features/nested_documentable.rb | 4 ---- spec/shared/features/nested_imageable.rb | 4 ---- spec/shared/models/image_validations.rb | 1 - 14 files changed, 5 insertions(+), 27 deletions(-) diff --git a/spec/features/comments/budget_investments_spec.rb b/spec/features/comments/budget_investments_spec.rb index 7a688afd2..e1b700ae0 100644 --- a/spec/features/comments/budget_investments_spec.rb +++ b/spec/features/comments/budget_investments_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -include ActionView::Helpers::DateHelper describe "Commenting Budget::Investments" do let(:user) { create :user } diff --git a/spec/features/comments/debates_spec.rb b/spec/features/comments/debates_spec.rb index 86cc31723..731beda03 100644 --- a/spec/features/comments/debates_spec.rb +++ b/spec/features/comments/debates_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -include ActionView::Helpers::DateHelper describe "Commenting debates" do let(:user) { create :user } diff --git a/spec/features/comments/legislation_annotations_spec.rb b/spec/features/comments/legislation_annotations_spec.rb index e09e423fe..18b9f40d2 100644 --- a/spec/features/comments/legislation_annotations_spec.rb +++ b/spec/features/comments/legislation_annotations_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -include ActionView::Helpers::DateHelper describe "Commenting legislation questions" do let(:user) { create :user } diff --git a/spec/features/comments/legislation_questions_spec.rb b/spec/features/comments/legislation_questions_spec.rb index 39a2b4c28..67510cae2 100644 --- a/spec/features/comments/legislation_questions_spec.rb +++ b/spec/features/comments/legislation_questions_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -include ActionView::Helpers::DateHelper describe "Commenting legislation questions" do let(:user) { create :user, :level_two } diff --git a/spec/features/comments/polls_spec.rb b/spec/features/comments/polls_spec.rb index e7953a3ec..b224e32a4 100644 --- a/spec/features/comments/polls_spec.rb +++ b/spec/features/comments/polls_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -include ActionView::Helpers::DateHelper describe "Commenting polls" do let(:user) { create :user } diff --git a/spec/features/comments/proposals_spec.rb b/spec/features/comments/proposals_spec.rb index f0c1d42204..e1f5c7daf 100644 --- a/spec/features/comments/proposals_spec.rb +++ b/spec/features/comments/proposals_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -include ActionView::Helpers::DateHelper describe "Commenting proposals" do let(:user) { create :user } diff --git a/spec/features/comments/topics_spec.rb b/spec/features/comments/topics_spec.rb index e5a44bfa8..0ebc30cf7 100644 --- a/spec/features/comments/topics_spec.rb +++ b/spec/features/comments/topics_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" -include ActionView::Helpers::DateHelper describe "Commenting topics from proposals" do let(:user) { create :user } diff --git a/spec/shared/features/documentable.rb b/spec/shared/features/documentable.rb index 5eb40d605..c360859d8 100644 --- a/spec/shared/features/documentable.rb +++ b/spec/shared/features/documentable.rb @@ -1,6 +1,4 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, documentable_path_arguments| - include ActionView::Helpers - let(:administrator) { create(:user) } let(:user) { create(:user) } let(:arguments) { {} } @@ -134,7 +132,7 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, click_on "Delete document" end - within "##{dom_id(documentable)}" do + within "##{ActionView::RecordIdentifier.dom_id(documentable)}" do expect(page).to have_selector "h1", text: documentable.title end end diff --git a/spec/shared/features/followable.rb b/spec/shared/features/followable.rb index 8bfbca12b..885ee5d9d 100644 --- a/spec/shared/features/followable.rb +++ b/spec/shared/features/followable.rb @@ -1,10 +1,12 @@ shared_examples "followable" do |followable_class_name, followable_path, followable_path_arguments| - include ActionView::Helpers - let!(:arguments) { {} } let!(:followable) { create(followable_class_name) } let!(:followable_dom_name) { followable_class_name.tr("_", "-") } + def dom_id(record) + ActionView::RecordIdentifier.dom_id(record) + end + before do followable_path_arguments.each do |argument_name, path_to_value| arguments.merge!("#{argument_name}": followable.send(path_to_value)) diff --git a/spec/shared/features/imageable.rb b/spec/shared/features/imageable.rb index fa78b2b07..b66823029 100644 --- a/spec/shared/features/imageable.rb +++ b/spec/shared/features/imageable.rb @@ -1,8 +1,4 @@ shared_examples "imageable" do |imageable_factory_name, imageable_path, imageable_path_arguments| - include ActionView::Helpers - include ImagesHelper - include ImageablesHelper - let!(:administrator) { create(:user) } let!(:user) { create(:user) } let!(:imageable_arguments) { {} } diff --git a/spec/shared/features/mappable.rb b/spec/shared/features/mappable.rb index 59488bd7c..063b02851 100644 --- a/spec/shared/features/mappable.rb +++ b/spec/shared/features/mappable.rb @@ -1,6 +1,4 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, mappable_new_path, mappable_edit_path, mappable_show_path, mappable_path_arguments, management: false| - include ActionView::Helpers - let!(:user) { create(:user, :level_two) } let!(:arguments) { {} } let!(:mappable) { create(mappable_factory_name.to_s.to_sym) } diff --git a/spec/shared/features/nested_documentable.rb b/spec/shared/features/nested_documentable.rb index 01a1c8b38..3a461b12b 100644 --- a/spec/shared/features/nested_documentable.rb +++ b/spec/shared/features/nested_documentable.rb @@ -1,8 +1,4 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_name, path, documentable_path_arguments, fill_resource_method_name, submit_button, documentable_success_notice| - include ActionView::Helpers - include DocumentsHelper - include DocumentablesHelper - let!(:administrator) { create(:user) } let!(:user) { create(:user, :level_two) } let!(:arguments) { {} } diff --git a/spec/shared/features/nested_imageable.rb b/spec/shared/features/nested_imageable.rb index 4ea707588..19592858f 100644 --- a/spec/shared/features/nested_imageable.rb +++ b/spec/shared/features/nested_imageable.rb @@ -1,8 +1,4 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_path_arguments, fill_resource_method_name, submit_button, imageable_success_notice, has_many_images = false| - include ActionView::Helpers - include ImagesHelper - include ImageablesHelper - let!(:user) { create(:user, :level_two) } let!(:administrator) { create(:administrator, user: user) } let!(:arguments) { {} } diff --git a/spec/shared/models/image_validations.rb b/spec/shared/models/image_validations.rb index e3a59b684..7285b1937 100644 --- a/spec/shared/models/image_validations.rb +++ b/spec/shared/models/image_validations.rb @@ -1,5 +1,4 @@ shared_examples "image validations" do |imageable_factory| - include ImagesHelper include ImageablesHelper let!(:image) { build(:image, imageable_factory.to_sym) } From ca2dc10ee90510d1e8f5774febc751fbb88165f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 16 Apr 2020 11:57:53 +0200 Subject: [PATCH 3/4] Simplify method to check an image max size We were converting megabytes to bytes with the `megabytes` method and then adding a `bytes_to_megabytes` method to convert it back to bytes, which is the same as not doing anything at all :). --- app/helpers/imageables_helper.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/helpers/imageables_helper.rb b/app/helpers/imageables_helper.rb index f2ed56b09..16b49caf9 100644 --- a/app/helpers/imageables_helper.rb +++ b/app/helpers/imageables_helper.rb @@ -4,11 +4,7 @@ module ImageablesHelper end def imageable_max_file_size - bytes_to_megabytes(Setting["uploads.images.max_size"].to_i.megabytes) - end - - def bytes_to_megabytes(bytes) - bytes / Numeric::MEGABYTE + Setting["uploads.images.max_size"].to_i end def imageable_accepted_content_types From 95a90b1895f046ef80fce844afa06b0d8f05fc27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 16 Apr 2020 12:04:10 +0200 Subject: [PATCH 4/4] Simplify method to calculate document max size Since we're only doing the convertion from bytes to megabytes in one place, IMHO adding an extra method makes the code harder to read. This way we don't have do include the DocumentsHelper in the specs anymore, reducing the risk of possible method naming collisions. --- app/helpers/documentables_helper.rb | 2 +- app/helpers/documents_helper.rb | 4 ---- spec/shared/models/document_validations.rb | 1 - 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/app/helpers/documentables_helper.rb b/app/helpers/documentables_helper.rb index be827159d..74d7ccf0d 100644 --- a/app/helpers/documentables_helper.rb +++ b/app/helpers/documentables_helper.rb @@ -8,7 +8,7 @@ module DocumentablesHelper end def max_file_size(documentable_class) - bytes_to_mega(documentable_class.max_file_size) + documentable_class.max_file_size / Numeric::MEGABYTE end def accepted_content_types(documentable_class) diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb index 1200b418a..0a57b7299 100644 --- a/app/helpers/documents_helper.rb +++ b/app/helpers/documents_helper.rb @@ -7,10 +7,6 @@ module DocumentsHelper document.errors[:attachment].join(", ") if document.errors.key?(:attachment) end - def bytes_to_mega(bytes) - bytes / Numeric::MEGABYTE - end - def render_destroy_document_link(builder, document) if !document.persisted? && document.cached_attachment.present? link_to t("documents.form.delete_button"), diff --git a/spec/shared/models/document_validations.rb b/spec/shared/models/document_validations.rb index 9ef685d6f..1a9a59a56 100644 --- a/spec/shared/models/document_validations.rb +++ b/spec/shared/models/document_validations.rb @@ -1,5 +1,4 @@ shared_examples "document validations" do |documentable_factory| - include DocumentsHelper include DocumentablesHelper let!(:document) { build(:document, documentable_factory.to_sym) }