From 5e600db21ce5d6544f559ea6d25aab2858dd465e Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Thu, 27 Jul 2017 16:38:52 -0400
Subject: [PATCH 01/10] fix #35161, add a first-time contributor badge

a new badge will be added when an user that doesn't yet
have any merged merge request is discussing on either issues or
merge requests that he created.

this is indented for people to use extra care when discussing with
a new contributor.
---
 app/helpers/notes_helper.rb                 |  5 ++
 app/views/projects/notes/_actions.html.haml |  3 +
 spec/helpers/notes_helper_spec.rb           | 65 +++++++++++++++++++++
 3 files changed, 73 insertions(+)

diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 083ace65b09..1e7d346ab8f 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -76,6 +76,11 @@ module NotesHelper
     note.project.team.human_max_access(note.author_id)
   end
 
+  def note_first_contribution_for_user?(note)
+    note.noteable.author_id == note.author_id &&
+    note.project.merge_requests.merged.where(author_id: note.author_id).empty?
+  end
+
   def discussion_path(discussion)
     if discussion.for_merge_request?
       return unless discussion.diff_discussion?
diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml
index fb07141d2ac..38548cbe93a 100644
--- a/app/views/projects/notes/_actions.html.haml
+++ b/app/views/projects/notes/_actions.html.haml
@@ -1,6 +1,9 @@
 - access = note_max_access_for_user(note)
 - if access
   %span.note-role= access
+- if note_first_contribution_for_user?(note)
+  %span.note-role.has-tooltip{ title: _("This user hasn't yet contributed code to this project. Handle with care.")}
+    = _('First-time contributor')
 
 - if note.resolvable?
   - can_resolve = can?(current_user, :resolve_note, note)
diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb
index 68540dd4e59..613d6d6a423 100644
--- a/spec/helpers/notes_helper_spec.rb
+++ b/spec/helpers/notes_helper_spec.rb
@@ -39,6 +39,71 @@ describe NotesHelper do
     end
   end
 
+  describe '#note_first_contribution_for_user?' do
+    let(:project) { create(:empty_project) }
+    let(:other_project) { create(:empty_project) }
+
+    let(:contributor) { create(:user) }
+    let(:first_time_contributor) { create(:user) }
+
+    # there should be a way to disable the lazy loading on these
+    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) }
+
+    context "notes for a merge request" do
+      let(:guest_merge_request) { create(:merge_request, author: guest, target_project: project, source_project: project) }
+      let(:contributor_note) { create(:note, noteable: merged_mr, author: contributor, project: merged_mr.project) }
+      let(:first_time_contributor_note) { create(:note, noteable: open_mr, author: first_time_contributor, project: open_mr.project) }
+      let(:first_time_contributor_note_other_project) { create(:note, noteable: merged_mr_other_project, author: first_time_contributor, project: merged_mr_other_project.project) }
+      let(:guest_note) { create(:note, noteable: guest_merge_request, author: first_time_contributor, project: project) }
+
+      it "is true when you don't have any merged MR" do
+        expect(helper.note_first_contribution_for_user? contributor_note).to be false
+        expect(helper.note_first_contribution_for_user? first_time_contributor_note).to be true
+      end
+
+      it "handle multiple projects separately" do
+        expect(helper.note_first_contribution_for_user? first_time_contributor_note).to be true
+        expect(helper.note_first_contribution_for_user? first_time_contributor_note_other_project).to be false
+      end
+
+      it "should only apply to the noteable's author" do
+        expect(helper.note_first_contribution_for_user? guest_note).to be false
+      end
+    end
+
+    context "notes for an issue" do
+      let(:guest_issue) { create(:issue, author: guest, project: project) }
+      let(:contributor_issue) { create(:issue, author: contributor, project: project) }
+      let(:first_time_contributor_issue) { create(:issue, author: first_time_contributor, project: project) }
+      let(:first_time_contributor_issue_other_project) { create(:issue, author: first_time_contributor, project: other_project) }
+        
+      let(:contributor_note) { create(:note, noteable: contributor_issue, author: contributor, project: project) }
+      let(:first_time_contributor_note) { create(:note, noteable: first_time_contributor_issue, author: first_time_contributor, project: project) }
+      let(:first_time_contributor_note_other_project) { create(:note, noteable: first_time_contributor_issue_other_project, author: first_time_contributor, project: other_project) }
+      let(:guest_note) { create(:note, noteable: guest_issue, author: first_time_contributor, project: project) }
+
+      it "is true when you don't have any merged MR" do
+        expect(merged_mr).to be
+        expect(helper.note_first_contribution_for_user? contributor_note).to be false
+        expect(helper.note_first_contribution_for_user? first_time_contributor_note).to be true
+      end
+
+      it "handle multiple projects separately" do
+        expect(merged_mr).to be
+        expect(merged_mr_other_project).to be
+        expect(helper.note_first_contribution_for_user? first_time_contributor_note).to be true
+        expect(helper.note_first_contribution_for_user? first_time_contributor_note_other_project).to be false
+      end
+
+      it "should only apply to the noteable's author" do
+        expect(merged_mr).to be
+        expect(helper.note_first_contribution_for_user? guest_note).to be false
+      end
+    end
+  end
+
   describe '#discussion_path' do
     let(:project) { create(:project, :repository) }
 

From 8fe1aa5dbbb41cdefffb7177d9eda44ac8652cc7 Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Fri, 28 Jul 2017 12:16:30 -0400
Subject: [PATCH 02/10] add changelog entry

---
 changelogs/unreleased/35161_first_time_contributor_badge.yml | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 changelogs/unreleased/35161_first_time_contributor_badge.yml

diff --git a/changelogs/unreleased/35161_first_time_contributor_badge.yml b/changelogs/unreleased/35161_first_time_contributor_badge.yml
new file mode 100644
index 00000000000..f3ab2d9db31
--- /dev/null
+++ b/changelogs/unreleased/35161_first_time_contributor_badge.yml
@@ -0,0 +1,4 @@
+---
+title: "First-time contributor badge"
+merge_request: 13143
+author: Micaël Bergeron <micaelbergeron@gmail.com>

From 966b1128d884a318dad4277e23368334fe67e836 Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Sat, 29 Jul 2017 11:04:42 -0400
Subject: [PATCH 03/10] WIP: refactor the first-contributor to Issuable

this will remove the need make N queries (per-note) at the
cost of having to mark notes with an attribute

this opens up the possibility for other special roles for notes
---
 app/controllers/concerns/renders_notes.rb     |  9 ++-
 app/controllers/projects/commit_controller.rb |  2 +-
 app/controllers/projects/issues_controller.rb |  2 +-
 .../merge_requests/diffs_controller.rb        |  2 +-
 .../projects/merge_requests_controller.rb     |  6 +-
 .../projects/snippets_controller.rb           |  2 +-
 app/controllers/snippets_controller.rb        |  2 +-
 app/helpers/issuables_helper.rb               |  9 +++
 app/helpers/notes_helper.rb                   |  7 +-
 app/models/concerns/issuable.rb               |  5 ++
 app/models/note.rb                            | 25 +++++-
 app/models/project_team.rb                    |  2 +-
 app/views/projects/notes/_actions.html.haml   |  7 +-
 lib/gitlab/access.rb                          |  6 +-
 spec/helpers/notes_helper_spec.rb             | 77 ++-----------------
 spec/models/concerns/issuable_spec.rb         | 73 ++++++++++++++++++
 16 files changed, 143 insertions(+), 93 deletions(-)

diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb
index 41c3114ad1e..cdcfefd90c9 100644
--- a/app/controllers/concerns/renders_notes.rb
+++ b/app/controllers/concerns/renders_notes.rb
@@ -1,7 +1,8 @@
 module RendersNotes
-  def prepare_notes_for_rendering(notes)
+  def prepare_notes_for_rendering(notes, noteable=nil)
     preload_noteable_for_regular_notes(notes)
     preload_max_access_for_authors(notes, @project)
+    preload_first_time_contribution_for_authors(noteable, notes) if noteable.is_a?(Issuable)
     Banzai::NoteRenderer.render(notes, @project, current_user)
 
     notes
@@ -19,4 +20,10 @@ module RendersNotes
   def preload_noteable_for_regular_notes(notes)
     ActiveRecord::Associations::Preloader.new.preload(notes.reject(&:for_commit?), :noteable)
   end
+
+  def preload_first_time_contribution_for_authors(issuable, notes)
+    return unless issuable.first_contribution?
+    same_author = lambda {|n| n.author_id == issuable.author_id}
+    notes.select(&same_author).each {|note| note.special_role = :first_time_contributor}
+  end
 end
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 6de125e7e80..1a775def506 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -127,7 +127,7 @@ class Projects::CommitController < Projects::ApplicationController
     @discussions = commit.discussions
 
     @notes = (@grouped_diff_discussions.values.flatten + @discussions).flat_map(&:notes)
-    @notes = prepare_notes_for_rendering(@notes)
+    @notes = prepare_notes_for_rendering(@notes, @commit)
   end
 
   def assign_change_commit_vars
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index dc9e6f71152..ab9f132b502 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -85,7 +85,7 @@ class Projects::IssuesController < Projects::ApplicationController
     @note     = @project.notes.new(noteable: @issue)
 
     @discussions = @issue.discussions
-    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
+    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes), @noteable)
 
     respond_to do |format|
       format.html
diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb
index 330b7df4541..109418c73f7 100644
--- a/app/controllers/projects/merge_requests/diffs_controller.rb
+++ b/app/controllers/projects/merge_requests/diffs_controller.rb
@@ -61,6 +61,6 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
     @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
 
     @grouped_diff_discussions = @merge_request.grouped_diff_discussions(@compare.diff_refs)
-    @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
+    @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes), @merge_request)
   end
 end
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 5095d7fd445..6c4a783e11a 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -60,12 +60,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
         # Build a note object for comment form
         @note = @project.notes.new(noteable: @merge_request)
 
-        @discussions = @merge_request.discussions
-        @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
-
         @noteable = @merge_request
         @commits_count = @merge_request.commits_count
 
+        @discussions = @merge_request.discussions
+        @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes), @noteable)
+
         labels
 
         set_pipeline_variables
diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb
index d07143d294f..7c19aa7bb23 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -64,7 +64,7 @@ class Projects::SnippetsController < Projects::ApplicationController
         @noteable = @snippet
 
         @discussions = @snippet.discussions
-        @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
+        @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes), @noteable)
         render 'show'
       end
 
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index 8c3abd0a085..c1cdc7c9831 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -66,7 +66,7 @@ class SnippetsController < ApplicationController
     @noteable = @snippet
 
     @discussions = @snippet.discussions
-    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
+    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes), @noteable)
 
     respond_to do |format|
       format.html do
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 717abf2082d..11fae16f04d 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -138,6 +138,8 @@ module IssuablesHelper
     end
 
     output << "&ensp;".html_safe
+    output << issuable_first_contribution_icon if issuable.first_contribution?
+
     output << content_tag(:span, (issuable.task_status if issuable.tasks?), id: "task_status", class: "hidden-xs hidden-sm")
     output << content_tag(:span, (issuable.task_status_short if issuable.tasks?), id: "task_status_short", class: "hidden-md hidden-lg")
 
@@ -173,6 +175,13 @@ module IssuablesHelper
     html.html_safe
   end
 
+  def issuable_first_contribution_icon
+    content_tag(:span, class: 'fa-stack has-tooltip', title: _('1st contribution!')) do
+      concat(icon('certificate', class: "fa-stack-2x"))
+      concat(content_tag(:strong, '1', class: 'fa-inverse fa-stack-1x'))
+    end 
+  end
+
   def assigned_issuables_count(issuable_type)
     case issuable_type
     when :issues
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 1e7d346ab8f..ce028195e51 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -73,12 +73,7 @@ module NotesHelper
   end
 
   def note_max_access_for_user(note)
-    note.project.team.human_max_access(note.author_id)
-  end
-
-  def note_first_contribution_for_user?(note)
-    note.noteable.author_id == note.author_id &&
-    note.project.merge_requests.merged.where(author_id: note.author_id).empty?
+    note.project.team.max_member_access(note.author_id)
   end
 
   def discussion_path(discussion)
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 681c3241dbb..7b1c4e7a2d5 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -334,4 +334,9 @@ module Issuable
     metrics = self.metrics || create_metrics
     metrics.record!
   end
+
+  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
 end
diff --git a/app/models/note.rb b/app/models/note.rb
index 1073c115630..d04bccc6e81 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -15,7 +15,18 @@ class Note < ActiveRecord::Base
   include IgnorableColumn
   include Editable
 
+  module SpecialRole
+    FIRST_TIME_CONTRIBUTOR = :first_time_contributor
+
+    class << self
+      def values
+        constants.map {|const| self.const_get(const)}
+      end
+    end
+  end
+
   ignore_column :original_discussion_id
+  ignore_column :special_role
 
   cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true
 
@@ -32,9 +43,12 @@ class Note < ActiveRecord::Base
   # Banzai::ObjectRenderer
   attr_accessor :user_visible_reference_count
 
-  # Attribute used to store the attributes that have ben changed by quick actions.
+  # Attribute used to store the attributes that have been changed by quick actions.
   attr_accessor :commands_changes
 
+  # 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
@@ -206,6 +220,15 @@ class Note < ActiveRecord::Base
     super(noteable_type.to_s.classify.constantize.base_class.to_s)
   end
 
+  def special_role=(role)
+    raise "Role is undefined, #{role} not found in #{SpecialRole.values}" unless SpecialRole.values.include? role
+    @special_role = role
+  end
+
+  def has_special_role?(role)
+    return @special_role == role
+  end
+
   def editable?
     !system?
   end
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 674eacd28e8..09049824ff7 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -150,7 +150,7 @@ class ProjectTeam
   end
 
   def human_max_access(user_id)
-    Gitlab::Access.options_with_owner.key(max_member_access(user_id))
+    Gitlab::Access.human_access(max_member_access(user_id))
   end
 
   # Determine the maximum access level for a group of users in bulk.
diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml
index 38548cbe93a..926ceff3ee2 100644
--- a/app/views/projects/notes/_actions.html.haml
+++ b/app/views/projects/notes/_actions.html.haml
@@ -1,9 +1,8 @@
-- access = note_max_access_for_user(note)
-- if access
-  %span.note-role= access
-- if note_first_contribution_for_user?(note)
+- if note.has_special_role? :first_time_contributor
   %span.note-role.has-tooltip{ title: _("This user hasn't yet contributed code to this project. Handle with care.")}
     = _('First-time contributor')
+- elsif access = access = note_max_access_for_user(note)
+  %span.note-role= Gitlab::Access.human_access(access)
 
 - if note.resolvable?
   - can_resolve = can?(current_user, :resolve_note, note)
diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb
index 4714ab18cc1..b4012ebbb99 100644
--- a/lib/gitlab/access.rb
+++ b/lib/gitlab/access.rb
@@ -67,10 +67,14 @@ module Gitlab
       def protection_values
         protection_options.values
       end
+
+      def human_access(access)
+        options_with_owner.key(access)
+      end
     end
 
     def human_access
-      Gitlab::Access.options_with_owner.key(access_field)
+      Gitlab::Access.human_access(access_field)
     end
 
     def owner?
diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb
index 613d6d6a423..cd15e27b497 100644
--- a/spec/helpers/notes_helper_spec.rb
+++ b/spec/helpers/notes_helper_spec.rb
@@ -23,10 +23,10 @@ describe NotesHelper do
   end
 
   describe "#notes_max_access_for_users" do
-    it 'returns human access levels' do
-      expect(helper.note_max_access_for_user(owner_note)).to eq('Owner')
-      expect(helper.note_max_access_for_user(master_note)).to eq('Master')
-      expect(helper.note_max_access_for_user(reporter_note)).to eq('Reporter')
+    it 'returns access levels' do
+      expect(helper.note_max_access_for_user(owner_note)).to eq(Gitlab::Access::OWNER)
+      expect(helper.note_max_access_for_user(master_note)).to eq(Gitlab::Access::MASTER)
+      expect(helper.note_max_access_for_user(reporter_note)).to eq(Gitlab::Access::REPORTER)
     end
 
     it 'handles access in different projects' do
@@ -34,73 +34,8 @@ describe NotesHelper do
       second_project.team << [master, :reporter]
       other_note = create(:note, author: master, project: second_project)
 
-      expect(helper.note_max_access_for_user(master_note)).to eq('Master')
-      expect(helper.note_max_access_for_user(other_note)).to eq('Reporter')
-    end
-  end
-
-  describe '#note_first_contribution_for_user?' do
-    let(:project) { create(:empty_project) }
-    let(:other_project) { create(:empty_project) }
-
-    let(:contributor) { create(:user) }
-    let(:first_time_contributor) { create(:user) }
-
-    # there should be a way to disable the lazy loading on these
-    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) }
-
-    context "notes for a merge request" do
-      let(:guest_merge_request) { create(:merge_request, author: guest, target_project: project, source_project: project) }
-      let(:contributor_note) { create(:note, noteable: merged_mr, author: contributor, project: merged_mr.project) }
-      let(:first_time_contributor_note) { create(:note, noteable: open_mr, author: first_time_contributor, project: open_mr.project) }
-      let(:first_time_contributor_note_other_project) { create(:note, noteable: merged_mr_other_project, author: first_time_contributor, project: merged_mr_other_project.project) }
-      let(:guest_note) { create(:note, noteable: guest_merge_request, author: first_time_contributor, project: project) }
-
-      it "is true when you don't have any merged MR" do
-        expect(helper.note_first_contribution_for_user? contributor_note).to be false
-        expect(helper.note_first_contribution_for_user? first_time_contributor_note).to be true
-      end
-
-      it "handle multiple projects separately" do
-        expect(helper.note_first_contribution_for_user? first_time_contributor_note).to be true
-        expect(helper.note_first_contribution_for_user? first_time_contributor_note_other_project).to be false
-      end
-
-      it "should only apply to the noteable's author" do
-        expect(helper.note_first_contribution_for_user? guest_note).to be false
-      end
-    end
-
-    context "notes for an issue" do
-      let(:guest_issue) { create(:issue, author: guest, project: project) }
-      let(:contributor_issue) { create(:issue, author: contributor, project: project) }
-      let(:first_time_contributor_issue) { create(:issue, author: first_time_contributor, project: project) }
-      let(:first_time_contributor_issue_other_project) { create(:issue, author: first_time_contributor, project: other_project) }
-        
-      let(:contributor_note) { create(:note, noteable: contributor_issue, author: contributor, project: project) }
-      let(:first_time_contributor_note) { create(:note, noteable: first_time_contributor_issue, author: first_time_contributor, project: project) }
-      let(:first_time_contributor_note_other_project) { create(:note, noteable: first_time_contributor_issue_other_project, author: first_time_contributor, project: other_project) }
-      let(:guest_note) { create(:note, noteable: guest_issue, author: first_time_contributor, project: project) }
-
-      it "is true when you don't have any merged MR" do
-        expect(merged_mr).to be
-        expect(helper.note_first_contribution_for_user? contributor_note).to be false
-        expect(helper.note_first_contribution_for_user? first_time_contributor_note).to be true
-      end
-
-      it "handle multiple projects separately" do
-        expect(merged_mr).to be
-        expect(merged_mr_other_project).to be
-        expect(helper.note_first_contribution_for_user? first_time_contributor_note).to be true
-        expect(helper.note_first_contribution_for_user? first_time_contributor_note_other_project).to be false
-      end
-
-      it "should only apply to the noteable's author" do
-        expect(merged_mr).to be
-        expect(helper.note_first_contribution_for_user? guest_note).to be false
-      end
+      expect(helper.note_max_access_for_user(master_note)).to eq(Gitlab::Access::MASTER)
+      expect(helper.note_max_access_for_user(other_note)).to eq(Gitlab::Access::REPORTER)
     end
   end
 
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 37f6fd3a25b..9b83b8bfb7e 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -480,4 +480,77 @@ describe Issuable do
       end
     end
   end
+
+  describe '#first_contribution?' do
+    let(:group) { create(:group) }
+    let(:project) { create(:empty_project, namespace: group) }
+    let(:other_project) { create(:empty_project) }
+    let(:owner) { create(:owner) }
+    let(:master) { create(:user) }
+    let(:reporter) { create(:user) }
+    let(:guest) { create(:user) }
+
+    let(:contributor) { create(:user) }
+    let(:first_time_contributor) { create(:user) }
+    let!(:access_users) { [owner, master, reporter] }
+
+    before do
+      group.add_owner(owner)
+      project.team << [master, :master]
+      project.team << [reporter, :reporter]
+      project.team << [guest, :guest]
+      project.team << [contributor, :guest]
+      project.team << [first_time_contributor, :guest]
+    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) }
+
+    context "for merge requests" do
+      it "is false for MASTER" do
+        mr = create(:merge_request, author: master, target_project: project, source_project: project)
+        expect(mr.first_contribution?).to be_falsey
+      end
+
+      it "is false for OWNER" do
+        mr = create(:merge_request, author: owner, target_project: project, source_project: project)
+        expect(mr.first_contribution?).to be_falsey
+      end
+
+      it "is false for REPORTER" do
+        mr = create(:merge_request, author: reporter, target_project: project, source_project: project)
+        expect(mr.first_contribution?).to be_falsey
+      end
+
+      it "is true when you don't have any merged MR" do
+        expect(open_mr.first_contribution?).to be_truthy
+        expect(merged_mr.first_contribution?).to be_falsey
+      end
+
+      it "handle multiple projects separately" do
+        expect(open_mr.first_contribution?).to be_truthy
+        expect(merged_mr_other_project.first_contribution?).to be_falsey
+      end
+    end
+
+    context "for issues" do
+      let(:contributor_issue) { create(:issue, author: contributor, project: project) }
+      let(:first_time_contributor_issue) { create(:issue, author: first_time_contributor, project: project) }
+      let(:first_time_contributor_issue_other_project) { create(:issue, author: first_time_contributor, project: other_project) }
+
+      it "is true when you don't have any merged MR" do
+        expect(merged_mr).to be
+        expect(first_time_contributor_issue.first_contribution?).to be_truthy
+        expect(contributor_issue.first_contribution?).to be_falsey
+      end
+
+      it "handle multiple projects separately" do
+        expect(merged_mr).to be
+        expect(merged_mr_other_project).to be
+        expect(first_time_contributor_issue.first_contribution?).to be_truthy
+        expect(first_time_contributor_issue_other_project.first_contribution?).to be_falsey
+      end
+    end
+  end
 end

From 45b83ed99afc5cfe24a8d84869894124d93d5b51 Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Wed, 2 Aug 2017 10:06:28 -0400
Subject: [PATCH 04/10] round of fixes after code review

---
 app/controllers/concerns/renders_notes.rb   |  7 ++--
 app/models/concerns/issuable.rb             |  1 +
 app/models/note.rb                          | 14 ++++++--
 app/views/projects/notes/_actions.html.haml |  4 +--
 spec/models/concerns/issuable_spec.rb       | 40 +++++++++++----------
 spec/models/note_spec.rb                    | 21 +++++++++++
 6 files changed, 61 insertions(+), 26 deletions(-)

diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb
index cdcfefd90c9..b6cf366c05c 100644
--- a/app/controllers/concerns/renders_notes.rb
+++ b/app/controllers/concerns/renders_notes.rb
@@ -1,5 +1,5 @@
 module RendersNotes
-  def prepare_notes_for_rendering(notes, noteable=nil)
+  def prepare_notes_for_rendering(notes, noteable = nil)
     preload_noteable_for_regular_notes(notes)
     preload_max_access_for_authors(notes, @project)
     preload_first_time_contribution_for_authors(noteable, notes) if noteable.is_a?(Issuable)
@@ -23,7 +23,10 @@ module RendersNotes
 
   def preload_first_time_contribution_for_authors(issuable, notes)
     return unless issuable.first_contribution?
+
     same_author = lambda {|n| n.author_id == issuable.author_id}
-    notes.select(&same_author).each {|note| note.special_role = :first_time_contributor}
+    notes.each do |note|
+      note.specialize!(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR, &same_author)
+    end
   end
 end
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 7b1c4e7a2d5..0cc60dddc69 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -337,6 +337,7 @@ module Issuable
 
   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
 end
diff --git a/app/models/note.rb b/app/models/note.rb
index d04bccc6e81..28cd54bd81c 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -26,7 +26,6 @@ class Note < ActiveRecord::Base
   end
 
   ignore_column :original_discussion_id
-  ignore_column :special_role
 
   cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true
 
@@ -155,6 +154,10 @@ class Note < ActiveRecord::Base
         .group(:noteable_id)
         .where(noteable_type: type, noteable_id: ids)
     end
+
+    def has_special_role?(role, note)
+      note.special_role == role
+    end
   end
 
   def cross_reference?
@@ -221,14 +224,19 @@ class Note < ActiveRecord::Base
   end
 
   def special_role=(role)
-    raise "Role is undefined, #{role} not found in #{SpecialRole.values}" unless SpecialRole.values.include? role
+    raise "Role is undefined, #{role} not found in #{SpecialRole.values}" unless SpecialRole.values.include?(role)
+
     @special_role = role
   end
 
   def has_special_role?(role)
-    return @special_role == role
+    self.class.has_special_role?(role, self)
   end
 
+  def specialize!(role)
+    self.special_role = role if !block_given? || yield(self)
+  end
+ 
   def editable?
     !system?
   end
diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml
index 926ceff3ee2..cf634faccfc 100644
--- a/app/views/projects/notes/_actions.html.haml
+++ b/app/views/projects/notes/_actions.html.haml
@@ -1,7 +1,7 @@
-- if note.has_special_role? :first_time_contributor
+- if note.has_special_role?(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR)
   %span.note-role.has-tooltip{ title: _("This user hasn't yet contributed code to this project. Handle with care.")}
     = _('First-time contributor')
-- elsif access = access = note_max_access_for_user(note)
+- elsif access = note_max_access_for_user(note)
   %span.note-role= Gitlab::Access.human_access(access)
 
 - if note.resolvable?
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 9b83b8bfb7e..9948340d976 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -492,15 +492,14 @@ describe Issuable do
 
     let(:contributor) { create(:user) }
     let(:first_time_contributor) { create(:user) }
-    let!(:access_users) { [owner, master, reporter] }
 
     before do
       group.add_owner(owner)
-      project.team << [master, :master]
-      project.team << [reporter, :reporter]
-      project.team << [guest, :guest]
-      project.team << [contributor, :guest]
-      project.team << [first_time_contributor, :guest]
+      project.add_master(master)
+      project.add_reporter(reporter)
+      project.add_guest(guest)
+      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) }
@@ -510,27 +509,30 @@ describe Issuable do
     context "for merge requests" do
       it "is false for MASTER" do
         mr = create(:merge_request, author: master, target_project: project, source_project: project)
-        expect(mr.first_contribution?).to be_falsey
+
+        expect(mr).not_to be_first_contribution
       end
 
       it "is false for OWNER" do
         mr = create(:merge_request, author: owner, target_project: project, source_project: project)
-        expect(mr.first_contribution?).to be_falsey
+        
+        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.first_contribution?).to be_falsey
+        
+        expect(mr).not_to be_first_contribution
       end
 
       it "is true when you don't have any merged MR" do
-        expect(open_mr.first_contribution?).to be_truthy
-        expect(merged_mr.first_contribution?).to be_falsey
+        expect(open_mr).to be_first_contribution
+        expect(merged_mr).not_to be_first_contribution
       end
 
-      it "handle multiple projects separately" do
-        expect(open_mr.first_contribution?).to be_truthy
-        expect(merged_mr_other_project.first_contribution?).to be_falsey
+      it "handles multiple projects separately" do
+        expect(open_mr).to be_first_contribution
+        expect(merged_mr_other_project).not_to be_first_contribution
       end
     end
 
@@ -541,15 +543,15 @@ describe Issuable do
 
       it "is true when you don't have any merged MR" do
         expect(merged_mr).to be
-        expect(first_time_contributor_issue.first_contribution?).to be_truthy
-        expect(contributor_issue.first_contribution?).to be_falsey
+        expect(first_time_contributor_issue).to be_first_contribution
+        expect(contributor_issue).not_to be_first_contribution
       end
 
-      it "handle multiple projects separately" do
+      it "handles multiple projects separately" do
         expect(merged_mr).to be
         expect(merged_mr_other_project).to be
-        expect(first_time_contributor_issue.first_contribution?).to be_truthy
-        expect(first_time_contributor_issue_other_project.first_contribution?).to be_falsey
+        expect(first_time_contributor_issue).to be_first_contribution
+        expect(first_time_contributor_issue_other_project).not_to be_first_contribution
       end
     end
   end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index b214074fdce..09caa2c9991 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -696,4 +696,25 @@ 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

From b44a1bcd0b94a68f680c24d0dfd6d3402af9a881 Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Tue, 15 Aug 2017 09:21:27 -0400
Subject: [PATCH 05/10] rework the contributor badge

 - only show in merge-requests
 - show as a little glyph
---
 app/assets/stylesheets/pages/notes.scss     | 17 ++++++++++++++---
 app/controllers/concerns/renders_notes.rb   | 11 ++++-------
 app/helpers/issuables_helper.rb             |  4 ++--
 app/models/concerns/issuable.rb             |  9 +++++----
 app/models/merge_request.rb                 |  5 +++++
 app/models/note.rb                          |  5 +++++
 app/views/projects/notes/_actions.html.haml |  8 ++++----
 spec/models/concerns/issuable_spec.rb       | 12 ++----------
 8 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index 45f2aed1531..077e661a77e 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -620,15 +620,26 @@ ul.notes {
 
 .note-role {
   position: relative;
-  padding: 0 7px;
+  display: inline-block;
   color: $notes-role-color;
   font-size: 12px;
   line-height: 20px;
-  border: 1px solid $border-color;
-  border-radius: $label-border-radius;
+
+  &.note-role-access {
+    top: -2px;
+    padding: 0 7px;
+    border: 1px solid $border-color;
+    border-radius: $label-border-radius;
+  }
+
+  &.note-role-special {
+    top: -3px;
+    text-shadow: 0 0 15px white;
+  }
 }
 
 
+
 /**
  * Line note button on the side of diffs
  */
diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb
index b6cf366c05c..4791bc561a4 100644
--- a/app/controllers/concerns/renders_notes.rb
+++ b/app/controllers/concerns/renders_notes.rb
@@ -2,7 +2,7 @@ module RendersNotes
   def prepare_notes_for_rendering(notes, noteable = nil)
     preload_noteable_for_regular_notes(notes)
     preload_max_access_for_authors(notes, @project)
-    preload_first_time_contribution_for_authors(noteable, notes) if noteable.is_a?(Issuable)
+    preload_first_time_contribution_for_authors(noteable, notes)
     Banzai::NoteRenderer.render(notes, @project, current_user)
 
     notes
@@ -21,12 +21,9 @@ module RendersNotes
     ActiveRecord::Associations::Preloader.new.preload(notes.reject(&:for_commit?), :noteable)
   end
 
-  def preload_first_time_contribution_for_authors(issuable, notes)
-    return unless issuable.first_contribution?
+  def preload_first_time_contribution_for_authors(noteable, notes)
+    return unless noteable.is_a?(Issuable) && noteable.first_contribution?
 
-    same_author = lambda {|n| n.author_id == issuable.author_id}
-    notes.each do |note|
-      note.specialize!(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR, &same_author)
-    end
+    notes.each {|n| n.specialize_for_first_contribution!(noteable)}
   end
 end
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 11fae16f04d..fbc1ba55d42 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -138,7 +138,7 @@ module IssuablesHelper
     end
 
     output << "&ensp;".html_safe
-    output << issuable_first_contribution_icon if issuable.first_contribution?
+    output << content_tag(:span, (issuable_first_contribution_icon if issuable.first_contribution?), class: 'has-tooltip', title: _('1st contribution!'))
 
     output << content_tag(:span, (issuable.task_status if issuable.tasks?), id: "task_status", class: "hidden-xs hidden-sm")
     output << content_tag(:span, (issuable.task_status_short if issuable.tasks?), id: "task_status_short", class: "hidden-md hidden-lg")
@@ -176,7 +176,7 @@ module IssuablesHelper
   end
 
   def issuable_first_contribution_icon
-    content_tag(:span, class: 'fa-stack has-tooltip', title: _('1st contribution!')) do
+    content_tag(:span, class: 'fa-stack') do
       concat(icon('certificate', class: "fa-stack-2x"))
       concat(content_tag(:strong, '1', class: 'fa-inverse fa-stack-1x'))
     end 
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 0cc60dddc69..765ccf540c3 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -335,9 +335,10 @@ module Issuable
     metrics.record!
   end
 
-  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?
+  ##
+  # Override in issuable specialization
+  #
+  def first_contribution?(*)
+    false
   end
 end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index b82f49d7073..a5b037d94c2 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -960,6 +960,11 @@ class MergeRequest < ActiveRecord::Base
     Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache
   end
 
+  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
+
   private
 
   def write_ref
diff --git a/app/models/note.rb b/app/models/note.rb
index 28cd54bd81c..b1fe60aa387 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -236,6 +236,11 @@ class Note < ActiveRecord::Base
   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)
+  end
  
   def editable?
     !system?
diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml
index cf634faccfc..22c81f76930 100644
--- a/app/views/projects/notes/_actions.html.haml
+++ b/app/views/projects/notes/_actions.html.haml
@@ -1,8 +1,8 @@
 - if note.has_special_role?(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR)
-  %span.note-role.has-tooltip{ title: _("This user hasn't yet contributed code to this project. Handle with care.")}
-    = _('First-time contributor')
-- elsif access = note_max_access_for_user(note)
-  %span.note-role= Gitlab::Access.human_access(access)
+  %span.note-role.note-role-special.has-tooltip{ title: _("This user hasn't yet contributed code 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)
 
 - if note.resolvable?
   - can_resolve = can?(current_user, :resolve_note, note)
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 9948340d976..dc959c58ba1 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -539,20 +539,12 @@ describe Issuable do
     context "for issues" do
       let(:contributor_issue) { create(:issue, author: contributor, project: project) }
       let(:first_time_contributor_issue) { create(:issue, author: first_time_contributor, project: project) }
-      let(:first_time_contributor_issue_other_project) { create(:issue, author: first_time_contributor, project: other_project) }
 
-      it "is true when you don't have any merged MR" do
+      it "is false even without merged MR" do
         expect(merged_mr).to be
-        expect(first_time_contributor_issue).to be_first_contribution
+        expect(first_time_contributor_issue).not_to be_first_contribution
         expect(contributor_issue).not_to be_first_contribution
       end
-
-      it "handles multiple projects separately" do
-        expect(merged_mr).to be
-        expect(merged_mr_other_project).to be
-        expect(first_time_contributor_issue).to be_first_contribution
-        expect(first_time_contributor_issue_other_project).not_to be_first_contribution
-      end
     end
   end
 end

From 643ed0f6a95055c7972781fa7c81e8b7a13ed78a Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Tue, 15 Aug 2017 10:58:48 -0400
Subject: [PATCH 06/10] fix note-role alignment

---
 app/assets/stylesheets/pages/notes.scss | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index 077e661a77e..e24f768e1d2 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -516,7 +516,7 @@ ul.notes {
 }
 
 .note-actions-item {
-  margin-left: 15px;
+  margin-left: 12px;
   display: flex;
   align-items: center;
 
@@ -624,16 +624,15 @@ ul.notes {
   color: $notes-role-color;
   font-size: 12px;
   line-height: 20px;
+  margin: 0 3px;
 
   &.note-role-access {
-    top: -2px;
     padding: 0 7px;
     border: 1px solid $border-color;
     border-radius: $label-border-radius;
   }
 
   &.note-role-special {
-    top: -3px;
     text-shadow: 0 0 15px white;
   }
 }

From 4130552b421f1b83e60f03715000efe56461fc6b Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Tue, 15 Aug 2017 12:18:02 -0400
Subject: [PATCH 07/10] remove lint

---
 app/assets/stylesheets/pages/notes.scss     | 2 +-
 app/views/projects/notes/_actions.html.haml | 2 +-
 spec/models/concerns/issuable_spec.rb       | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index e24f768e1d2..e437bad4912 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -633,7 +633,7 @@ ul.notes {
   }
 
   &.note-role-special {
-    text-shadow: 0 0 15px white;
+    text-shadow: 0 0 15px $gl-text-color-inverted;
   }
 }
 
diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml
index 22c81f76930..999a1b850e1 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 user hasn't yet contributed code 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/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index dc959c58ba1..098d33e46c5 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -483,8 +483,8 @@ describe Issuable do
 
   describe '#first_contribution?' do
     let(:group) { create(:group) }
-    let(:project) { create(:empty_project, namespace: group) }
-    let(:other_project) { create(:empty_project) }
+    let(:project) { create(:project, namespace: group) }
+    let(:other_project) { create(:project) }
     let(:owner) { create(:owner) }
     let(:master) { create(:user) }
     let(:reporter) { create(:user) }

From 65bcd141c883b1efd9518ff5f588e1dcb1d80f64 Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Tue, 29 Aug 2017 09:46:40 -0400
Subject: [PATCH 08/10] add controller spec

also fix some code styling issues
---
 app/models/concerns/issuable.rb               |  2 +-
 app/models/merge_request.rb                   |  3 ++-
 app/models/note.rb                            | 11 ++++------
 app/views/projects/notes/_actions.html.haml   |  2 +-
 .../merge_requests_controller_spec.rb         | 22 +++++++++++++++++++
 spec/models/concerns/issuable_spec.rb         |  6 ++---
 spec/models/note_spec.rb                      | 21 ------------------
 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

From d828932eac3ec1a2983edef03bbd3ab99057d531 Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Mon, 4 Sep 2017 14:47:46 -0400
Subject: [PATCH 09/10] fixup whitespace

---
 app/helpers/issuables_helper.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index fbc1ba55d42..8d88a3a252c 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -179,7 +179,7 @@ module IssuablesHelper
     content_tag(:span, class: 'fa-stack') do
       concat(icon('certificate', class: "fa-stack-2x"))
       concat(content_tag(:strong, '1', class: 'fa-inverse fa-stack-1x'))
-    end 
+    end
   end
 
   def assigned_issuables_count(issuable_type)

From 7d2302605c8f40ebbfe72ad7efa5a32aa4c806fe Mon Sep 17 00:00:00 2001
From: "micael.bergeron" <micael.bergeron@solutionstlm.com>
Date: Wed, 6 Sep 2017 08:47:58 -0400
Subject: [PATCH 10/10] fix the failing spec

---
 spec/features/merge_requests/diff_notes_avatars_spec.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb
index e77f1f92731..ca536f2800c 100644
--- a/spec/features/merge_requests/diff_notes_avatars_spec.rb
+++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb
@@ -139,7 +139,7 @@ feature 'Diff note avatars', js: true do
         end
 
         page.within find("[id='#{position.line_code(project.repository)}']") do
-          find('.diff-notes-collapse').click
+          find('.diff-notes-collapse').trigger('click')
 
           expect(page).to have_selector('img.js-diff-comment-avatar', count: 2)
         end
@@ -152,7 +152,7 @@ feature 'Diff note avatars', js: true do
           page.within '.js-discussion-note-form' do
             find('.js-note-text').native.send_keys('Test')
 
-            find('.js-comment-button').trigger 'click'
+            find('.js-comment-button').trigger('click')
 
             wait_for_requests
           end