Merge branch 'jp-notes-update-validation' into 'master'

Do not allow to change note's confidentiality

See merge request gitlab-org/gitlab!84174

app/models/note.rb

@@ -97,6 +97,8 @@ class Note < ApplicationRecord
   validates :author, presence: true
   validates :discussion_id, presence: true, format: { with: /\A\h{40}\z/ }
 
+  validate :ensure_confidentiality_not_changed, on: :update
+
   validate unless: [:for_commit?, :importing?, :skip_project_check?] do |note|
     unless note.noteable.try(:project) == note.project
       errors.add(:project, 'does not match noteable project')
@@ -718,6 +720,13 @@ class Note < ApplicationRecord
   def noteable_label_url_method
     for_merge_request? ? :project_merge_requests_url : :project_issues_url
   end
+
+  def ensure_confidentiality_not_changed
+    return unless will_save_change_to_attribute?(:confidential)
+    return unless attribute_change_to_be_saved(:confidential).include?(true)
+
+    errors.add(:confidential, _('can not be changed for existing notes'))
+  end
 end
 
 Note.prepend_mod_with('Note')

app/services/notes/update_service.rb

@@ -27,10 +27,7 @@ module Notes
         note.assign_attributes(last_edited_at: Time.current, updated_by: current_user)
       end
 
-      note.with_transaction_returning_status do
-        update_confidentiality(note)
-        note.save
-      end
+      note.save
 
       unless only_commands || note.for_personal_snippet?
         note.create_new_cross_references!(current_user)
@@ -88,15 +85,6 @@ module Notes
       TodoService.new.update_note(note, current_user, old_mentioned_users)
     end
 
-    # This method updates confidentiality of all discussion notes at once
-    def update_confidentiality(note)
-      return unless params.key?(:confidential)
-      return unless note.is_a?(DiscussionNote) # we don't need to do bulk update for single notes
-      return unless note.start_of_discussion? # don't update all notes if a response is being updated
-
-      Note.id_in(note.discussion.notes.map(&:id)).update_all(confidential: params[:confidential])
-    end
-
     def track_note_edit_usage_for_issues(note)
       Gitlab::UsageDataCounters::IssueActivityUniqueCounter.track_issue_comment_edited_action(author: note.author)
     end

locale/gitlab.pot

@@ -43868,6 +43868,9 @@ msgstr ""
 msgid "can contain only lowercase letters, digits, and '_'."
 msgstr ""
 
+msgid "can not be changed for existing notes"
+msgstr ""
+
 msgid "can only be changed by a group admin."
 msgstr ""
 

spec/models/note_spec.rb

@@ -105,6 +105,36 @@ RSpec.describe Note do
         it { is_expected.to be_valid }
       end
     end
+
+    describe 'confidentiality' do
+      context 'for existing public note' do
+        let_it_be(:existing_note) { create(:note) }
+
+        it 'is not possible to change the note to confidential' do
+          existing_note.confidential = true
+
+          expect(existing_note).not_to be_valid
+          expect(existing_note.errors[:confidential]).to include('can not be changed for existing notes')
+        end
+
+        it 'is possible to change confidentiality from nil to false' do
+          existing_note.confidential = false
+
+          expect(existing_note).to be_valid
+        end
+      end
+
+      context 'for existing confidential note' do
+        let_it_be(:existing_note) { create(:note, confidential: true) }
+
+        it 'is not possible to change the note to public' do
+          existing_note.confidential = false
+
+          expect(existing_note).not_to be_valid
+          expect(existing_note.errors[:confidential]).to include('can not be changed for existing notes')
+        end
+      end
+    end
   end
 
   describe 'callbacks' do

spec/requests/api/graphql/mutations/notes/update/note_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe 'Updating a Note' do
   let!(:note) { create(:note, note: original_body) }
   let(:original_body) { 'Initial body text' }
   let(:updated_body) { 'Updated body text' }
-  let(:params) { { body: updated_body, confidential: true } }
+  let(:params) { { body: updated_body } }
   let(:mutation) do
     variables = params.merge(id: GitlabSchema.id_from_object(note).to_s)
 
@@ -28,7 +28,6 @@ RSpec.describe 'Updating a Note' do
       post_graphql_mutation(mutation, current_user: current_user)
 
       expect(note.reload.note).to eq(original_body)
-      expect(note.confidential).to be_falsey
     end
   end
 
@@ -41,46 +40,19 @@ RSpec.describe 'Updating a Note' do
       post_graphql_mutation(mutation, current_user: current_user)
 
       expect(note.reload.note).to eq(updated_body)
-      expect(note.confidential).to be_truthy
     end
 
     it 'returns the updated Note' do
       post_graphql_mutation(mutation, current_user: current_user)
 
       expect(mutation_response['note']['body']).to eq(updated_body)
-      expect(mutation_response['note']['confidential']).to be_truthy
-    end
-
-    context 'when only confidential param is present' do
-      let(:params) { { confidential: true } }
-
-      it 'updates only the note confidentiality' do
-        post_graphql_mutation(mutation, current_user: current_user)
-
-        expect(note.reload.note).to eq(original_body)
-        expect(note.confidential).to be_truthy
-      end
-    end
-
-    context 'when only body param is present' do
-      let(:params) { { body: updated_body } }
-
-      before do
-        note.update_column(:confidential, true)
-      end
-
-      it 'updates only the note body' do
-        post_graphql_mutation(mutation, current_user: current_user)
-
-        expect(note.reload.note).to eq(updated_body)
-        expect(note.confidential).to be_truthy
-      end
     end
 
     context 'when there are ActiveRecord validation errors' do
-      let(:updated_body) { '' }
+      let(:params) { { body: '', confidential: true } }
 
-      it_behaves_like 'a mutation that returns errors in the response', errors: ["Note can't be blank"]
+      it_behaves_like 'a mutation that returns errors in the response',
+        errors: ["Note can't be blank", 'Confidential can not be changed for existing notes']
 
       it 'does not update the Note' do
         post_graphql_mutation(mutation, current_user: current_user)

spec/services/notes/update_service_spec.rb

@@ -138,45 +138,6 @@ RSpec.describe Notes::UpdateService do
       end
     end
 
-    context 'setting confidentiality' do
-      let(:opts) { { confidential: true } }
-
-      context 'simple note' do
-        it 'updates the confidentiality' do
-          expect { update_note(opts) }.to change { note.reload.confidential }.from(nil).to(true)
-        end
-      end
-
-      context 'discussion notes' do
-        let(:note) { create(:discussion_note, project: project, noteable: issue, author: user, note: "Old note #{user2.to_reference}") }
-        let!(:response_note_1) { create(:discussion_note, project: project, noteable: issue, in_reply_to: note) }
-        let!(:response_note_2) { create(:discussion_note, project: project, noteable: issue, in_reply_to: note, confidential: false) }
-        let!(:other_note) { create(:note, project: project, noteable: issue) }
-
-        context 'when updating the root note' do
-          it 'updates the confidentiality of the root note and all the responses' do
-            update_note(opts)
-
-            expect(note.reload.confidential).to be_truthy
-            expect(response_note_1.reload.confidential).to be_truthy
-            expect(response_note_2.reload.confidential).to be_truthy
-            expect(other_note.reload.confidential).to be_falsey
-          end
-        end
-
-        context 'when updating one of the response notes' do
-          it 'updates only the confidentiality of the note that is being updated' do
-            Notes::UpdateService.new(project, user, opts).execute(response_note_1)
-
-            expect(note.reload.confidential).to be_falsey
-            expect(response_note_1.reload.confidential).to be_truthy
-            expect(response_note_2.reload.confidential).to be_falsey
-            expect(other_note.reload.confidential).to be_falsey
-          end
-        end
-      end
-    end
-
     context 'todos' do
       shared_examples 'does not update todos' do
         it 'keep todos' do

spec/support/shared_examples/models/concerns/integrations/slack_mattermost_notifier_shared_examples.rb

@@ -227,9 +227,7 @@ RSpec.shared_examples Integrations::SlackMattermostNotifier do |integration_name
         end
 
         context 'for confidential notes' do
-          before_all do
-            issue_note.update!(confidential: true)
-          end
+          let_it_be(:issue_note) { create(:note_on_issue, project: project, note: "issue note", confidential: true) }
 
           it 'falls back to note channel' do
             expect(::Slack::Messenger).to execute_with_options(channel: ['random'])

spec/support/shared_examples/requests/api/notes_shared_examples.rb

@@ -306,7 +306,7 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
   end
 
   describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/notes/:note_id" do
-    let(:params) { { body: 'Hello!', confidential: false } }
+    let(:params) { { body: 'Hello!' } }
 
     subject do
       put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/notes/#{note.id}", user), params: params
@@ -314,44 +314,27 @@ RSpec.shared_examples 'noteable API' do |parent_type, noteable_type, id_name|
 
     context 'when eveything is ok' do
       before do
-        note.update!(confidential: true)
+        subject
       end
 
-      context 'with multiple params present' do
-        before do
-          subject
-        end
-
-        it 'returns modified note' do
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['body']).to eq('Hello!')
-          expect(json_response['confidential']).to be_falsey
-        end
-
-        it 'updates the note' do
-          expect(note.reload.note).to eq('Hello!')
-          expect(note.confidential).to be_falsey
-        end
+      it 'returns modified note' do
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['body']).to eq('Hello!')
       end
 
-      context 'when only body param is present' do
-        let(:params) { { body: 'Hello!' } }
-
-        it 'updates only the note text' do
-          expect { subject }.not_to change { note.reload.confidential }
-
-          expect(note.note).to eq('Hello!')
-        end
+      it 'updates the note' do
+        expect(note.reload.note).to eq('Hello!')
+        expect(note.confidential).to be_falsey
       end
+    end
 
-      context 'when only confidential param is present' do
-        let(:params) { { confidential: false } }
+    context 'when also confidential param is set' do
+      let(:params) { { body: 'Hello!', confidential: true } }
 
-        it 'updates only the note text' do
-          expect { subject }.not_to change { note.reload.note }
+      it 'fails to update the note' do
+        expect { subject }.not_to change { note.reload.note }
 
-          expect(note.confidential).to be_falsey
-        end
+        expect(response).to have_gitlab_http_status(:bad_request)
       end
     end