Fix vote counting

Riyad Preukschas
1 parent 6c6f415c
Showing 7 changed files with 184 additions and 115 deletions Show diff stats
app/helpers/notes_helper.rb
app/models/note.rb
app/roles/votes.rb
app/views/notes/_note.html.haml
spec/factories.rb
spec/models/note_spec.rb
spec/roles/votes_spec.rb
 module NotesHelper
-  def loading_more_notes?
-    params[:loading_more].present?
-  end
-
-  def loading_new_notes?
-    params[:loading_new].present?
-  end
-
    # Helps to distinguish e.g. commit notes in mr notes list
   def note_for_main_target?(note)
     !@mixed_targets || (@target.class.name == note.noteable_type && !note.for_diff_line?)
@@ -23,4 +15,12 @@ module NotesHelper
       link_to "#{note.diff_file_name}:L#{note.diff_new_line}", diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code)
     end
   end
+
+  def loading_more_notes?
+    params[:loading_more].present?
+  end
+
+  def loading_new_notes?
+    params[:loading_new].present?
+  end
 end
@@ -85,7 +85,9 @@ class Note &lt; ActiveRecord::Base
   # Returns true if this is a downvote note,
   # otherwise false is returned
   def downvote?
-    note.start_with?('-1') || note.start_with?(':-1:')
+    votable? && (note.start_with?('-1') ||
+                 note.start_with?(':-1:')
+                )
   end
   def for_commit?
@@ -100,6 +102,10 @@ class Note &lt; ActiveRecord::Base
     line_code.present?
   end
+  def for_issue?
+    noteable_type == "Issue"
+  end
+
   def for_merge_request?
     noteable_type == "MergeRequest"
   end
@@ -150,6 +156,12 @@ class Note &lt; ActiveRecord::Base
   # Returns true if this is an upvote note,
   # otherwise false is returned
   def upvote?
-    note.start_with?('+1') || note.start_with?(':+1:')
+    votable? && (note.start_with?('+1') ||
+                 note.start_with?(':+1:')
+                )
+  end
+
+  def votable?
+    for_issue? || (for_merge_request? && !for_diff_line?)
   end
 end
 module Votes
-  # Return the number of +1 comments (upvotes)
-  def upvotes
-    notes.select(&:upvote?).size
+
+  # Return the number of -1 comments (downvotes)
+  def downvotes
+    notes.select(&:downvote?).size
   end
-  def upvotes_in_percent
+  def downvotes_in_percent
     if votes_count.zero?
       0
     else
-      100.0 / votes_count * upvotes
+      100.0 - upvotes_in_percent
     end
   end
-  # Return the number of -1 comments (downvotes)
-  def downvotes
-    notes.select(&:downvote?).size
+  # Return the number of +1 comments (upvotes)
+  def upvotes
+    notes.select(&:upvote?).size
   end
-  def downvotes_in_percent
+  def upvotes_in_percent
     if votes_count.zero?
       0
     else
-      100.0 - upvotes_in_percent
+      100.0 / votes_count * upvotes
     end
   end
@@ -14,16 +14,14 @@
       = time_ago_in_words(note.updated_at)
       ago
-    -# only show vote if it's a note for the main target
-    - if note_for_main_target?(note)
-      - if note.upvote?
-        %span.vote.upvote.label.label-success
-          %i.icon-thumbs-up
-          \+1
-      - if note.downvote?
-        %span.vote.downvote.label.label-error
-          %i.icon-thumbs-down
-          \-1
+    - if note.upvote?
+      %span.vote.upvote.label.label-success
+        %i.icon-thumbs-up
+        \+1
+    - if note.downvote?
+      %span.vote.downvote.label.label-error
+        %i.icon-thumbs-down
+        \-1
   .note-body
@@ -91,6 +91,32 @@ FactoryGirl.define do
   factory :note do
     project
     note "Note"
+    author
+
+    factory :note_on_commit, traits: [:on_commit]
+    factory :note_on_commit_line, traits: [:on_commit, :on_line]
+    factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
+    factory :note_on_merge_request, traits: [:on_merge_request]
+    factory :note_on_merge_request_line, traits: [:on_merge_request, :on_line]
+
+    trait :on_commit do
+      noteable_id   "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a"
+      noteable_type "Commit"
+    end
+
+    trait :on_line do
+      line_code "0_184_184"
+    end
+
+    trait :on_merge_request do
+      noteable_id   1
+      noteable_type "MergeRequest"
+    end
+
+    trait :on_issue do
+      noteable_id 1
+      noteable_type "Issue"
+    end
   end
   factory :event do
@@ -43,79 +43,105 @@ describe Note do
     let(:project) { create(:project) }
     it "recognizes a neutral note" do
-      note = create(:note, note: "This is not a +1 note")
+      note = create(:votable_note, note: "This is not a +1 note")
       note.should_not be_upvote
       note.should_not be_downvote
     end
     it "recognizes a neutral emoji note" do
-      note = build(:note, note: "I would :+1: this, but I don't want to")
+      note = build(:votable_note, note: "I would :+1: this, but I don't want to")
       note.should_not be_upvote
       note.should_not be_downvote
     end
     it "recognizes a +1 note" do
-      note = create(:note, note: "+1 for this")
+      note = create(:votable_note, note: "+1 for this")
       note.should be_upvote
     end
     it "recognizes a +1 emoji as a vote" do
-      note = build(:note, note: ":+1: for this")
+      note = build(:votable_note, note: ":+1: for this")
       note.should be_upvote
     end
     it "recognizes a -1 note" do
-      note = create(:note, note: "-1 for this")
+      note = create(:votable_note, note: "-1 for this")
       note.should be_downvote
     end
     it "recognizes a -1 emoji as a vote" do
-      note = build(:note, note: ":-1: for this")
+      note = build(:votable_note, note: ":-1: for this")
       note.should be_downvote
     end
   end
-  let(:project) { create(:project) }
-  let(:commit) { project.commit }
-
   describe "Commit notes" do
-    before do
-      @note = create(:note,
-                     noteable_id: commit.id,
-                     noteable_type: "Commit")
-    end
+    let!(:note) { create(:note_on_commit, note: "+1 from me") }
+    let!(:commit) { note.noteable }
     it "should be accessible through #noteable" do
-      @note.noteable_id.should == commit.id
-      @note.noteable.should be_a(Commit)
-      @note.noteable.should == commit
+      note.noteable_id.should == commit.id
+      note.noteable.should be_a(Commit)
+      note.noteable.should == commit
     end
     it "should save a valid note" do
-      @note.noteable_id.should == commit.id
-      @note.noteable == commit
+      note.noteable_id.should == commit.id
+      note.noteable == commit
     end
     it "should be recognized by #for_commit?" do
-      @note.should be_for_commit
+      note.should be_for_commit
     end
-  end
-  describe "Pre-line commit notes" do
-    before do
-      @note = create(:note,
-                     noteable_id: commit.id,
-                     noteable_type: "Commit",
-                     line_code: "0_16_1")
+    it "should not be votable" do
+      note.should_not be_votable
     end
+  end
+
+  describe "Commit diff line notes" do
+    let!(:note) { create(:note_on_commit_line, note: "+1 from me") }
+    let!(:commit) { note.noteable }
     it "should save a valid note" do
-      @note.noteable_id.should == commit.id
-      @note.noteable.id.should == commit.id
+      note.noteable_id.should == commit.id
+      note.noteable.id.should == commit.id
     end
     it "should be recognized by #for_diff_line?" do
-      @note.should be_for_diff_line
+      note.should be_for_diff_line
+    end
+
+    it "should be recognized by #for_commit_diff_line?" do
+      note.should be_for_commit_diff_line
+    end
+
+    it "should not be votable" do
+      note.should_not be_votable
+    end
+  end
+
+  describe "Issue notes" do
+    let!(:note) { create(:note_on_issue, note: "+1 from me") }
+
+    it "should not be votable" do
+      note.should be_votable
+    end
+  end
+
+  describe "Merge request notes" do
+    let!(:note) { create(:note_on_merge_request, note: "+1 from me") }
+
+    it "should not be votable" do
+      note.should be_votable
+    end
+  end
+
+  describe "Merge request diff line notes" do
+    let!(:note) { create(:note_on_merge_request_line, note: "+1 from me") }
+
+    it "should not be votable" do
+      note.should_not be_votable
     end
   end
 require 'spec_helper'
 describe Issue do
-  let(:issue) { create(:issue) }
+  it { should include_module(Votes) }
+end
+
+describe MergeRequest do
+  let(:merge_request) { FactoryGirl.create(:merge_request_with_diffs) }
+
+  it { should include_module(Votes) }
   describe "#upvotes" do
     it "with no notes has a 0/0 score" do
-      issue.upvotes.should == 0
+      merge_request.upvotes.should == 0
     end
     it "should recognize non-+1 notes" do
-      issue.notes << create(:note, note: "No +1 here")
-      issue.should have(1).note
-      issue.notes.first.upvote?.should be_false
-      issue.upvotes.should == 0
+      merge_request.notes << create(:note, note: "No +1 here")
+      merge_request.should have(1).note
+      merge_request.notes.first.upvote?.should be_false
+      merge_request.upvotes.should == 0
     end
     it "should recognize a single +1 note" do
-      issue.notes << create(:note, note: "+1 This is awesome")
-      issue.upvotes.should == 1
+      merge_request.notes << create(:note, note: "+1 This is awesome")
+      merge_request.upvotes.should == 1
     end
     it "should recognize multiple +1 notes" do
-      issue.notes << create(:note, note: "+1 This is awesome")
-      issue.notes << create(:note, note: "+1 I want this")
-      issue.upvotes.should == 2
+      merge_request.notes << create(:note, note: "+1 This is awesome")
+      merge_request.notes << create(:note, note: "+1 I want this")
+      merge_request.upvotes.should == 2
     end
   end
   describe "#downvotes" do
     it "with no notes has a 0/0 score" do
-      issue.downvotes.should == 0
+      merge_request.downvotes.should == 0
     end
     it "should recognize non--1 notes" do
-      issue.notes << create(:note, note: "Almost got a -1")
-      issue.should have(1).note
-      issue.notes.first.downvote?.should be_false
-      issue.downvotes.should == 0
+      merge_request.notes << create(:note, note: "Almost got a -1")
+      merge_request.should have(1).note
+      merge_request.notes.first.downvote?.should be_false
+      merge_request.downvotes.should == 0
     end
     it "should recognize a single -1 note" do
-      issue.notes << create(:note, note: "-1 This is bad")
-      issue.downvotes.should == 1
+      merge_request.notes << create(:note, note: "-1 This is bad")
+      merge_request.downvotes.should == 1
     end
     it "should recognize multiple -1 notes" do
-      issue.notes << create(:note, note: "-1 This is bad")
-      issue.notes << create(:note, note: "-1 Away with this")
-      issue.downvotes.should == 2
+      merge_request.notes << create(:note, note: "-1 This is bad")
+      merge_request.notes << create(:note, note: "-1 Away with this")
+      merge_request.downvotes.should == 2
     end
   end
   describe "#votes_count" do
     it "with no notes has a 0/0 score" do
-      issue.votes_count.should == 0
+      merge_request.votes_count.should == 0
     end
     it "should recognize non notes" do
-      issue.notes << create(:note, note: "No +1 here")
-      issue.should have(1).note
-      issue.votes_count.should == 0
+      merge_request.notes << create(:note, note: "No +1 here")
+      merge_request.should have(1).note
+      merge_request.votes_count.should == 0
     end
     it "should recognize a single +1 note" do
-      issue.notes << create(:note, note: "+1 This is awesome")
-      issue.votes_count.should == 1
+      merge_request.notes << create(:note, note: "+1 This is awesome")
+      merge_request.votes_count.should == 1
     end
     it "should recognize a single -1 note" do
-      issue.notes << create(:note, note: "-1 This is bad")
-      issue.votes_count.should == 1
+      merge_request.notes << create(:note, note: "-1 This is bad")
+      merge_request.votes_count.should == 1
     end
     it "should recognize multiple notes" do
-      issue.notes << create(:note, note: "+1 This is awesome")
-      issue.notes << create(:note, note: "-1 This is bad")
-      issue.notes << create(:note, note: "+1 I want this")
-      issue.votes_count.should == 3
+      merge_request.notes << create(:note, note: "+1 This is awesome")
+      merge_request.notes << create(:note, note: "-1 This is bad")
+      merge_request.notes << create(:note, note: "+1 I want this")
+      merge_request.votes_count.should == 3
     end
   end
   describe "#upvotes_in_percent" do
     it "with no notes has a 0% score" do
-      issue.upvotes_in_percent.should == 0
+      merge_request.upvotes_in_percent.should == 0
     end
     it "should count a single 1 note as 100%" do
-      issue.notes << create(:note, note: "+1 This is awesome")
-      issue.upvotes_in_percent.should == 100
+      merge_request.notes << create(:note, note: "+1 This is awesome")
+      merge_request.upvotes_in_percent.should == 100
     end
     it "should count multiple +1 notes as 100%" do
-      issue.notes << create(:note, note: "+1 This is awesome")
-      issue.notes << create(:note, note: "+1 I want this")
-      issue.upvotes_in_percent.should == 100
+      merge_request.notes << create(:note, note: "+1 This is awesome")
+      merge_request.notes << create(:note, note: "+1 I want this")
+      merge_request.upvotes_in_percent.should == 100
     end
     it "should count fractions for multiple +1 and -1 notes correctly" do
-      issue.notes << create(:note, note: "+1 This is awesome")
-      issue.notes << create(:note, note: "+1 I want this")
-      issue.notes << create(:note, note: "-1 This is bad")
-      issue.notes << create(:note, note: "+1 me too")
-      issue.upvotes_in_percent.should == 75
+      merge_request.notes << create(:note, note: "+1 This is awesome")
+      merge_request.notes << create(:note, note: "+1 I want this")
+      merge_request.notes << create(:note, note: "-1 This is bad")
+      merge_request.notes << create(:note, note: "+1 me too")
+      merge_request.upvotes_in_percent.should == 75
     end
   end
   describe "#downvotes_in_percent" do
     it "with no notes has a 0% score" do
-      issue.downvotes_in_percent.should == 0
+      merge_request.downvotes_in_percent.should == 0
     end
     it "should count a single -1 note as 100%" do
-      issue.notes << create(:note, note: "-1 This is bad")
-      issue.downvotes_in_percent.should == 100
+      merge_request.notes << create(:note, note: "-1 This is bad")
+      merge_request.downvotes_in_percent.should == 100
     end
     it "should count multiple -1 notes as 100%" do
-      issue.notes << create(:note, note: "-1 This is bad")
-      issue.notes << create(:note, note: "-1 Away with this")
-      issue.downvotes_in_percent.should == 100
+      merge_request.notes << create(:note, note: "-1 This is bad")
+      merge_request.notes << create(:note, note: "-1 Away with this")
+      merge_request.downvotes_in_percent.should == 100
     end
     it "should count fractions for multiple +1 and -1 notes correctly" do
-      issue.notes << create(:note, note: "+1 This is awesome")
-      issue.notes << create(:note, note: "+1 I want this")
-      issue.notes << create(:note, note: "-1 This is bad")
-      issue.notes << create(:note, note: "+1 me too")
-      issue.downvotes_in_percent.should == 25
+      merge_request.notes << create(:note, note: "+1 This is awesome")
+      merge_request.notes << create(:note, note: "+1 I want this")
+      merge_request.notes << create(:note, note: "-1 This is bad")
+      merge_request.notes << create(:note, note: "+1 me too")
+      merge_request.downvotes_in_percent.should == 25
     end
   end
 end
	@@ -91,6 +91,32 @@ FactoryGirl.define do		@@ -91,6 +91,32 @@ FactoryGirl.define do
91	factory :note do	91	factory :note do
92	project	92	project
93	note "Note"	93	note "Note"
		94	+ author
		95	+
		96	+ factory :note_on_commit, traits: [:on_commit]
		97	+ factory :note_on_commit_line, traits: [:on_commit, :on_line]
		98	+ factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note]
		99	+ factory :note_on_merge_request, traits: [:on_merge_request]
		100	+ factory :note_on_merge_request_line, traits: [:on_merge_request, :on_line]
		101	+
		102	+ trait :on_commit do
		103	+ noteable_id "bcf03b5de6c33f3869ef70d68cf06e679d1d7f9a"
		104	+ noteable_type "Commit"
		105	+ end
		106	+
		107	+ trait :on_line do
		108	+ line_code "0_184_184"
		109	+ end
		110	+
		111	+ trait :on_merge_request do
		112	+ noteable_id 1
		113	+ noteable_type "MergeRequest"
		114	+ end
		115	+
		116	+ trait :on_issue do
		117	+ noteable_id 1
		118	+ noteable_type "Issue"
		119	+ end
94	end	120	end
95		121
96	factory :event do	122	factory :event do