diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 45f2aed1531..e437bad4912 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; @@ -620,15 +620,25 @@ 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; + margin: 0 3px; + + &.note-role-access { + padding: 0 7px; + border: 1px solid $border-color; + border-radius: $label-border-radius; + } + + &.note-role-special { + text-shadow: 0 0 15px $gl-text-color-inverted; + } } + /** * 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 41c3114ad1e..4791bc561a4 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) 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(noteable, notes) + return unless noteable.is_a?(Issuable) && noteable.first_contribution? + + notes.each {|n| n.specialize_for_first_contribution!(noteable)} + 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 2cf2e7dddb0..ce2999e6696 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -134,6 +134,8 @@ module IssuablesHelper end output << " ".html_safe + 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") @@ -169,6 +171,13 @@ module IssuablesHelper html.html_safe end + def issuable_first_contribution_icon + 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 + 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 083ace65b09..ce028195e51 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -73,7 +73,7 @@ module NotesHelper end def note_max_access_for_user(note) - note.project.team.human_max_access(note.author_id) + 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..265f6e48540 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -334,4 +334,11 @@ module Issuable metrics = self.metrics || create_metrics metrics.record! end + + ## + # 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..2a56bab48a3 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -960,6 +960,12 @@ 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 1073c115630..f44590e2144 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -15,6 +15,16 @@ 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 cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true @@ -32,9 +42,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 @@ -141,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? @@ -206,6 +223,22 @@ 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) + self.class.has_special_role?(role, self) + end + + def specialize_for_first_contribution!(noteable) + return unless noteable.author_id == self.author_id + + self.special_role = Note::SpecialRole::FIRST_TIME_CONTRIBUTOR + 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 fb07141d2ac..de76832331a 100644 --- a/app/views/projects/notes/_actions.html.haml +++ b/app/views/projects/notes/_actions.html.haml @@ -1,6 +1,8 @@ -- access = note_max_access_for_user(note) -- if access - %span.note-role= access +- if note.has_special_role?(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR) + %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) - if note.resolvable? - can_resolve = can?(current_user, :resolve_note, note) 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 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/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/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 diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb index 68540dd4e59..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,8 +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') + 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..fb5fb7daaab 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -480,4 +480,71 @@ describe Issuable do end end end + + describe '#first_contribution?' do + let(:group) { create(:group) } + let(:project) { create(:project, namespace: group) } + let(:other_project) { create(: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) } + + before do + group.add_owner(owner) + 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) } + 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).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).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 + + it "is true when you don't have any merged MR" do + expect(open_mr).to be_first_contribution + expect(merged_mr).not_to be_first_contribution + end + + 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 + + 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) } + + it "is false even without merged MR" do + expect(merged_mr).to be + expect(first_time_contributor_issue).not_to be_first_contribution + expect(contributor_issue).not_to be_first_contribution + end + end + end end