Merge branch 'confidential-notes-update-prepare' into 'master'

Add updating notes confidentiality to Service

See merge request gitlab-org/gitlab!36812

app/models/note.rb

@@ -5,6 +5,7 @@
 # A note of this type is never resolvable.
 class Note < ApplicationRecord
   extend ActiveModel::Naming
+  include Gitlab::Utils::StrongMemoize
   include Participable
   include Mentionable
   include Awardable
@@ -446,9 +447,11 @@ class Note < ApplicationRecord
   # Consider using `#to_discussion` if we do not need to render the discussion
   # and all its notes and if we don't care about the discussion's resolvability status.
   def discussion
+    strong_memoize(:discussion) do
       full_discussion = self.noteable.notes.find_discussion(self.discussion_id) if part_of_discussion?
       full_discussion || to_discussion
     end
+  end
 
   def start_of_discussion?
     discussion.first_note == self

app/services/notes/update_service.rb

@@ -10,6 +10,7 @@ module Notes
       note.assign_attributes(params.merge(updated_by: current_user))
 
       note.with_transaction_returning_status do
+        update_confidentiality(note)
         note.save
       end
 
@@ -79,6 +80,15 @@ 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
   end
 end
 

spec/controllers/projects/discussions_controller_spec.rb

@@ -182,7 +182,8 @@ RSpec.describe Projects::DiscussionsController do
         it "unresolves the discussion" do
           delete :unresolve, params: request_params
 
-          expect(note.reload.discussion.resolved?).to be false
+          # discussion is memoized and reload doesn't clear the memoization
+          expect(Note.find(note.id).discussion.resolved?).to be false
         end
 
         it "returns status 200" do

spec/controllers/projects/merge_requests/drafts_controller_spec.rb

@@ -331,8 +331,10 @@ RSpec.describe Projects::MergeRequests::DraftsController do
       notes = merge_request.notes.reload
 
       expect(notes.pluck(:note)).to include(*drafts.map(&:note))
-      expect(note.discussion.notes.last.note).to eq(draft_reply.note)
-      expect(diff_note.discussion.notes.last.note).to eq(diff_draft_reply.note)
+
+      # discussion is memoized and reload doesn't clear the memoization
+      expect(Note.find(note.id).discussion.notes.last.note).to eq(draft_reply.note)
+      expect(Note.find(diff_note.id).discussion.notes.last.note).to eq(diff_draft_reply.note)
     end
 
     it 'can publish just a single draft note' do
@@ -376,7 +378,8 @@ RSpec.describe Projects::MergeRequests::DraftsController do
 
         post :publish, params: params
 
-        discussion = note.discussion
+        # discussion is memoized and reload doesn't clear the memoization
+        discussion = Note.find(note.id).discussion
 
         expect(discussion.notes.last.note).to eq(draft_reply.note)
         expect(discussion.resolved?).to eq(false)

spec/mailers/notify_spec.rb

@@ -1180,9 +1180,7 @@ RSpec.describe Notify do
 
         context 'when note is not on text' do
           before do
-            allow_next_instance_of(DiffDiscussion) do |instance|
-              allow(instance).to receive(:on_text?).and_return(false)
-            end
+            allow(note.discussion).to receive(:on_text?).and_return(false)
           end
 
           it 'does not include diffs with character-level highlighting' do

spec/services/draft_notes/publish_service_spec.rb

@@ -237,7 +237,8 @@ RSpec.describe DraftNotes::PublishService do
     it 'resolves the thread' do
       publish(draft: draft_note)
 
-      expect(note.discussion.resolved?).to be true
+      # discussion is memoized and reload doesn't clear the memoization
+      expect(Note.find(note.id).discussion.resolved?).to be true
     end
 
     it 'sends notifications if all threads are resolved' do

spec/services/notes/update_service_spec.rb

@@ -59,6 +59,45 @@ 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