From f484518ee5ce0c36ec5e63bc3a061c69fc80c765 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 24 Feb 2023 13:25:23 +1100 Subject: [PATCH 1/7] Remove unused test enterprise --- spec/system/admin/enterprises/index_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/system/admin/enterprises/index_spec.rb b/spec/system/admin/enterprises/index_spec.rb index 2245641582..8261cedcf4 100644 --- a/spec/system/admin/enterprises/index_spec.rb +++ b/spec/system/admin/enterprises/index_spec.rb @@ -35,7 +35,6 @@ describe 'Enterprises Index' do end context "editing enterprises in bulk" do - let!(:s){ create(:supplier_enterprise) } let!(:d){ create(:distributor_enterprise, sells: 'none') } let!(:d_manager) { create(:user, enterprise_limit: 1) } From 401210ef443d04d364255a460cf4747d63d3d55b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 24 Feb 2023 13:34:40 +1100 Subject: [PATCH 2/7] DRY and clarify test case --- spec/system/admin/enterprises/index_spec.rb | 28 ++++++++++----------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/spec/system/admin/enterprises/index_spec.rb b/spec/system/admin/enterprises/index_spec.rb index 8261cedcf4..c06b4540b8 100644 --- a/spec/system/admin/enterprises/index_spec.rb +++ b/spec/system/admin/enterprises/index_spec.rb @@ -74,29 +74,27 @@ describe 'Enterprises Index' do login_as_admin_and_visit admin_enterprises_path end - def enterprise_row_index(enterprise_name) - enterprise_row_number = all('tr').index { |tr| tr.text.include? enterprise_name } - enterprise_row_number - 1 - end - it "does not update the enterprises and displays errors" do - d_row_index = enterprise_row_index(d.name) - within("tr.enterprise-#{d.id}") do - select d_manager.email, - from: "sets_enterprise_set_collection_attributes_#{d_row_index}_owner_id" - end + select_new_owner(d_manager, d) + select_new_owner(d_manager, second_distributor) - second_distributor_row_index = enterprise_row_index(second_distributor.name) - within("tr.enterprise-#{second_distributor.id}") do - select d_manager.email, - from: "sets_enterprise_set_collection_attributes_#{second_distributor_row_index}_owner_id" - end click_button "Update" + expect(flash_message).to eq('Update failed') expect(page).to have_content "#{d_manager.email} is not permitted to own any more enterprises (limit is 1)." second_distributor.reload expect(second_distributor.owner).to_not eq d_manager end + + def select_new_owner(user, enterprise) + enterprise_row_number = all('tr').index { |tr| tr.text.include? enterprise.name } + enterprise_row_number -= 1 + + within("tr.enterprise-#{enterprise.id}") do + select user.email, + from: "sets_enterprise_set_collection_attributes_#{enterprise_row_number}_owner_id" + end + end end end end From e6eb9412d9739daa71da4a5dd6df5177b7db4a41 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 24 Feb 2023 14:39:35 +1100 Subject: [PATCH 3/7] Simplify owner selection in spec Instead of knowing the input id generated by select2, we now rely on the fact that the owner is in the fifth column. Both could change but this is less code. --- spec/system/admin/enterprises/index_spec.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/spec/system/admin/enterprises/index_spec.rb b/spec/system/admin/enterprises/index_spec.rb index c06b4540b8..a2b5c47901 100644 --- a/spec/system/admin/enterprises/index_spec.rb +++ b/spec/system/admin/enterprises/index_spec.rb @@ -87,12 +87,9 @@ describe 'Enterprises Index' do end def select_new_owner(user, enterprise) - enterprise_row_number = all('tr').index { |tr| tr.text.include? enterprise.name } - enterprise_row_number -= 1 - - within("tr.enterprise-#{enterprise.id}") do - select user.email, - from: "sets_enterprise_set_collection_attributes_#{enterprise_row_number}_owner_id" + # The fifth table column contains the owner field. + within("tr.enterprise-#{enterprise.id} td:nth-child(5)") do + select user.email end end end From e9fa360d61b68d4bd9647101d112c2dc1505767b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 24 Feb 2023 13:41:59 +1100 Subject: [PATCH 4/7] Take test setup as granted and simplify --- spec/system/admin/enterprises/index_spec.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spec/system/admin/enterprises/index_spec.rb b/spec/system/admin/enterprises/index_spec.rb index a2b5c47901..a5cbdf6174 100644 --- a/spec/system/admin/enterprises/index_spec.rb +++ b/spec/system/admin/enterprises/index_spec.rb @@ -39,8 +39,7 @@ describe 'Enterprises Index' do let!(:d_manager) { create(:user, enterprise_limit: 1) } before do - d_manager.enterprise_roles.build(enterprise: d).save - expect(d.owner).to_not eq d_manager + d.users << d_manager end context "without violating rules" do @@ -68,8 +67,7 @@ describe 'Enterprises Index' do let!(:second_distributor) { create(:distributor_enterprise, sells: 'none') } before do - d_manager.enterprise_roles.build(enterprise: second_distributor).save - expect(d.owner).to_not eq d_manager + second_distributor.users << d_manager login_as_admin_and_visit admin_enterprises_path end From 16289a62f1dafc63d3aff7560697195b16cf25d8 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 24 Feb 2023 13:51:35 +1100 Subject: [PATCH 5/7] Rename test vars for clarity --- spec/system/admin/enterprises/index_spec.rb | 24 ++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/spec/system/admin/enterprises/index_spec.rb b/spec/system/admin/enterprises/index_spec.rb index a5cbdf6174..670bb0baca 100644 --- a/spec/system/admin/enterprises/index_spec.rb +++ b/spec/system/admin/enterprises/index_spec.rb @@ -35,11 +35,11 @@ describe 'Enterprises Index' do end context "editing enterprises in bulk" do - let!(:d){ create(:distributor_enterprise, sells: 'none') } - let!(:d_manager) { create(:user, enterprise_limit: 1) } + let!(:distributor){ create(:distributor_enterprise, sells: 'none') } + let!(:mani) { create(:user, enterprise_limit: 1) } before do - d.users << d_manager + distributor.users << mani end context "without violating rules" do @@ -48,18 +48,18 @@ describe 'Enterprises Index' do end it "updates the enterprises" do - within("tr.enterprise-#{d.id}") do + within("tr.enterprise-#{distributor.id}") do expect(page).to have_checked_field "sets_enterprise_set_collection_attributes_0_visible" uncheck "sets_enterprise_set_collection_attributes_0_visible" select 'any', from: "sets_enterprise_set_collection_attributes_0_sells" - select d_manager.email, from: 'sets_enterprise_set_collection_attributes_0_owner_id' + select mani.email, from: 'sets_enterprise_set_collection_attributes_0_owner_id' end click_button "Update" expect(flash_message).to eq('Enterprises updated successfully') - distributor = Enterprise.find(d.id) + distributor.reload expect(distributor.visible).to eq "hidden" expect(distributor.sells).to eq 'any' - expect(distributor.owner).to eq d_manager + expect(distributor.owner).to eq mani end end @@ -67,21 +67,21 @@ describe 'Enterprises Index' do let!(:second_distributor) { create(:distributor_enterprise, sells: 'none') } before do - second_distributor.users << d_manager + second_distributor.users << mani login_as_admin_and_visit admin_enterprises_path end it "does not update the enterprises and displays errors" do - select_new_owner(d_manager, d) - select_new_owner(d_manager, second_distributor) + select_new_owner(mani, distributor) + select_new_owner(mani, second_distributor) click_button "Update" expect(flash_message).to eq('Update failed') - expect(page).to have_content "#{d_manager.email} is not permitted to own any more enterprises (limit is 1)." + expect(page).to have_content "#{mani.email} is not permitted to own any more enterprises (limit is 1)." second_distributor.reload - expect(second_distributor.owner).to_not eq d_manager + expect(second_distributor.owner).to_not eq mani end def select_new_owner(user, enterprise) From 116e8440046b0b8e1d0c28f042995682a0c99846 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 24 Feb 2023 14:19:26 +1100 Subject: [PATCH 6/7] Stabilise flaky spec with defined enterprise order --- spec/system/admin/enterprises/index_spec.rb | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/spec/system/admin/enterprises/index_spec.rb b/spec/system/admin/enterprises/index_spec.rb index 670bb0baca..0d8b7c7447 100644 --- a/spec/system/admin/enterprises/index_spec.rb +++ b/spec/system/admin/enterprises/index_spec.rb @@ -35,7 +35,9 @@ describe 'Enterprises Index' do end context "editing enterprises in bulk" do - let!(:distributor){ create(:distributor_enterprise, sells: 'none') } + let!(:distributor) { + create(:distributor_enterprise, sells: 'none', name: "Adam's Market") + } let!(:mani) { create(:user, enterprise_limit: 1) } before do @@ -64,7 +66,11 @@ describe 'Enterprises Index' do end context "with data that violates rules" do - let!(:second_distributor) { create(:distributor_enterprise, sells: 'none') } + let!(:second_distributor) { + # Choose name appearing at the end of the list because the spec + # relies on the order of saving records during update. + create(:distributor_enterprise, sells: "none", name: "Zed's Shop") + } before do second_distributor.users << mani @@ -76,11 +82,14 @@ describe 'Enterprises Index' do select_new_owner(mani, distributor) select_new_owner(mani, second_distributor) - click_button "Update" + expect { + click_button "Update" + + expect(flash_message).to eq('Update failed') + expect(page).to have_content "#{mani.email} is not permitted to own any more enterprises (limit is 1)." + second_distributor.reload + }.to_not change { second_distributor.owner } - expect(flash_message).to eq('Update failed') - expect(page).to have_content "#{mani.email} is not permitted to own any more enterprises (limit is 1)." - second_distributor.reload expect(second_distributor.owner).to_not eq mani end From 10aa7730e577918241708ff7b75b5f3e4241acce Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 28 Feb 2023 12:50:55 +1100 Subject: [PATCH 7/7] Rename user var again for clarity --- spec/system/admin/enterprises/index_spec.rb | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/spec/system/admin/enterprises/index_spec.rb b/spec/system/admin/enterprises/index_spec.rb index 0d8b7c7447..3eff5ae356 100644 --- a/spec/system/admin/enterprises/index_spec.rb +++ b/spec/system/admin/enterprises/index_spec.rb @@ -38,10 +38,10 @@ describe 'Enterprises Index' do let!(:distributor) { create(:distributor_enterprise, sells: 'none', name: "Adam's Market") } - let!(:mani) { create(:user, enterprise_limit: 1) } + let!(:manager) { create(:user, enterprise_limit: 1) } before do - distributor.users << mani + distributor.users << manager end context "without violating rules" do @@ -54,14 +54,14 @@ describe 'Enterprises Index' do expect(page).to have_checked_field "sets_enterprise_set_collection_attributes_0_visible" uncheck "sets_enterprise_set_collection_attributes_0_visible" select 'any', from: "sets_enterprise_set_collection_attributes_0_sells" - select mani.email, from: 'sets_enterprise_set_collection_attributes_0_owner_id' + select manager.email, from: 'sets_enterprise_set_collection_attributes_0_owner_id' end click_button "Update" expect(flash_message).to eq('Enterprises updated successfully') distributor.reload expect(distributor.visible).to eq "hidden" expect(distributor.sells).to eq 'any' - expect(distributor.owner).to eq mani + expect(distributor.owner).to eq manager end end @@ -73,24 +73,24 @@ describe 'Enterprises Index' do } before do - second_distributor.users << mani + second_distributor.users << manager login_as_admin_and_visit admin_enterprises_path end it "does not update the enterprises and displays errors" do - select_new_owner(mani, distributor) - select_new_owner(mani, second_distributor) + select_new_owner(manager, distributor) + select_new_owner(manager, second_distributor) expect { click_button "Update" expect(flash_message).to eq('Update failed') - expect(page).to have_content "#{mani.email} is not permitted to own any more enterprises (limit is 1)." + expect(page).to have_content "#{manager.email} is not permitted to own any more enterprises (limit is 1)." second_distributor.reload }.to_not change { second_distributor.owner } - expect(second_distributor.owner).to_not eq mani + expect(second_distributor.owner).to_not eq manager end def select_new_owner(user, enterprise)