From 27513d11416b387b632a34f6101494d07e527640 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 17 Jan 2024 15:43:01 +0000 Subject: [PATCH] Added support for internal notes on merge requests Updates the comment form on merge requests to support internal notes. Internal notes can only be normal notes or discussions, not diff discussions. Changelog: added https://gitlab.com/gitlab-org/gitlab/-/issues/362834 --- .../notes/components/comment_form.vue | 5 +---- app/models/note.rb | 2 +- app/policies/merge_request_policy.rb | 5 +++++ .../merge_request_noteable_entity.rb | 4 ++++ ee/app/models/ee/note.rb | 5 ----- .../entities/merge_request_noteable.json | 6 ++++-- ee/spec/models/note_spec.rb | 18 ++++++++++++++++++ .../entities/merge_request_noteable.json | 6 ++++-- .../notes/components/comment_form_spec.js | 2 +- spec/models/note_spec.rb | 10 +++++++++- spec/policies/merge_request_policy_spec.rb | 6 ++++-- 11 files changed, 51 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue index 87b55b19c08afc..fc6f0e0ecf7430 100644 --- a/app/assets/javascripts/notes/components/comment_form.vue +++ b/app/assets/javascripts/notes/components/comment_form.vue @@ -117,10 +117,7 @@ export default { return this.getNoteableData.current_user.can_create_note; }, canSetInternalNote() { - return ( - this.getNoteableData.current_user.can_create_confidential_note && - (this.isIssue || this.isEpic) - ); + return this.getNoteableData.current_user.can_create_confidential_note; }, issueActionButtonTitle() { const openOrClose = this.isOpen ? 'close' : 'reopen'; diff --git a/app/models/note.rb b/app/models/note.rb index 953632ba9109cf..10ab85b24b75ae 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -935,7 +935,7 @@ def ensure_note_type_can_be_confidential end def noteable_can_have_confidential_note? - for_issue? + for_issuable? end def set_internal_flag diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb index 49f9225a1d36ca..27bcd816a08e8f 100644 --- a/app/policies/merge_request_policy.rb +++ b/app/policies/merge_request_policy.rb @@ -14,6 +14,7 @@ class MergeRequestPolicy < IssuablePolicy rule { ~can?(:read_merge_request) }.policy do prevent :create_note prevent :accept_merge_request + prevent :mark_note_as_internal end rule { can_approve }.policy do @@ -43,6 +44,10 @@ class MergeRequestPolicy < IssuablePolicy enable :set_merge_request_metadata end + rule { can?(:reporter_access) }.policy do + enable :mark_note_as_internal + end + private def can_approve? diff --git a/app/serializers/merge_request_noteable_entity.rb b/app/serializers/merge_request_noteable_entity.rb index 04b801e29adb20..4122cb5f9a4543 100644 --- a/app/serializers/merge_request_noteable_entity.rb +++ b/app/serializers/merge_request_noteable_entity.rb @@ -49,6 +49,10 @@ class MergeRequestNoteableEntity < IssuableEntity expose :can_update do |merge_request| can?(current_user, :update_merge_request, merge_request) end + + expose :can_create_confidential_note do |merge_request| + can?(request.current_user, :mark_note_as_internal, merge_request) + end end expose :locked_discussion_docs_path, if: -> (merge_request) { merge_request.discussion_locked? } do |merge_request| diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb index 12075ad57d1143..85a74bff2dc67a 100644 --- a/ee/app/models/ee/note.rb +++ b/ee/app/models/ee/note.rb @@ -132,10 +132,5 @@ def group_reporter?(user, group) def banzai_context_params { group: noteable.group, label_url_method: :group_epics_url } end - - override :noteable_can_have_confidential_note? - def noteable_can_have_confidential_note? - for_epic? || super - end end end diff --git a/ee/spec/fixtures/api/schemas/entities/merge_request_noteable.json b/ee/spec/fixtures/api/schemas/entities/merge_request_noteable.json index 6be0ae47f3f55d..1c78345b5a27ed 100644 --- a/ee/spec/fixtures/api/schemas/entities/merge_request_noteable.json +++ b/ee/spec/fixtures/api/schemas/entities/merge_request_noteable.json @@ -24,11 +24,13 @@ "type": "object", "required": [ "can_create_note", - "can_update" + "can_update", + "can_create_confidential_note" ], "properties": { "can_create_note": { "type": "boolean" }, - "can_update": { "type": "boolean" } + "can_update": { "type": "boolean" }, + "can_create_confidential_note": { "type": "boolean" } }, "additionalProperties": false }, diff --git a/ee/spec/models/note_spec.rb b/ee/spec/models/note_spec.rb index b8e282d91867b9..8355dfb7baa6c7 100644 --- a/ee/spec/models/note_spec.rb +++ b/ee/spec/models/note_spec.rb @@ -13,6 +13,24 @@ let(:set_mentionable_text) { ->(txt) { subject.note = txt } } end + describe 'validation' do + describe 'confidentiality' do + context 'for a new note' do + let(:note_params) { { confidential: true, noteable: noteable, project: noteable.project } } + + subject(:note) { build(:note, **note_params) } + + context 'when noteable is a epic' do + let_it_be(:noteable) { create(:epic) } + + it 'can not be set confidential' do + expect(note).to be_valid + end + end + end + end + end + describe '#ensure_namespace_id' do context 'for an epic note' do let_it_be(:epic) { create(:epic) } diff --git a/spec/fixtures/api/schemas/entities/merge_request_noteable.json b/spec/fixtures/api/schemas/entities/merge_request_noteable.json index 6f3c29b16e9ef5..2e0d7f998a24e8 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_noteable.json +++ b/spec/fixtures/api/schemas/entities/merge_request_noteable.json @@ -24,11 +24,13 @@ "type": "object", "required": [ "can_create_note", - "can_update" + "can_update", + "can_create_confidential_note" ], "properties": { "can_create_note": { "type": "boolean" }, - "can_update": { "type": "boolean" } + "can_update": { "type": "boolean" }, + "can_create_confidential_note": { "type": "boolean" } }, "additionalProperties": false }, diff --git a/spec/frontend/notes/components/comment_form_spec.js b/spec/frontend/notes/components/comment_form_spec.js index 8f761476c7c99e..849490af554b01 100644 --- a/spec/frontend/notes/components/comment_form_spec.js +++ b/spec/frontend/notes/components/comment_form_spec.js @@ -619,7 +619,7 @@ describe('issue_comment_form component', () => { noteableType | rendered | message ${'Issue'} | ${true} | ${'render'} ${'Epic'} | ${true} | ${'render'} - ${'MergeRequest'} | ${false} | ${'not render'} + ${'MergeRequest'} | ${true} | ${'render'} `( 'should $message checkbox when noteableType is $noteableType', ({ noteableType, rendered }) => { diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 597950596422e3..8f755e4741b7ef 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -176,9 +176,17 @@ expect(subject).to be_valid end - context 'when noteable is not allowed to have confidential notes' do + context 'when noteable is a merge request' do let_it_be(:noteable) { create(:merge_request) } + it 'can not be set confidential' do + expect(subject).to be_valid + end + end + + context 'when noteable is not allowed to have confidential notes' do + let_it_be(:noteable) { create(:snippet) } + it 'can not be set confidential' do expect(subject).not_to be_valid expect(subject.errors[:confidential]).to include('can not be set for this resource') diff --git a/spec/policies/merge_request_policy_spec.rb b/spec/policies/merge_request_policy_spec.rb index c21e1244402d4f..12b38fe1d93216 100644 --- a/spec/policies/merge_request_policy_spec.rb +++ b/spec/policies/merge_request_policy_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe MergeRequestPolicy do +RSpec.describe MergeRequestPolicy, feature_category: :code_review_workflow do include ExternalAuthorizationServiceHelpers let_it_be(:guest) { create(:user) } @@ -23,7 +23,8 @@ def permissions(user, merge_request) create_todo approve_merge_request create_note - update_subscription].freeze + update_subscription + mark_note_as_internal].freeze shared_examples_for 'a denied user' do let(:perms) { permissions(subject, merge_request) } @@ -47,6 +48,7 @@ def permissions(user, merge_request) :create_merge_request_from | false :approve_merge_request | false :update_merge_request | false + :mark_note_as_internal | true end with_them do -- GitLab