Commit 45b83ed9 authored by micael.bergeron's avatar micael.bergeron

round of fixes after code review

parent 966b1128
module RendersNotes 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_noteable_for_regular_notes(notes)
preload_max_access_for_authors(notes, @project) 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) if noteable.is_a?(Issuable)
...@@ -23,7 +23,10 @@ module RendersNotes ...@@ -23,7 +23,10 @@ module RendersNotes
def preload_first_time_contribution_for_authors(issuable, notes) def preload_first_time_contribution_for_authors(issuable, notes)
return unless issuable.first_contribution? return unless issuable.first_contribution?
same_author = lambda {|n| n.author_id == issuable.author_id} 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
end end
...@@ -337,6 +337,7 @@ module Issuable ...@@ -337,6 +337,7 @@ module Issuable
def first_contribution? def first_contribution?
return false if project.team.max_member_access(author_id) > Gitlab::Access::GUEST return false if project.team.max_member_access(author_id) > Gitlab::Access::GUEST
project.merge_requests.merged.where(author_id: author_id).empty? project.merge_requests.merged.where(author_id: author_id).empty?
end end
end end
...@@ -26,7 +26,6 @@ class Note < ActiveRecord::Base ...@@ -26,7 +26,6 @@ class Note < ActiveRecord::Base
end end
ignore_column :original_discussion_id ignore_column :original_discussion_id
ignore_column :special_role
cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true
...@@ -155,6 +154,10 @@ class Note < ActiveRecord::Base ...@@ -155,6 +154,10 @@ class Note < ActiveRecord::Base
.group(:noteable_id) .group(:noteable_id)
.where(noteable_type: type, noteable_id: ids) .where(noteable_type: type, noteable_id: ids)
end end
def has_special_role?(role, note)
note.special_role == role
end
end end
def cross_reference? def cross_reference?
...@@ -221,12 +224,17 @@ class Note < ActiveRecord::Base ...@@ -221,12 +224,17 @@ class Note < ActiveRecord::Base
end end
def special_role=(role) 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 @special_role = role
end end
def has_special_role?(role) 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 end
def editable? def editable?
......
- 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.")} %span.note-role.has-tooltip{ title: _("This user hasn't yet contributed code to this project. Handle with care.")}
= _('First-time contributor') = _('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) %span.note-role= Gitlab::Access.human_access(access)
- if note.resolvable? - if note.resolvable?
......
...@@ -492,15 +492,14 @@ describe Issuable do ...@@ -492,15 +492,14 @@ describe Issuable do
let(:contributor) { create(:user) } let(:contributor) { create(:user) }
let(:first_time_contributor) { create(:user) } let(:first_time_contributor) { create(:user) }
let!(:access_users) { [owner, master, reporter] }
before do before do
group.add_owner(owner) group.add_owner(owner)
project.team << [master, :master] project.add_master(master)
project.team << [reporter, :reporter] project.add_reporter(reporter)
project.team << [guest, :guest] project.add_guest(guest)
project.team << [contributor, :guest] project.add_guest(contributor)
project.team << [first_time_contributor, :guest] project.add_guest(first_time_contributor)
end end
let(:merged_mr) { create(:merge_request, :merged, author: contributor, target_project: project, source_project: project) } let(:merged_mr) { create(:merge_request, :merged, author: contributor, target_project: project, source_project: project) }
...@@ -510,27 +509,30 @@ describe Issuable do ...@@ -510,27 +509,30 @@ describe Issuable do
context "for merge requests" do context "for merge requests" do
it "is false for MASTER" do it "is false for MASTER" do
mr = create(:merge_request, author: master, target_project: project, source_project: project) 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 end
it "is false for OWNER" do it "is false for OWNER" do
mr = create(:merge_request, author: owner, target_project: project, source_project: project) 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 end
it "is false for REPORTER" do it "is false for REPORTER" do
mr = create(:merge_request, author: reporter, target_project: project, source_project: project) 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 end
it "is true when you don't have any merged MR" do it "is true when you don't have any merged MR" do
expect(open_mr.first_contribution?).to be_truthy expect(open_mr).to be_first_contribution
expect(merged_mr.first_contribution?).to be_falsey expect(merged_mr).not_to be_first_contribution
end end
it "handle multiple projects separately" do it "handles multiple projects separately" do
expect(open_mr.first_contribution?).to be_truthy expect(open_mr).to be_first_contribution
expect(merged_mr_other_project.first_contribution?).to be_falsey expect(merged_mr_other_project).not_to be_first_contribution
end end
end end
...@@ -541,15 +543,15 @@ describe Issuable do ...@@ -541,15 +543,15 @@ describe Issuable do
it "is true when you don't have any merged MR" do it "is true when you don't have any merged MR" do
expect(merged_mr).to be expect(merged_mr).to be
expect(first_time_contributor_issue.first_contribution?).to be_truthy expect(first_time_contributor_issue).to be_first_contribution
expect(contributor_issue.first_contribution?).to be_falsey expect(contributor_issue).not_to be_first_contribution
end end
it "handle multiple projects separately" do it "handles multiple projects separately" do
expect(merged_mr).to be expect(merged_mr).to be
expect(merged_mr_other_project).to be expect(merged_mr_other_project).to be
expect(first_time_contributor_issue.first_contribution?).to be_truthy expect(first_time_contributor_issue).to be_first_contribution
expect(first_time_contributor_issue_other_project.first_contribution?).to be_falsey expect(first_time_contributor_issue_other_project).not_to be_first_contribution
end end
end end
end end
......
...@@ -696,4 +696,25 @@ describe Note do ...@@ -696,4 +696,25 @@ describe Note do
note.destroy! note.destroy!
end end
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 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