diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-03-10 04:29:11 +0300 |
---|---|---|
committer | Luke "Jared" Bennett <lbennett@gitlab.com> | 2017-04-05 19:44:14 +0300 |
commit | 08bbb9fce66cb46d3262e6cd4c4379b59f065be0 (patch) | |
tree | 159eeb7ca43419f29926d6e77637db18bddd20a9 /app/models | |
parent | 8bdfee8ba5fb0a8f48501e63274c8f9ce5708007 (diff) |
Add option to start a new discussion on an MR
Diffstat (limited to 'app/models')
-rw-r--r-- | app/models/commit.rb | 5 | ||||
-rw-r--r-- | app/models/commit_discussion.rb | 9 | ||||
-rw-r--r-- | app/models/concerns/note_on_diff.rb | 8 | ||||
-rw-r--r-- | app/models/concerns/noteable.rb | 17 | ||||
-rw-r--r-- | app/models/concerns/resolvable_note.rb | 61 | ||||
-rw-r--r-- | app/models/diff_discussion.rb | 69 | ||||
-rw-r--r-- | app/models/diff_note.rb | 115 | ||||
-rw-r--r-- | app/models/discussion.rb | 152 | ||||
-rw-r--r-- | app/models/discussion_note.rb | 9 | ||||
-rw-r--r-- | app/models/individual_note_discussion.rb | 13 | ||||
-rw-r--r-- | app/models/issue.rb | 1 | ||||
-rw-r--r-- | app/models/legacy_diff_discussion.rb | 21 | ||||
-rw-r--r-- | app/models/legacy_diff_note.rb | 10 | ||||
-rw-r--r-- | app/models/merge_request.rb | 37 | ||||
-rw-r--r-- | app/models/note.rb | 112 | ||||
-rw-r--r-- | app/models/sent_notification.rb | 63 | ||||
-rw-r--r-- | app/models/simple_discussion.rb | 9 | ||||
-rw-r--r-- | app/models/snippet.rb | 1 |
18 files changed, 429 insertions, 283 deletions
diff --git a/app/models/commit.rb b/app/models/commit.rb index ce92cc369ad..5c452f78546 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -2,6 +2,7 @@ class Commit extend ActiveModel::Naming include ActiveModel::Conversion + include Noteable include Participable include Mentionable include Referable @@ -203,6 +204,10 @@ class Commit project.notes.for_commit_id(self.id) end + def discussion_notes + notes.non_diff_notes + end + def notes_with_associations notes.includes(:author) end diff --git a/app/models/commit_discussion.rb b/app/models/commit_discussion.rb new file mode 100644 index 00000000000..bcca3155335 --- /dev/null +++ b/app/models/commit_discussion.rb @@ -0,0 +1,9 @@ +class CommitDiscussion < Discussion + def self.override_discussion_id(note) + discussion_id(note) + end + + def potentially_resolvable? + false + end +end diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb index b8dd27a7afe..9ce40f329d8 100644 --- a/app/models/concerns/note_on_diff.rb +++ b/app/models/concerns/note_on_diff.rb @@ -24,12 +24,4 @@ module NoteOnDiff def diff_attributes raise NotImplementedError end - - def can_be_award_emoji? - false - end - - def to_discussion - Discussion.new([self]) - end end diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb new file mode 100644 index 00000000000..7900af6aaac --- /dev/null +++ b/app/models/concerns/noteable.rb @@ -0,0 +1,17 @@ +module Noteable + def discussion_notes + notes + end + + delegate :find_discussion, :find_original_discussion, to: :discussion_notes + + def discussions + @discussions ||= discussion_notes + .inc_relations_for_view + .discussions(self) + end + + def grouped_diff_discussions + notes.inc_relations_for_view.grouped_diff_discussions + end +end diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb new file mode 100644 index 00000000000..eecb77ebf80 --- /dev/null +++ b/app/models/concerns/resolvable_note.rb @@ -0,0 +1,61 @@ +module ResolvableNote + extend ActiveSupport::Concern + + included do + belongs_to :resolved_by, class_name: "User" + + validates :resolved_by, presence: true, if: :resolved? + + # Keep this scope in sync with the logic in `#resolvable?` in `Note` subclasses that are resolvable + scope :resolvable, -> { where(type: %w(DiffNote DiscussionNote)).user.where(noteable_type: 'MergeRequest') } + scope :resolved, -> { resolvable.where.not(resolved_at: nil) } + scope :unresolved, -> { resolvable.where(resolved_at: nil) } + end + + module ClassMethods + # 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 + + # If you update this method remember to also update the scope `resolvable` + def resolvable? + to_discussion.potentially_resolvable? && !system? + end + + def resolved? + return false unless resolvable? + + self.resolved_at.present? + end + + def to_be_resolved? + resolvable? && !resolved? + end + + # If you update this method remember to also update `.resolve!` + def resolve!(current_user) + return unless resolvable? + return if resolved? + + self.resolved_at = Time.now + self.resolved_by = current_user + save! + end + + # If you update this method remember to also update `.unresolve!` + def unresolve! + return unless resolvable? + return unless resolved? + + self.resolved_at = nil + self.resolved_by = nil + save! + end +end diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb new file mode 100644 index 00000000000..9a934c7520c --- /dev/null +++ b/app/models/diff_discussion.rb @@ -0,0 +1,69 @@ +class DiffDiscussion < Discussion + NUMBER_OF_TRUNCATED_DIFF_LINES = 16 + + delegate :line_code, + :original_line_code, + :diff_file, + :for_line?, + :active?, + + to: :first_note + + delegate :blob, + :highlighted_diff_lines, + :diff_lines, + + to: :diff_file, + allow_nil: true + + def self.build_discussion_id(note) + [*super(note), *unique_position_identifier(note)] + end + + def self.build_original_discussion_id(note) + [*Discussion.build_discussion_id(note), *note.original_position.key] + end + + def self.unique_position_identifier(note) + note.position.key + end + + def diff_discussion? + true + end + + def legacy_diff_discussion? + false + end + + def active? + return @active if @active.present? + + @active = first_note.active? + end + MEMOIZED_VALUES << :active + + def reply_attributes + super.merge(first_note.diff_attributes) + end + + # Returns an array of at most 16 highlighted lines above a diff note + def truncated_diff_lines(highlight: true) + lines = highlight ? highlighted_diff_lines : diff_lines + prev_lines = [] + + lines.each do |line| + if line.meta? + prev_lines.clear + else + prev_lines << line + + break if for_line?(line) + + prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES + end + end + + prev_lines + end +end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 895a91139c9..e5f437f8647 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -9,58 +9,44 @@ class DiffNote < Note validates :diff_line, presence: true validates :line_code, presence: true, line_code: true validates :noteable_type, inclusion: { in: %w(Commit MergeRequest) } - validates :resolved_by, presence: true, if: :resolved? 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 + before_validation :set_line_code # We need to do this again, because it's already in `Note`, but is affected by # `update_position` and needs to run after that. - before_validation :set_discussion_id + before_validation :set_discussion_id, if: :position_changed? after_save :keep_around_commits - 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? true end + def discussion_class(*) + DiffDiscussion + end + def diff_attributes - { position: position.to_json } + { + original_position: original_position.to_json, + position: position.to_json, + } end - def position=(new_position) - if new_position.is_a?(String) - new_position = JSON.parse(new_position) rescue nil - end + %i(original_position= position=).each do |meth| + define_method meth do |new_position| + if new_position.is_a?(String) + new_position = JSON.parse(new_position) rescue nil + end - if new_position.is_a?(Hash) - new_position = new_position.with_indifferent_access - new_position = Gitlab::Diff::Position.new(new_position) - end + if new_position.is_a?(Hash) + new_position = new_position.with_indifferent_access + new_position = Gitlab::Diff::Position.new(new_position) + end - super(new_position) + super(new_position) + end end def diff_file @@ -88,43 +74,6 @@ class DiffNote < Note 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 - - def resolved? - return false unless resolvable? - - 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? - - self.resolved_at = Time.now - self.resolved_by = current_user - save! - end - - # If you update this method remember to also update `.unresolve!` - def unresolve! - return unless resolvable? - return unless resolved? - - self.resolved_at = nil - self.resolved_by = nil - save! - end - - def discussion - return unless resolvable? - - self.noteable.find_diff_discussion(self.discussion_id) - end - private def supported? @@ -140,33 +89,13 @@ class DiffNote < Note end def set_original_position - self.original_position = self.position.dup + self.original_position = self.position.dup unless self.original_position&.complete? end def set_line_code self.line_code = self.position.line_code(self.project.repository) end - def ensure_original_discussion_id - return unless self.persisted? - return if self.original_discussion_id - - set_original_discussion_id - update_column(:original_discussion_id, self.original_discussion_id) - end - - def set_original_discussion_id - self.original_discussion_id = Digest::SHA1.hexdigest(build_original_discussion_id) - end - - def build_discussion_id - self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position) - end - - def build_original_discussion_id - self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position) - end - def update_position return unless supported? return if for_commit? diff --git a/app/models/discussion.rb b/app/models/discussion.rb index bbe813db823..314aea2c63a 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -1,5 +1,5 @@ class Discussion - NUMBER_OF_TRUNCATED_DIFF_LINES = 16 + MEMOIZED_VALUES = [] # rubocop:disable Style/MutableConstant attr_reader :notes @@ -11,12 +11,6 @@ class Discussion :for_commit?, :for_merge_request?, - :line_code, - :original_line_code, - :diff_file, - :for_line?, - :active?, - to: :first_note delegate :resolved_at, @@ -25,29 +19,46 @@ class Discussion to: :last_resolved_note, allow_nil: true - delegate :blob, - :highlighted_diff_lines, - :diff_lines, + def self.build(notes, noteable = nil) + notes.first.discussion_class(noteable).new(notes, noteable) + end - to: :diff_file, - allow_nil: true + def self.build_collection(notes, noteable = nil) + notes.group_by { |n| n.discussion_id(noteable) }.values.map { |notes| build(notes, noteable) } + end - def self.for_notes(notes) - notes.group_by(&:discussion_id).values.map { |notes| new(notes) } + def self.discussion_id(note) + Digest::SHA1.hexdigest(build_discussion_id(note).join("-")) end - def self.for_diff_notes(notes) - notes.group_by(&:line_code).values.map { |notes| new(notes) } + # Optionally override the discussion ID at runtime depending on circumstances + def self.override_discussion_id(note) + nil end - def initialize(notes) - @notes = notes + def self.build_discussion_id(note) + noteable_id = note.noteable_id || note.commit_id + [:discussion, note.noteable_type.try(:underscore), noteable_id] end - def last_resolved_note - return unless resolved? + def self.original_discussion_id(note) + original_discussion_id = build_original_discussion_id(note) + if original_discussion_id + Digest::SHA1.hexdigest(original_discussion_id.join("-")) + else + note.discussion_id + end + end - @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last + # Optionally build a separate original discussion ID that will never change, + # if the main discussion ID _can_ change, like in the case of DiffDiscussion. + def self.build_original_discussion_id(note) + nil + end + + def initialize(notes, noteable = nil) + @notes = notes + @noteable = noteable end def last_updated_at @@ -59,42 +70,64 @@ class Discussion end def id - first_note.discussion_id + first_note.discussion_id(noteable) end alias_method :to_param, :id + def original_id + first_note.original_discussion_id + end + def diff_discussion? - first_note.diff_note? + false + end + + def render_as_individual_notes? + false end - def legacy_diff_discussion? - notes.any?(&:legacy_diff_note?) + def potentially_resolvable? + first_note.for_merge_request? end def resolvable? return @resolvable if @resolvable.present? - @resolvable = diff_discussion? && notes.any?(&:resolvable?) + @resolvable = potentially_resolvable? && notes.any?(&:resolvable?) end + MEMOIZED_VALUES << :resolvable def resolved? return @resolved if @resolved.present? @resolved = resolvable? && notes.none?(&:to_be_resolved?) end + MEMOIZED_VALUES << :resolved def first_note - @first_note ||= @notes.first + @first_note ||= notes.first end + MEMOIZED_VALUES << :first_note def first_note_to_resolve - @first_note_to_resolve ||= notes.detect(&:to_be_resolved?) + return unless resolvable? + + @first_note_to_resolve ||= notes.find(&:to_be_resolved?) end + MEMOIZED_VALUES << :first_note_to_resolve + + def last_resolved_note + return unless resolved? + + @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last + end + MEMOIZED_VALUES << :last_resolved_note def last_note - @last_note ||= @notes.last + @last_note ||= notes.last end + MEMOIZED_VALUES << :last_note def resolved_notes notes.select(&:resolved?) @@ -124,25 +157,12 @@ class Discussion update { |notes| notes.unresolve! } end - def for_target?(target) - self.noteable == target && !diff_discussion? - end - - def active? - return @active if @active.present? - - @active = first_note.active? - end - def collapsed? - return false unless diff_discussion? - if resolvable? # New diff discussions only disappear once they are marked resolved resolved? else - # Old diff discussions disappear once they become outdated - !active? + false end end @@ -151,52 +171,22 @@ class Discussion end def reply_attributes - data = { - noteable_type: first_note.noteable_type, - noteable_id: first_note.noteable_id, - commit_id: first_note.commit_id, - discussion_id: self.id, - } - - if diff_discussion? - data[:note_type] = first_note.type - - data.merge!(first_note.diff_attributes) - end - - data - end - - # Returns an array of at most 16 highlighted lines above a diff note - def truncated_diff_lines(highlight: true) - lines = highlight ? highlighted_diff_lines : diff_lines - prev_lines = [] - - lines.each do |line| - if line.meta? - prev_lines.clear - else - prev_lines << line - - break if for_line?(line) - - prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES - end - end - - prev_lines + first_note.slice(:type, :noteable_type, :noteable_id, :commit_id) end private def update - notes_relation = DiffNote.where(id: notes.map(&:id)).fresh + # Do not select `Note.resolvable`, so that system notes remain in the collection + notes_relation = Note.where(id: notes.map(&:id)) + yield(notes_relation) # Set the notes array to the updated notes - @notes = notes_relation.to_a + @notes = notes_relation.fresh.to_a - # Reset the memoized values - @last_resolved_note = @resolvable = @resolved = @first_note = @last_note = nil + MEMOIZED_VALUES.each do |var| + instance_variable_set(:"@#{var}", nil) + end end end diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb new file mode 100644 index 00000000000..fc168ae74a9 --- /dev/null +++ b/app/models/discussion_note.rb @@ -0,0 +1,9 @@ +class DiscussionNote < Note + NOTEABLE_TYPES = %w(MergeRequest).freeze + + validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } + + def discussion_class(*) + SimpleDiscussion + end +end diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb new file mode 100644 index 00000000000..8c82d217126 --- /dev/null +++ b/app/models/individual_note_discussion.rb @@ -0,0 +1,13 @@ +class IndividualNoteDiscussion < Discussion + def self.build_discussion_id(note) + [*super(note), SecureRandom.hex] + end + + def potentially_resolvable? + false + end + + def render_as_individual_notes? + true + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index 472796df9df..5010b6e1edf 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -3,6 +3,7 @@ require 'carrierwave/orm/activerecord' class Issue < ActiveRecord::Base include InternalId include Issuable + include Noteable include Referable include Sortable include Spammable diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb new file mode 100644 index 00000000000..e13f17abdbe --- /dev/null +++ b/app/models/legacy_diff_discussion.rb @@ -0,0 +1,21 @@ +class LegacyDiffDiscussion < DiffDiscussion + def self.unique_position_identifier(note) + note.line_code + end + + def self.build_original_discussion_id(note) + Discussion.build_original_discussion_id(note) + end + + def legacy_diff_discussion? + true + end + + def potentially_resolvable? + false + end + + def collapsed? + !active? + end +end diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 40277a9b139..4952a1ce2ca 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -7,10 +7,8 @@ class LegacyDiffNote < Note before_create :set_diff - class << self - def build_discussion_id(noteable_type, noteable_id, line_code) - [super(noteable_type, noteable_id), line_code].join("-") - end + def discussion_class(*) + LegacyDiffDiscussion end def legacy_diff_note? @@ -119,8 +117,4 @@ class LegacyDiffNote < Note diffs = noteable.raw_diffs(Commit.max_diff_options) diffs.find { |d| d.new_path == self.diff.new_path } end - - def build_discussion_id - self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code) - end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 8d740adb771..546fd2b8e35 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1,6 +1,7 @@ class MergeRequest < ActiveRecord::Base include InternalId include Issuable + include Noteable include Referable include Sortable @@ -475,44 +476,32 @@ class MergeRequest < ActiveRecord::Base ) end - def discussions - @discussions ||= self.related_notes. - inc_relations_for_view. - fresh. - discussions - end - - def diff_discussions - @diff_discussions ||= self.notes.diff_notes.discussions - end + alias_method :discussion_notes, :related_notes def resolvable_discussions - @resolvable_discussions ||= diff_discussions.select(&:to_be_resolved?) - end - - def discussions_can_be_resolved_by?(user) - resolvable_discussions.all? { |discussion| discussion.can_resolve?(user) } - end - - def find_diff_discussion(discussion_id) - notes = self.notes.diff_notes.where(discussion_id: discussion_id).fresh.to_a - return if notes.empty? - - Discussion.new(notes) + @resolvable_discussions ||= notes.resolvable.discussions end def discussions_resolvable? - diff_discussions.any?(&:resolvable?) + resolvable_discussions.any?(&:resolvable?) end def discussions_resolved? - discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?) + discussions_resolvable? && resolvable_discussions.none?(&:to_be_resolved?) end def discussions_to_be_resolved? discussions_resolvable? && !discussions_resolved? end + def discussions_to_be_resolved + @discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?) + end + + def discussions_can_be_resolved_by?(user) + discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) } + end + def mergeable_discussions_state? return true unless project.only_allow_merge_if_all_discussions_are_resolved? diff --git a/app/models/note.rb b/app/models/note.rb index 16d66cb1427..6385747b571 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -8,6 +8,7 @@ class Note < ActiveRecord::Base include FasterCacheKeys include CacheMarkdownField include AfterCommitQueue + include ResolvableNote cache_markdown_field :note, pipeline: :note @@ -32,9 +33,6 @@ class Note < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :updated_by, class_name: "User" - # Only used by DiffNote, but defined here so that it can be used in `Note.includes` - belongs_to :resolved_by, class_name: "User" - has_many :todos, dependent: :destroy has_many :events, as: :target, dependent: :destroy has_one :system_note_metadata @@ -54,6 +52,7 @@ class Note < ActiveRecord::Base validates :noteable_id, presence: true, unless: [:for_commit?, :importing?] validates :commit_id, presence: true, if: :for_commit? validates :author, presence: true + validates :discussion_id, :original_discussion_id, presence: true, format: { with: /\A\h{40}\z/ } validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note| unless note.noteable.try(:project) == note.project @@ -76,7 +75,7 @@ class Note < ActiveRecord::Base end scope :diff_notes, ->{ where(type: %w(LegacyDiffNote DiffNote)) } - scope :non_diff_notes, ->{ where(type: ['Note', nil]) } + scope :non_diff_notes, ->{ where(type: ['Note', 'DiscussionNote', nil]) } scope :with_associations, -> do # FYI noteable cannot be loaded for LegacyDiffNote for commits @@ -84,9 +83,9 @@ class Note < ActiveRecord::Base project: [:project_members, { group: [:group_members] }]) end - after_initialize :ensure_discussion_id + after_initialize :ensure_discussion_id, :ensure_original_discussion_id before_validation :nullify_blank_type, :nullify_blank_line_code - before_validation :set_discussion_id + before_validation :set_discussion_id, :set_original_discussion_id, on: :create after_save :keep_around_commit, unless: :for_personal_snippet? after_save :expire_etag_cache @@ -95,22 +94,31 @@ class Note < ActiveRecord::Base ActiveModel::Name.new(self, nil, 'note') end - def build_discussion_id(noteable_type, noteable_id) - [:discussion, noteable_type.try(:underscore), noteable_id].join("-") + def discussions(noteable = nil) + Discussion.build_collection(fresh, noteable) end - def discussion_id(*args) - Digest::SHA1.hexdigest(build_discussion_id(*args)) + def find_original_discussion(discussion_id) + note = find_by(original_discussion_id: discussion_id) + return unless note + + note.to_discussion end - def discussions - Discussion.for_notes(fresh) + def find_discussion(discussion_id) + notes = where(discussion_id: discussion_id).fresh.to_a + return if notes.empty? + + Discussion.build(notes) end def grouped_diff_discussions - active_notes = diff_notes.fresh.select(&:active?) - Discussion.for_diff_notes(active_notes). - map { |d| [d.line_code, d] }.to_h + diff_notes. + fresh. + select(&:active?). + group_by(&:line_code). + map { |line_code, notes| [line_code, DiffDiscussion.build(notes)] }. + to_h end def count_for_collection(ids, type) @@ -121,7 +129,7 @@ class Note < ActiveRecord::Base end def cross_reference? - system && SystemNoteService.cross_reference?(note) + system? && SystemNoteService.cross_reference?(note) end def diff_note? @@ -140,18 +148,6 @@ class Note < ActiveRecord::Base true end - def resolvable? - false - end - - def resolved? - false - end - - def to_be_resolved? - resolvable? && !resolved? - end - def max_attachment_size current_application_settings.max_attachment_size.megabytes.to_i end @@ -228,7 +224,7 @@ class Note < ActiveRecord::Base end def can_be_award_emoji? - noteable.is_a?(Awardable) + noteable.is_a?(Awardable) && !part_of_discussion? end def contains_emoji_only? @@ -239,6 +235,42 @@ class Note < ActiveRecord::Base for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore end + def can_be_discussion_note? + DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type) + end + + def discussion_class(noteable = nil) + # When commit notes are rendered on an MR's Discussion page, they are + # displayed in one discussion instead of individually + if noteable && noteable != self.noteable && for_commit? + CommitDiscussion + else + IndividualNoteDiscussion + end + end + + def discussion_id(noteable = nil) + discussion_class(noteable).override_discussion_id(self) || super() + end + + # Returns a discussion containing just this note + def to_discussion(noteable = nil) + Discussion.build([self], noteable) + end + + # Returns the entire discussion this note is part of + def discussion + if part_of_discussion? + self.noteable.notes.find_discussion(self.discussion_id) + else + to_discussion + end + end + + def part_of_discussion? + !to_discussion.render_as_individual_notes? + end + private def keep_around_commit @@ -264,17 +296,21 @@ class Note < ActiveRecord::Base end def set_discussion_id - self.discussion_id = Digest::SHA1.hexdigest(build_discussion_id) + self.discussion_id ||= discussion_class.discussion_id(self) end - def build_discussion_id - if for_merge_request? - # Notes on merge requests are always in a discussion of their own, - # so we generate a unique discussion ID. - [:discussion, :note, SecureRandom.hex].join("-") - else - self.class.build_discussion_id(noteable_type, noteable_id || commit_id) - end + def ensure_original_discussion_id + return unless self.persisted? + # Needed in case the SELECT statement doesn't ask for `original_discussion_id` + return unless self.has_attribute?(:original_discussion_id) + return if self.original_discussion_id + + set_original_discussion_id + update_column(:original_discussion_id, self.original_discussion_id) + end + + def set_original_discussion_id + self.original_discussion_id = discussion_class.original_discussion_id(self) end def expire_etag_cache diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index f4bcb49b34d..6770af6fffd 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -5,10 +5,11 @@ class SentNotification < ActiveRecord::Base belongs_to :noteable, polymorphic: true belongs_to :recipient, class_name: "User" - validates :project, :recipient, :reply_key, presence: true - validates :reply_key, uniqueness: true + validates :project, :recipient, presence: true + validates :reply_key, presence: true, uniqueness: true validates :noteable_id, presence: true, unless: :for_commit? validates :commit_id, presence: true, if: :for_commit? + validates :in_reply_to_discussion_id, format: { with: /\A\h{40}\z/, allow_nil: true } validate :note_valid after_save :keep_around_commit @@ -34,23 +35,20 @@ class SentNotification < ActiveRecord::Base end attrs.reverse_merge!( - project: noteable.project, - noteable_type: noteable.class.name, - noteable_id: noteable_id, - commit_id: commit_id, - recipient_id: recipient_id, - reply_key: reply_key + project: noteable.project, + recipient_id: recipient_id, + reply_key: reply_key, + + noteable_type: noteable.class.name, + noteable_id: noteable_id, + commit_id: commit_id, ) create(attrs) end def record_note(note, recipient_id, reply_key, attrs = {}) - if note.diff_note? - attrs[:note_type] = note.type - - attrs.merge!(note.diff_attributes) - end + attrs[:in_reply_to_discussion_id] = note.original_discussion_id record(note.noteable, recipient_id, reply_key, attrs) end @@ -89,31 +87,34 @@ class SentNotification < ActiveRecord::Base self.reply_key end - def note_attributes - { - project: self.project, - author: self.recipient, - type: self.note_type, - noteable_type: self.noteable_type, - noteable_id: self.noteable_id, - commit_id: self.commit_id, - line_code: self.line_code, - position: self.position.to_json + def note_params + attrs = { + noteable_type: self.noteable_type, + noteable_id: self.noteable_id, + commit_id: self.commit_id } - end - def create_note(note) - Notes::CreateService.new( - self.project, - self.recipient, - self.note_attributes.merge(note: note) - ).execute + if self.in_reply_to_discussion_id.present? + attrs[:in_reply_to_discussion_id] = self.in_reply_to_discussion_id + else + attrs.merge!( + type: self.note_type, + + # LegacyDiffNote + line_code: self.line_code, + + # DiffNote + position: self.position.to_json + ) + end + + attrs end private def note_valid - Note.new(note_attributes.merge(note: "Test")).valid? + Notes::BuildService.new(self.project, self.recipient, note_params.merge(note: 'Test')).execute.valid? end def keep_around_commit diff --git a/app/models/simple_discussion.rb b/app/models/simple_discussion.rb new file mode 100644 index 00000000000..a5ef5c565e7 --- /dev/null +++ b/app/models/simple_discussion.rb @@ -0,0 +1,9 @@ +class SimpleDiscussion < Discussion + def self.build_discussion_id(note) + [*super(note), SecureRandom.hex] + end + + def reply_attributes + super.merge(discussion_id: self.id) + end +end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 30aca62499c..380835707e8 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -2,6 +2,7 @@ class Snippet < ActiveRecord::Base include Gitlab::VisibilityLevel include Linguist::BlobHelper include CacheMarkdownField + include Noteable include Participable include Referable include Sortable |