diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-04-05 01:27:23 +0300 |
---|---|---|
committer | Luke "Jared" Bennett <lbennett@gitlab.com> | 2017-04-05 19:44:14 +0300 |
commit | c319f2114177f011cd0c6c23b04f7c19971268bf (patch) | |
tree | b91a2ace5426bea9a7c6a60eabbd44da394fa80c | |
parent | afa53810deab37c95da245510a7cf85e8846a162 (diff) |
Address review comments
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 |