From 297466fc80a8ef55ec2c6d460cd6c64f85ffe83f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 22 Sep 2019 21:37:29 +0200 Subject: [PATCH 1/7] Extract draft version variable to a `let` block This way we can reuse the code (in some cases) and we can avoid instance variables (in some other cases). --- .../legislation/draft_versions_spec.rb | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/spec/features/legislation/draft_versions_spec.rb b/spec/features/legislation/draft_versions_spec.rb index fc27e6815..07886c2b2 100644 --- a/spec/features/legislation/draft_versions_spec.rb +++ b/spec/features/legislation/draft_versions_spec.rb @@ -142,12 +142,12 @@ describe "Legislation Draft Versions" do context "Annotations", :js do let(:user) { create(:user) } + let(:draft_version) { create(:legislation_draft_version, :published) } before { login_as user } scenario "Visit as anonymous" do logout - draft_version = create(:legislation_draft_version, :published) visit legislation_process_draft_version_path(draft_version.process, draft_version) @@ -158,8 +158,6 @@ describe "Legislation Draft Versions" do end scenario "Create" do - draft_version = create(:legislation_draft_version, :published) - visit legislation_process_draft_version_path(draft_version.process, draft_version) page.find(:css, ".legislation-annotatable").double_click @@ -182,7 +180,6 @@ describe "Legislation Draft Versions" do end scenario "View annotations and comments" do - draft_version = create(:legislation_draft_version, :published) annotation1 = create(:legislation_annotation, draft_version: draft_version, text: "my annotation", ranges: [{ "start" => "/p[1]", "startOffset" => 5, "end" => "/p[1]", "endOffset" => 10 }]) create(:legislation_annotation, draft_version: draft_version, text: "my other annotation", @@ -201,7 +198,6 @@ describe "Legislation Draft Versions" do end scenario "Publish new comment for an annotation from comments box" do - draft_version = create(:legislation_draft_version, :published) annotation = create(:legislation_annotation, draft_version: draft_version, text: "my annotation", ranges: [{ "start" => "/p[1]", "startOffset" => 6, "end" => "/p[1]", "endOffset" => 11 }]) @@ -219,13 +215,12 @@ describe "Legislation Draft Versions" do end context "Merged annotations", :js do - let(:user) { create(:user) } + let(:draft_version) { create(:legislation_draft_version, :published) } before { login_as user } scenario "View annotations and comments in an included range" do - draft_version = create(:legislation_draft_version, :published) annotation1 = create(:legislation_annotation, draft_version: draft_version, text: "my annotation", ranges: [{ "start" => "/p[1]", "startOffset" => 1, "end" => "/p[1]", "endOffset" => 5 }]) annotation2 = create(:legislation_annotation, draft_version: draft_version, text: "my other annotation", @@ -246,16 +241,17 @@ describe "Legislation Draft Versions" do end context "Annotations page" do + let(:draft_version) { create(:legislation_draft_version, :published) } + before do - @draft_version = create(:legislation_draft_version, :published) - create(:legislation_annotation, draft_version: @draft_version, text: "my annotation", quote: "ipsum", + create(:legislation_annotation, draft_version: draft_version, text: "my annotation", quote: "ipsum", ranges: [{ "start" => "/p[1]", "startOffset" => 6, "end" => "/p[1]", "endOffset" => 11 }]) - create(:legislation_annotation, draft_version: @draft_version, text: "my other annotation", quote: "audiam", + create(:legislation_annotation, draft_version: draft_version, text: "my other annotation", quote: "audiam", ranges: [{ "start" => "/p[3]", "startOffset" => 6, "end" => "/p[3]", "endOffset" => 11 }]) end scenario "See all annotations for a draft version" do - visit legislation_process_draft_version_annotations_path(@draft_version.process, @draft_version) + visit legislation_process_draft_version_annotations_path(draft_version.process, draft_version) expect(page).to have_content "ipsum" expect(page).to have_content "audiam" @@ -298,16 +294,17 @@ describe "Legislation Draft Versions" do end context "Annotation comments page" do + let(:draft_version) { create(:legislation_draft_version, :published) } + before do - @draft_version = create(:legislation_draft_version, :published) - create(:legislation_annotation, draft_version: @draft_version, text: "my annotation", quote: "ipsum", + create(:legislation_annotation, draft_version: draft_version, text: "my annotation", quote: "ipsum", ranges: [{ "start" => "/p[1]", "startOffset" => 6, "end" => "/p[1]", "endOffset" => 11 }]) - @annotation = create(:legislation_annotation, draft_version: @draft_version, text: "my other annotation", quote: "audiam", + @annotation = create(:legislation_annotation, draft_version: draft_version, text: "my other annotation", quote: "audiam", ranges: [{ "start" => "/p[3]", "startOffset" => 6, "end" => "/p[3]", "endOffset" => 11 }]) end scenario "See one annotation with replies for a draft version" do - visit legislation_process_draft_version_annotation_path(@draft_version.process, @draft_version, @annotation) + visit legislation_process_draft_version_annotation_path(draft_version.process, draft_version, @annotation) expect(page).not_to have_content "ipsum" expect(page).not_to have_content "my annotation" From 2522144ddfb10cac608bc026e5337b38790d2af6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 22 Sep 2019 01:11:33 +0200 Subject: [PATCH 2/7] Remove useless assignments in draft versions specs Now that we've moved the `draft_version` variable with `let`, these variables can be removed without making it harder to read the code vertically. --- spec/features/legislation/draft_versions_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/features/legislation/draft_versions_spec.rb b/spec/features/legislation/draft_versions_spec.rb index 07886c2b2..31bd0a69a 100644 --- a/spec/features/legislation/draft_versions_spec.rb +++ b/spec/features/legislation/draft_versions_spec.rb @@ -198,8 +198,8 @@ describe "Legislation Draft Versions" do end scenario "Publish new comment for an annotation from comments box" do - annotation = create(:legislation_annotation, draft_version: draft_version, text: "my annotation", - ranges: [{ "start" => "/p[1]", "startOffset" => 6, "end" => "/p[1]", "endOffset" => 11 }]) + create(:legislation_annotation, draft_version: draft_version, text: "my annotation", + ranges: [{ "start" => "/p[1]", "startOffset" => 6, "end" => "/p[1]", "endOffset" => 11 }]) visit legislation_process_draft_version_path(draft_version.process, draft_version) @@ -221,10 +221,10 @@ describe "Legislation Draft Versions" do before { login_as user } scenario "View annotations and comments in an included range" do - annotation1 = create(:legislation_annotation, draft_version: draft_version, text: "my annotation", - ranges: [{ "start" => "/p[1]", "startOffset" => 1, "end" => "/p[1]", "endOffset" => 5 }]) - annotation2 = create(:legislation_annotation, draft_version: draft_version, text: "my other annotation", - ranges: [{ "start" => "/p[1]", "startOffset" => 1, "end" => "/p[1]", "endOffset" => 10 }]) + create(:legislation_annotation, draft_version: draft_version, text: "my annotation", + ranges: [{ "start" => "/p[1]", "startOffset" => 1, "end" => "/p[1]", "endOffset" => 5 }]) + create(:legislation_annotation, draft_version: draft_version, text: "my other annotation", + ranges: [{ "start" => "/p[1]", "startOffset" => 1, "end" => "/p[1]", "endOffset" => 10 }]) visit legislation_process_draft_version_path(draft_version.process, draft_version) From e3a2c0c3a9e32d2743417256d58882bf74e51b2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 23 Sep 2019 05:25:14 +0200 Subject: [PATCH 3/7] Use `let` in valuator group This way we can remove a useless assignment without making the code harder to read vertically. --- spec/features/admin/valuator_groups_spec.rb | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/spec/features/admin/valuator_groups_spec.rb b/spec/features/admin/valuator_groups_spec.rb index e7fca9e77..14fb3cc72 100644 --- a/spec/features/admin/valuator_groups_spec.rb +++ b/spec/features/admin/valuator_groups_spec.rb @@ -81,10 +81,10 @@ describe "Valuator groups" do end context "Assign valuators to groups" do + let(:valuator) { create(:valuator) } scenario "Add a valuator to a group" do - valuator = create(:valuator) - group = create(:valuator_group, name: "Health") + create(:valuator_group, name: "Health") visit edit_admin_valuator_path(valuator) @@ -96,10 +96,8 @@ describe "Valuator groups" do end scenario "Update a valuator's group" do - valuator = create(:valuator) - group1 = create(:valuator_group, name: "Health") - group2 = create(:valuator_group, name: "Economy") - valuator.update(valuator_group: group1) + valuator.update(valuator_group: create(:valuator_group, name: "Economy")) + create(:valuator_group, name: "Health") visit edit_admin_valuator_path(valuator) select "Economy", from: "valuator_valuator_group_id" @@ -110,9 +108,7 @@ describe "Valuator groups" do end scenario "Remove a valuator from a group" do - valuator = create(:valuator) - group1 = create(:valuator_group, name: "Health") - valuator.update(valuator_group: group1) + valuator.update(valuator_group: create(:valuator_group, name: "Health")) visit edit_admin_valuator_path(valuator) select "", from: "valuator_valuator_group_id" From d5c0e1c8dfc4ce5c92e81c118f5ee3d0781e28e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 25 Sep 2019 19:05:48 +0200 Subject: [PATCH 4/7] Use `let` to creat users in voter spec We remove duplication, and we better isolate useless assignments. --- spec/models/poll/voter_spec.rb | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/spec/models/poll/voter_spec.rb b/spec/models/poll/voter_spec.rb index 55fa0c4ab..12c421310 100644 --- a/spec/models/poll/voter_spec.rb +++ b/spec/models/poll/voter_spec.rb @@ -9,6 +9,7 @@ describe Poll::Voter do let(:officer_assignment) { create(:poll_officer_assignment) } describe "validations" do + let(:user) { create(:user, :level_two) } it "is valid" do expect(voter).to be_valid @@ -31,8 +32,6 @@ describe Poll::Voter do end it "is not valid if the user has already voted in the same poll or booth_assignment" do - user = create(:user, :level_two) - voter1 = create(:poll_voter, user: user, poll: poll) voter2 = build(:poll_voter, user: user, poll: poll) @@ -41,8 +40,6 @@ describe Poll::Voter do end it "is not valid if the user has already voted in the same poll/booth" do - user = create(:user, :level_two) - voter1 = create(:poll_voter, user: user, poll: poll, booth_assignment: booth_assignment) voter2 = build(:poll_voter, user: user, poll: poll, booth_assignment: booth_assignment) @@ -51,8 +48,6 @@ describe Poll::Voter do end it "is not valid if the user has already voted in different booth in the same poll" do - user = create(:user, :level_two) - create(:poll_voter, :from_booth, user: user, poll: poll, booth: create(:poll_booth)) voter = build(:poll_voter, :from_booth, user: user, poll: poll, booth: booth) @@ -62,8 +57,6 @@ describe Poll::Voter do end it "is valid if the user has already voted in the same booth in different poll" do - user = create(:user, :level_two) - create(:poll_voter, :from_booth, user: user, booth: booth, poll: create(:poll)) voter = build(:poll_voter, :from_booth, user: user, booth: booth, poll: poll) From 55c4a953ea2082a367d60bdf3b200020dd445462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 25 Sep 2019 19:42:13 +0200 Subject: [PATCH 5/7] Remove `let` only used once --- spec/models/poll/voter_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/models/poll/voter_spec.rb b/spec/models/poll/voter_spec.rb index 12c421310..1d7beed42 100644 --- a/spec/models/poll/voter_spec.rb +++ b/spec/models/poll/voter_spec.rb @@ -6,7 +6,6 @@ describe Poll::Voter do let(:booth) { create(:poll_booth) } let(:booth_assignment) { create(:poll_booth_assignment, poll: poll, booth: booth) } let(:voter) { create(:poll_voter) } - let(:officer_assignment) { create(:poll_officer_assignment) } describe "validations" do let(:user) { create(:user, :level_two) } @@ -87,7 +86,7 @@ describe Poll::Voter do it "is valid with a booth origin" do voter.origin = "booth" - voter.officer_assignment = officer_assignment + voter.officer_assignment = create(:poll_officer_assignment) expect(voter).to be_valid end From 4ead96b6663e8b9e2b6d02351fc83fc9f675deba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 25 Sep 2019 19:46:24 +0200 Subject: [PATCH 6/7] Use `let` only in the examples using the variables These variables were only used in the `validations` block; the rest of the tests re-defined them, making the code hard to follow. --- spec/models/poll/voter_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/models/poll/voter_spec.rb b/spec/models/poll/voter_spec.rb index 1d7beed42..4c7e6dc62 100644 --- a/spec/models/poll/voter_spec.rb +++ b/spec/models/poll/voter_spec.rb @@ -1,13 +1,11 @@ require "rails_helper" describe Poll::Voter do - - let(:poll) { create(:poll) } - let(:booth) { create(:poll_booth) } - let(:booth_assignment) { create(:poll_booth_assignment, poll: poll, booth: booth) } - let(:voter) { create(:poll_voter) } - describe "validations" do + let(:poll) { create(:poll) } + let(:booth) { create(:poll_booth) } + let(:booth_assignment) { create(:poll_booth_assignment, poll: poll, booth: booth) } + let(:voter) { create(:poll_voter) } let(:user) { create(:user, :level_two) } it "is valid" do From 72c10ab27920993e034af371d5f90eb3fb53c4c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 28 Sep 2019 02:35:44 +0200 Subject: [PATCH 7/7] Remove duplication in email specs using `let` --- spec/lib/email_digests_spec.rb | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/spec/lib/email_digests_spec.rb b/spec/lib/email_digests_spec.rb index 15c9d743e..3a8413686 100644 --- a/spec/lib/email_digests_spec.rb +++ b/spec/lib/email_digests_spec.rb @@ -33,27 +33,23 @@ describe EmailDigest do end describe "pending_notifications?" do + let(:user) { create(:user) } it "returns true when notifications have not been emailed" do - user = create(:user) - - notification = create(:notification, :for_proposal_notification, user: user) + create(:notification, :for_proposal_notification, user: user) email_digest = EmailDigest.new(user) expect(email_digest.pending_notifications?).to be end it "returns false when notifications have been emailed" do - user = create(:user) - - notification = create(:notification, :for_proposal_notification, user: user, emailed_at: Time.current) + create(:notification, :for_proposal_notification, user: user, emailed_at: Time.current) email_digest = EmailDigest.new(user) expect(email_digest.pending_notifications?).not_to be end it "returns false when there are no notifications for a user" do - user = create(:user) email_digest = EmailDigest.new(user) expect(email_digest.pending_notifications?).not_to be end @@ -61,11 +57,10 @@ describe EmailDigest do end describe "deliver" do + let(:user) { create(:user) } it "delivers email if notifications pending" do - user = create(:user) - - notification = create(:notification, :for_proposal_notification, user: user) + create(:notification, :for_proposal_notification, user: user) reset_mailer email_digest = EmailDigest.new(user) @@ -76,8 +71,6 @@ describe EmailDigest do end it "does not deliver email if no notifications pending" do - user = create(:user) - create(:notification, :for_proposal_notification, user: user, emailed_at: Time.current) reset_mailer