add controller spec

also fix some code styling issues

7 files changed, 33 insertions, 34 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 765ccf540c3..265f6e48540 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -338,7 +338,7 @@ module Issuable
   ##
   # Override in issuable specialization
   #
-  def first_contribution?(*)
+  def first_contribution?
     false
   end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index a5b037d94c2..2a56bab48a3 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -960,8 +960,9 @@ class MergeRequest < ActiveRecord::Base
     Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache
   end
 
-  def first_contribution?(*)
+  def first_contribution?
     return false if project.team.max_member_access(author_id) > Gitlab::Access::GUEST
+
     project.merge_requests.merged.where(author_id: author_id).empty?
   end
 
diff --git a/app/models/note.rb b/app/models/note.rb
index b1fe60aa387..f44590e2144 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -47,7 +47,7 @@ class Note < ActiveRecord::Base
 
   # A special role that may be displayed on issuable's discussions
   attr_accessor :special_role
-  
+
   default_value_for :system, false
 
   attr_mentionable :note, pipeline: :note
@@ -233,15 +233,12 @@ class Note < ActiveRecord::Base
     self.class.has_special_role?(role, self)
   end
 
-  def specialize!(role)
-    self.special_role = role if !block_given? || yield(self)
-  end
-
   def specialize_for_first_contribution!(noteable)
     return unless noteable.author_id == self.author_id
-    specialize!(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR)
+
+    self.special_role = Note::SpecialRole::FIRST_TIME_CONTRIBUTOR
   end
- 
+
   def editable?
     !system?
   end
diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml
index 999a1b850e1..de76832331a 100644
--- a/app/views/projects/notes/_actions.html.haml
+++ b/app/views/projects/notes/_actions.html.haml
@@ -1,5 +1,5 @@
 - if note.has_special_role?(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR)
-  %span.note-role.note-role-special.has-tooltip{ title: _("This user hasn't yet contributed code to this project. Handle with care.") }
+  %span.note-role.note-role-special.has-tooltip{ title: _("This is the author's first Merge Request to this project. Handle with care.") }
     = issuable_first_contribution_icon
 - if access = note_max_access_for_user(note)
   %span.note-role.note-role-access= Gitlab::Access.human_access(access)
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index bb67db268fa..6775012bab5 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -56,6 +56,28 @@ describe Projects::MergeRequestsController do
 
         expect(response).to be_success
       end
+
+      context "loads notes" do
+        let(:first_contributor) { create(:user) }
+        let(:contributor) { create(:user) }
+        let(:merge_request) { create(:merge_request, author: first_contributor, target_project: project, source_project: project) }
+        let(:contributor_merge_request) { create(:merge_request, :merged, author: contributor, target_project: project, source_project: project) }
+        # the order here is important
+        # as the controller reloads these from DB, references doesn't correspond after
+        let!(:first_contributor_note) { create(:note, author: first_contributor, noteable: merge_request, project: project) }
+        let!(:contributor_note) { create(:note, author: contributor, noteable: merge_request, project: project) }
+        let!(:owner_note) { create(:note, author: user, noteable: merge_request, project: project) }
+
+        it "with special_role FIRST_TIME_CONTRIBUTOR" do
+          go(format: :html)
+
+          notes = assigns(:notes)
+          expect(notes).to match(a_collection_containing_exactly(an_object_having_attributes(special_role: Note::SpecialRole::FIRST_TIME_CONTRIBUTOR),
+                                                                 an_object_having_attributes(special_role: nil),
+                                                                 an_object_having_attributes(special_role: nil)
+                                                                ))
+        end
+      end
     end
 
     describe 'as json' do
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 098d33e46c5..fb5fb7daaab 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -501,7 +501,7 @@ describe Issuable do
       project.add_guest(contributor)
       project.add_guest(first_time_contributor)
     end
-    
+
     let(:merged_mr) { create(:merge_request, :merged, author: contributor, target_project: project, source_project: project) }
     let(:open_mr)  { create(:merge_request, author: first_time_contributor, target_project: project, source_project: project) }
     let(:merged_mr_other_project) { create(:merge_request, :merged, author: first_time_contributor, target_project: other_project, source_project: other_project) }
@@ -515,13 +515,13 @@ describe Issuable do
 
       it "is false for OWNER" do
         mr = create(:merge_request, author: owner, target_project: project, source_project: project)
-        
+
         expect(mr).not_to be_first_contribution
       end
 
       it "is false for REPORTER" do
         mr = create(:merge_request, author: reporter, target_project: project, source_project: project)
-        
+
         expect(mr).not_to be_first_contribution
       end
 
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 09caa2c9991..b214074fdce 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -696,25 +696,4 @@ describe Note do
       note.destroy!
     end
   end
-
-  describe '#specialize!' do
-    let(:note) { build(:note) }
-    let(:role) { Note::SpecialRole::FIRST_TIME_CONTRIBUTOR }
-
-    it 'should set special role without a block' do
-      expect { note.specialize!(role) }.to change { note.special_role }.from(nil).to(role)
-    end
-
-    it 'should set special role with a truthy block' do
-      tautology = -> (*) { true }
-      
-      expect { note.specialize!(role, &tautology) }.to change { note.special_role }.from(nil).to(role)
-    end
-
-    it 'should not set special role with a falsey block' do
-      contradiction = -> (*) { false }
-
-      expect { note.specialize!(role, &contradiction) }.not_to change { note.special_role }
-    end
-  end
 end