From 08bbb9fce66cb46d3262e6cd4c4379b59f065be0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 9 Mar 2017 19:29:11 -0600 Subject: Add option to start a new discussion on an MR --- app/models/note.rb | 112 +++++++++++++++++++++++++++++++++++------------------ 1 file changed, 74 insertions(+), 38 deletions(-) (limited to 'app/models/note.rb') 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 -- cgit v1.2.1