Merge branch '21109-discussion-resolve-runs-a-single-update-query-per-note-but-should-run-a-single-update-query-for-all-notes-instead' into 'master'

Optimize discussion notes resolving and unresolving

## What does this MR do?

Optimize discussion notes resolving and unresolving

## Are there points in the code the reviewer needs to double check?

Some changes had to be made to the discussion spec to account for the fact that notes are not  individually updated now. I only focused on adapting them for the purpose of the regression fix, but admittedly they could be further improved in readability.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [ ] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes #21109

See merge request !6141

3 files changed, 79 insertions, 59 deletions

diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 83e38095feb..6919002dedc 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -28,6 +28,11 @@ FactoryGirl.define do
           diff_refs: noteable.diff_refs
         )
       end
+
+      trait :resolved do
+        resolved_at { Time.now }
+        resolved_by { create(:user) }
+      end
     end
 
     factory :diff_note_on_commit, traits: [:on_commit], class: DiffNote do
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 6a640474cfe..3db5937a4f3 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -31,6 +31,43 @@ describe DiffNote, models: true do
 
   subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
 
+  describe ".resolve!" do
+    let(:current_user) { create(:user) }
+    let!(:commit_note) { create(:diff_note_on_commit) }
+    let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) }
+    let!(:unresolved_note) { create(:diff_note_on_merge_request) }
+
+    before do
+      described_class.resolve!(current_user)
+
+      commit_note.reload
+      resolved_note.reload
+      unresolved_note.reload
+    end
+
+    it 'resolves only the resolvable, not yet resolved notes' do
+      expect(commit_note.resolved_at).to be_nil
+      expect(resolved_note.resolved_by).not_to eq(current_user)
+      expect(unresolved_note.resolved_at).not_to be_nil
+      expect(unresolved_note.resolved_by).to eq(current_user)
+    end
+  end
+
+  describe ".unresolve!" do
+    let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) }
+
+    before do
+      described_class.unresolve!
+
+      resolved_note.reload
+    end
+
+    it 'unresolves the resolved notes' do
+      expect(resolved_note.resolved_by).to be_nil
+      expect(resolved_note.resolved_at).to be_nil
+    end
+  end
+
   describe "#position=" do
     context "when provided a string" do
       it "sets the position" do
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index 179f2e73662..0142706d140 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -238,27 +238,19 @@ describe Discussion, model: true do
 
     context "when resolvable" do
       let(:user) { create(:user) }
+      let(:second_note) { create(:diff_note_on_commit) } # unresolvable
 
       before do
         allow(subject).to receive(:resolvable?).and_return(true)
-
-        allow(first_note).to receive(:resolvable?).and_return(true)
-        allow(second_note).to receive(:resolvable?).and_return(false)
-        allow(third_note).to receive(:resolvable?).and_return(true)
       end
 
       context "when all resolvable notes are resolved" do
         before do
           first_note.resolve!(user)
           third_note.resolve!(user)
-        end
 
-        it "calls resolve! on every resolvable note" do
-          expect(first_note).to receive(:resolve!).with(current_user)
-          expect(second_note).not_to receive(:resolve!)
-          expect(third_note).to receive(:resolve!).with(current_user)
-
-          subject.resolve!(current_user)
+          first_note.reload
+          third_note.reload
         end
 
         it "doesn't change resolved_at on the resolved notes" do
@@ -309,46 +301,44 @@ describe Discussion, model: true do
           first_note.resolve!(user)
         end
 
-        it "calls resolve! on every resolvable note" do
-          expect(first_note).to receive(:resolve!).with(current_user)
-          expect(second_note).not_to receive(:resolve!)
-          expect(third_note).to receive(:resolve!).with(current_user)
-
-          subject.resolve!(current_user)
-        end
-
         it "doesn't change resolved_at on the resolved note" do
           expect(first_note.resolved_at).not_to be_nil
 
-          expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_at }
+          expect { subject.resolve!(current_user) }.
+            not_to change { first_note.reload.resolved_at }
         end
 
         it "doesn't change resolved_by on the resolved note" do
           expect(first_note.resolved_by).to eq(user)
 
-          expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_by }
+          expect { subject.resolve!(current_user) }.
+            not_to change { first_note.reload && first_note.resolved_by }
         end
 
         it "doesn't change the resolved state on the resolved note" do
           expect(first_note.resolved?).to be true
 
-          expect { subject.resolve!(current_user) }.not_to change { first_note.resolved? }
+          expect { subject.resolve!(current_user) }.
+            not_to change { first_note.reload && first_note.resolved? }
         end
 
         it "sets resolved_at on the unresolved note" do
           subject.resolve!(current_user)
+          third_note.reload
 
           expect(third_note.resolved_at).not_to be_nil
         end
 
         it "sets resolved_by on the unresolved note" do
           subject.resolve!(current_user)
+          third_note.reload
 
           expect(third_note.resolved_by).to eq(current_user)
         end
 
         it "marks the unresolved note as resolved" do
           subject.resolve!(current_user)
+          third_note.reload
 
           expect(third_note.resolved?).to be true
         end
@@ -373,16 +363,10 @@ describe Discussion, model: true do
       end
 
       context "when no resolvable notes are resolved" do
-        it "calls resolve! on every resolvable note" do
-          expect(first_note).to receive(:resolve!).with(current_user)
-          expect(second_note).not_to receive(:resolve!)
-          expect(third_note).to receive(:resolve!).with(current_user)
-
-          subject.resolve!(current_user)
-        end
-
         it "sets resolved_at on the unresolved notes" do
           subject.resolve!(current_user)
+          first_note.reload
+          third_note.reload
 
           expect(first_note.resolved_at).not_to be_nil
           expect(third_note.resolved_at).not_to be_nil
@@ -390,6 +374,8 @@ describe Discussion, model: true do
 
         it "sets resolved_by on the unresolved notes" do
           subject.resolve!(current_user)
+          first_note.reload
+          third_note.reload
 
           expect(first_note.resolved_by).to eq(current_user)
           expect(third_note.resolved_by).to eq(current_user)
@@ -397,6 +383,8 @@ describe Discussion, model: true do
 
         it "marks the unresolved notes as resolved" do
           subject.resolve!(current_user)
+          first_note.reload
+          third_note.reload
 
           expect(first_note.resolved?).to be true
           expect(third_note.resolved?).to be true
@@ -404,18 +392,24 @@ describe Discussion, model: true do
 
         it "sets resolved_at" do
           subject.resolve!(current_user)
+          first_note.reload
+          third_note.reload
 
           expect(subject.resolved_at).not_to be_nil
         end
 
         it "sets resolved_by" do
           subject.resolve!(current_user)
+          first_note.reload
+          third_note.reload
 
           expect(subject.resolved_by).to eq(current_user)
         end
 
         it "marks as resolved" do
           subject.resolve!(current_user)
+          first_note.reload
+          third_note.reload
 
           expect(subject.resolved?).to be true
         end
@@ -451,16 +445,10 @@ describe Discussion, model: true do
           third_note.resolve!(user)
         end
 
-        it "calls unresolve! on every resolvable note" do
-          expect(first_note).to receive(:unresolve!)
-          expect(second_note).not_to receive(:unresolve!)
-          expect(third_note).to receive(:unresolve!)
-
-          subject.unresolve!
-        end
-
         it "unsets resolved_at on the resolved notes" do
           subject.unresolve!
+          first_note.reload
+          third_note.reload
 
           expect(first_note.resolved_at).to be_nil
           expect(third_note.resolved_at).to be_nil
@@ -468,6 +456,8 @@ describe Discussion, model: true do
 
         it "unsets resolved_by on the resolved notes" do
           subject.unresolve!
+          first_note.reload
+          third_note.reload
 
           expect(first_note.resolved_by).to be_nil
           expect(third_note.resolved_by).to be_nil
@@ -475,6 +465,8 @@ describe Discussion, model: true do
 
         it "unmarks the resolved notes as resolved" do
           subject.unresolve!
+          first_note.reload
+          third_note.reload
 
           expect(first_note.resolved?).to be false
           expect(third_note.resolved?).to be false
@@ -482,12 +474,16 @@ describe Discussion, model: true do
 
         it "unsets resolved_at" do
           subject.unresolve!
+          first_note.reload
+          third_note.reload
 
           expect(subject.resolved_at).to be_nil
         end
 
         it "unsets resolved_by" do
           subject.unresolve!
+          first_note.reload
+          third_note.reload
 
           expect(subject.resolved_by).to be_nil
         end
@@ -504,40 +500,22 @@ describe Discussion, model: true do
           first_note.resolve!(user)
         end
 
-        it "calls unresolve! on every resolvable note" do
-          expect(first_note).to receive(:unresolve!)
-          expect(second_note).not_to receive(:unresolve!)
-          expect(third_note).to receive(:unresolve!)
-
-          subject.unresolve!
-        end
-
         it "unsets resolved_at on the resolved note" do
           subject.unresolve!
 
-          expect(first_note.resolved_at).to be_nil
+          expect(subject.first_note.resolved_at).to be_nil
         end
 
         it "unsets resolved_by on the resolved note" do
           subject.unresolve!
 
-          expect(first_note.resolved_by).to be_nil
+          expect(subject.first_note.resolved_by).to be_nil
         end
 
         it "unmarks the resolved note as resolved" do
           subject.unresolve!
 
-          expect(first_note.resolved?).to be false
-        end
-      end
-
-      context "when no resolvable notes are resolved" do
-        it "calls unresolve! on every resolvable note" do
-          expect(first_note).to receive(:unresolve!)
-          expect(second_note).not_to receive(:unresolve!)
-          expect(third_note).to receive(:unresolve!)
-
-          subject.unresolve!
+          expect(subject.first_note.resolved?).to be false
         end
       end
     end