From 804e513c7f0e4b4d8f5f81714743dbf9fbdb28f4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Wed, 7 Apr 2021 18:05:19 +0200
Subject: [PATCH 1/7] Remove redundant setting resets in `after` blocks

We forgot to remove these lines in commit da121ebc53.
---
 spec/system/proposals_spec.rb | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb
index 3975a6999c7..6948474c863 100644
--- a/spec/system/proposals_spec.rb
+++ b/spec/system/proposals_spec.rb
@@ -200,8 +200,6 @@
       proposal = create(:proposal)
       visit proposal_path(proposal)
       expect(page).to have_content "Access the community"
-
-      Setting["feature.community"] = false
     end
 
     scenario "Can not access the community" do
@@ -746,7 +744,6 @@
     expect(page).not_to have_current_path(edit_proposal_path(proposal))
     expect(page).to have_current_path(root_path)
     expect(page).to have_content "You do not have permission"
-    Setting["max_votes_for_proposal_edit"] = 1000
   end
 
   scenario "Update should be posible for the author of an editable proposal" do

From 95e38448c045b631c2124b1c2664a5afe80bbf02 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 10 Apr 2021 12:51:07 +0200
Subject: [PATCH 2/7] Reduce number of requests in dashboard poll tests

Just like we did in commit 0ec8878db, we remove the useless initial
request in the `before` filter since most tests started by visiting a
different URL.

We also reduce the risk of database inconsistency which comes with
setting up the database after the browser has been started.
---
 spec/system/dashboard/polls_spec.rb | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/spec/system/dashboard/polls_spec.rb b/spec/system/dashboard/polls_spec.rb
index d1280c9871d..367cc1c0855 100644
--- a/spec/system/dashboard/polls_spec.rb
+++ b/spec/system/dashboard/polls_spec.rb
@@ -3,16 +3,16 @@
 describe "Polls" do
   let!(:proposal) { create(:proposal, :draft) }
 
-  before do
-    login_as(proposal.author)
-    visit proposal_dashboard_path(proposal)
-  end
+  before { login_as(proposal.author) }
 
   scenario "Has a link to polls feature" do
+    visit proposal_dashboard_path(proposal)
+
     expect(page).to have_link("Polls")
   end
 
   scenario "Create a poll" do
+    visit proposal_dashboard_path(proposal)
     click_link "Polls"
     click_link "Create poll"
 
@@ -80,6 +80,7 @@
   end
 
   scenario "Create a poll redirects back to form when invalid data" do
+    visit proposal_dashboard_path(proposal)
     click_link "Polls"
     click_link "Create poll"
 

From cab750c8f1bbcff3e34fbdfc294c0ea6b3680512 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 10 Apr 2021 12:54:16 +0200
Subject: [PATCH 3/7] Remove redundant request in ballots spec

---
 spec/system/budgets/ballots_spec.rb | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/spec/system/budgets/ballots_spec.rb b/spec/system/budgets/ballots_spec.rb
index 6d5f6501470..1a9c161f83b 100644
--- a/spec/system/budgets/ballots_spec.rb
+++ b/spec/system/budgets/ballots_spec.rb
@@ -41,10 +41,7 @@
   end
 
   context "Voting" do
-    before do
-      login_as(user)
-      visit budget_path(budget)
-    end
+    before { login_as(user) }
 
     let!(:city) { create(:budget_group, budget: budget, name: "City") }
     let!(:districts) { create(:budget_group, budget: budget, name: "Districts") }

From 6f5180e51246cf1fd2fdccfa2a7b780a63ebf242 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 10 Apr 2021 12:59:31 +0200
Subject: [PATCH 4/7] Reduce number of requests in user search specs

We were adding a `visit` in a `before` block but then we started the
search tests with another `visit`.

We also created records in the database in between, which increased the
risk of database inconsistency since the process running the browser had
already been started.
---
 spec/system/admin/administrators_spec.rb | 13 +++++++++----
 spec/system/admin/managers_spec.rb       |  8 +++++---
 spec/system/admin/moderators_spec.rb     |  8 +++++---
 spec/system/admin/sdg/managers_spec.rb   | 11 +++++++----
 spec/system/admin/users_spec.rb          | 11 +++++++----
 spec/system/admin/valuators_spec.rb      |  8 ++++++--
 6 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/spec/system/admin/administrators_spec.rb b/spec/system/admin/administrators_spec.rb
index 32d27ce373d..9c0405ef89e 100644
--- a/spec/system/admin/administrators_spec.rb
+++ b/spec/system/admin/administrators_spec.rb
@@ -5,12 +5,11 @@
   let!(:user) { create(:user, username: "Jose Luis Balbin") }
   let!(:user_administrator) { create(:administrator, description: "admin_alias") }
 
-  before do
-    login_as(admin.user)
-    visit admin_administrators_path
-  end
+  before { login_as(admin.user) }
 
   scenario "Index" do
+    visit admin_administrators_path
+
     expect(page).to have_content user_administrator.id
     expect(page).to have_content user_administrator.name
     expect(page).to have_content user_administrator.email
@@ -19,6 +18,8 @@
   end
 
   scenario "Create Administrator" do
+    visit admin_administrators_path
+
     fill_in "search", with: user.email
     click_button "Search"
 
@@ -30,6 +31,8 @@
   end
 
   scenario "Delete Administrator" do
+    visit admin_administrators_path
+
     within "#administrator_#{user_administrator.id}" do
       accept_confirm { click_link "Delete" }
     end
@@ -40,6 +43,8 @@
   end
 
   scenario "Delete Administrator when its the current user" do
+    visit admin_administrators_path
+
     within "#administrator_#{admin.id}" do
       accept_confirm { click_link "Delete" }
     end
diff --git a/spec/system/admin/managers_spec.rb b/spec/system/admin/managers_spec.rb
index 8178187f0fa..acf3de906c2 100644
--- a/spec/system/admin/managers_spec.rb
+++ b/spec/system/admin/managers_spec.rb
@@ -4,17 +4,17 @@
   let!(:user) { create(:user) }
   let!(:manager) { create(:manager) }
 
-  before do
+  scenario "Index" do
     visit admin_managers_path
-  end
 
-  scenario "Index" do
     expect(page).to have_content manager.name
     expect(page).to have_content manager.email
     expect(page).not_to have_content user.name
   end
 
   scenario "Create Manager" do
+    visit admin_managers_path
+
     fill_in "search", with: user.email
     click_button "Search"
 
@@ -26,6 +26,8 @@
   end
 
   scenario "Delete Manager" do
+    visit admin_managers_path
+
     accept_confirm { click_link "Delete" }
 
     within("#managers") do
diff --git a/spec/system/admin/moderators_spec.rb b/spec/system/admin/moderators_spec.rb
index b9946d29b5d..8fd62b35080 100644
--- a/spec/system/admin/moderators_spec.rb
+++ b/spec/system/admin/moderators_spec.rb
@@ -4,17 +4,17 @@
   let!(:user)      { create(:user, username: "Jose Luis Balbin") }
   let!(:moderator) { create(:moderator) }
 
-  before do
+  scenario "Index" do
     visit admin_moderators_path
-  end
 
-  scenario "Index" do
     expect(page).to have_content moderator.name
     expect(page).to have_content moderator.email
     expect(page).not_to have_content user.name
   end
 
   scenario "Create Moderator" do
+    visit admin_moderators_path
+
     fill_in "search", with: user.email
     click_button "Search"
 
@@ -26,6 +26,8 @@
   end
 
   scenario "Delete Moderator" do
+    visit admin_moderators_path
+
     accept_confirm { click_link "Delete" }
 
     within("#moderators") do
diff --git a/spec/system/admin/sdg/managers_spec.rb b/spec/system/admin/sdg/managers_spec.rb
index 3830b259a20..9de82126867 100644
--- a/spec/system/admin/sdg/managers_spec.rb
+++ b/spec/system/admin/sdg/managers_spec.rb
@@ -4,18 +4,19 @@
   let!(:user) { create(:user) }
   let!(:sdg_manager) { create(:sdg_manager) }
 
-  before do
-    login_as(create(:administrator).user)
-    visit admin_sdg_managers_path
-  end
+  before { login_as(create(:administrator).user) }
 
   scenario "Index" do
+    visit admin_sdg_managers_path
+
     expect(page).to have_content sdg_manager.name
     expect(page).to have_content sdg_manager.email
     expect(page).not_to have_content user.name
   end
 
   scenario "Create SDG Manager" do
+    visit admin_sdg_managers_path
+
     fill_in "search", with: user.email
     click_button "Search"
 
@@ -29,6 +30,8 @@
   end
 
   scenario "Delete SDG Manager" do
+    visit admin_sdg_managers_path
+
     accept_confirm { click_link "Delete" }
 
     within("#sdg_managers") do
diff --git a/spec/system/admin/users_spec.rb b/spec/system/admin/users_spec.rb
index 5024a98d930..a63b8fda053 100644
--- a/spec/system/admin/users_spec.rb
+++ b/spec/system/admin/users_spec.rb
@@ -4,12 +4,11 @@
   let(:admin) { create(:administrator) }
   let!(:user) { create(:user, username: "Jose Luis Balbin") }
 
-  before do
-    login_as(admin.user)
-    visit admin_users_path
-  end
+  before { login_as(admin.user) }
 
   scenario "Index" do
+    visit admin_users_path
+
     expect(page).to have_link user.name
     expect(page).to have_content user.email
     expect(page).to have_content admin.name
@@ -17,6 +16,8 @@
   end
 
   scenario "The username links to their public profile" do
+    visit admin_users_path
+
     within_window(window_opened_by { click_link user.name }) do
       expect(page).to have_current_path(user_path(user))
     end
@@ -45,6 +46,8 @@
   end
 
   scenario "Search" do
+    visit admin_users_path
+
     fill_in :search, with: "Luis"
     click_button "Search"
 
diff --git a/spec/system/admin/valuators_spec.rb b/spec/system/admin/valuators_spec.rb
index 023db8a2f4e..fd218360d70 100644
--- a/spec/system/admin/valuators_spec.rb
+++ b/spec/system/admin/valuators_spec.rb
@@ -4,8 +4,6 @@
   let!(:user) { create(:user, username: "Jose Luis Balbin") }
   let!(:valuator) { create(:valuator, description: "Very reliable") }
 
-  before { visit admin_valuators_path }
-
   scenario "Show" do
     visit admin_valuator_path(valuator)
 
@@ -16,12 +14,16 @@
   end
 
   scenario "Index" do
+    visit admin_valuators_path
+
     expect(page).to have_content(valuator.name)
     expect(page).to have_content(valuator.email)
     expect(page).not_to have_content(user.name)
   end
 
   scenario "Create" do
+    visit admin_valuators_path
+
     fill_in "search", with: user.email
     click_button "Search"
 
@@ -51,6 +53,8 @@
   end
 
   scenario "Destroy" do
+    visit admin_valuators_path
+
     accept_confirm { click_link "Delete" }
 
     within("#valuators") do

From 8d70c9f15064abaa2cff3c467290cb8800a75c2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 10 Apr 2021 13:03:02 +0200
Subject: [PATCH 5/7] Set up data before starting the browser in tests

Changing the database after the process running the browser has started
is proving to be one of the reasons tests are failing sometimes, so
we're reducing the number of times were that happens. In this case, we
were changing a setting.
---
 spec/system/admin/site_customization/pages_spec.rb |  4 ++--
 spec/system/officing/residence_spec.rb             | 11 +++++++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/spec/system/admin/site_customization/pages_spec.rb b/spec/system/admin/site_customization/pages_spec.rb
index ff9039c372f..63ea0c8d4d2 100644
--- a/spec/system/admin/site_customization/pages_spec.rb
+++ b/spec/system/admin/site_customization/pages_spec.rb
@@ -14,13 +14,13 @@
       slugs = %w[accessibility conditions faq privacy welcome_not_verified
                  welcome_level_two_verified welcome_level_three_verified]
 
-      visit admin_site_customization_pages_path
-
       expect(SiteCustomization::Page.count).to be 7
       slugs.each do |slug|
         expect(SiteCustomization::Page.find_by(slug: slug).status).to eq "published"
       end
 
+      visit admin_site_customization_pages_path
+
       expect(all("[id^='site_customization_page_']").count).to be 7
       slugs.each do |slug|
         expect(page).to have_content slug
diff --git a/spec/system/officing/residence_spec.rb b/spec/system/officing/residence_spec.rb
index db9e303537f..9ccee04edf4 100644
--- a/spec/system/officing/residence_spec.rb
+++ b/spec/system/officing/residence_spec.rb
@@ -114,14 +114,15 @@
   context "With remote census configuration", :remote_census do
     before do
       create(:poll_officer_assignment, officer: officer)
-      login_through_form_as_officer(officer.user)
-      visit officing_root_path
     end
 
     describe "Display form fields according to the remote census configuration" do
       scenario "by default (without custom census) not display date_of_birth and postal_code" do
         Setting["feature.remote_census"] = false
 
+        login_through_form_as_officer(officer.user)
+        visit officing_root_path
+
         within("#side_menu") do
           click_link "Validate document"
         end
@@ -134,6 +135,9 @@
       end
 
       scenario "with all custom census not display year_of_birth" do
+        login_through_form_as_officer(officer.user)
+        visit officing_root_path
+
         within("#side_menu") do
           click_link "Validate document"
         end
@@ -149,6 +153,9 @@
     scenario "can verify voter with date_of_birth and postal_code fields" do
       mock_valid_remote_census_response
 
+      login_through_form_as_officer(officer.user)
+      visit officing_root_path
+
       within("#side_menu") do
         click_link "Validate document"
       end

From b52d381da5e3f9f2c518e8014855da5c0cb0f3f8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 10 Apr 2021 13:32:36 +0200
Subject: [PATCH 6/7] Simplify emulating last_sign_in_at

We were updating the database after starting the browser to emulate the
behavior where a user logs in a day before the current request. We can
use `current_sign_in_at` instead and devise will automatically copy that
value to `last_sign_in_at` after users visit a page.

This way we avoid setting up the database after the process runnin the
browser has been started.
---
 spec/system/dashboard/dashboard_spec.rb | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/spec/system/dashboard/dashboard_spec.rb b/spec/system/dashboard/dashboard_spec.rb
index 319f0f6c5c4..a7c132ea379 100644
--- a/spec/system/dashboard/dashboard_spec.rb
+++ b/spec/system/dashboard/dashboard_spec.rb
@@ -484,10 +484,7 @@
   end
 
   describe "detect_new_actions_after_last_login" do
-    before do
-      visit proposal_dashboard_path(proposal)
-      proposal.author.update!(last_sign_in_at: Date.yesterday)
-    end
+    before { proposal.author.update!(current_sign_in_at: 1.day.ago) }
 
     scenario "Display tag 'new' on resouce when it is new for author since last login" do
       resource = create(:dashboard_action, :resource, :active, day_offset: 0,
@@ -503,7 +500,7 @@
     scenario "Not display tag 'new' on resouce when there is not new resources since last login" do
       resource = create(:dashboard_action, :resource, :active, day_offset: 0,
                                                                published_proposal: false)
-      proposal.author.update!(last_sign_in_at: Date.current)
+      proposal.author.update!(current_sign_in_at: Date.current)
 
       visit progress_proposal_dashboard_path(proposal)
 
@@ -526,7 +523,7 @@
     scenario "Not display tag 'new' on proposed_action when there is not new since last login" do
       proposed_action = create(:dashboard_action, :proposed_action, :active, day_offset: 0,
                                                                      published_proposal: false)
-      proposal.author.update!(last_sign_in_at: Date.current)
+      proposal.author.update!(current_sign_in_at: Date.current)
 
       visit progress_proposal_dashboard_path(proposal)
 
@@ -547,7 +544,7 @@
 
     scenario "Not display tag 'new' on sidebar when there is not a new resouce since last login" do
       create(:dashboard_action, :resource, :active, day_offset: 0, published_proposal: false)
-      proposal.author.update!(last_sign_in_at: Date.current)
+      proposal.author.update!(current_sign_in_at: Date.current)
 
       visit progress_proposal_dashboard_path(proposal)
 

From 16fdffdf9661605854bd983cf6b3c1ba28f75b83 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 10 Apr 2021 20:51:29 +0200
Subject: [PATCH 7/7] Reduce number of requests in user tests

We were adding a `visit` in a `before` block but then we started some
tests with another `visit`.

We also destroyed records in the database in between, which increased
the risk of database inconsistency since the process running the browser
had already been started.

Besides, some tests were wrong; they were visiting a page with the
browser, then destroying records in the database, and then checking the
page without reloading the browser. Since we aren't automatically
refreshing the affected areas of the page, obviously the page content
before and after destroying records is exactly the same, and the test
was passing because it's testing content that isn't there in any
situation.
---
 spec/system/users_spec.rb | 65 ++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/spec/system/users_spec.rb b/spec/system/users_spec.rb
index 8e4560e0f05..646645983f9 100644
--- a/spec/system/users_spec.rb
+++ b/spec/system/users_spec.rb
@@ -4,16 +4,14 @@
   describe "Show (public page)" do
     let(:user) { create(:user) }
 
-    before do
-      1.times { create(:debate, author: user) }
-      2.times { create(:proposal, author: user) }
-      3.times { create(:budget_investment, author: user) }
-      4.times { create(:comment, user: user) }
+    let!(:debates)     { 1.times.map { create(:debate, author: user) } }
+    let!(:proposals)   { 2.times.map { create(:proposal, author: user) } }
+    let!(:investments) { 3.times.map { create(:budget_investment, author: user) } }
+    let!(:comments)    { 4.times.map { create(:comment, user: user) } }
 
+    scenario "shows user public activity" do
       visit user_path(user)
-    end
 
-    scenario "shows user public activity" do
       expect(page).to have_content("1 Debate")
       expect(page).to have_content("2 Proposals")
       expect(page).to have_content("3 Investments")
@@ -21,7 +19,9 @@
     end
 
     scenario "shows only items where user has activity" do
-      user.proposals.destroy_all
+      proposals.each(&:destroy)
+
+      visit user_path(user)
 
       expect(page).not_to have_content("0 Proposals")
       expect(page).to have_content("1 Debate")
@@ -30,85 +30,92 @@
     end
 
     scenario "default filter is proposals" do
-      user.proposals.each do |proposal|
+      visit user_path(user)
+
+      proposals.each do |proposal|
         expect(page).to have_content(proposal.title)
       end
 
-      user.debates.each do |debate|
+      debates.each do |debate|
         expect(page).not_to have_content(debate.title)
       end
 
-      user.comments.each do |comment|
+      comments.each do |comment|
         expect(page).not_to have_content(comment.body)
       end
     end
 
     scenario "shows debates by default if user has no proposals" do
-      user.proposals.destroy_all
+      proposals.each(&:destroy)
+
       visit user_path(user)
 
-      expect(page).to have_content(user.debates.first.title)
+      expect(page).to have_content(debates.first.title)
     end
 
     scenario "shows investments by default if user has no proposals nor debates" do
-      user.proposals.destroy_all
-      user.debates.destroy_all
+      proposals.each(&:destroy)
+      debates.each(&:destroy)
+
       visit user_path(user)
 
-      expect(page).to have_content(user.budget_investments.first.title)
+      expect(page).to have_content(investments.first.title)
     end
 
     scenario "shows comments by default if user has no proposals nor debates nor investments" do
-      user.proposals.destroy_all
-      user.debates.destroy_all
-      user.budget_investments.destroy_all
+      proposals.each(&:destroy)
+      debates.each(&:destroy)
+      investments.each(&:destroy)
+
       visit user_path(user)
 
-      user.comments.each do |comment|
+      comments.each do |comment|
         expect(page).to have_content(comment.body)
       end
     end
 
     scenario "filters" do
+      visit user_path(user)
+
       click_link "1 Debate"
 
-      user.debates.each do |debate|
+      debates.each do |debate|
         expect(page).to have_content(debate.title)
       end
 
-      user.proposals.each do |proposal|
+      proposals.each do |proposal|
         expect(page).not_to have_content(proposal.title)
       end
 
-      user.comments.each do |comment|
+      comments.each do |comment|
         expect(page).not_to have_content(comment.body)
       end
 
       click_link "4 Comments"
 
-      user.comments.each do |comment|
+      comments.each do |comment|
         expect(page).to have_content(comment.body)
       end
 
-      user.proposals.each do |proposal|
+      proposals.each do |proposal|
         expect(page).not_to have_content(proposal.title)
       end
 
-      user.debates.each do |debate|
+      debates.each do |debate|
         expect(page).not_to have_content(debate.title)
       end
 
       click_link "2 Proposals"
 
-      user.proposals.each do |proposal|
+      proposals.each do |proposal|
         expect(page).to have_content(proposal.title)
       end
 
-      user.comments.each do |comment|
+      comments.each do |comment|
         expect(page).not_to have_content(comment.body)
       end
 
-      user.debates.each do |debate|
+      debates.each do |debate|
         expect(page).not_to have_content(debate.title)
       end
     end