Commit ec620639 authored by Luke Duncalfe's avatar Luke Duncalfe

Add Note#start_of_discussion? method

@engwan identified a race condition with how we were using
Discussion#new_discussion?

- A new discussion is created.
- A reply to the discussion is quickly posted.
- When a work is run #new_discussion? and would now return false
  because the Discussion has 2 notes.

As he suggested:

> I think we should remove Discussion#new_discussion? and instead have
> something like Note#start_of_discussion? which checks if the current
> note is the first note of a discussion.

This method replaces Discussion#new_discussion? with a more reliable
method of checking if a note is the start of a discussion, and updates
it use in the codebase.

In his review, @oswaldo identified a method with the same functionality
existed in `DiffNote#discussion_first_note?` already. This MR removes
that method, as DiffNote inherits from Note.

https://gitlab.com/gitlab-org/gitlab/issues/36329
parent 97f3a449
......@@ -88,10 +88,6 @@ class DiffNote < Note
line&.suggestible?
end
def discussion_first_note?
self == discussion.first_note
end
def banzai_render_context(field)
super.merge(suggestions_filter_enabled: true)
end
......@@ -108,7 +104,7 @@ class DiffNote < Note
end
def should_create_diff_file?
on_text? && note_diff_file.nil? && discussion_first_note?
on_text? && note_diff_file.nil? && start_of_discussion?
end
def fetch_diff_file
......
......@@ -139,10 +139,6 @@ class Discussion
false
end
def new_discussion?
notes.length == 1
end
def last_note
@last_note ||= notes.last
end
......
......@@ -409,6 +409,10 @@ class Note < ApplicationRecord
full_discussion || to_discussion
end
def start_of_discussion?
discussion.first_note == self
end
def part_of_discussion?
!to_discussion.individual_note?
end
......
......@@ -4,7 +4,7 @@ module Notes
class BaseService < ::BaseService
def clear_noteable_diffs_cache(note)
if note.is_a?(DiffNote) &&
note.discussion_first_note? &&
note.start_of_discussion? &&
note.position.unfolded_diff?(project.repository)
note.noteable.diffs.clear_cache
end
......
......@@ -11,7 +11,7 @@
- if discussion.nil?
commented
- else
- if discussion.new_discussion?
- if note.start_of_discussion?
started a new
- else
commented on a
......
......@@ -7,7 +7,7 @@
<% if discussion.nil? -%>
<%= 'commented' -%>:
<% else -%>
<% if discussion.new_discussion? -%>
<% if note.start_of_discussion? -%>
<%= 'started a new discussion' -%>
<% else -%>
<%= 'commented on a discussion' -%>
......
......@@ -17,7 +17,7 @@ module EE
private
def create_design_discussion_system_note?
note && note.for_design? && note.discussion.new_discussion?
note && note.for_design? && note.start_of_discussion?
end
end
end
......
......@@ -990,7 +990,8 @@ describe Notify do
end
context 'when a comment on an existing discussion' do
let!(:second_note) { create(model, author: note_author, noteable: nil, in_reply_to: note) }
let(:first_note) { create_note }
let(:note) { create(model, author: note_author, noteable: nil, in_reply_to: first_note) }
it 'contains an introduction' do
is_expected.to have_body_text 'commented on a'
......@@ -1000,7 +1001,11 @@ describe Notify do
describe 'on a commit' do
let(:commit) { project.commit }
let(:note) { create(:discussion_note_on_commit, commit_id: commit.id, project: project, author: note_author) }
let(:note) { create_note }
def create_note
create(:discussion_note_on_commit, commit_id: commit.id, project: project, author: note_author)
end
before do
allow(note).to receive(:noteable).and_return(commit)
......@@ -1027,9 +1032,13 @@ describe Notify do
end
describe 'on a merge request' do
let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, author: note_author) }
let(:note) { create_note }
let(:note_on_merge_request_path) { project_merge_request_path(project, merge_request, anchor: "note_#{note.id}") }
def create_note
create(:discussion_note_on_merge_request, noteable: merge_request, project: project, author: note_author)
end
before do
allow(note).to receive(:noteable).and_return(merge_request)
end
......@@ -1055,9 +1064,13 @@ describe Notify do
end
describe 'on an issue' do
let(:note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: note_author) }
let(:note) { create_note }
let(:note_on_issue_path) { project_issue_path(project, issue, anchor: "note_#{note.id}") }
def create_note
create(:discussion_note_on_issue, noteable: issue, project: project, author: note_author)
end
before do
allow(note).to receive(:noteable).and_return(issue)
end
......@@ -1134,7 +1147,8 @@ describe Notify do
end
context 'when a comment on an existing discussion' do
let!(:second_note) { create(model, author: note_author, noteable: nil, in_reply_to: note) }
let(:first_note) { create(model) }
let(:note) { create(model, author: note_author, noteable: nil, in_reply_to: first_note) }
it 'contains an introduction' do
is_expected.to have_body_text 'commented on a discussion on'
......
......@@ -456,6 +456,19 @@ describe Note do
end
end
describe '#start_of_discussion?' do
let_it_be(:note) { create(:discussion_note_on_merge_request) }
let_it_be(:reply) { create(:discussion_note_on_merge_request, in_reply_to: note) }
it 'returns true when note is the start of a discussion' do
expect(note).to be_start_of_discussion
end
it 'returns false when note is a reply' do
expect(reply).not_to be_start_of_discussion
end
end
describe '.find_discussion' do
let!(:note) { create(:discussion_note_on_merge_request) }
let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note) }
......
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