From 966b1128d884a318dad4277e23368334fe67e836 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Sat, 29 Jul 2017 11:04:42 -0400 Subject: 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 +- .../projects/merge_requests/diffs_controller.rb | 2 +- .../projects/merge_requests_controller.rb | 6 +- app/controllers/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 << " ".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 -- cgit v1.2.1