From b89c358d03c45cdfc3ba2c769133475cd293213a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 6 Sep 2021 14:40:42 +0200 Subject: [PATCH 01/20] Remove comments related to code from Madrid CONSUL doesn't implement blank votes via web; the comment was based on the code used in Madrid, which was actually very complex. And the concept of "all city" was also specific to Madrid. Poll questions aren't associated to a geozone, so the geozone will depend on the poll they're associated to. --- spec/models/poll/stats_spec.rb | 4 ---- spec/system/admin/poll/questions_spec.rb | 2 -- 2 files changed, 6 deletions(-) diff --git a/spec/models/poll/stats_spec.rb b/spec/models/poll/stats_spec.rb index 166a92569..abe080fe8 100644 --- a/spec/models/poll/stats_spec.rb +++ b/spec/models/poll/stats_spec.rb @@ -56,10 +56,6 @@ describe Poll::Stats do end end - describe "#total_web_white" do - pending "Too complex to test" - end - describe "#total_web_null" do it "returns 0" do expect(stats.total_web_null).to eq(0) diff --git a/spec/system/admin/poll/questions_spec.rb b/spec/system/admin/poll/questions_spec.rb index 7dc1c49a7..6819d520d 100644 --- a/spec/system/admin/poll/questions_spec.rb +++ b/spec/system/admin/poll/questions_spec.rb @@ -151,8 +151,6 @@ describe "Admin poll questions", :admin do expect(page).to have_content(question2.title) end - pending "Mark all city by default when creating a poll question from a successful proposal" - context "Poll select box" do scenario "translates the poll name in options" do poll = create(:poll, name_en: "Name in English", name_es: "Nombre en Español") From 1f55be7c5ee09f9a8db03a223603ba5b49b8c8b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 6 Sep 2021 14:43:57 +0200 Subject: [PATCH 02/20] Remove obsolete pending test reference The test "Sender email" already checks the receiver's name appears in the copy sent to the sender. --- spec/system/emails_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/system/emails_spec.rb b/spec/system/emails_spec.rb index 57db5c376..2d15aebd7 100644 --- a/spec/system/emails_spec.rb +++ b/spec/system/emails_spec.rb @@ -255,8 +255,6 @@ describe "Emails" do expect(email).to have_body_text(direct_message.body) expect(email).to have_body_text(direct_message.receiver.name) end - - pending "In the copy sent to the sender, display the receiver's name" end context "Proposal notification digest" do From 3752fef6bf9933f8980d3928c2e35dd6e3225c2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 6 Sep 2021 15:27:02 +0200 Subject: [PATCH 03/20] Remove map page in debates The map feature was never implemented for debates (only for proposals and budget investments) and it was crashing for debates because the page didn't load the geozones. And we don't have a "geozone" field in the debates form either. So we're removing the map page alongside its (pending implementation) tests. --- app/controllers/debates_controller.rb | 2 +- app/views/debates/map.html.erb | 1 - config/routes/debate.rb | 1 - spec/models/tag_cloud_spec.rb | 1 - spec/system/debates_spec.rb | 63 --------------------------- 5 files changed, 1 insertion(+), 67 deletions(-) delete mode 100644 app/views/debates/map.html.erb diff --git a/app/controllers/debates_controller.rb b/app/controllers/debates_controller.rb index df92c3a5e..2bd4cde03 100644 --- a/app/controllers/debates_controller.rb +++ b/app/controllers/debates_controller.rb @@ -4,7 +4,7 @@ class DebatesController < ApplicationController include FlagActions include Translatable - before_action :authenticate_user!, except: [:index, :show, :map] + before_action :authenticate_user!, except: [:index, :show] before_action :set_view, only: :index before_action :debates_recommendations, only: :index, if: :current_user diff --git a/app/views/debates/map.html.erb b/app/views/debates/map.html.erb deleted file mode 100644 index d3aee6ef8..000000000 --- a/app/views/debates/map.html.erb +++ /dev/null @@ -1 +0,0 @@ -<%= render "shared/map", new_url_path: new_debate_path %> diff --git a/config/routes/debate.rb b/config/routes/debate.rb index eeb3b6afe..58f1f9215 100644 --- a/config/routes/debate.rb +++ b/config/routes/debate.rb @@ -8,7 +8,6 @@ resources :debates do end collection do - get :map get :suggest put "recommendations/disable", only: :index, controller: "debates", action: :disable_recommendations end diff --git a/spec/models/tag_cloud_spec.rb b/spec/models/tag_cloud_spec.rb index 8102ae5d4..dbc2fcbf4 100644 --- a/spec/models/tag_cloud_spec.rb +++ b/spec/models/tag_cloud_spec.rb @@ -87,7 +87,6 @@ describe TagCloud do end xit "returns tags scoped by category for debates" - xit "returns tags scoped by geozone for debates" it "orders tags by count" do 3.times { create(:proposal, tag_list: "participation") } diff --git a/spec/system/debates_spec.rb b/spec/system/debates_spec.rb index 3272b82e0..e56e77c9f 100644 --- a/spec/system/debates_spec.rb +++ b/spec/system/debates_spec.rb @@ -668,69 +668,6 @@ describe "Debates" do expect(page).to have_content("User deleted") end - context "Filter" do - context "By geozone" do - let(:california) { Geozone.create(name: "California") } - let(:new_york) { Geozone.create(name: "New York") } - - before do - create(:debate, geozone: california, title: "Bigger sequoias") - create(:debate, geozone: california, title: "Green beach") - create(:debate, geozone: new_york, title: "Sully monument") - end - - pending "From map" do - visit debates_path - - click_link "map" - within("#html_map") do - url = find("area[title='California']")[:href] - visit url - end - - within("#debates") do - expect(page).to have_css(".debate", count: 2) - expect(page).to have_content("Bigger sequoias") - expect(page).to have_content("Green beach") - expect(page).not_to have_content("Sully monument") - end - end - - pending "From geozone list" do - visit debates_path - - click_link "map" - within("#geozones") do - click_link "California" - end - within("#debates") do - expect(page).to have_css(".debate", count: 2) - expect(page).to have_content("Bigger sequoias") - expect(page).to have_content("Green beach") - expect(page).not_to have_content("Sully monument") - end - end - - pending "From debate" do - debate = create(:debate, geozone: california, title: "Surf college") - - visit debate_path(debate) - - within("#geozone") do - click_link "California" - end - - within("#debates") do - expect(page).to have_css(".debate", count: 3) - expect(page).to have_content("Surf college") - expect(page).to have_content("Bigger sequoias") - expect(page).to have_content("Green beach") - expect(page).not_to have_content("Sully monument") - end - end - end - end - context "Suggesting debates" do scenario "Shows up to 5 suggestions" do create(:debate, title: "First debate has 1 vote", cached_votes_up: 1) From c77759469d5ad08c5844204eee3cec41bda56b71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 6 Sep 2021 22:50:36 +0200 Subject: [PATCH 04/20] Enable poll comments test This feature was actually implemented, but the test was checking the wrong selectors. --- spec/system/comments/polls_spec.rb | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/spec/system/comments/polls_spec.rb b/spec/system/comments/polls_spec.rb index 105c2b91f..6528f37b3 100644 --- a/spec/system/comments/polls_spec.rb +++ b/spec/system/comments/polls_spec.rb @@ -20,23 +20,21 @@ describe "Commenting polls" do end scenario "Show" do - skip "Feature not implemented yet, review soon" - - parent_comment = create(:comment, commentable: poll) - first_child = create(:comment, commentable: poll, parent: parent_comment) - second_child = create(:comment, commentable: poll, parent: parent_comment) + parent_comment = create(:comment, commentable: poll, body: "Parent") + create(:comment, commentable: poll, parent: parent_comment, body: "First subcomment") + create(:comment, commentable: poll, parent: parent_comment, body: "Last subcomment") visit comment_path(parent_comment) - expect(page).to have_css(".comment", count: 3) - expect(page).to have_content parent_comment.body - expect(page).to have_content first_child.body - expect(page).to have_content second_child.body + expect(page).to have_css ".comment", count: 3 + expect(page).to have_content "Parent" + expect(page).to have_content "First subcomment" + expect(page).to have_content "Last subcomment" expect(page).to have_link "Go back to #{poll.name}", href: poll_path(poll) - expect(page).to have_selector("ul#comment_#{parent_comment.id}>li", count: 2) - expect(page).to have_selector("ul#comment_#{first_child.id}>li", count: 1) - expect(page).to have_selector("ul#comment_#{second_child.id}>li", count: 1) + within ".comment", text: "Parent" do + expect(page).to have_css ".comment", count: 2 + end end scenario "Link to comment show" do From 0eb666db4d0094e424bba9f3c9f5da3ef6d5b8cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 6 Sep 2021 23:10:51 +0200 Subject: [PATCH 05/20] Allow commenting on polls as moderator/admin So it works the same way as everywhere else. --- app/models/abilities/administrator.rb | 2 +- app/models/abilities/moderator.rb | 2 +- spec/models/abilities/administrator_spec.rb | 4 ++++ spec/models/abilities/moderator_spec.rb | 3 +++ spec/system/comments/polls_spec.rb | 12 ------------ 5 files changed, 9 insertions(+), 14 deletions(-) diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index 7d3c698b1..0f492cd0b 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -49,7 +49,7 @@ module Abilities can :mark_featured, Debate can :unmark_featured, Debate - can :comment_as_administrator, [Debate, Comment, Proposal, Poll::Question, Budget::Investment, + can :comment_as_administrator, [Debate, Comment, Proposal, Poll, Poll::Question, Budget::Investment, Legislation::Question, Legislation::Proposal, Legislation::Annotation, Topic] can [:search, :create, :index, :destroy, :update], ::Administrator diff --git a/app/models/abilities/moderator.rb b/app/models/abilities/moderator.rb index eac434f22..9058c70a0 100644 --- a/app/models/abilities/moderator.rb +++ b/app/models/abilities/moderator.rb @@ -5,7 +5,7 @@ module Abilities def initialize(user) merge Abilities::Moderation.new(user) - can :comment_as_moderator, [Debate, Comment, Proposal, Budget::Investment, Poll::Question, + can :comment_as_moderator, [Debate, Comment, Proposal, Budget::Investment, Poll, Poll::Question, Legislation::Question, Legislation::Annotation, Legislation::Proposal, Topic] end end diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index 7a82b4482..424638a1b 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -16,6 +16,7 @@ describe Abilities::Administrator do let(:budget_investment) { create(:budget_investment) } let(:finished_investment) { create(:budget_investment, budget: create(:budget, :finished)) } let(:legislation_question) { create(:legislation_question) } + let(:poll) { create(:poll) } let(:poll_question) { create(:poll_question) } let(:past_process) { create(:legislation_process, :past) } @@ -71,6 +72,9 @@ describe Abilities::Administrator do it { should be_able_to(:comment_as_administrator, legislation_question) } it { should_not be_able_to(:comment_as_moderator, legislation_question) } + it { should be_able_to(:comment_as_administrator, poll) } + it { should_not be_able_to(:comment_as_moderator, poll) } + it { should be_able_to(:summary, past_process) } it { should_not be_able_to(:summary, past_draft_process) } it { should_not be_able_to(:summary, open_process) } diff --git a/spec/models/abilities/moderator_spec.rb b/spec/models/abilities/moderator_spec.rb index 9f3cb14dc..b9df69067 100644 --- a/spec/models/abilities/moderator_spec.rb +++ b/spec/models/abilities/moderator_spec.rb @@ -13,6 +13,7 @@ describe Abilities::Moderator do let(:comment) { create(:comment) } let(:proposal) { create(:proposal) } let(:legislation_question) { create(:legislation_question) } + let(:poll) { create(:poll) } let(:own_debate) { create(:debate, author: user) } let(:own_comment) { create(:comment, author: user) } @@ -101,9 +102,11 @@ describe Abilities::Moderator do it { should be_able_to(:comment_as_moderator, debate) } it { should be_able_to(:comment_as_moderator, proposal) } it { should be_able_to(:comment_as_moderator, legislation_question) } + it { should be_able_to(:comment_as_moderator, poll) } it { should_not be_able_to(:comment_as_administrator, debate) } it { should_not be_able_to(:comment_as_administrator, proposal) } it { should_not be_able_to(:comment_as_administrator, legislation_question) } + it { should_not be_able_to(:comment_as_administrator, poll) } end it { should_not be_able_to(:read, SDG::Target) } diff --git a/spec/system/comments/polls_spec.rb b/spec/system/comments/polls_spec.rb index 6528f37b3..8179fd476 100644 --- a/spec/system/comments/polls_spec.rb +++ b/spec/system/comments/polls_spec.rb @@ -315,8 +315,6 @@ describe "Commenting polls" do describe "Moderators" do scenario "can create comment as a moderator" do - skip "Feature not implemented yet, review soon" - moderator = create(:moderator) login_as(moderator.user) @@ -335,8 +333,6 @@ describe "Commenting polls" do end scenario "can create reply as a moderator" do - skip "Feature not implemented yet, review soon" - citizen = create(:user, username: "Ana") manuela = create(:user, username: "Manuela") moderator = create(:moderator, user: manuela) @@ -364,8 +360,6 @@ describe "Commenting polls" do end scenario "can not comment as an administrator" do - skip "Feature not implemented yet, review soon" - moderator = create(:moderator) login_as(moderator.user) @@ -377,8 +371,6 @@ describe "Commenting polls" do describe "Administrators" do scenario "can create comment as an administrator" do - skip "Feature not implemented yet, review soon" - admin = create(:administrator) login_as(admin.user) @@ -397,8 +389,6 @@ describe "Commenting polls" do end scenario "can create reply as an administrator" do - skip "Feature not implemented yet, review soon" - citizen = create(:user, username: "Ana") manuela = create(:user, username: "Manuela") admin = create(:administrator, user: manuela) @@ -426,8 +416,6 @@ describe "Commenting polls" do end scenario "can not comment as a moderator", :admin do - skip "Feature not implemented yet, review soon" - visit poll_path(poll) expect(page).not_to have_content "Comment as moderator" From 42b8729cd86064eebede5c62af1f06e83a79e573 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 6 Sep 2021 14:43:53 +0200 Subject: [PATCH 06/20] Remove duplicate map test We were skipping the test, but there's an identical test right below it. --- spec/shared/system/mappable.rb | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index ca05c87f5..d73620697 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -232,17 +232,6 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, expect(page).not_to have_css(".map_location") end - scenario "No errors on update" do - skip "" - do_login_for mappable.author, management: management - - visit send(mappable_edit_path, id: mappable.id) - click_link "Remove map marker" - click_on "Save changes" - - expect(page).not_to have_content "Map location can't be blank" - end - scenario "No need to skip map on update" do do_login_for mappable.author, management: management From d5867db2cf7e6c4ef6397499d03415a999d3f696 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 7 Sep 2021 01:14:41 +0200 Subject: [PATCH 07/20] Remove unnecessary condition to skip tag list test This file only has tests related to tags; if the model doesn't have tags, we simply wouldn't include `it_behaves_as` in their tests instead of including it and then skipping it. --- spec/shared/models/sanitizable.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/spec/shared/models/sanitizable.rb b/spec/shared/models/sanitizable.rb index 356a625be..49053fb5c 100644 --- a/spec/shared/models/sanitizable.rb +++ b/spec/shared/models/sanitizable.rb @@ -2,12 +2,6 @@ shared_examples "sanitizable" do let(:sanitizable) { build(model_name(described_class)) } describe "#tag_list" do - before do - unless described_class.included_modules.include?(Taggable) - skip "#{described_class} does not have a tag list" - end - end - it "sanitizes the tag list" do sanitizable.tag_list = "user_id=1" From fa15ac0c3b9958b647d1e0eab182ce698d840ec8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 7 Sep 2021 14:46:10 +0200 Subject: [PATCH 08/20] Implement pending email digest test --- spec/system/emails_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/system/emails_spec.rb b/spec/system/emails_spec.rb index 2d15aebd7..1231106ef 100644 --- a/spec/system/emails_spec.rb +++ b/spec/system/emails_spec.rb @@ -304,6 +304,7 @@ describe "Emails" do notification2.reload expect(notification1.emailed_at).to be expect(notification2.emailed_at).to be + expect(email_digest.notifications).to be_empty end scenario "notifications moderated are not sent" do @@ -320,8 +321,6 @@ describe "Emails" do expect { open_last_email }.to raise_error "No email has been sent!" end - - xscenario "Delete all Notifications included in the digest after email sent" end context "User invites" do From 6ddb22c1ea9bb918954f8c5fc813c2ba75758e2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 7 Sep 2021 14:53:20 +0200 Subject: [PATCH 09/20] Enable test checking alert to finish valuation It looks like it was disabled because it was failing sometimes for some reason. I haven't found the reason, though; we're changing the test a little bit to make it easier to read. Enabling it will let us find out whether it still fails. --- spec/system/admin/budget_investments_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index fd0d8779e..6430f62da 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/spec/system/admin/budget_investments_spec.rb @@ -1264,19 +1264,17 @@ describe "Admin budget investments", :admin do expect(find("#js-investment-report-alert")).to be_checked end - # The feature tested in this scenario works as expected but some underlying reason - # we're not aware of makes it fail at random - xscenario "Shows alert with unfeasible status when 'Valuation finished' is checked" do + scenario "Shows alert with unfeasible status when 'Valuation finished' is checked" do budget_investment = create(:budget_investment, :unfeasible) visit admin_budget_budget_investment_path(budget_investment.budget, budget_investment) click_link "Edit dossier" - expect(page).to have_content("Valuation finished") - valuation = find_field("budget_investment[valuation_finished]") + expect(page).to have_field "Valuation finished", checked: false + accept_confirm { check("Valuation finished") } - expect(valuation).to be_checked + expect(page).to have_field "Valuation finished", checked: true end scenario "Undoes check in 'Valuation finished' if user clicks 'cancel' on alert" do From 695d5d8765c691eb8b04a704c5eb9e92059f4fa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 7 Sep 2021 14:57:53 +0200 Subject: [PATCH 10/20] Enable passing legislation comment test It was disabled in commit 792b15b22 for unknown reasons. --- spec/system/comments/legislation_annotations_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/system/comments/legislation_annotations_spec.rb b/spec/system/comments/legislation_annotations_spec.rb index 18b669f1f..07658eafb 100644 --- a/spec/system/comments/legislation_annotations_spec.rb +++ b/spec/system/comments/legislation_annotations_spec.rb @@ -131,7 +131,7 @@ describe "Commenting legislation questions" do expect(c2.body).to appear_before(c3.body) end - xscenario "Creation date works differently in roots and in child comments, even when sorting by confidence_score" do + scenario "Creation date works differently in roots and in child comments, even when sorting by confidence_score" do old_root = create(:comment, commentable: legislation_annotation, created_at: Time.current - 10) new_root = create(:comment, commentable: legislation_annotation, created_at: Time.current) old_child = create(:comment, commentable: legislation_annotation, parent_id: new_root.id, created_at: Time.current - 10) From 6c322e20f499d6d8ded8007ac49e489c32a3ac35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 8 Sep 2021 00:04:23 +0200 Subject: [PATCH 11/20] Implement tests to disable homepage settings They were marked as pending. Note Capybara doesn't support finding a button by its `aria-labelledby` attribute, so we're using the ugly `click_button "Yes"`, like we did in commit fabe97e50. --- spec/system/admin/homepage/homepage_spec.rb | 51 ++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/spec/system/admin/homepage/homepage_spec.rb b/spec/system/admin/homepage/homepage_spec.rb index 314e68247..274c537bf 100644 --- a/spec/system/admin/homepage/homepage_spec.rb +++ b/spec/system/admin/homepage/homepage_spec.rb @@ -119,7 +119,56 @@ describe "Homepage", :admin do expect(page).to have_css(".legislation-process", count: 3) end - xscenario "Deactivate" + scenario "Deactivate proposals" do + Setting["homepage.widgets.feeds.proposals"] = true + create(:proposal) + + visit admin_homepage_path + + within("#widget_feed_#{proposals_feed.id}") do + click_button "Yes" + + expect(page).to have_button "No" + end + + visit root_path + + expect(page).not_to have_content "Most active proposals" + end + + scenario "Deactivate debates" do + Setting["homepage.widgets.feeds.debates"] = true + create(:debate) + + visit admin_homepage_path + + within("#widget_feed_#{debates_feed.id}") do + click_button "Yes" + + expect(page).to have_button "No" + end + + visit root_path + + expect(page).not_to have_content "Most active debates" + end + + scenario "Deactivate processes" do + Setting["homepage.widgets.feeds.processes"] = true + create(:legislation_process) + + visit admin_homepage_path + + within("#widget_feed_#{processes_feed.id}") do + click_button "Yes" + + expect(page).to have_button "No" + end + + visit root_path + + expect(page).not_to have_content "Open processes" + end end scenario "Cards" do From 4a8a4eacddcfa820a08a7d6224cf8bd463425af4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 8 Sep 2021 00:41:51 +0200 Subject: [PATCH 12/20] Remove pending tag cloud test for debates This feature was only enabled for proposals five years ago, and it hasn't changed since then. The pending test only gets in the way. Implement. Or implement not. There is no pending. --- spec/models/tag_cloud_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/models/tag_cloud_spec.rb b/spec/models/tag_cloud_spec.rb index dbc2fcbf4..349d7922c 100644 --- a/spec/models/tag_cloud_spec.rb +++ b/spec/models/tag_cloud_spec.rb @@ -86,8 +86,6 @@ describe TagCloud do expect(tag_names(tag_cloud)).to contain_exactly("parks") end - xit "returns tags scoped by category for debates" - it "orders tags by count" do 3.times { create(:proposal, tag_list: "participation") } create(:proposal, tag_list: "corruption") From c5791278b2ac1db12121599c1dc5d41e4f205746 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 8 Sep 2021 01:00:23 +0200 Subject: [PATCH 13/20] Implement pending card image expectations --- spec/system/admin/widgets/cards_spec.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/spec/system/admin/widgets/cards_spec.rb b/spec/system/admin/widgets/cards_spec.rb index ed299dd03..af4f215d2 100644 --- a/spec/system/admin/widgets/cards_spec.rb +++ b/spec/system/admin/widgets/cards_spec.rb @@ -207,6 +207,19 @@ describe "Cards", :admin do end end + scenario "Show image if it is present" do + card_1 = create(:widget_card, cardable: custom_page, title: "Card one") + card_2 = create(:widget_card, cardable: custom_page, title: "Card two") + + card_1.update!(image: create(:image, imageable: card_1, attachment: fixture_file_upload("clippy.jpg"))) + card_2.update!(image: nil) + + visit custom_page.url + + within(".card", text: "CARD ONE") { expect(page).to have_css "img" } + within(".card", text: "CARD TWO") { expect(page).not_to have_css "img" } + end + scenario "Edit" do create(:widget_card, cardable: custom_page, title: "Original title") @@ -247,8 +260,6 @@ describe "Cards", :admin do end end - pending "add image expectactions" - def attach_image_to_card click_link "Add image" attach_file "Choose image", file_fixture("clippy.jpg") From 702fc843915b94264cea99b31451125e892e6168 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 8 Sep 2021 02:53:54 +0200 Subject: [PATCH 14/20] Remove pending reportable tests These tests are obsolete since commit 5ed308c6f. --- spec/models/concerns/reportable.rb | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/spec/models/concerns/reportable.rb b/spec/models/concerns/reportable.rb index e35e4e759..ef28edef2 100644 --- a/spec/models/concerns/reportable.rb +++ b/spec/models/concerns/reportable.rb @@ -57,14 +57,6 @@ shared_examples "reportable" do expect(saved_reportable.results_enabled?).to be false expect(saved_reportable.results_enabled).to be false end - - it "uses the `has_one` relation instead of the original column" do - skip "there's no original column" unless reportable.has_attribute?(:results_enabled) - - reportable.update!(results_enabled: true) - - expect(reportable.read_attribute(:results_enabled)).to be false - end end describe "#stats_enabled" do @@ -93,13 +85,5 @@ shared_examples "reportable" do expect(saved_reportable.stats_enabled?).to be false expect(saved_reportable.stats_enabled).to be false end - - it "uses the `has_one` relation instead of the original column" do - skip "there's no original column" unless reportable.has_attribute?(:stats_enabled) - - reportable.update!(stats_enabled: true) - - expect(reportable.read_attribute(:stats_enabled)).to be false - end end end From 2927174e06698603c02ab19a574458e82056425f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 8 Sep 2021 03:02:51 +0200 Subject: [PATCH 15/20] Remove unnecessary locales check in specs We define the available locales in the test environment, so Spanish is always available in this environment even if it isn't available in the production environment. --- spec/system/verification/level_two_verification_spec.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/spec/system/verification/level_two_verification_spec.rb b/spec/system/verification/level_two_verification_spec.rb index 499c3d8eb..513ac2b62 100644 --- a/spec/system/verification/level_two_verification_spec.rb +++ b/spec/system/verification/level_two_verification_spec.rb @@ -24,10 +24,7 @@ describe "Level two verification" do end context "In Spanish, with no fallbacks" do - before do - skip unless I18n.available_locales.include?(:es) - allow(I18n.fallbacks).to receive(:[]).and_return([:es]) - end + before { allow(I18n.fallbacks).to receive(:[]).and_return([:es]) } scenario "Works normally" do user = create(:user) From e49c32638d9903202f9ec540379e2c20ad35b72e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 8 Sep 2021 03:24:15 +0200 Subject: [PATCH 16/20] Use `if` instead of `skip` to skip tests This way the tests won't appear as "pending" when running the test suite, and so we get rid of a lot of noise in the test results. There doesn't seem to be a way to call `skip` without the test being marked as "pending". Note that in the globalizable tests we need to build a factory before deciding whether an atribute is required or not (particularly for the milestone factory, since milestone attributes are required depending on the presence of other attributes). This isn't possible before we're inside the test, so we can't add an `if:` condition to the test. So we're adding the condition inside the test instead. A minor inconvenience of this method is the test still runs even when the condition is `false`. --- spec/models/concerns/globalizable.rb | 64 ++++++++++----------- spec/shared/system/flaggable.rb | 4 +- spec/shared/system/mappable.rb | 4 +- spec/shared/system/nested_documentable.rb | 11 +--- spec/shared/system/nested_imageable.rb | 21 +++---- spec/shared/system/remotely_translatable.rb | 33 +++-------- spec/system/admin/poll/questions_spec.rb | 11 +--- 7 files changed, 57 insertions(+), 91 deletions(-) diff --git a/spec/models/concerns/globalizable.rb b/spec/models/concerns/globalizable.rb index 8b6555b87..54135137c 100644 --- a/spec/models/concerns/globalizable.rb +++ b/spec/models/concerns/globalizable.rb @@ -47,15 +47,15 @@ shared_examples_for "globalizable" do |factory_name| end it "Does not create invalid translations in the database" do - skip("cannot have invalid translations") if required_fields.empty? + if required_fields.any? + record.update(translations_attributes: [{ locale: :fr }]) - record.update(translations_attributes: [{ locale: :fr }]) + expect(record.translations.map(&:locale)).to match_array %i[en es fr] - expect(record.translations.map(&:locale)).to match_array %i[en es fr] + record.reload - record.reload - - expect(record.translations.map(&:locale)).to match_array %i[en es] + expect(record.translations.map(&:locale)).to match_array %i[en es] + end end it "Does not automatically add a translation for the current locale" do @@ -84,17 +84,17 @@ shared_examples_for "globalizable" do |factory_name| end it "Does not save invalid translations" do - skip("cannot have invalid translations") if required_fields.empty? + if required_fields.any? + record.update(translations_attributes: [ + { id: record.translations.find_by(locale: :es).id, attribute => "" } + ]) - record.update(translations_attributes: [ - { id: record.translations.find_by(locale: :es).id, attribute => "" } - ]) + I18n.with_locale(:es) { expect(record.send(attribute)).to eq "" } - I18n.with_locale(:es) { expect(record.send(attribute)).to eq "" } + record.reload - record.reload - - I18n.with_locale(:es) { expect(record.send(attribute)).to eq "En español" } + I18n.with_locale(:es) { expect(record.send(attribute)).to eq "En español" } + end end it "Does not automatically add a translation for the current locale" do @@ -122,33 +122,33 @@ shared_examples_for "globalizable" do |factory_name| end it "Does not remove all translations" do - skip("cannot have invalid translations") if required_fields.empty? + if required_fields.any? + record.translations_attributes = [ + { id: record.translations.find_by(locale: :en).id, _destroy: true }, + { id: record.translations.find_by(locale: :es).id, _destroy: true } + ] - record.translations_attributes = [ - { id: record.translations.find_by(locale: :en).id, _destroy: true }, - { id: record.translations.find_by(locale: :es).id, _destroy: true } - ] + expect(record).not_to be_valid - expect(record).not_to be_valid + record.reload - record.reload - - expect(record.translations.map(&:locale)).to match_array %i[en es] + expect(record.translations.map(&:locale)).to match_array %i[en es] + end end it "Does not remove translations when there's invalid data" do - skip("cannot have invalid translations") if required_fields.empty? + if required_fields.any? + record.translations_attributes = [ + { id: record.translations.find_by(locale: :es).id, attribute => "" }, + { id: record.translations.find_by(locale: :en).id, _destroy: true } + ] - record.translations_attributes = [ - { id: record.translations.find_by(locale: :es).id, attribute => "" }, - { id: record.translations.find_by(locale: :en).id, _destroy: true } - ] + expect(record).not_to be_valid - expect(record).not_to be_valid + record.reload - record.reload - - expect(record.translations.map(&:locale)).to match_array %i[en es] + expect(record.translations.map(&:locale)).to match_array %i[en es] + end end end diff --git a/spec/shared/system/flaggable.rb b/spec/shared/system/flaggable.rb index 3b5ad64ef..b40f3c480 100644 --- a/spec/shared/system/flaggable.rb +++ b/spec/shared/system/flaggable.rb @@ -91,9 +91,7 @@ shared_examples "flaggable" do |factory_name, admin: false| end end - scenario "Flagging a comment with a child does not update its children" do - skip "Only for comments" unless flaggable.is_a?(Comment) - + scenario "Flagging a comment with a child does not update its children", if: factory_name =~ /comment/ do child_comment = create(:comment, commentable: flaggable.commentable, parent: flaggable) login_as(user) diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index d73620697..b1551a47a 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -173,9 +173,7 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, end end - describe "At #{mappable_edit_path}" do - before { skip } if mappable_edit_path.blank? - + describe "At #{mappable_edit_path}", if: mappable_edit_path.present? do scenario "Should edit map on #{mappable_factory_name} and contain default values" do do_login_for mappable.author, management: management diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index 46e6f9f83..dafc01796 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -201,10 +201,8 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na expect(page).to have_content documentable_success_notice end - scenario "Should show new document after successful creation with one uploaded file" do - if documentable_factory_name == "dashboard_action" - skip("Not render Documents count on dashboard_actions") - end + scenario "Should show new document after successful creation with one uploaded file", + unless: documentable_factory_name == "dashboard_action" do do_login_for user_to_login, management: management visit send(path, arguments) send(fill_resource_method_name) if fill_resource_method_name @@ -223,10 +221,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na end scenario "Should show resource with new document after successful creation with - maximum allowed uploaded files" do - if documentable_factory_name == "dashboard_action" - skip("Not render Documents count on dashboard_actions") - end + maximum allowed uploaded files", unless: documentable_factory_name == "dashboard_action" do do_login_for user_to_login, management: management visit send(path, arguments) diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index 626f16174..cbe7a48b3 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -128,8 +128,8 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p end end - scenario "Render image preview after sending the form with validation errors" do - skip "Question answers behave differently" if imageable.is_a?(Poll::Question::Answer) + scenario "Render image preview after sending the form with validation errors", + unless: imageable_factory_name == "poll_question_answer" do do_login_for user, management: management visit send(path, arguments) @@ -154,17 +154,14 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p expect(page).not_to have_selector("#nested-image .image") end - scenario "Should show successful notice when resource filled correctly without any nested images" do - if has_many_images - skip "no need to test, there are no attributes for the parent resource" - else - do_login_for user, management: management - visit send(path, arguments) + scenario "Should show successful notice when resource filled correctly without any nested images", + unless: has_many_images do + do_login_for user, management: management + visit send(path, arguments) - send(fill_resource_method_name) if fill_resource_method_name - click_on submit_button - expect(page).to have_content imageable_success_notice - end + send(fill_resource_method_name) if fill_resource_method_name + click_on submit_button + expect(page).to have_content imageable_success_notice end scenario "Should show successful notice when resource filled correctly and after valid file uploads" do diff --git a/spec/shared/system/remotely_translatable.rb b/spec/shared/system/remotely_translatable.rb index 0b49ac745..c38678e46 100644 --- a/spec/shared/system/remotely_translatable.rb +++ b/spec/shared/system/remotely_translatable.rb @@ -38,8 +38,7 @@ shared_examples "remotely_translatable" do |factory_name, path_name, path_argume expect(page).not_to have_button("Traducir página") end - scenario "should not be present when there are no resources to translate" do - skip("only index_path") if show_path?(path_name) + scenario "should not be present when there are no resources to translate", if: index_path?(path_name) do resource.destroy! visit path @@ -60,13 +59,8 @@ shared_examples "remotely_translatable" do |factory_name, path_name, path_argume end end - describe "should ignore missing translations on resource comments" do - before do - if show_path?(path_name) || !commentable?(resource) - skip("only index_path") - end - end - + describe "should ignore missing translations on resource comments", + if: index_path?(path_name) && commentable?(factory_name) do scenario "is not present when a resource translation exists but its comment has not tanslations" do add_translations(resource, :es) create(:comment, commentable: resource) @@ -79,13 +73,7 @@ shared_examples "remotely_translatable" do |factory_name, path_name, path_argume end end - describe "should evaluate missing translations on resource comments" do - before do - if index_path?(path_name) - skip("only show_path") - end - end - + describe "should evaluate missing translations on resource comments", if: show_path?(path_name) do scenario "display when exists resource translations but the comment does not have a translation" do add_translations(resource, :es) create(:comment, commentable: resource) @@ -109,9 +97,7 @@ shared_examples "remotely_translatable" do |factory_name, path_name, path_argume end end - describe "should evaluate missing translations on featured_debates" do - before { skip("only debates index path") if path_name != "debates_path" } - + describe "should evaluate missing translations on featured_debates", if: path_name == "debates_path" do scenario "display when exists featured_debates without tanslations" do add_translations(resource, :es) create_featured_debates @@ -124,9 +110,8 @@ shared_examples "remotely_translatable" do |factory_name, path_name, path_argume end end - describe "should evaluate missing translations on featured_proposals" do - before { skip("only proposals index path") if path_name != "proposals_path" } - + describe "should evaluate missing translations on featured_proposals", + if: path_name == "proposals_path" do scenario "display when exists featured_proposals without tanslations" do add_translations(resource, :es) create_featured_proposals @@ -230,8 +215,8 @@ def show_path?(path) !index_path?(path) end -def commentable?(resource) - Comment::COMMENTABLE_TYPES.include?(resource.class.to_s) +def commentable?(factory_name) + Comment::COMMENTABLE_TYPES.include?(FactoryBot.factories[factory_name].build_class.to_s) end def generate_response(resource) diff --git a/spec/system/admin/poll/questions_spec.rb b/spec/system/admin/poll/questions_spec.rb index 6819d520d..f7062ea67 100644 --- a/spec/system/admin/poll/questions_spec.rb +++ b/spec/system/admin/poll/questions_spec.rb @@ -167,11 +167,8 @@ describe "Admin poll questions", :admin do options: ["Seleccionar votación", poll.name_es]) end - scenario "uses fallback if name is not translated to current locale" do - unless globalize_french_fallbacks.first == :es - skip("Spec only useful when French falls back to Spanish") - end - + scenario "uses fallback if name is not translated to current locale", + if: Globalize.fallbacks(:fr).reject { |locale| locale.match(/fr/) }.first == :es do poll = create(:poll, name_en: "Name in English", name_es: "Nombre en Español") proposal = create(:proposal) @@ -186,8 +183,4 @@ describe "Admin poll questions", :admin do options: ["Sélectionner un vote", poll.name_es]) end end - - def globalize_french_fallbacks - Globalize.fallbacks(:fr).reject { |locale| locale.match(/fr/) } - end end From c0f71c4c8d17e0183fa217c9b6fb76dbe0149a56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 4 Apr 2022 12:30:31 +0200 Subject: [PATCH 17/20] Complete proposal notification test We were finishing the test with the first "visit", so it was doing nothing (other than potentially generating concurrency issues with other tests). --- spec/system/proposal_notifications_spec.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/spec/system/proposal_notifications_spec.rb b/spec/system/proposal_notifications_spec.rb index b26f9606e..f41be4d65 100644 --- a/spec/system/proposal_notifications_spec.rb +++ b/spec/system/proposal_notifications_spec.rb @@ -313,12 +313,23 @@ describe "Proposal Notifications" do scenario "Proposal retired by author" do author = create(:user) user = create(:user) - proposal = create(:proposal, author: author, voters: [user]) + proposal = create(:proposal, :retired, author: author, followers: [user]) login_as(author) - visit root_path visit new_proposal_notification_path(proposal_id: proposal.id) + + fill_in "Title", with: "Thank you for supporting my proposal" + fill_in "Message", with: "Please share it with others so we can make it happen!" + click_button "Send notification" + + expect(page).to have_content "Your message has been sent correctly." + + logout + login_as(user) + visit notifications_path + + expect(page).to have_content "This resource is not available anymore" end context "Group notifications" do From fb99d8cb33da6e7ba3c655b9aa4960cc1a2a58a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 4 Apr 2022 13:45:40 +0200 Subject: [PATCH 18/20] Remove obsolete "pending" test I'd say this feature is actually tested in the "proposal polls specific validations"; the empty test was probably added by accident in commit 4b8cc85c4. --- spec/models/poll/poll_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/models/poll/poll_spec.rb b/spec/models/poll/poll_spec.rb index 18cf75c4a..0066231f1 100644 --- a/spec/models/poll/poll_spec.rb +++ b/spec/models/poll/poll_spec.rb @@ -35,8 +35,6 @@ describe Poll do poll.ends_at = 2.months.ago expect(poll).not_to be_valid end - - pending "no overlapping polls for proposal polls are allowed" end describe "proposal polls specific validations" do From e637bce3d8e6bbe425f65b060dd4b2848207996f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 4 Apr 2022 15:48:31 +0200 Subject: [PATCH 19/20] Fix error messages for question answer images Since we were creating a new answer in the form, we weren't getting the errors associated to the answer the administrator was trying to create, and so we were skipping the test. Using the answer which contains the information about validation errors fixes the issue and so we don't have to skip the tests. --- .../admin/poll/questions/answers/images/new.html.erb | 2 +- spec/shared/system/nested_imageable.rb | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/app/views/admin/poll/questions/answers/images/new.html.erb b/app/views/admin/poll/questions/answers/images/new.html.erb index fa373fbc1..0ba27107b 100644 --- a/app/views/admin/poll/questions/answers/images/new.html.erb +++ b/app/views/admin/poll/questions/answers/images/new.html.erb @@ -1,5 +1,5 @@
- <%= form_for(Poll::Question::Answer.new, + <%= form_for(@answer, url: admin_answer_images_path(@answer), method: :post) do |f| %> <%= render "shared/errors", resource: @answer %> diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index cbe7a48b3..fb83243ba 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -119,12 +119,8 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p click_link "Add image" click_on submit_button - if has_many_images - # Pending. Review soon and test - else - within "#nested-image .image" do - expect(page).to have_content("can't be blank", count: 2) - end + within "#nested-image .image" do + expect(page).to have_content("can't be blank", count: 2) end end From b68aa67b1d03770d801e3e9a4199fc74de02e46c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 4 Apr 2022 16:00:30 +0200 Subject: [PATCH 20/20] Remove unnecessary "pending" comment The test is already working with poll question answers (which are the only ones using `has_many_images`) as well. --- spec/shared/system/nested_imageable.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index fb83243ba..1d6d1899e 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -186,12 +186,8 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p click_on submit_button imageable_redirected_to_resource_show_or_navigate_to(imageable) - if has_many_images - # Pending. Review soon and test - else - expect(page).to have_selector "figure img" - expect(page).to have_selector "figure figcaption" if show_caption_for?(imageable_factory_name) - end + expect(page).to have_selector "figure img" + expect(page).to have_selector "figure figcaption" if show_caption_for?(imageable_factory_name) end scenario "Different URLs for different images" do