rework the contributor badge

 - only show in merge-requests
 - show as a little glyph

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