Commit e708b91d authored by Douwe Maan's avatar Douwe Maan Committed by Ruben Davila
Browse files

Merge branch...

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
parent 629e21c0
......@@ -139,6 +139,7 @@ v 8.11.5
- Optimize branch lookups and force a repository reload for Repository#find_branch. !6087
- Fix member expiration date picker after update. !6184
- Fix suggested colors options for new labels in the admin area. !6138
- Optimize discussion notes resolving and unresolving
- Fix GitLab import button
- Fix confidential issues being exposed as public using gitlab.com export
- Remove gitorious from import_sources. !6180
......
......@@ -13,6 +13,11 @@ class DiffNote < Note
validate :positions_complete
validate :verify_supported
# Keep this scope in sync with the logic in `#resolvable?`
scope :resolvable, -> { user.where(noteable_type: 'MergeRequest') }
scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
scope :unresolved, -> { resolvable.where(resolved_at: nil) }
after_initialize :ensure_original_discussion_id
before_validation :set_original_position, :update_position, on: :create
before_validation :set_line_code, :set_original_discussion_id
......@@ -25,6 +30,16 @@ class << self
def build_discussion_id(noteable_type, noteable_id, position)
[super(noteable_type, noteable_id), *position.key].join("-")
end
# This method must be kept in sync with `#resolve!`
def resolve!(current_user)
unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
end
# This method must be kept in sync with `#unresolve!`
def unresolve!
resolved.update_all(resolved_at: nil, resolved_by_id: nil)
end
end
def new_diff_note?
......@@ -73,6 +88,7 @@ def active?(diff_refs = nil)
self.position.diff_refs == diff_refs
end
# If you update this method remember to also update the scope `resolvable`
def resolvable?
!system? && for_merge_request?
end
......@@ -83,6 +99,7 @@ def resolved?
self.resolved_at.present?
end
# If you update this method remember to also update `.resolve!`
def resolve!(current_user)
return unless resolvable?
return if resolved?
......@@ -92,6 +109,7 @@ def resolve!(current_user)
save!
end
# If you update this method remember to also update `.unresolve!`
def unresolve!
return unless resolvable?
return unless resolved?
......
class Discussion
NUMBER_OF_TRUNCATED_DIFF_LINES = 16
attr_reader :first_note, :last_note, :notes
attr_reader :notes
delegate :created_at,
:project,
......@@ -36,8 +36,6 @@ def self.for_diff_notes(notes)
end
def initialize(notes)
@first_note = notes.first
@last_note = notes.last
@notes = notes
end
......@@ -70,17 +68,25 @@ def legacy_diff_discussion?
end
def resolvable?
return @resolvable if defined?(@resolvable)
return @resolvable if @resolvable.present?
@resolvable = diff_discussion? && notes.any?(&:resolvable?)
end
def resolved?
return @resolved if defined?(@resolved)
return @resolved if @resolved.present?
@resolved = resolvable? && notes.none?(&:to_be_resolved?)
end
def first_note
@first_note ||= @notes.first
end
def last_note
@last_note ||= @notes.last
end
def resolved_notes
notes.select(&:resolved?)
end
......@@ -100,17 +106,13 @@ def can_resolve?(current_user)
def resolve!(current_user)
return unless resolvable?
notes.each do |note|
note.resolve!(current_user) if note.resolvable?
end
update { |notes| notes.resolve!(current_user) }
end
def unresolve!
return unless resolvable?
notes.each do |note|
note.unresolve! if note.resolvable?
end
update { |notes| notes.unresolve! }
end
def for_target?(target)
......@@ -118,7 +120,7 @@ def for_target?(target)
end
def active?
return @active if defined?(@active)
return @active if @active.present?
@active = first_note.active?
end
......@@ -174,4 +176,17 @@ def truncated_diff_lines
prev_lines
end
private
def update
notes_relation = DiffNote.where(id: notes.map(&:id)).fresh
yield(notes_relation)
# Set the notes array to the updated notes
@notes = notes_relation.to_a
# Reset the memoized values
@last_resolved_note = @resolvable = @resolved = @first_note = @last_note = nil
end
end
......@@ -28,6 +28,11 @@
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
......
......@@ -31,6 +31,43 @@
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
......
......@@ -238,27 +238,19 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment