Refactoring poll/vote/skip logic (#15441)

* Refactoring poll/vote/skip logic

Favor asking the poll if the user has already voted instead of relying
on the poll vote or skip to know about the implementation
details (e.g. should a poll vote know that there's a related model call
poll skipped?  likely not)

This refactor also tidies up the specs, for which there's quite a bit of
chatter.

Found via the `flay` gem.

* Adding method documentation

Also need to bump gitpod's build as it halted for the prior commit.

app/models/poll.rb

@@ -18,6 +18,22 @@ class Poll < ApplicationRecord
   before_save :evaluate_markdown
   after_create :create_poll_options
 
+  # We only want a user to be able to vote (or abstain) once per poll.
+  # This query helps validate that constraint.
+  #
+  #
+  # @param user_id [Integer]
+  #
+  # @return [TrueClass] if the given user has a registered vote or skip
+  # @return [FalseClass] if the given user does not have a poll vote
+  #         nor poll skip.
+  def vote_previously_recorded_for?(user_id:)
+    return true if poll_votes.where(user_id: user_id).any?
+    return true if poll_skips.where(user_id: user_id).any?
+
+    false
+  end
+
   def voting_data
     { votes_count: poll_votes_count, votes_distribution: poll_options.pluck(:id, :poll_votes_count) }
   end

app/models/poll_skip.rb

@@ -14,7 +14,9 @@ class PollSkip < ApplicationRecord
   private
 
   def one_vote_per_poll_per_user
-    already_voted = poll.poll_votes.where(user_id: user_id).any? || poll.poll_skips.where(user_id: user_id).any?
-    errors.add(:base, "cannot vote more than once in one poll") if already_voted
+    return false unless poll
+    return false unless poll.vote_previously_recorded_for?(user_id: user_id)
+
+    errors.add(:base, "cannot vote more than once in one poll")
   end
 end

app/models/poll_vote.rb

@@ -28,10 +28,9 @@ class PollVote < ApplicationRecord
 
   def one_vote_per_poll_per_user
     return false unless poll
+    return false unless poll.vote_previously_recorded_for?(user_id: user_id)
 
-    has_votes = (
-      poll.poll_votes.where(user_id: user_id).any? || poll.poll_skips.where(user_id: user_id).any?)
-    errors.add(:base, "cannot vote more than once in one poll") if has_votes
+    errors.add(:base, "cannot vote more than once in one poll")
   end
 
   def touch_poll_votes_count

spec/models/poll_skip_spec.rb

@@ -5,25 +5,23 @@ RSpec.describe PollSkip, type: :model do
   let(:user) { create(:user) }
   let(:poll) { create(:poll, article: article) }
 
-  describe "validations" do
-    context "when checking against poll" do
-      before do
-        create(:poll_skip, user: user, poll: poll)
-      end
+  context "when user has already voted (or skipped) on the poll" do
+    it "is invalid" do
+      vote = build(:poll_skip, user: user, poll: poll)
+      allow(vote.poll).to receive(:vote_previously_recorded_for?).with(user_id: user.id).and_return(true)
 
-      it "is unique across poll and user" do
-        expect(build(:poll_skip, user: user, poll: poll)).not_to be_valid
-      end
-
-      it "is valid if it belongs to the same user but to a different poll" do
-        second_poll = create(:poll, article: article)
-        expect(build(:poll_skip, user: user, poll: second_poll)).to be_valid
-      end
+      expect(vote).not_to be_valid
+      expect(vote.errors[:base]).to include("cannot vote more than once in one poll")
     end
+  end
 
-    it "is unique across user and poll votes for the poll" do
-      create(:poll_vote, user: user, poll: poll, poll_option: poll.poll_options.last)
-      expect(build(:poll_skip, user: user, poll: poll)).not_to be_valid
+  context "when user has not voted nor skipped the poll" do
+    it "is valid" do
+      vote = build(:poll_skip, user: user, poll: poll)
+      allow(vote.poll).to receive(:vote_previously_recorded_for?).with(user_id: user.id).and_return(false)
+
+      expect(vote).to be_valid
+      expect(vote.errors[:base]).to be_empty
     end
   end
 end

spec/models/poll_spec.rb

@@ -35,6 +35,33 @@ RSpec.describe Poll, type: :model do
     end
   end
 
+  describe "#vote_previously_recorded_for?" do
+    subject(:registered) { poll.vote_previously_recorded_for?(user_id: user.id) }
+
+    let(:poll) {  create(:poll) }
+    let(:user) {  create(:user) }
+
+    context "when user has existing poll_votes" do
+      before do
+        create(:poll_vote, poll: poll, user: user)
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context "when user has existing poll_skips" do
+      before do
+        create(:poll_skip, poll: poll, user: user)
+      end
+
+      it { is_expected.to be_truthy }
+    end
+
+    context "when user has no poll_skips nor poll votes" do
+      it { is_expected.to be_falsey }
+    end
+  end
+
   context "when callbacks are triggered after create" do
     it "creates options from input" do
       options = %w[hello goodbye heyheyhey]

spec/models/poll_vote_spec.rb

@@ -9,50 +9,23 @@ RSpec.describe PollVote, type: :model do
     expect(described_class.new.valid?).to be(false)
   end
 
-  it "limits one vote per user per poll" do
-    create(:poll_vote, poll_option_id: poll.poll_options.last.id, user_id: user.id, poll_id: poll.id)
-    described_class.create(poll_option_id: poll.poll_options.first.id, user_id: user.id, poll_id: poll.id)
-    described_class.create(poll_option_id: poll.poll_options.last.id, user_id: user.id, poll_id: poll.id)
-    expect(user.poll_votes.size).to eq(1)
+  context "when user has already voted (or skipped) on the poll" do
+    it "is invalid" do
+      vote = build(:poll_vote, poll_option: poll.poll_options.last, user: user, poll: poll)
+      allow(vote.poll).to receive(:vote_previously_recorded_for?).with(user_id: user.id).and_return(true)
+
+      expect(vote).not_to be_valid
+      expect(vote.errors[:base]).to include("cannot vote more than once in one poll")
+    end
   end
 
-  it "allows one vote per user across multiple polls" do
-    second_poll = create(:poll, article_id: article.id)
-    create(:poll_vote, poll_option_id: poll.poll_options.last.id, user_id: user.id, poll_id: poll.id)
-    create(:poll_vote, poll_option_id: second_poll.poll_options.last.id, user_id: user.id, poll_id: second_poll.id)
-    expect(user.reload.poll_votes.size).to eq(2)
-  end
+  context "when user has not voted nor skipped the poll" do
+    it "is valid" do
+      vote = build(:poll_vote, poll_option: poll.poll_options.last, user: user, poll: poll)
+      allow(vote.poll).to receive(:vote_previously_recorded_for?).with(user_id: user.id).and_return(false)
 
-  it "allows multiple people to vote in one poll" do
-    second_user = create(:user)
-    create(:poll_vote, poll_option_id: poll.poll_options.last.id, user_id: user.id, poll_id: poll.id)
-    create(:poll_vote, poll_option_id: poll.poll_options.last.id, user_id: second_user.id, poll_id: poll.id)
-    expect(user.poll_votes.size).to eq(1)
-    expect(second_user.poll_votes.size).to eq(1)
-  end
-
-  it "disallows a user to skip poll after voting" do
-    create(:poll_vote, poll_option_id: poll.poll_options.last.id, user_id: user.id, poll_id: poll.id)
-    PollSkip.create(poll_id: poll.id, user_id: user.id)
-    expect(user.poll_skips.size).to eq(0)
-  end
-
-  it "disallows a vote after skipping" do
-    PollSkip.create(poll_id: poll.id, user_id: user.id)
-    described_class.create(poll_option_id: poll.poll_options.last.id, user_id: user.id, poll_id: poll.id)
-    expect(user.poll_votes.size).to eq(0)
-  end
-
-  it "updates poll voting count after create" do
-    create(:poll_vote, poll_option_id: poll.poll_options.last.id, user_id: user.id, poll_id: poll.id)
-    expect(poll.reload.poll_votes_count).to eq(1)
-    expect(poll.reload.poll_options.last.poll_votes_count).to eq(1)
-  end
-
-  it "updates poll voting count after create with accuracy alongside skips" do
-    create(:poll_vote, poll_option_id: poll.poll_options.last.id, user_id: user.id, poll_id: poll.id)
-    PollSkip.create(poll_id: poll.id, user_id: user.id)
-    expect(poll.reload.poll_votes_count).to eq(1)
-    expect(poll.reload.poll_options.last.poll_votes_count).to eq(1)
+      expect(vote).to be_valid
+      expect(vote.errors[:base]).to be_empty
+    end
   end
 end