Commit 966b1128 authored by micael.bergeron's avatar micael.bergeron

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
parent 8fe1aa5d
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
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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)
......
......@@ -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
......@@ -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
......
......@@ -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.
......
- 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)
......
......@@ -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?
......
......@@ -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
......
......@@ -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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment