summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/helpers/notes_helper.rb1
-rw-r--r--app/mailers/emails/notes.rb16
-rw-r--r--app/models/concerns/discussion_on_diff.rb1
-rw-r--r--app/models/concerns/note_on_diff.rb1
-rw-r--r--app/models/concerns/noteable.rb26
-rw-r--r--app/models/concerns/resolvable_discussion.rb25
-rw-r--r--app/models/concerns/resolvable_note.rb2
-rw-r--r--app/models/diff_discussion.rb2
-rw-r--r--app/models/discussion.rb46
-rw-r--r--app/models/discussion_note.rb2
-rw-r--r--app/models/individual_note_discussion.rb3
-rw-r--r--app/models/legacy_diff_discussion.rb5
-rw-r--r--app/models/legacy_diff_note.rb4
-rw-r--r--app/models/merge_request.rb24
-rw-r--r--app/models/note.rb4
-rw-r--r--app/models/out_of_context_discussion.rb15
-rw-r--r--app/models/sent_notification.rb2
-rw-r--r--app/models/simple_discussion.rb3
-rw-r--r--app/services/notes/build_service.rb2
-rw-r--r--app/services/notes/create_service.rb2
-rw-r--r--app/views/notify/note_merge_request_email.text.erb2
-rw-r--r--changelogs/unreleased/new-resolvable-discussion.yml4
-rw-r--r--db/migrate/20170308220217_add_index_to_note_original_discussion_id.rb18
-rw-r--r--db/schema.rb1
-rw-r--r--spec/factories/notes.rb12
-rw-r--r--spec/features/merge_requests/diff_notes_avatars_spec.rb4
-rw-r--r--spec/mailers/notify_spec.rb12
-rw-r--r--spec/mailers/previews/notify_preview.rb115
-rw-r--r--spec/models/concerns/discussion_on_diff_spec.rb24
-rw-r--r--spec/models/concerns/noteable_spec.rb232
-rw-r--r--spec/models/concerns/resolvable_discussion_spec.rb5
-rw-r--r--spec/models/concerns/resolvable_note_spec.rb24
-rw-r--r--spec/models/diff_discussion_spec.rb25
-rw-r--r--spec/models/discussion_note_spec.rb4
-rw-r--r--spec/models/discussion_spec.rb12
-rw-r--r--spec/models/individual_note_discussion_spec.rb4
-rw-r--r--spec/models/merge_request_spec.rb176
-rw-r--r--spec/models/note_spec.rb30
-rw-r--r--spec/models/out_of_context_discussion_spec.rb4
-rw-r--r--spec/models/simple_discussion_spec.rb11
40 files changed, 504 insertions, 401 deletions
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 443e0143647..5f3d89cf6cb 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -51,7 +51,6 @@ module NotesHelper
return unless current_user
data = { discussion_id: discussion.id, line_type: line_type }
- data[:line_code] = discussion.line_code if discussion.respond_to?(:line_code)
button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
data: data, title: 'Add a reply'
diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb
index 3c78e1fcd68..00707a0023e 100644
--- a/app/mailers/emails/notes.rb
+++ b/app/mailers/emails/notes.rb
@@ -5,11 +5,7 @@ module Emails
@commit = @note.noteable
@target_url = namespace_project_commit_url(*note_target_url_options)
-
- mail_answer_thread(@commit,
- from: sender(@note.author_id),
- to: recipient(recipient_id),
- subject: subject("#{@commit.title} (#{@commit.short_id})"))
+ mail_answer_thread(@commit, note_thread_options(recipient_id))
end
def note_issue_email(recipient_id, note_id)
@@ -54,16 +50,18 @@ module Emails
{
from: sender(@note.author_id),
to: recipient(recipient_id),
- subject: subject("#{@note.noteable.title} (#{@note.noteable.to_reference})")
+ subject: subject("#{@note.noteable.title} (#{@note.noteable.reference_link_text})")
}
end
def setup_note_mail(note_id, recipient_id)
- @note = Note.find(note_id)
+ # `note_id` is a `Note` when originating in `NotifyPreview`
+ @note = note_id.is_a?(Note) ? note_id : Note.find(note_id)
@project = @note.project
- return unless @project
- @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key)
+ if @project && @note.persisted?
+ @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key)
+ end
end
end
end
diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb
index 28abed967f0..87db0c810c3 100644
--- a/app/models/concerns/discussion_on_diff.rb
+++ b/app/models/concerns/discussion_on_diff.rb
@@ -1,3 +1,4 @@
+# Contains functionality shared between `DiffDiscussion` and `LegacyDiffDiscussion`.
module DiscussionOnDiff
extend ActiveSupport::Concern
diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb
index 9ce40f329d8..1a5a7007a2b 100644
--- a/app/models/concerns/note_on_diff.rb
+++ b/app/models/concerns/note_on_diff.rb
@@ -1,3 +1,4 @@
+# Contains functionality shared between `DiffNote` and `LegacyDiffNote`.
module NoteOnDiff
extend ActiveSupport::Concern
diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb
index d378152eb56..631df4757a4 100644
--- a/app/models/concerns/noteable.rb
+++ b/app/models/concerns/noteable.rb
@@ -12,6 +12,32 @@ module Noteable
end
def grouped_diff_discussions
+ # Doesn't use `discussion_notes`, because this may include commit diff notes
+ # besides MR diff notes, that we do no want to display on the MR Changes tab.
notes.inc_relations_for_view.grouped_diff_discussions
end
+
+ def resolvable_discussions
+ @resolvable_discussions ||= discussion_notes.resolvable.discussions(self)
+ end
+
+ def discussions_resolvable?
+ resolvable_discussions.any?(&:resolvable?)
+ end
+
+ def discussions_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
end
diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb
index 22cd9bb7fd4..5cb51196a94 100644
--- a/app/models/concerns/resolvable_discussion.rb
+++ b/app/models/concerns/resolvable_discussion.rb
@@ -2,6 +2,15 @@ module ResolvableDiscussion
extend ActiveSupport::Concern
included do
+ # A number of properties of this `Discussion`, like `first_note` and `resolvable?`, are memoized.
+ # When this discussion is resolved or unresolved, the values of these properties potentially change.
+ # To make sure all memoized values are reset when this happens, `update` resets all instance variables with names in
+ # `memoized_variables`. If you add a memoized method in `ResolvableDiscussion` or any `Discussion` subclass,
+ # please make sure the instance variable name is added to `memoized_values`, like below.
+ cattr_accessor :memoized_values, instance_accessor: false do
+ []
+ end
+
memoized_values.push(
:resolvable,
:resolved,
@@ -78,4 +87,20 @@ module ResolvableDiscussion
update { |notes| notes.unresolve! }
end
+
+ private
+
+ def update
+ # 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.fresh.to_a
+
+ self.class.memoized_values.each do |var|
+ instance_variable_set(:"@#{var}", nil)
+ end
+ end
end
diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb
index 1155ec22369..2c70678429a 100644
--- a/app/models/concerns/resolvable_note.rb
+++ b/app/models/concerns/resolvable_note.rb
@@ -8,7 +8,7 @@ module ResolvableNote
validates :resolved_by, presence: true, if: :resolved?
- # Keep this scope in sync with the logic in `#potentially_resolvable?` in `Discussion` subclasses that are resolvable.
+ # Keep this scope in sync with the logic in `#potentially_resolvable?` in subclasses of `Discussion` that are resolvable.
# `RESOLVABLE_TYPES` should include names of all subclasses that are resolvable (where the method can return true), and
# the scope should also match the criteria `ResolvableDiscussion#potentially_resolvable?` puts on resolvability.
scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: 'MergeRequest') }
diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb
index a89cce4bf5e..22912b30c6a 100644
--- a/app/models/diff_discussion.rb
+++ b/app/models/diff_discussion.rb
@@ -1,4 +1,4 @@
-# A discussion on merge request or commit diffs consisting of `DiffNote` notes
+# A discussion on merge request or commit diffs consisting of `DiffNote` notes.
class DiffDiscussion < Discussion
include DiscussionOnDiff
diff --git a/app/models/discussion.rb b/app/models/discussion.rb
index a77076f02b6..22130b8a508 100644
--- a/app/models/discussion.rb
+++ b/app/models/discussion.rb
@@ -1,8 +1,5 @@
+# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes.
class Discussion
- cattr_accessor :memoized_values, instance_accessor: false do
- []
- end
-
include ResolvableDiscussion
attr_reader :notes, :noteable
@@ -25,23 +22,34 @@ class Discussion
notes.group_by { |n| n.discussion_id(noteable) }.values.map { |notes| build(notes, noteable) }
end
+ # Returns an alphanumeric discussion ID based on `build_discussion_id`
def self.discussion_id(note)
Digest::SHA1.hexdigest(build_discussion_id(note).join("-"))
end
- # Optionally override the discussion ID at runtime depending on circumstances
- def self.override_discussion_id(note)
- nil
+ # Returns an array of discussion ID components
+ def self.build_discussion_id(note)
+ [*base_discussion_id(note), SecureRandom.hex]
end
- def self.build_discussion_id_base(note)
+ def self.base_discussion_id(note)
noteable_id = note.noteable_id || note.commit_id
[:discussion, note.noteable_type.try(:underscore), noteable_id]
end
- # Returns an array of discussion ID components
- def self.build_discussion_id(note)
- [*build_discussion_id_base(note), SecureRandom.hex]
+ # To turn a list of notes into a list of discussions, they are grouped by discussion ID.
+ # When notes on a commit are displayed in context of a merge request that contains that commit,
+ # these notes are to be displayed as if they were part of one discussion, even though they were actually
+ # individual notes on the commit with different discussion IDs, so that it's clear that these are not
+ # notes on the merge request itself.
+ # To get these out-of-context notes to end up in the same discussion, we need to get them to return the same
+ # `discussion_id` when this grouping happens. To enable this, `Note#discussion_id` calls out
+ # to the `override_discussion_id` method on the appropriate `Discussion` subclass, as determined by
+ # the `discussion_class` method on `Note` or a subclass of `Note`.
+ # If no override is necessary, return `nil`.
+ # For the case described above, see `OutOfContextDiscussion.override_discussion_id`.
+ def self.override_discussion_id(note)
+ nil
end
def initialize(notes, noteable = nil)
@@ -97,20 +105,4 @@ class Discussion
def reply_attributes
first_note.slice(:type, :noteable_type, :noteable_id, :commit_id, :discussion_id)
end
-
- private
-
- def update
- # 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.fresh.to_a
-
- self.class.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
index 8957161805a..324d019cf79 100644
--- a/app/models/discussion_note.rb
+++ b/app/models/discussion_note.rb
@@ -5,6 +5,6 @@ class DiscussionNote < Note
validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
def discussion_class(*)
- SimpleDiscussion
+ Discussion
end
end
diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb
index 506b0a98528..42318fa3d6e 100644
--- a/app/models/individual_note_discussion.rb
+++ b/app/models/individual_note_discussion.rb
@@ -1,7 +1,6 @@
# A discussion to wrap a single `Note` note on the root of an issue, merge request,
-# commit, or snippet, that is not displayed as a discussion
+# commit, or snippet, that is not displayed as a discussion.
class IndividualNoteDiscussion < Discussion
- # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
def potentially_resolvable?
false
end
diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb
index 39b57f6d743..6515762c92d 100644
--- a/app/models/legacy_diff_discussion.rb
+++ b/app/models/legacy_diff_discussion.rb
@@ -1,4 +1,6 @@
-# A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes
+# A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes.
+# All new diff discussions are of the type `DiffDiscussion`, but any diff discussions created
+# before the introduction of the new implementation still use `LegacyDiffDiscussion`.
class LegacyDiffDiscussion < Discussion
include DiscussionOnDiff
@@ -6,7 +8,6 @@ class LegacyDiffDiscussion < Discussion
true
end
- # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
def potentially_resolvable?
false
end
diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb
index 8fdebef042b..b9fddb2ea79 100644
--- a/app/models/legacy_diff_note.rb
+++ b/app/models/legacy_diff_note.rb
@@ -1,4 +1,6 @@
-# A note on merge request or commit diffs, using the legacy implementation
+# A note on merge request or commit diffs, using the legacy implementation.
+# All new diff notes are of the type `DiffNote`, but any diff notes created
+# before the introduction of the new implementation still use `LegacyDiffNote`.
class LegacyDiffNote < Note
include NoteOnDiff
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index a1efa17180e..77c902fdf09 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -478,30 +478,6 @@ class MergeRequest < ActiveRecord::Base
alias_method :discussion_notes, :related_notes
- def resolvable_discussions
- @resolvable_discussions ||= notes.resolvable.discussions
- end
-
- def discussions_resolvable?
- resolvable_discussions.any?(&:resolvable?)
- end
-
- def discussions_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 3d2decf6930..6cb9e84ce26 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -227,7 +227,8 @@ class Note < ActiveRecord::Base
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
+ # displayed in one discussion instead of individually.
+ # See also `#discussion_id` and `Discussion.override_discussion_id`.
if noteable && noteable != self.noteable
OutOfContextDiscussion
else
@@ -235,6 +236,7 @@ class Note < ActiveRecord::Base
end
end
+ # See `Discussion.override_discussion_id` for details.
def discussion_id(noteable = nil)
discussion_class(noteable).override_discussion_id(self) || super()
end
diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb
index 7be9aa19707..62b62ea726c 100644
--- a/app/models/out_of_context_discussion.rb
+++ b/app/models/out_of_context_discussion.rb
@@ -1,13 +1,18 @@
-# A discussion to wrap a number of `Note` notes on the root of a commit when they
-# are displayed in context of a merge request as if they were part of a discussion.
+# When notes on a commit are displayed in the context of a merge request that contains that commit,
+# they are displayed as if they were a discussion.
+# This represents one of those discussions, consisting of `Note` notes.
class OutOfContextDiscussion < Discussion
- # To make sure all out-of-context notes are displayed in one discussion,
+ # Returns an array of discussion ID components
+ def self.build_discussion_id(note)
+ base_discussion_id(note)
+ end
+
+ # To make sure all out-of-context notes end up grouped as one discussion,
# we override the discussion ID to be a newly generated but consistent ID.
def self.override_discussion_id(note)
- Digest::SHA1.hexdigest(build_discussion_id_base(note).join("-"))
+ discussion_id(note)
end
- # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
def potentially_resolvable?
false
end
diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb
index 7d65b2b7993..bfaf0eb2fae 100644
--- a/app/models/sent_notification.rb
+++ b/app/models/sent_notification.rb
@@ -102,6 +102,8 @@ class SentNotification < ActiveRecord::Base
if self.in_reply_to_discussion_id.present?
attrs[:in_reply_to_discussion_id] = self.in_reply_to_discussion_id
else
+ # Remove in GitLab 10.0, when we will not support replying to SentNotifications
+ # that don't have `in_reply_to_discussion_id` anymore.
attrs.merge!(
type: self.note_type,
diff --git a/app/models/simple_discussion.rb b/app/models/simple_discussion.rb
deleted file mode 100644
index cd2c142211c..00000000000
--- a/app/models/simple_discussion.rb
+++ /dev/null
@@ -1,3 +0,0 @@
-# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes
-class SimpleDiscussion < Discussion
-end
diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb
index 4619ba552d7..7d5dca5282c 100644
--- a/app/services/notes/build_service.rb
+++ b/app/services/notes/build_service.rb
@@ -1,5 +1,5 @@
module Notes
- class BuildService < BaseService
+ class BuildService < ::BaseService
def execute
in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
new_discussion = params.delete(:new_discussion)
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index c08cddcbee5..f3954f6f8c4 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -1,5 +1,5 @@
module Notes
- class CreateService < BaseService
+ class CreateService < ::BaseService
def execute
merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
diff --git a/app/views/notify/note_merge_request_email.text.erb b/app/views/notify/note_merge_request_email.text.erb
index 381f0366971..413d9e6e9ac 100644
--- a/app/views/notify/note_merge_request_email.text.erb
+++ b/app/views/notify/note_merge_request_email.text.erb
@@ -1 +1 @@
-<%= render partial: 'note_email'%>
+<%= render 'note_email' %>
diff --git a/changelogs/unreleased/new-resolvable-discussion.yml b/changelogs/unreleased/new-resolvable-discussion.yml
index af1de6a45e7..f4dc4ea3ede 100644
--- a/changelogs/unreleased/new-resolvable-discussion.yml
+++ b/changelogs/unreleased/new-resolvable-discussion.yml
@@ -1,4 +1,4 @@
---
title: Add option to start a new resolvable discussion in an MR
-merge_request:
-author:
+merge_request: 7527
+author:
diff --git a/db/migrate/20170308220217_add_index_to_note_original_discussion_id.rb b/db/migrate/20170308220217_add_index_to_note_original_discussion_id.rb
deleted file mode 100644
index 2c823ed84a9..00000000000
--- a/db/migrate/20170308220217_add_index_to_note_original_discussion_id.rb
+++ /dev/null
@@ -1,18 +0,0 @@
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
-class AddIndexToNoteOriginalDiscussionId < ActiveRecord::Migration
- include Gitlab::Database::MigrationHelpers
-
- DOWNTIME = false
-
- disable_ddl_transaction!
-
- def up
- add_concurrent_index :notes, :original_discussion_id
- end
-
- def down
- remove_index :notes, :original_discussion_id if index_exists? :notes, :original_discussion_id
- end
-end
diff --git a/db/schema.rb b/db/schema.rb
index 1b23bab5af2..af7dc07d5ba 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -736,7 +736,6 @@ ActiveRecord::Schema.define(version: 20170402231018) do
add_index "notes", ["note"], name: "index_notes_on_note_trigram", using: :gin, opclasses: {"note"=>"gin_trgm_ops"}
add_index "notes", ["noteable_id", "noteable_type"], name: "index_notes_on_noteable_id_and_noteable_type", using: :btree
add_index "notes", ["noteable_type"], name: "index_notes_on_noteable_type", using: :btree
- add_index "notes", ["original_discussion_id"], name: "index_notes_on_original_discussion_id", using: :btree
add_index "notes", ["project_id", "noteable_type"], name: "index_notes_on_project_id_and_noteable_type", using: :btree
add_index "notes", ["updated_at"], name: "index_notes_on_updated_at", using: :btree
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index 0f9056a4914..90c35e2c7f8 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -25,17 +25,11 @@ FactoryGirl.define do
end
end
- factory :discussion_note_on_issue, traits: [:on_issue], class: DiscussionNote do
- association :project, :repository
- end
+ factory :discussion_note_on_issue, traits: [:on_issue], class: DiscussionNote
- factory :discussion_note_on_commit, traits: [:on_commit], class: DiscussionNote do
- association :project, :repository
- end
+ factory :discussion_note_on_commit, traits: [:on_commit], class: DiscussionNote
- factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote do
- association :project, :repository
- end
+ factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote
factory :legacy_diff_note_on_merge_request, traits: [:on_merge_request, :legacy_diff_note], class: LegacyDiffNote do
association :project, :repository
diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb
index 5769769a5ba..218d95a88b8 100644
--- a/spec/features/merge_requests/diff_notes_avatars_spec.rb
+++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb
@@ -163,9 +163,9 @@ feature 'Diff note avatars', feature: true, js: true do
end
context 'multiple comments' do
- let!(:notes) { create_list(:diff_note_on_merge_request, 3, project: project, noteable: merge_request, in_reply_to: note) }
-
before do
+ create_list(:diff_note_on_merge_request, 3, project: project, noteable: merge_request, in_reply_to: note)
+
visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, view: view)
wait_for_ajax
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index c107e4c4457..e6f0a3b5920 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -647,14 +647,14 @@ describe Notify do
shared_examples 'a discussion note email' do |model|
it_behaves_like 'it should have Gmail Actions links'
- it 'is sent as the author' do
+ it 'is sent to the given recipient as the author' do
sender = subject.header[:from].addrs[0]
- expect(sender.display_name).to eq(note_author.name)
- expect(sender.address).to eq(gitlab_sender)
- end
- it 'is sent to the given recipient' do
- is_expected.to deliver_to recipient.notification_email
+ aggregate_failures do
+ expect(sender.display_name).to eq(note_author.name)
+ expect(sender.address).to eq(gitlab_sender)
+ expect(subject).to deliver_to(recipient.notification_email)
+ end
end
it 'contains the message from the note' do
diff --git a/spec/mailers/previews/notify_preview.rb b/spec/mailers/previews/notify_preview.rb
new file mode 100644
index 00000000000..d9463efcb4e
--- /dev/null
+++ b/spec/mailers/previews/notify_preview.rb
@@ -0,0 +1,115 @@
+class NotifyPreview < ActionMailer::Preview
+ def note_merge_request_email_for_individual_note
+ note_email(:note_merge_request_email) do
+ note = <<-MD.strip_heredoc
+ This is an individual note on a merge request :smiley:
+
+ In this notification email, we expect to see:
+
+ - The note contents (that's what you're looking at)
+ - A link to view this note on Gitlab
+ - An explanation for why the user is receiving this notification
+ MD
+
+ create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, note: note)
+ end
+ end
+
+ def note_merge_request_email_for_discussion
+ note_email(:note_merge_request_email) do
+ note = <<-MD.strip_heredoc
+ This is a new discussion on a merge request :smiley:
+
+ In this notification email, we expect to see:
+
+ - A line saying who started this discussion
+ - The note contents (that's what you're looking at)
+ - A link to view this discussion on Gitlab
+ - An explanation for why the user is receiving this notification
+ MD
+
+ create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, type: 'DiscussionNote', note: note)
+ end
+ end
+
+ def note_merge_request_email_for_diff_discussion
+ note_email(:note_merge_request_email) do
+ note = <<-MD.strip_heredoc
+ This is a new discussion on a merge request :smiley:
+
+ In this notification email, we expect to see:
+
+ - A line saying who started this discussion and on what file
+ - The diff
+ - The note contents (that's what you're looking at)
+ - A link to view this discussion on Gitlab
+ - An explanation for why the user is receiving this notification
+ MD
+
+ position = Gitlab::Diff::Position.new(
+ old_path: "files/ruby/popen.rb",
+ new_path: "files/ruby/popen.rb",
+ old_line: nil,
+ new_line: 14,
+ diff_refs: merge_request.diff_refs
+ )
+
+ create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, type: 'DiffNote', position: position, note: note)
+ end
+ end
+
+ private
+
+ def project
+ @project ||= Project.find_by_full_path('gitlab-org/gitlab-test')
+ end
+
+ def issue
+ @issue ||= project.issues.last
+ end
+
+ def merge_request
+ @merge_request ||= project.merge_requests.find_by(source_branch: 'master', target_branch: 'feature')
+ end
+
+ def commit
+ @commit ||= project.commit('570e7b2abdd848b95f2f578043fc23bd6f6fd24d')
+ end
+
+ def user
+ @user ||= User.last
+ end
+
+ def note_body
+ <<-MD.strip_heredoc
+ Hello :smiley:
+
+ We expect a blank line between:
+ - The heading ("Adminstrator started...")
+ - The diff
+ MD
+ end
+
+ def create_note(params)
+ Notes::CreateService.new(project, user, params).execute
+ end
+
+ def note_email(method)
+ cleanup do
+ note = yield
+
+ Notify.public_send(method, user.id, note)
+ end
+ end
+
+ def cleanup
+ email = nil
+
+ ActiveRecord::Base.transaction do
+ email = yield
+ raise ActiveRecord::Rollback
+ end
+
+ email
+ end
+end
diff --git a/spec/models/concerns/discussion_on_diff_spec.rb b/spec/models/concerns/discussion_on_diff_spec.rb
new file mode 100644
index 00000000000..0002a00770f
--- /dev/null
+++ b/spec/models/concerns/discussion_on_diff_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+
+describe DiffDiscussion, DiscussionOnDiff, model: true do
+ subject { create(:diff_note_on_merge_request).to_discussion }
+
+ describe "#truncated_diff_lines" do
+ let(:truncated_lines) { subject.truncated_diff_lines }
+
+ context "when diff is greater than allowed number of truncated diff lines " do
+ it "returns fewer lines" do
+ expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
+
+ expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
+ end
+ end
+
+ context "when some diff lines are meta" do
+ it "returns no meta lines" do
+ expect(subject.diff_lines).to include(be_meta)
+ expect(truncated_lines).not_to include(be_meta)
+ end
+ end
+ end
+end
diff --git a/spec/models/concerns/noteable_spec.rb b/spec/models/concerns/noteable_spec.rb
index 0a181c008f3..92cc8859a8c 100644
--- a/spec/models/concerns/noteable_spec.rb
+++ b/spec/models/concerns/noteable_spec.rb
@@ -1,14 +1,14 @@
require 'spec_helper'
describe MergeRequest, Noteable, model: true do
- let(:merge_request) { create(:merge_request) }
- let(:project) { merge_request.project }
- let!(:active_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) }
- let!(:active_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: active_diff_note1) }
- let!(:active_diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: active_position2) }
- let!(:outdated_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position) }
- let!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position, in_reply_to: outdated_diff_note1) }
- let!(:discussion_note1) { create(:discussion_note_on_merge_request, project: project, noteable: merge_request) }
+ let!(:active_diff_note1) { create(:diff_note_on_merge_request) }
+ let(:project) { active_diff_note1.project }
+ subject { active_diff_note1.noteable }
+ let!(:active_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: subject, in_reply_to: active_diff_note1) }
+ let!(:active_diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: subject, position: active_position2) }
+ let!(:outdated_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: subject, position: outdated_position) }
+ let!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: subject, in_reply_to: outdated_diff_note1) }
+ let!(:discussion_note1) { create(:discussion_note_on_merge_request, project: project, noteable: subject) }
let!(:discussion_note2) { create(:discussion_note_on_merge_request, in_reply_to: discussion_note1) }
let!(:commit_diff_note1) { create(:diff_note_on_commit, project: project) }
let!(:commit_diff_note2) { create(:diff_note_on_commit, project: project, in_reply_to: commit_diff_note1) }
@@ -17,8 +17,8 @@ describe MergeRequest, Noteable, model: true do
let!(:commit_discussion_note1) { create(:discussion_note_on_commit, project: project) }
let!(:commit_discussion_note2) { create(:discussion_note_on_commit, in_reply_to: commit_discussion_note1) }
let!(:commit_discussion_note3) { create(:discussion_note_on_commit, project: project) }
- let!(:note1) { create(:note, project: project, noteable: merge_request) }
- let!(:note2) { create(:note, project: project, noteable: merge_request) }
+ let!(:note1) { create(:note, project: project, noteable: subject) }
+ let!(:note2) { create(:note, project: project, noteable: subject) }
let(:active_position2) do
Gitlab::Diff::Position.new(
@@ -26,7 +26,7 @@ describe MergeRequest, Noteable, model: true do
new_path: "files/ruby/popen.rb",
old_line: 16,
new_line: 22,
- diff_refs: merge_request.diff_refs
+ diff_refs: subject.diff_refs
)
end
@@ -41,29 +41,29 @@ describe MergeRequest, Noteable, model: true do
end
describe '#discussions' do
- subject { merge_request.discussions }
+ let(:discussions) { subject.discussions }
it 'includes discussions for diff notes, commit diff notes, commit notes, and regular notes' do
- expect(subject).to eq([
- DiffDiscussion.new([active_diff_note1, active_diff_note2], merge_request),
- DiffDiscussion.new([active_diff_note3], merge_request),
- DiffDiscussion.new([outdated_diff_note1, outdated_diff_note2], merge_request),
- SimpleDiscussion.new([discussion_note1, discussion_note2], merge_request),
- DiffDiscussion.new([commit_diff_note1, commit_diff_note2], merge_request),
- OutOfContextDiscussion.new([commit_note1, commit_note2], merge_request),
- SimpleDiscussion.new([commit_discussion_note1, commit_discussion_note2], merge_request),
- SimpleDiscussion.new([commit_discussion_note3], merge_request),
- IndividualNoteDiscussion.new([note1], merge_request),
- IndividualNoteDiscussion.new([note2], merge_request)
+ expect(discussions).to eq([
+ DiffDiscussion.new([active_diff_note1, active_diff_note2], subject),
+ DiffDiscussion.new([active_diff_note3], subject),
+ DiffDiscussion.new([outdated_diff_note1, outdated_diff_note2], subject),
+ Discussion.new([discussion_note1, discussion_note2], subject),
+ DiffDiscussion.new([commit_diff_note1, commit_diff_note2], subject),
+ OutOfContextDiscussion.new([commit_note1, commit_note2], subject),
+ Discussion.new([commit_discussion_note1, commit_discussion_note2], subject),
+ Discussion.new([commit_discussion_note3], subject),
+ IndividualNoteDiscussion.new([note1], subject),
+ IndividualNoteDiscussion.new([note2], subject)
])
end
end
describe '#grouped_diff_discussions' do
- subject { merge_request.grouped_diff_discussions }
+ let(:grouped_diff_discussions) { subject.grouped_diff_discussions }
it "includes active discussions" do
- discussions = subject.values.flatten
+ discussions = grouped_diff_discussions.values.flatten
expect(discussions.count).to eq(2)
expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id])
@@ -74,12 +74,188 @@ describe MergeRequest, Noteable, model: true do
end
it "doesn't include outdated discussions" do
- expect(subject.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
+ expect(grouped_diff_discussions.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
end
it "groups the discussions by line code" do
- expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
- expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
+ expect(grouped_diff_discussions[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
+ expect(grouped_diff_discussions[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
+ end
+ end
+
+ context "discussion status" do
+ let(:first_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+ let(:second_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+ let(:third_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+
+ before do
+ allow(subject).to receive(:resolvable_discussions).and_return([first_discussion, second_discussion, third_discussion])
+ end
+
+ describe "#discussions_resolvable?" do
+ context "when all discussions are unresolvable" do
+ before do
+ allow(first_discussion).to receive(:resolvable?).and_return(false)
+ allow(second_discussion).to receive(:resolvable?).and_return(false)
+ allow(third_discussion).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_resolvable?).to be false
+ end
+ end
+
+ context "when some discussions are unresolvable and some discussions are resolvable" do
+ before do
+ allow(first_discussion).to receive(:resolvable?).and_return(true)
+ allow(second_discussion).to receive(:resolvable?).and_return(false)
+ allow(third_discussion).to receive(:resolvable?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.discussions_resolvable?).to be true
+ end
+ end
+
+ context "when all discussions are resolvable" do
+ before do
+ allow(first_discussion).to receive(:resolvable?).and_return(true)
+ allow(second_discussion).to receive(:resolvable?).and_return(true)
+ allow(third_discussion).to receive(:resolvable?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.discussions_resolvable?).to be true
+ end
+ end
+ end
+
+ describe "#discussions_resolved?" do
+ context "when discussions are not resolvable" do
+ before do
+ allow(subject).to receive(:discussions_resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_resolved?).to be false
+ end
+ end
+
+ context "when discussions are resolvable" do
+ before do
+ allow(subject).to receive(:discussions_resolvable?).and_return(true)
+
+ allow(first_discussion).to receive(:resolvable?).and_return(true)
+ allow(second_discussion).to receive(:resolvable?).and_return(false)
+ allow(third_discussion).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when all resolvable discussions are resolved" do
+ before do
+ allow(first_discussion).to receive(:resolved?).and_return(true)
+ allow(third_discussion).to receive(:resolved?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.discussions_resolved?).to be true
+ end
+ end
+
+ context "when some resolvable discussions are not resolved" do
+ before do
+ allow(first_discussion).to receive(:resolved?).and_return(true)
+ allow(third_discussion).to receive(:resolved?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_resolved?).to be false
+ end
+ end
+ end
+ end
+
+ describe "#discussions_to_be_resolved?" do
+ context "when discussions are not resolvable" do
+ before do
+ allow(subject).to receive(:discussions_resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_to_be_resolved?).to be false
+ end
+ end
+
+ context "when discussions are resolvable" do
+ before do
+ allow(subject).to receive(:discussions_resolvable?).and_return(true)
+
+ allow(first_discussion).to receive(:resolvable?).and_return(true)
+ allow(second_discussion).to receive(:resolvable?).and_return(false)
+ allow(third_discussion).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when all resolvable discussions are resolved" do
+ before do
+ allow(first_discussion).to receive(:resolved?).and_return(true)
+ allow(third_discussion).to receive(:resolved?).and_return(true)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_to_be_resolved?).to be false
+ end
+ end
+
+ context "when some resolvable discussions are not resolved" do
+ before do
+ allow(first_discussion).to receive(:resolved?).and_return(true)
+ allow(third_discussion).to receive(:resolved?).and_return(false)
+ end
+
+ it "returns true" do
+ expect(subject.discussions_to_be_resolved?).to be true
+ end
+ end
+ end
+ end
+
+ describe "#discussions_to_be_resolved" do
+ before do
+ allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
+ allow(second_discussion).to receive(:to_be_resolved?).and_return(false)
+ allow(third_discussion).to receive(:to_be_resolved?).and_return(false)
+ end
+
+ it 'includes only discussions that need to be resolved' do
+ expect(subject.discussions_to_be_resolved).to eq([first_discussion])
+ end
+ end
+
+ describe '#discussions_can_be_resolved_by?' do
+ let(:user) { build(:user) }
+
+ context 'all discussions can be resolved by the user' do
+ before do
+ allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ end
+
+ it 'allows a user to resolve the discussions' do
+ expect(subject.discussions_can_be_resolved_by?(user)).to be(true)
+ end
+ end
+
+ context 'one discussion cannot be resolved by the user' do
+ before do
+ allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false)
+ end
+
+ it 'allows a user to resolve the discussions' do
+ expect(subject.discussions_can_be_resolved_by?(user)).to be(false)
+ end
+ end
end
end
end
diff --git a/spec/models/concerns/resolvable_discussion_spec.rb b/spec/models/concerns/resolvable_discussion_spec.rb
index 2d2182fbae9..18327fe262d 100644
--- a/spec/models/concerns/resolvable_discussion_spec.rb
+++ b/spec/models/concerns/resolvable_discussion_spec.rb
@@ -5,8 +5,9 @@ describe Discussion, ResolvableDiscussion, models: true do
let(:first_note) { create(:discussion_note_on_merge_request) }
let(:merge_request) { first_note.noteable }
- let(:second_note) { create(:discussion_note_on_merge_request, in_reply_to: first_note) }
- let(:third_note) { create(:discussion_note_on_merge_request) }
+ let(:project) { first_note.project }
+ let(:second_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
+ let(:third_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
describe "#resolvable?" do
context "when potentially resolvable" do
diff --git a/spec/models/concerns/resolvable_note_spec.rb b/spec/models/concerns/resolvable_note_spec.rb
index a5a26958410..1503ccdff11 100644
--- a/spec/models/concerns/resolvable_note_spec.rb
+++ b/spec/models/concerns/resolvable_note_spec.rb
@@ -1,15 +1,17 @@
require 'spec_helper'
describe Note, ResolvableNote, models: true do
- subject { create(:discussion_note_on_merge_request) }
+ let(:project) { create(:project) }
+ let(:merge_request) { create(:merge_request, source_project: project) }
+ subject { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
context 'resolvability scopes' do
- let!(:note1) { create(:note) }
- let!(:note2) { create(:diff_note_on_commit) }
- let!(:note3) { create(:diff_note_on_merge_request, :resolved) }
- let!(:note4) { create(:discussion_note_on_merge_request) }
- let!(:note5) { create(:discussion_note_on_issue) }
- let!(:note6) { create(:discussion_note_on_merge_request, :system) }
+ let!(:note1) { create(:note, project: project) }
+ let!(:note2) { create(:diff_note_on_commit, project: project) }
+ let!(:note3) { create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+ let!(:note4) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+ let!(:note5) { create(:discussion_note_on_issue, project: project) }
+ let!(:note6) { create(:discussion_note_on_merge_request, :system, noteable: merge_request, project: project) }
describe '.potentially_resolvable' do
it 'includes diff and discussion notes on merge requests' do
@@ -38,9 +40,9 @@ describe Note, ResolvableNote, models: true do
describe ".resolve!" do
let(:current_user) { create(:user) }
- let!(:commit_note) { create(:diff_note_on_commit) }
- let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved) }
- let!(:unresolved_note) { create(:discussion_note_on_merge_request) }
+ let!(:commit_note) { create(:diff_note_on_commit, project: project) }
+ let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+ let!(:unresolved_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
before do
described_class.resolve!(current_user)
@@ -59,7 +61,7 @@ describe Note, ResolvableNote, models: true do
end
describe ".unresolve!" do
- let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved) }
+ let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
before do
described_class.unresolve!
diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb
index ba7dd5280bd..48e7c0a822c 100644
--- a/spec/models/diff_discussion_spec.rb
+++ b/spec/models/diff_discussion_spec.rb
@@ -4,8 +4,10 @@ describe DiffDiscussion, model: true do
subject { described_class.new([first_note, second_note, third_note]) }
let(:first_note) { create(:diff_note_on_merge_request) }
- let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
- let(:third_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
+ let(:merge_request) { first_note.noteable }
+ let(:project) { first_note.project }
+ let(:second_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
+ let(:third_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
describe '#reply_attributes' do
it 'includes position and original_position' do
@@ -14,23 +16,4 @@ describe DiffDiscussion, model: true do
expect(attributes[:original_position]).to eq(first_note.original_position.to_json)
end
end
-
- describe "#truncated_diff_lines" do
- let(:truncated_lines) { subject.truncated_diff_lines }
-
- context "when diff is greater than allowed number of truncated diff lines " do
- it "returns fewer lines" do
- expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
-
- expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
- end
- end
-
- context "when some diff lines are meta" do
- it "returns no meta lines" do
- expect(subject.diff_lines).to include(be_meta)
- expect(truncated_lines).not_to include(be_meta)
- end
- end
- end
end
diff --git a/spec/models/discussion_note_spec.rb b/spec/models/discussion_note_spec.rb
deleted file mode 100644
index c9dd3d45270..00000000000
--- a/spec/models/discussion_note_spec.rb
+++ /dev/null
@@ -1,4 +0,0 @@
-require 'spec_helper'
-
-describe DiscussionNote, models: true do
-end
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index 718b208e8f8..0221e23ced8 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -3,15 +3,15 @@ require 'spec_helper'
describe Discussion, model: true do
subject { described_class.new([first_note, second_note, third_note]) }
- let(:first_note) { create(:discussion_note_on_merge_request) }
+ let(:first_note) { create(:diff_note_on_merge_request) }
let(:merge_request) { first_note.noteable }
- let(:second_note) { create(:discussion_note_on_merge_request, in_reply_to: first_note) }
- let(:third_note) { create(:discussion_note_on_merge_request) }
+ let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
+ let(:third_note) { create(:diff_note_on_merge_request) }
describe '.build' do
it 'returns a discussion of the right type' do
discussion = described_class.build([first_note, second_note], merge_request)
- expect(discussion).to be_a(SimpleDiscussion)
+ expect(discussion).to be_a(DiffDiscussion)
expect(discussion.notes.count).to be(2)
expect(discussion.first_note).to be(first_note)
expect(discussion.noteable).to be(merge_request)
@@ -22,8 +22,8 @@ describe Discussion, model: true do
it 'returns an array of discussions of the right type' do
discussions = described_class.build_collection([first_note, second_note, third_note], merge_request)
expect(discussions).to eq([
- SimpleDiscussion.new([first_note, second_note], merge_request),
- SimpleDiscussion.new([third_note], merge_request)
+ DiffDiscussion.new([first_note, second_note], merge_request),
+ DiffDiscussion.new([third_note], merge_request)
])
end
end
diff --git a/spec/models/individual_note_discussion_spec.rb b/spec/models/individual_note_discussion_spec.rb
deleted file mode 100644
index 1f7c2c820e0..00000000000
--- a/spec/models/individual_note_discussion_spec.rb
+++ /dev/null
@@ -1,4 +0,0 @@
-require 'spec_helper'
-
-describe IndividualNoteDiscussion, models: true do
-end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 5d0862abb92..b615d956686 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1224,182 +1224,6 @@ describe MergeRequest, models: true do
end
end
- context "discussion status" do
- let(:first_discussion) { create(:discussion_note_on_merge_request).to_discussion }
- let(:second_discussion) { create(:discussion_note_on_merge_request).to_discussion }
- let(:third_discussion) { create(:discussion_note_on_merge_request).to_discussion }
-
- before do
- allow(subject).to receive(:resolvable_discussions).and_return([first_discussion, second_discussion, third_discussion])
- end
-
- describe "#discussions_resolvable?" do
- context "when all discussions are unresolvable" do
- before do
- allow(first_discussion).to receive(:resolvable?).and_return(false)
- allow(second_discussion).to receive(:resolvable?).and_return(false)
- allow(third_discussion).to receive(:resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.discussions_resolvable?).to be false
- end
- end
-
- context "when some discussions are unresolvable and some discussions are resolvable" do
- before do
- allow(first_discussion).to receive(:resolvable?).and_return(true)
- allow(second_discussion).to receive(:resolvable?).and_return(false)
- allow(third_discussion).to receive(:resolvable?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.discussions_resolvable?).to be true
- end
- end
-
- context "when all discussions are resolvable" do
- before do
- allow(first_discussion).to receive(:resolvable?).and_return(true)
- allow(second_discussion).to receive(:resolvable?).and_return(true)
- allow(third_discussion).to receive(:resolvable?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.discussions_resolvable?).to be true
- end
- end
- end
-
- describe "#discussions_resolved?" do
- context "when discussions are not resolvable" do
- before do
- allow(subject).to receive(:discussions_resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.discussions_resolved?).to be false
- end
- end
-
- context "when discussions are resolvable" do
- before do
- allow(subject).to receive(:discussions_resolvable?).and_return(true)
-
- allow(first_discussion).to receive(:resolvable?).and_return(true)
- allow(second_discussion).to receive(:resolvable?).and_return(false)
- allow(third_discussion).to receive(:resolvable?).and_return(true)
- end
-
- context "when all resolvable discussions are resolved" do
- before do
- allow(first_discussion).to receive(:resolved?).and_return(true)
- allow(third_discussion).to receive(:resolved?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.discussions_resolved?).to be true
- end
- end
-
- context "when some resolvable discussions are not resolved" do
- before do
- allow(first_discussion).to receive(:resolved?).and_return(true)
- allow(third_discussion).to receive(:resolved?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.discussions_resolved?).to be false
- end
- end
- end
- end
-
- describe "#discussions_to_be_resolved?" do
- context "when discussions are not resolvable" do
- before do
- allow(subject).to receive(:discussions_resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.discussions_to_be_resolved?).to be false
- end
- end
-
- context "when discussions are resolvable" do
- before do
- allow(subject).to receive(:discussions_resolvable?).and_return(true)
-
- allow(first_discussion).to receive(:resolvable?).and_return(true)
- allow(second_discussion).to receive(:resolvable?).and_return(false)
- allow(third_discussion).to receive(:resolvable?).and_return(true)
- end
-
- context "when all resolvable discussions are resolved" do
- before do
- allow(first_discussion).to receive(:resolved?).and_return(true)
- allow(third_discussion).to receive(:resolved?).and_return(true)
- end
-
- it "returns false" do
- expect(subject.discussions_to_be_resolved?).to be false
- end
- end
-
- context "when some resolvable discussions are not resolved" do
- before do
- allow(first_discussion).to receive(:resolved?).and_return(true)
- allow(third_discussion).to receive(:resolved?).and_return(false)
- end
-
- it "returns true" do
- expect(subject.discussions_to_be_resolved?).to be true
- end
- end
- end
- end
-
- describe "#discussions_to_be_resolved" do
- before do
- allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
- allow(second_discussion).to receive(:to_be_resolved?).and_return(false)
- allow(third_discussion).to receive(:to_be_resolved?).and_return(false)
- end
-
- it 'includes only discussions that need to be resolved' do
- expect(subject.discussions_to_be_resolved).to eq([first_discussion])
- end
- end
-
- describe '#discussions_can_be_resolved_by?' do
- let(:user) { build(:user) }
-
- context 'all discussions can be resolved by the user' do
- before do
- allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
- allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
- allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true)
- end
-
- it 'allows a user to resolve the discussions' do
- expect(subject.discussions_can_be_resolved_by?(user)).to be(true)
- end
- end
-
- context 'one discussion cannot be resolved by the user' do
- before do
- allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
- allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
- allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false)
- end
-
- it 'allows a user to resolve the discussions' do
- expect(subject.discussions_can_be_resolved_by?(user)).to be(false)
- end
- end
- end
- end
-
describe '#conflicts_can_be_resolved_in_ui?' do
def create_merge_request(source_branch)
create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 2c7f6e59b19..3c4bf3f4ddb 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -250,7 +250,7 @@ describe Note, models: true do
let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note) }
let(:merge_request) { note.noteable }
- it 'returns a discussion with multiple note' do
+ it 'returns a discussion with multiple notes' do
discussion = merge_request.notes.find_discussion(note.discussion_id)
expect(discussion).not_to be_nil
@@ -379,57 +379,57 @@ describe Note, models: true do
describe '#can_be_discussion_note?' do
context 'for a note on a merge request' do
- let(:note) { build(:note_on_merge_request) }
-
it 'returns true' do
+ note = build(:note_on_merge_request)
+
expect(note.can_be_discussion_note?).to be_truthy
end
end
context 'for a note on an issue' do
- let(:note) { build(:note_on_issue) }
-
it 'returns true' do
+ note = build(:note_on_issue)
+
expect(note.can_be_discussion_note?).to be_truthy
end
end
context 'for a note on a commit' do
- let(:note) { build(:note_on_commit) }
-
it 'returns true' do
+ note = build(:note_on_commit)
+
expect(note.can_be_discussion_note?).to be_truthy
end
end
context 'for a note on a snippet' do
- let(:note) { build(:note_on_project_snippet) }
-
it 'returns true' do
+ note = build(:note_on_project_snippet)
+
expect(note.can_be_discussion_note?).to be_truthy
end
end
context 'for a diff note on merge request' do
- let(:note) { build(:diff_note_on_merge_request) }
-
it 'returns false' do
+ note = build(:diff_note_on_merge_request)
+
expect(note.can_be_discussion_note?).to be_falsey
end
end
context 'for a diff note on commit' do
- let(:note) { build(:diff_note_on_commit) }
-
it 'returns false' do
+ note = build(:diff_note_on_commit)
+
expect(note.can_be_discussion_note?).to be_falsey
end
end
context 'for a discussion note' do
- let(:note) { build(:discussion_note_on_merge_request) }
-
it 'returns false' do
+ note = build(:discussion_note_on_merge_request)
+
expect(note.can_be_discussion_note?).to be_falsey
end
end
diff --git a/spec/models/out_of_context_discussion_spec.rb b/spec/models/out_of_context_discussion_spec.rb
deleted file mode 100644
index 1bcd118855e..00000000000
--- a/spec/models/out_of_context_discussion_spec.rb
+++ /dev/null
@@ -1,4 +0,0 @@
-require 'spec_helper'
-
-describe OutOfContextDiscussion, model: true do
-end
diff --git a/spec/models/simple_discussion_spec.rb b/spec/models/simple_discussion_spec.rb
deleted file mode 100644
index 1a342fbc740..00000000000
--- a/spec/models/simple_discussion_spec.rb
+++ /dev/null
@@ -1,11 +0,0 @@
-require 'spec_helper'
-
-describe SimpleDiscussion, model: true do
- subject { create(:discussion_note_on_issue).to_discussion }
-
- describe '#reply_attributes' do
- it 'includes discussion_id' do
- expect(subject.reply_attributes[:discussion_id]).to eq(subject.id)
- end
- end
-end