From 65bcd141c883b1efd9518ff5f588e1dcb1d80f64 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Tue, 29 Aug 2017 09:46:40 -0400 Subject: add controller spec also fix some code styling issues --- app/models/concerns/issuable.rb | 2 +- app/models/merge_request.rb | 3 ++- app/models/note.rb | 11 ++++------- app/views/projects/notes/_actions.html.haml | 2 +- .../projects/merge_requests_controller_spec.rb | 22 ++++++++++++++++++++++ spec/models/concerns/issuable_spec.rb | 6 +++--- spec/models/note_spec.rb | 21 --------------------- 7 files changed, 33 insertions(+), 34 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 765ccf540c3..265f6e48540 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -338,7 +338,7 @@ module Issuable ## # Override in issuable specialization # - def first_contribution?(*) + def first_contribution? false end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index a5b037d94c2..2a56bab48a3 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -960,8 +960,9 @@ class MergeRequest < ActiveRecord::Base Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache end - def first_contribution?(*) + 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 diff --git a/app/models/note.rb b/app/models/note.rb index b1fe60aa387..f44590e2144 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -47,7 +47,7 @@ class Note < ActiveRecord::Base # 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 @@ -233,15 +233,12 @@ class Note < ActiveRecord::Base self.class.has_special_role?(role, self) end - 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) + + self.special_role = Note::SpecialRole::FIRST_TIME_CONTRIBUTOR end - + def editable? !system? end diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml index 999a1b850e1..de76832331a 100644 --- a/app/views/projects/notes/_actions.html.haml +++ b/app/views/projects/notes/_actions.html.haml @@ -1,5 +1,5 @@ - if note.has_special_role?(Note::SpecialRole::FIRST_TIME_CONTRIBUTOR) - %span.note-role.note-role-special.has-tooltip{ title: _("This user hasn't yet contributed code to this project. Handle with care.") } + %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) 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/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 098d33e46c5..fb5fb7daaab 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/spec/models/concerns/issuable_spec.rb @@ -501,7 +501,7 @@ describe Issuable do 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) } @@ -515,13 +515,13 @@ describe Issuable do 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 diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 09caa2c9991..b214074fdce 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -696,25 +696,4 @@ describe Note do note.destroy! 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 -- cgit v1.2.1