Unverified Commit c319f211 authored by Douwe Maan's avatar Douwe Maan Committed by Luke "Jared" Bennett

Address review comments

parent afa53810
......@@ -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'
......
......@@ -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
# Contains functionality shared between `DiffDiscussion` and `LegacyDiffDiscussion`.
module DiscussionOnDiff
extend ActiveSupport::Concern
......
# Contains functionality shared between `DiffNote` and `LegacyDiffNote`.
module NoteOnDiff
extend ActiveSupport::Concern
......
......@@ -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
......@@ -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
......@@ -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') }
......
# 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
......
# 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
......@@ -5,6 +5,6 @@ class DiscussionNote < Note
validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
def discussion_class(*)
SimpleDiscussion
Discussion
end
end
# 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
......
# 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
......
# 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
......
......@@ -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?
......
......@@ -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
......
# 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
......
......@@ -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,
......
# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes
class SimpleDiscussion < Discussion
end
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)
......
module Notes
class CreateService < BaseService
class CreateService < ::BaseService
def execute
merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
......
<%= render partial: 'note_email'%>
<%= render 'note_email' %>
---
title: Add option to start a new resolvable discussion in an MR
merge_request:
author:
merge_request: 7527
author:
# 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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
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
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
This diff is collapsed.
......@@ -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
......
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: