From 1b8a07972724db6f9c4a74c1d702c39007a75504 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 12 Mar 2025 15:23:11 +0100 Subject: [PATCH] Don't reload records in system tests As mentioned in commits like a586ba806, a7664ad81, 006128da5, b41fbfa52 and c480cdd91, accessing the database after starting the browser with the `visit` method sometimes results in database corruption and failing tests on our CI due to the process running the test accessing the database after the process running the browser has started. For example, one of these tests has recently failed on our CI: ``` 3) Users Create a level 3 user with email from scratch Failure/Error: expect(user.reload).to be_confirmed expected `#.confirmed?` to be truthy, got false ``` IMHO this is also a bad practice for system tests, since these tests should be checking what users experience. So we're modifying the tests to check the results of users interaction from the point of view of the users. For example, instead of checking that a user is now level 3 verified in the database, we're checking that the user interface states that the user is level 3 verified. Note we're adding an offset when editing the map marker by clicking on `map-location` with `.click(x: 30, y: 30)`. This way we make sure that both the latitude and longitude change from the original values; we used to clicking in the middle (no offset), which didn't change the longitude and changed the latitude just by coincidence. Also note we aren't changing tests with the `:no_js` tag, since these tests don't run a real browser in a separate process. In the future, we should also change most of these tests so they don't access the database and they use a real browser. --- spec/shared/system/mappable.rb | 34 +++++++++----- spec/support/common_actions/polls.rb | 6 +-- .../information_texts_spec.rb | 33 ++++++------- .../management/document_verifications_spec.rb | 6 ++- .../management/email_verifications_spec.rb | 47 ++++++++++++------- spec/system/management/users_spec.rb | 7 ++- spec/system/polls/voter_spec.rb | 3 +- spec/system/verification/letter_spec.rb | 5 +- .../level_three_verification_spec.rb | 17 ++----- spec/system/verification/residence_spec.rb | 13 +++-- spec/system/verification/sms_spec.rb | 11 +---- 11 files changed, 98 insertions(+), 84 deletions(-) diff --git a/spec/shared/system/mappable.rb b/spec/shared/system/mappable.rb index 9412652e3..680c15ea7 100644 --- a/spec/shared/system/mappable.rb +++ b/spec/shared/system/mappable.rb @@ -193,21 +193,35 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, end scenario "Should edit default values from map on #{mappable_factory_name} edit page" do + original_longitude = map_location.longitude + original_latitude = map_location.latitude + do_login_for mappable.author, management: management visit send(mappable_edit_path, id: mappable.id) - find(".map-location").click + find(".map-location").click(x: 30, y: 30) + + new_latitude = find_field( + "#{mappable_factory_name}_map_location_attributes_latitude", type: :hidden + ).value + new_longitude = find_field( + "#{mappable_factory_name}_map_location_attributes_longitude", type: :hidden + ).value + click_button "Save changes" expect(page).not_to have_button "Save changes" - mappable.reload - - expect(page).to have_css(".map-location") - expect(page).not_to have_css ".map-location[data-marker-latitude='#{map_location.latitude}']" - expect(page).to have_css ".map-location[data-marker-latitude='#{mappable.map_location.latitude}']" + expect(page).to have_css ".map-location" + expect(page).to have_css ".map-location[data-marker-latitude='#{new_latitude}']" + expect(page).to have_css ".map-location[data-marker-longitude='#{new_longitude}']" + expect(page).not_to have_css ".map-location[data-marker-latitude='#{original_latitude}']" + expect(page).not_to have_css ".map-location[data-marker-longitude='#{original_longitude}']" end scenario "Should edit mappable on #{mappable_factory_name} without change map" do + original_longitude = map_location.longitude + original_latitude = map_location.latitude + do_login_for mappable.author, management: management visit send(mappable_edit_path, id: mappable.id) @@ -215,11 +229,9 @@ shared_examples "mappable" do |mappable_factory_name, mappable_association_name, click_button "Save changes" expect(page).not_to have_button "Save changes" - mappable.reload - - expect(page).to have_css(".map-location") - expect(page).to have_css ".map-location[data-marker-latitude='#{map_location.latitude}']" - expect(page).to have_css ".map-location[data-marker-latitude='#{mappable.map_location.latitude}']" + expect(page).to have_css ".map-location" + expect(page).to have_css ".map-location[data-marker-latitude='#{original_latitude}']" + expect(page).to have_css ".map-location[data-marker-longitude='#{original_longitude}']" end scenario "Can not display map on #{mappable_factory_name} edit when remove map marker" do diff --git a/spec/support/common_actions/polls.rb b/spec/support/common_actions/polls.rb index ad277c948..92d294038 100644 --- a/spec/support/common_actions/polls.rb +++ b/spec/support/common_actions/polls.rb @@ -22,15 +22,13 @@ module Polls expect(Poll::Voter.count).to eq(1) end - def confirm_phone(user = nil) - user ||= User.last - + def confirm_phone(code:) fill_in "sms_phone", with: "611111111" click_button "Send" expect(page).to have_content "Enter the confirmation code sent to you by text message" - fill_in "sms_confirmation_code", with: user.reload.sms_confirmation_code + fill_in "sms_confirmation_code", with: code click_button "Send" expect(page).to have_content "Code correct" diff --git a/spec/system/admin/site_customization/information_texts_spec.rb b/spec/system/admin/site_customization/information_texts_spec.rb index ea8edd81c..c64d1fdfb 100644 --- a/spec/system/admin/site_customization/information_texts_spec.rb +++ b/spec/system/admin/site_customization/information_texts_spec.rb @@ -104,17 +104,21 @@ describe "Admin custom information texts", :admin do end scenario "Remove a translation" do - featured = create(:i18n_content, key: "debates.index.featured_debates", - value_en: "Custom featured", - value_es: "Destacar personalizado") + create(:i18n_content, key: "debates.index.featured_debates", + value_en: "Custom featured", + value_es: "Destacar personalizado") - page_title = create(:i18n_content, key: "debates.new.start_new", - value_en: "Start a new debate", - value_es: "Empezar un debate") + create(:i18n_content, key: "debates.new.start_new", + value_en: "Start a new custom debate", + value_es: "Empezar un nuevo debate personalizado") visit admin_site_customization_information_texts_path(tab: "debates") select "Español", from: "Current language" + + expect(page).to have_field "debates.index.featured_debates", with: "Destacar personalizado" + expect(page).to have_field "debates.new.start_new", with: "Empezar un nuevo debate personalizado" + click_link "Remove language" click_button "Save" @@ -124,20 +128,13 @@ describe "Admin custom information texts", :admin do visit admin_site_customization_information_texts_path(tab: "debates") - expect(page).to have_field "debates.index.featured_debates" + expect(page).to have_field "debates.index.featured_debates", with: "Custom featured" + expect(page).to have_field "debates.new.start_new", with: "Start a new custom debate" - select "English", from: "Current language" + select "Español", from: "Add language" - expect(page).to have_content "Start a new debate" - expect(page).to have_content "Custom featured" - - featured.reload - page_title.reload - - expect(page_title.value_es).to be nil - expect(featured.value_es).to be nil - expect(page_title.value_en).to eq "Start a new debate" - expect(featured.value_en).to eq "Custom featured" + expect(page).to have_field "debates.index.featured_debates", with: "Destacar" + expect(page).to have_field "debates.new.start_new", with: "Empezar un debate" end end end diff --git a/spec/system/management/document_verifications_spec.rb b/spec/system/management/document_verifications_spec.rb index 328bc647f..37be7feaf 100644 --- a/spec/system/management/document_verifications_spec.rb +++ b/spec/system/management/document_verifications_spec.rb @@ -26,7 +26,11 @@ describe "DocumentVerifications" do expect(page).to have_content "already verified" - expect(user.reload).to be_level_three_verified + visit management_document_verifications_path + fill_in "document_verification_document_number", with: user.document_number + click_button "Check document" + + expect(page).to have_content "already verified" end describe "Verifying througth Census" do diff --git a/spec/system/management/email_verifications_spec.rb b/spec/system/management/email_verifications_spec.rb index 54f511a05..ced8a369b 100644 --- a/spec/system/management/email_verifications_spec.rb +++ b/spec/system/management/email_verifications_spec.rb @@ -2,35 +2,46 @@ require "rails_helper" describe "EmailVerifications" do scenario "Verifying a level 1 user via email" do - login_as_manager - user = create(:user) - visit management_document_verifications_path - fill_in "document_verification_document_number", with: "12345678Z" - click_button "Check document" + in_browser(:user) do + login_as user + visit root_path - expect(page).to have_content "Please introduce the email used on the account" + expect(page).to have_content "Debates" + end - fill_in "email_verification_email", with: user.email - click_button "Send verification email" + in_browser(:manager) do + login_as_manager + visit management_document_verifications_path + fill_in "document_verification_document_number", with: "12345678Z" + click_button "Check document" - expect(page).to have_content "In order to completely verify this user, " \ - "it is necessary that the user clicks on a link" + expect(page).to have_content "Please introduce the email used on the account" - user.reload + fill_in "email_verification_email", with: user.email + click_button "Send verification email" - login_as(user) + expect(page).to have_content "In order to completely verify this user, " \ + "it is necessary that the user clicks on a link" + end sent_token = /.*email_verification_token=(.*)".*/.match(ActionMailer::Base.deliveries.last.body.to_s)[1] - visit email_path(email_verification_token: sent_token) - expect(page).to have_content "You are a verified user" + in_browser(:user) do + visit email_path(email_verification_token: sent_token) - expect(page).not_to have_link "Verify my account" - expect(page).to have_content "Account verified" + expect(page).to have_content "You are a verified user" + expect(page).to have_content "Account verified" + expect(page).not_to have_link "Verify my account" + end - expect(user.reload.document_number).to eq("12345678Z") - expect(user).to be_level_three_verified + in_browser(:manager) do + visit management_document_verifications_path + fill_in "document_verification_document_number", with: "12345678Z" + click_button "Check document" + + expect(page).to have_content "This user account is already verified" + end end end diff --git a/spec/system/management/users_spec.rb b/spec/system/management/users_spec.rb index b35cec7c8..4a21eb072 100644 --- a/spec/system/management/users_spec.rb +++ b/spec/system/management/users_spec.rb @@ -37,9 +37,12 @@ describe "Users" do click_button "Confirm" - expect(user.reload).to be_confirmed - expect(page).to have_content "Your account has been confirmed." + + visit account_path + + expect(page).to have_field "Username", with: "pepe" + expect(page).to have_content "Account verified" end scenario "Create a level 3 user without email from scratch" do diff --git a/spec/system/polls/voter_spec.rb b/spec/system/polls/voter_spec.rb index f907ace9e..eed5f51b3 100644 --- a/spec/system/polls/voter_spec.rb +++ b/spec/system/polls/voter_spec.rb @@ -189,6 +189,7 @@ describe "Voter" do end scenario "Voting in poll and then verifiying account" do + allow_any_instance_of(Verification::Sms).to receive(:generate_confirmation_code).and_return("1357") user = create(:user) login_through_form_as_officer(officer.user) @@ -200,7 +201,7 @@ describe "Voter" do click_link "Verify my account" verify_residence - confirm_phone(user) + confirm_phone(code: "1357") visit poll_path(poll) diff --git a/spec/system/verification/letter_spec.rb b/spec/system/verification/letter_spec.rb index 71fd9ba6e..5905677fc 100644 --- a/spec/system/verification/letter_spec.rb +++ b/spec/system/verification/letter_spec.rb @@ -15,10 +15,9 @@ describe "Verify Letter" do "we will send it to the address featuring in the data " \ "we have on file." - user.reload + visit verification_path - expect(user.letter_requested_at).to be - expect(user.letter_verification_code).to be + expect(page).to have_field "Code you received in letter" end scenario "Deny access unless verified residence" do diff --git a/spec/system/verification/level_three_verification_spec.rb b/spec/system/verification/level_three_verification_spec.rb index 060688b84..c692e1f3b 100644 --- a/spec/system/verification/level_three_verification_spec.rb +++ b/spec/system/verification/level_three_verification_spec.rb @@ -2,6 +2,7 @@ require "rails_helper" describe "Level three verification" do scenario "Verification with residency and verified sms" do + allow_any_instance_of(Verification::Sms).to receive(:generate_confirmation_code).and_return("2015") create(:geozone) user = create(:user) @@ -23,8 +24,7 @@ describe "Level three verification" do expect(page).to have_content "Security code confirmation" - user = user.reload - fill_in "sms_confirmation_code", with: user.sms_confirmation_code + fill_in "sms_confirmation_code", with: "2015" click_button "Send" expect(page).to have_content "Code correct. Your account is now verified" @@ -65,6 +65,7 @@ describe "Level three verification" do end scenario "Verification with residency and sms and letter" do + allow_any_instance_of(Verification::Sms).to receive(:generate_confirmation_code).and_return("1234") create(:geozone) user = create(:user) login_as(user) @@ -73,17 +74,7 @@ describe "Level three verification" do click_link "Verify my account" verify_residence - - fill_in "sms_phone", with: "611111111" - click_button "Send" - - expect(page).to have_content "Security code confirmation" - - user = user.reload - fill_in "sms_confirmation_code", with: user.sms_confirmation_code - click_button "Send" - - expect(page).to have_content "Code correct" + confirm_phone(code: "1234") click_link "Send me a letter with the code" diff --git a/spec/system/verification/residence_spec.rb b/spec/system/verification/residence_spec.rb index 959024cd5..eeb790648 100644 --- a/spec/system/verification/residence_spec.rb +++ b/spec/system/verification/residence_spec.rb @@ -40,11 +40,16 @@ describe "Residence" do scenario "When trying to verify a deregistered account old votes are reassigned" do erased_user = create(:user, document_number: "12345678Z", document_type: "1", erased_at: Time.current) - vote = create(:vote, voter: erased_user) new_user = create(:user) + debate = create(:debate, title: "Improve everything") + create(:vote, voter: erased_user, votable: debate) login_as(new_user) + visit debate_path(debate) + + expect(page).to have_css "button[aria-pressed='false']", exact_text: "I agree" + visit account_path click_link "Verify my account" @@ -58,9 +63,9 @@ describe "Residence" do expect(page).to have_content "Residence verified" - expect(vote.reload.voter).to eq(new_user) - expect(erased_user.reload.document_number).to be_blank - expect(new_user.reload.document_number).to eq("12345678Z") + visit debate_path(debate) + + expect(page).to have_css "button[aria-pressed='true']", exact_text: "I agree" end scenario "Error on verify" do diff --git a/spec/system/verification/sms_spec.rb b/spec/system/verification/sms_spec.rb index 13b232f05..0c70f0fd4 100644 --- a/spec/system/verification/sms_spec.rb +++ b/spec/system/verification/sms_spec.rb @@ -2,19 +2,12 @@ require "rails_helper" describe "SMS Verification" do scenario "Verify" do + allow_any_instance_of(Verification::Sms).to receive(:generate_confirmation_code).and_return("5678") user = create(:user, residence_verified_at: Time.current) login_as(user) visit new_sms_path - - fill_in "sms_phone", with: "611111111" - click_button "Send" - - expect(page).to have_content "Security code confirmation" - - user = user.reload - fill_in "sms_confirmation_code", with: user.sms_confirmation_code - click_button "Send" + confirm_phone(code: "5678") expect(page).to have_content "Code correct" end