From ae067ee322e6702fb5ef0fd4f0cc4d4d5106cbde Mon Sep 17 00:00:00 2001
From: Riyad Preukschas <riyad@informatik.uni-bremen.de>
Date: Tue, 30 Oct 2012 03:27:36 +0100
Subject: [PATCH] Fix vote counting

---
 app/helpers/notes_helper.rb     |  16 ++---
 app/models/note.rb              |  16 ++++-
 app/roles/votes.rb              |  25 +++----
 app/views/notes/_note.html.haml |  18 +++--
 spec/factories.rb               |  26 +++++++
 spec/models/note_spec.rb        |  86 ++++++++++++++---------
 spec/roles/votes_spec.rb        | 116 +++++++++++++++++---------------
 7 files changed, 186 insertions(+), 117 deletions(-)

diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 11d3a2ec..02dec2a0 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -1,12 +1,4 @@
 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
diff --git a/app/models/note.rb b/app/models/note.rb
index a7bde1c5..17b76615 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -85,7 +85,9 @@ class Note < 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 < 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 < 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
diff --git a/app/roles/votes.rb b/app/roles/votes.rb
index 043a6feb..10fa120c 100644
--- a/app/roles/votes.rb
+++ b/app/roles/votes.rb
@@ -1,16 +1,4 @@
 module Votes
-  # Return the number of +1 comments (upvotes)
-  def upvotes
-    notes.select(&:upvote?).size
-  end
-
-  def upvotes_in_percent
-    if votes_count.zero?
-      0
-    else
-      100.0 / votes_count * upvotes
-    end
-  end
 
   # Return the number of -1 comments (downvotes)
   def downvotes
@@ -25,6 +13,19 @@ module Votes
     end
   end
 
+  # Return the number of +1 comments (upvotes)
+  def upvotes
+    notes.select(&:upvote?).size
+  end
+
+  def upvotes_in_percent
+    if votes_count.zero?
+      0
+    else
+      100.0 / votes_count * upvotes
+    end
+  end
+
   # Return the total number of votes
   def votes_count
     upvotes + downvotes
diff --git a/app/views/notes/_note.html.haml b/app/views/notes/_note.html.haml
index cf88d291..c0c0b725 100644
--- a/app/views/notes/_note.html.haml
+++ b/app/views/notes/_note.html.haml
@@ -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
diff --git a/spec/factories.rb b/spec/factories.rb
index a26a77dd..ac49f14c 100644
--- a/spec/factories.rb
+++ b/spec/factories.rb
@@ -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
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 4f9352b9..4c1afd8a 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -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
+
+    it "should not be votable" do
+      note.should_not be_votable
     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")
-    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
 
diff --git a/spec/roles/votes_spec.rb b/spec/roles/votes_spec.rb
index 98666022..7e49ac78 100644
--- a/spec/roles/votes_spec.rb
+++ b/spec/roles/votes_spec.rb
@@ -1,132 +1,138 @@
 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