Commit 3d1cade1 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'new-resolvable-discussion' into 'master'

Add option to start a new resolvable discussion in an MR

Closes #24378

See merge request !7527
parents 40407764 f058b52b
import DropLab from './droplab/drop_lab';
import InputSetter from './droplab/plugins/input_setter';
class CommentTypeToggle {
constructor(opts = {}) {
this.dropdownTrigger = opts.dropdownTrigger;
this.dropdownList = opts.dropdownList;
this.noteTypeInput = opts.noteTypeInput;
this.submitButton = opts.submitButton;
this.closeButton = opts.closeButton;
this.reopenButton = opts.reopenButton;
}
initDroplab() {
this.droplab = new DropLab();
const config = this.setConfig();
this.droplab.init(this.dropdownTrigger, this.dropdownList, [InputSetter], config);
}
setConfig() {
const config = {
InputSetter: [{
input: this.noteTypeInput,
valueAttribute: 'data-value',
},
{
input: this.submitButton,
valueAttribute: 'data-submit-text',
}],
};
if (this.closeButton) {
config.InputSetter.push({
input: this.closeButton,
valueAttribute: 'data-close-text',
}, {
input: this.closeButton,
valueAttribute: 'data-close-text',
inputAttribute: 'data-alternative-text',
});
}
if (this.reopenButton) {
config.InputSetter.push({
input: this.reopenButton,
valueAttribute: 'data-reopen-text',
}, {
input: this.reopenButton,
valueAttribute: 'data-reopen-text',
inputAttribute: 'data-alternative-text',
});
}
return config;
}
}
export default CommentTypeToggle;
......@@ -42,10 +42,14 @@ import Vue from 'vue';
}
},
created() {
this.discussion = CommentsStore.state[this.discussionId];
if (this.discussionId) {
this.discussion = CommentsStore.state[this.discussionId];
}
},
mounted: function () {
const $textarea = $(`#new-discussion-note-form-${this.discussionId} .note-textarea`);
if (!this.discussionId) return;
const $textarea = $(`.js-discussion-note-form[data-discussion-id=${this.discussionId}] .note-textarea`);
this.textareaIsEmpty = $textarea.val() === '';
$textarea.on('input.comment-and-resolve-btn', () => {
......@@ -53,7 +57,9 @@ import Vue from 'vue';
});
},
destroyed: function () {
$(`#new-discussion-note-form-${this.discussionId} .note-textarea`).off('input.comment-and-resolve-btn');
if (!this.discussionId) return;
$(`.js-discussion-note-form[data-discussion-id=${this.discussionId}] .note-textarea`).off('input.comment-and-resolve-btn');
}
});
......
......@@ -35,6 +35,8 @@ Object.assign(DropDown.prototype, {
},
clickEvent: function(e) {
if (e.target.tagName === 'UL') return;
var selected = utils.closest(e.target, 'LI');
if (!selected) return;
......
......@@ -35,8 +35,6 @@ const InputSetter = {
const newValue = selectedItem.getAttribute(config.valueAttribute);
const inputAttribute = config.inputAttribute;
if (!newValue) return;
if (input.hasAttribute(inputAttribute)) return input.setAttribute(inputAttribute, newValue);
if (input.tagName === 'INPUT') return input.value = newValue;
return input.textContent = newValue;
......
......@@ -55,14 +55,19 @@ window.FilesCommentButton = (function() {
textFileElement = this.getTextFileElement($currentTarget);
buttonParentElement.append(this.buildButton({
discussionID: lineContentElement.attr('data-discussion-id'),
lineType: lineContentElement.attr('data-line-type'),
noteableType: textFileElement.attr('data-noteable-type'),
noteableID: textFileElement.attr('data-noteable-id'),
commitID: textFileElement.attr('data-commit-id'),
noteType: lineContentElement.attr('data-note-type'),
position: lineContentElement.attr('data-position'),
lineType: lineContentElement.attr('data-line-type'),
discussionID: lineContentElement.attr('data-discussion-id'),
lineCode: lineContentElement.attr('data-line-code')
// LegacyDiffNote
lineCode: lineContentElement.attr('data-line-code'),
// DiffNote
position: lineContentElement.attr('data-position')
}));
};
......@@ -76,14 +81,19 @@ window.FilesCommentButton = (function() {
FilesCommentButton.prototype.buildButton = function(buttonAttributes) {
return $commentButtonTemplate.clone().attr({
'data-discussion-id': buttonAttributes.discussionID,
'data-line-type': buttonAttributes.lineType,
'data-noteable-type': buttonAttributes.noteableType,
'data-noteable-id': buttonAttributes.noteableID,
'data-commit-id': buttonAttributes.commitID,
'data-note-type': buttonAttributes.noteType,
// LegacyDiffNote
'data-line-code': buttonAttributes.lineCode,
'data-position': buttonAttributes.position,
'data-discussion-id': buttonAttributes.discussionID,
'data-line-type': buttonAttributes.lineType
// DiffNote
'data-position': buttonAttributes.position
});
};
......@@ -121,7 +131,7 @@ window.FilesCommentButton = (function() {
};
FilesCommentButton.prototype.validateLineContent = function(lineContentElement) {
return lineContentElement.attr('data-discussion-id') && lineContentElement.attr('data-discussion-id') !== '';
return lineContentElement.attr('data-note-type') && lineContentElement.attr('data-note-type') !== '';
};
return FilesCommentButton;
......
......@@ -29,7 +29,8 @@ GLForm.prototype.setupForm = function() {
this.form.find('.div-dropzone').remove();
this.form.addClass('gfm-form');
// remove notify commit author checkbox for non-commit notes
gl.utils.disableButtonIfEmptyField(this.form.find('.js-note-text'), this.form.find('.js-comment-button'));
gl.utils.disableButtonIfEmptyField(this.form.find('.js-note-text'), this.form.find('.js-comment-button, .js-note-new-discussion'));
gl.GfmAutoComplete.setup(this.form.find('.js-gfm-input'));
new DropzoneInput(this.form);
autosize(this.textarea);
......
......@@ -279,7 +279,7 @@ $(function () {
// Disable form buttons while a form is submitting
$body.on('ajax:complete, ajax:beforeSend, submit', 'form', function (e) {
var buttons;
buttons = $('[type="submit"]', this);
buttons = $('[type="submit"], .js-disable-on-submit', this);
switch (e.type) {
case 'ajax:beforeSend':
case 'submit':
......
This diff is collapsed.
......@@ -8,6 +8,7 @@
$.fn.renderGFM = function() {
this.find('.js-syntax-highlight').syntaxHighlight();
this.find('.js-render-math').renderMath();
return this;
};
$(document).on('ready load', function() {
......
......@@ -310,3 +310,94 @@
margin-bottom: 10px;
}
}
.comment-type-dropdown {
.comment-btn {
width: auto;
}
.dropdown-toggle {
float: right;
.toggle-icon {
color: $white-light;
padding-right: 2px;
margin-top: 2px;
pointer-events: none;
}
}
.dropdown-menu {
top: initial;
bottom: 40px;
width: 298px;
}
.description {
display: inline-block;
white-space: normal;
margin-left: 8px;
padding-right: 33px;
}
li {
padding-top: 6px;
& > a {
margin: 0;
padding: 0;
color: inherit;
border-radius: 0;
text-overflow: inherit;
&:hover,
&:focus {
background-color: inherit;
color: inherit;
}
}
&:hover,
&:focus {
background-color: $dropdown-hover-color;
color: $white-light;
}
&.droplab-item-selected i {
visibility: visible;
}
i {
visibility: hidden;
}
}
i {
display: inline-block;
vertical-align: top;
padding-top: 2px;
}
.divider {
margin: 0 8px;
padding: 0;
border-top: $gray-darkest;
}
@media (max-width: $screen-xs-max) {
display: flex;
width: 100%;
.comment-btn {
flex-grow: 1;
flex-shrink: 0;
width: auto;
}
.dropdown-toggle {
flex-grow: 0;
flex-shrink: 1;
width: auto;
}
}
}
......@@ -294,6 +294,18 @@ ul.notes {
border-width: 1px;
}
.discussion-notes {
&:not(:first-child) {
border-top: 1px solid $white-normal;
margin-top: 20px;
}
&:not(:last-child) {
border-bottom: 1px solid $white-normal;
margin-bottom: 20px;
}
}
.notes {
background-color: $white-light;
}
......
module RendersNotes
def prepare_notes_for_rendering(notes)
preload_noteable_for_regular_notes(notes)
preload_max_access_for_authors(notes, @project)
Banzai::NoteRenderer.render(notes, @project, current_user)
notes
end
private
def preload_max_access_for_authors(notes, project)
user_ids = notes.map(&:author_id)
project.team.max_member_access_for_user_ids(user_ids)
end
def preload_noteable_for_regular_notes(notes)
ActiveRecord::Associations::Preloader.new.preload(notes.reject(&:for_commit?), :noteable)
end
end
......@@ -2,6 +2,7 @@
#
# Not to be confused with CommitsController, plural.
class Projects::CommitController < Projects::ApplicationController
include RendersNotes
include CreatesCommit
include DiffForPath
include DiffHelper
......@@ -113,22 +114,19 @@ class Projects::CommitController < Projects::ApplicationController
end
def define_note_vars
@grouped_diff_discussions = commit.notes.grouped_diff_discussions
@notes = commit.notes.non_diff_notes.fresh
Banzai::NoteRenderer.render(
@grouped_diff_discussions.values.flat_map(&:notes) + @notes,
@project,
current_user,
)
@noteable = @commit
@note = @project.build_commit_note(commit)
@noteable = @commit
@comments_target = {
@new_diff_note_attrs = {
noteable_type: 'Commit',
commit_id: @commit.id
}
@grouped_diff_discussions = commit.grouped_diff_discussions
@discussions = commit.discussions
@notes = (@grouped_diff_discussions.values.flatten + @discussions).flat_map(&:notes)
@notes = prepare_notes_for_rendering(@notes)
end
def assign_change_commit_vars
......
......@@ -28,7 +28,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
end
def discussion
@discussion ||= @merge_request.find_diff_discussion(params[:id]) || render_404
@discussion ||= @merge_request.find_discussion(params[:id]) || render_404
end
def authorize_resolve_discussion!
......
class Projects::IssuesController < Projects::ApplicationController
include NotesHelper
include RendersNotes
include ToggleSubscriptionAction
include IssuableActions
include ToggleAwardEmoji
......@@ -84,15 +84,11 @@ class Projects::IssuesController < Projects::ApplicationController
end
def show
raw_notes = @issue.notes.inc_relations_for_view.fresh
@notes = Banzai::NoteRenderer.
render(raw_notes, @project, current_user, @path, @project_wiki, @ref)
@note = @project.notes.new(noteable: @issue)
@noteable = @issue
@note = @project.notes.new(noteable: @issue)
preload_max_access_for_authors(@notes, @project)
@discussions = @issue.discussions
@notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
respond_to do |format|
format.html
......
......@@ -3,7 +3,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
include DiffForPath
include DiffHelper
include IssuableActions
include NotesHelper
include RendersNotes
include ToggleAwardEmoji
include IssuableCollections
......@@ -574,20 +574,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
@note = @project.notes.new(noteable: @merge_request)
@discussions = @merge_request.discussions
preload_noteable_for_regular_notes(@discussions.flat_map(&:notes))
# This is not executed lazily
@notes = Banzai::NoteRenderer.render(
@discussions.flat_map(&:notes),
@project,
current_user,
@path,
@project_wiki,
@ref
)
preload_max_access_for_authors(@notes, @project)
@notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
end
def define_widget_vars
......@@ -600,22 +587,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
end
def define_diff_comment_vars
@comments_target = {
@new_diff_note_attrs = {
noteable_type: 'MergeRequest',
noteable_id: @merge_request.id
}
@use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
@grouped_diff_discussions = @merge_request.notes.inc_relations_for_view.grouped_diff_discussions
Banzai::NoteRenderer.render(
@grouped_diff_discussions.values.flat_map(&:notes),
@project,
current_user,
@path,
@project_wiki,
@ref
)
@grouped_diff_discussions = @merge_request.grouped_diff_discussions
@notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
end
def define_pipelines_vars
......
class Projects::NotesController < Projects::ApplicationController
include RendersNotes
include ToggleAwardEmoji
# Authorize
......@@ -6,13 +7,15 @@ class Projects::NotesController < Projects::ApplicationController
before_action :authorize_create_note!, only: [:create]
before_action :authorize_admin_note!, only: [:update, :destroy]
before_action :authorize_resolve_note!, only: [:resolve, :unresolve]
before_action :find_current_user_notes, only: [:index]
def index
current_fetched_at = Time.now.to_i
notes_json = { notes: [], last_fetched_at: current_fetched_at }
@notes = notes_finder.execute.inc_relations_for_view
@notes = prepare_notes_for_rendering(@notes)
@notes.each do |note|
next if note.cross_reference_not_visible_for?(current_user)
......@@ -23,7 +26,10 @@ class Projects::NotesController < Projects::ApplicationController
end
def create
create_params = note_params.merge(merge_request_diff_head_sha: params[:merge_request_diff_head_sha])
create_params = note_params.merge(
merge_request_diff_head_sha: params[:merge_request_diff_head_sha],
in_reply_to_discussion_id: params[:in_reply_to_discussion_id]
)
@note = Notes::CreateService.new(project, current_user, create_params).execute
if @note.is_a?(Note)
......@@ -111,6 +117,17 @@ class Projects::NotesController < Projects::ApplicationController
)
end
def discussion_html(discussion)
return if discussion.individual_note?
render_to_string(
"discussions/_discussion",
layout: false,
formats: [:html],
locals: { discussion: discussion }
)
end
def diff_discussion_html(discussion)
return unless discussion.diff_discussion?
......@@ -118,13 +135,13 @@ class Projects::NotesController < Projects::ApplicationController
template = "discussions/_parallel_diff_discussion"
locals =
if params[:line_type] == 'old'
{ discussion_left: discussion, discussion_right: nil }
{ discussions_left: [discussion], discussions_right: nil }
else
{ discussion_left: nil, discussion_right: discussion }
{ discussions_left: nil, discussions_right: [discussion] }
end
else
template = "discussions/_diff_discussion"
locals = { discussion: discussion }
locals = { discussions: [discussion] }
end
render_to_string(
......@@ -135,54 +152,28 @@ class Projects::NotesController < Projects::ApplicationController
)
end
def discussion_html(discussion)
return unless discussion.diff_discussion?
render_to_string(
"discussions/_discussion",
layout: false,
formats: [:html],
locals: { discussion: discussion }
)
end
def note_json(note)
attrs = {
id: note.id
commands_changes: note.commands_changes
}
if note.persisted?
Banzai::NoteRenderer.render([note], @project, current_user)
attrs.merge!(
valid: true,
discussion_id: note.discussion_id,
id: note.id,
discussion_id: note.discussion_id(noteable),
html: note_html(note),
note: note.note
)
if note.diff_note?
discussion = note.to_discussion
discussion = note.to_discussion(noteable)
unless discussion.individual_note?
attrs.merge!(
discussion_resolvable: discussion.resolvable?,
diff_discussion_html: diff_discussion_html(discussion),
discussion_html: discussion_html(discussion)
)
# The discussion_id is used to add the comment to the correct discussion
# element on the merge request page. Among other things, the discussion_id
# contains the sha of head commit of the merge request.
# When new commits are pushed into the merge request after the initial
# load of the merge request page, the discussion elements will still have
# the old discussion_ids, with the old head commit sha. The new comment,
# however, will have the new discussion_id with the new commit sha.
# To ensure that these new comments will still end up in the correct
# discussion element, we also send the original discussion_id, with the
# old commit sha, along, and fall back on this value when no discussion
# element with the new discussion_id could be found.
if note.new_diff_note? && note.position != note.original_position
attrs[:original_discussion_id] = note.original_discussion_id
end
end
else
attrs.merge!(
......@@ -191,7 +182,6 @@ class Projects::NotesController < Projects::ApplicationController
)
end
attrs[:commands_changes] = note.commands_changes
attrs
end
......@@ -205,14 +195,30 @@ class Projects::NotesController < Projects::ApplicationController
def note_params
params.require(:note).permit(
:note, :noteable, :noteable_id, :noteable_type, :project_id,
:attachment, :line_code, :commit_id, :type, :position
:project_id,
:noteable_type,
:noteable_id,
:commit_id,
:noteable,
:type,
:note,
:attachment,
# LegacyDiffNote
:line_code,
# DiffNote
:position
)
end
def find_current_user_notes
@notes = NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at))
.execute.inc_author
def notes_finder
@notes_finder ||= NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at))
end
def noteable
@noteable ||= notes_finder.target
end
def last_fetched_at
......
class Projects::SnippetsController < Projects::ApplicationController
include RendersNotes
include ToggleAwardEmoji
include SpammableActions
include SnippetsActions
......@@ -55,8 +56,10 @@ class Projects::SnippetsController < Projects::ApplicationController
def show
@note = @project.notes.new(noteable: @snippet)
@notes = Banzai::NoteRenderer.render(@snippet.notes.fresh, @project, current_user)
@noteable = @snippet
@discussions = @snippet.discussions
@notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
end
def destroy
......
......@@ -17,29 +17,46 @@ class NotesFinder
@project = project
@current_user = current_user
@params = params
init_collection
end
def execute
@notes = since_fetch_at(@params[:last_fetched_at]) if @params[:last_fetched_at]
@notes
notes = init_collection
notes = since_fetch_at(notes)
notes.fresh
end
private
def target
return @target if defined?(@target)
def init_collection
@notes =
if @params[:target_id]
on_target(@params[:target_type], @params[:target_id])
target_type = @params[:target_type]
target_id = @params[:target_id]
return @target = nil unless target_type && target_id
@target =
if target_type == "commit"
if Ability.allowed?(@current_user, :download_code, @project)
@project.commit(target_id)
end
else
notes_of_any_type
noteables_for_type(target_type).find(target_id)
end
end
private
def init_collection
if target
notes_on_target
else
notes_of_any_type
end
end
def notes_of_any_type
types = %w(commit issue merge_request snippet)
note_relations = types.map { |t| notes_for_type(t) }
note_relations.map!{ |notes| search(@params[:search], notes) } if @params[:search]
note_relations.map! { |notes| search(notes) }
UnionFinder.new.find_union(note_relations, Note)
end
......@@ -69,17 +86,11 @@ class NotesFinder
end
end
def on_target(target_type, target_id)
if target_type == "commit"
notes_for_type('commit').for_commit_id(target_id)
def notes_on_target
if target.respond_to?(:related_notes)
target.related_notes
else
target = noteables_for_type(target_type).find(target_id)
if target.respond_to?(:related_notes)
target.related_notes
else
target.notes
end
target.notes
end
end
......@@ -87,17 +98,21 @@ class NotesFinder
#
# This method uses ILIKE on PostgreSQL and LIKE on MySQL.
#
def search(query, notes_relation = @notes)
def search(notes)
query = @params[:search]
return notes unless query
pattern = "%#{query}%"
notes_relation.where(Note.arel_table[:note].matches(pattern))
notes.where(Note.arel_table[:note].matches(pattern))
end
# Notes changed since last fetch
# Uses overlapping intervals to avoid worrying about race conditions
def since_fetch_at(fetch_time)
def since_fetch_at(notes)
return notes unless @params[:last_fetched_at]
# Default to 0 to remain compatible with old clients
last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i)
@notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh
notes.updated_after(last_fetched_at - FETCH_OVERLAP)
end
end
......@@ -62,19 +62,19 @@ module DiffHelper
end
def parallel_diff_discussions(left, right, diff_file)
discussion_left = discussion_right = nil
discussions_left = discussions_right = nil
if left && (left.unchanged? || left.removed?)
line_code = diff_file.line_code(left)
discussion_left = @grouped_diff_discussions[line_code]
discussions_left = @grouped_diff_discussions[line_code]
end
if right && right.added?
line_code = diff_file.line_code(right)
discussion_right = @grouped_diff_discussions[line_code]
discussions_right = @grouped_diff_discussions[line_code]
end
[discussion_left, discussion_right]
[discussions_left, discussions_right]
end
def inline_diff_btn
......
......@@ -24,57 +24,24 @@ module NotesHelper
end
def diff_view_data
return {} unless @comments_target
return {} unless @new_diff_note_attrs
@comments_target.slice(:noteable_id, :noteable_type, :commit_id)
@new_diff_note_attrs.slice(:noteable_id, :noteable_type, :commit_id)
end
def diff_view_line_data(line_code, position, line_type)
return if @diff_notes_disabled
use_legacy_diff_note = @use_legacy_diff_notes
# If the controller doesn't force the use of legacy diff notes, we
# determine this on a line-by-line basis by seeing if there already exist
# active legacy diff notes at this line, in which case newly created notes
# will use the legacy technology as well.
# We do this because the discussion_id values of legacy and "new" diff
# notes, which are used to group notes on the merge request discussion tab,
# are incompatible.
# If we didn't, diff notes that would show for the same line on the changes
# tab, would show in different discussions on the discussion tab.
use_legacy_diff_note ||= begin
discussion = @grouped_diff_discussions[line_code]
discussion && discussion.legacy_diff_discussion?
end
data = {
line_code: line_code,
line_type: line_type,
}
if use_legacy_diff_note
discussion_id = LegacyDiffNote.discussion_id(
@comments_target[:noteable_type],
@comments_target[:noteable_id] || @comments_target[:commit_id],
line_code
)
data.merge!(
note_type: LegacyDiffNote.name,
discussion_id: discussion_id
)
if @use_legacy_diff_notes
data[:note_type] = LegacyDiffNote.name
else
discussion_id = DiffNote.discussion_id(
@comments_target[:noteable_type],
@comments_target[:noteable_id] || @comments_target[:commit_id],
position
)
data.merge!(
position: position.to_json,
note_type: DiffNote.name,
discussion_id: discussion_id
)
data[:note_type] = DiffNote.name
data[:position] = position.to_json
end
data
......@@ -83,21 +50,12 @@ module NotesHelper
def link_to_reply_discussion(discussion, line_type = nil)
return unless current_user
data = discussion.reply_attributes.merge(line_type: line_type)
data = { discussion_id: discussion.id, line_type: line_type }
button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
data: data, title: 'Add a reply'
end
def preload_max_access_for_authors(notes, project)
user_ids = notes.map(&:author_id)
project.team.max_member_access_for_user_ids(user_ids)
end
def preload_noteable_for_regular_notes(notes)
ActiveRecord::Associations::Preloader.new.preload(notes.select { |note| !note.for_commit? }, :noteable)
end
def note_max_access_for_user(note)
note.project.team.human_max_access(note.author_id)
end
......
......@@ -4,13 +4,8 @@ module Emails
setup_note_mail(note_id, recipient_id)
@commit = @note.noteable
@discussion = @note.to_discussion if @note.diff_note?
@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)
......@@ -25,7 +20,6 @@ module Emails
setup_note_mail(note_id, recipient_id)
@merge_request = @note.noteable
@discussion = @note.to_discussion if @note.diff_note?
@target_url = namespace_project_merge_request_url(*note_target_url_options)
mail_answer_thread(@merge_request, note_thread_options(recipient_id))
end
......@@ -56,15 +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
@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
......@@ -111,7 +111,7 @@ class Notify < BaseMailer
headers["X-GitLab-#{model.class.name}-ID"] = model.id
headers['X-GitLab-Reply-Key'] = reply_key
if Gitlab::IncomingEmail.enabled?
if Gitlab::IncomingEmail.enabled? && @sent_notification
address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key))
address.display_name = @project.name_with_namespace
......@@ -176,6 +176,6 @@ class Notify < BaseMailer
end
headers['List-Unsubscribe'] = list_unsubscribe_methods.map { |e| "<#{e}>" }.join(',')
@sent_notification_url = unsubscribe_sent_notification_url(@sent_notification)
@unsubscribe_url = unsubscribe_sent_notification_url(@sent_notification)
end
end
......@@ -2,6 +2,7 @@ class Commit
extend ActiveModel::Naming
include ActiveModel::Conversion
include Noteable
include Participable
include Mentionable
include Referable
......@@ -203,6 +204,10 @@ class Commit
project.notes.for_commit_id(self.id)
end
def discussion_notes
notes.non_diff_notes
end
def notes_with_associations
notes.includes(:author)
end
......
# Contains functionality shared between `DiffDiscussion` and `LegacyDiffDiscussion`.
module DiscussionOnDiff
extend ActiveSupport::Concern
included do
NUMBER_OF_TRUNCATED_DIFF_LINES = 16
memoized_values << :active
delegate :line_code,
:original_line_code,
:diff_file,
:diff_line,
:for_line?,
:active?,
to: :first_note
delegate :file_path,
:blob,
:highlighted_diff_lines,
:diff_lines,
to: :diff_file,
allow_nil: true
end
def diff_discussion?
true
end
def active?
return @active if @active.present?
@active = first_note.active?
end
# Returns an array of at most 16 highlighted lines above a diff note
def truncated_diff_lines(highlight: true)
lines = highlight ? highlighted_diff_lines : diff_lines
prev_lines = []
lines.each do |line|
if line.meta?
prev_lines.clear
else
prev_lines << line
break if for_line?(line)
prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
end
end
prev_lines
end
end
# Module that can be included into a model to make it easier to ignore database
# columns.
#
# Example:
#
# class User < ActiveRecord::Base
# include IgnorableColumn
#
# ignore_column :updated_at
# end
#
module IgnorableColumn
extend ActiveSupport::Concern
module ClassMethods
def columns
super.reject { |column| ignored_columns.include?(column.name) }
end
def ignored_columns
@ignored_columns ||= Set.new
end
def ignore_column(name)
ignored_columns << name.to_s
end
end
end
......@@ -292,17 +292,6 @@ module Issuable
self.class.to_ability_name
end
# Convert this Issuable class name to a format usable by notifications.
#
# Examples:
#
# issuable.class # => MergeRequest
# issuable.human_class_name # => "merge request"
def human_class_name
@human_class_name ||= self.class.name.titleize.downcase
end
# Returns a Hash of attributes to be used for Twitter card metadata
def card_attributes
{
......
# Contains functionality shared between `DiffNote` and `LegacyDiffNote`.
module NoteOnDiff
extend ActiveSupport::Concern
......@@ -24,12 +25,4 @@ module NoteOnDiff
def diff_attributes
raise NotImplementedError
end
def can_be_award_emoji?
false
end
def to_discussion
Discussion.new([self])
end
end
module Noteable
# Names of all implementers of `Noteable` that support resolvable notes.
RESOLVABLE_TYPES = %w(MergeRequest).freeze
def base_class_name
self.class.base_class.name
end
# Convert this Noteable class name to a format usable by notifications.
#
# Examples:
#
# noteable.class # => MergeRequest
# noteable.human_class_name # => "merge request"
def human_class_name
@human_class_name ||= base_class_name.titleize.downcase
end
def supports_resolvable_notes?
RESOLVABLE_TYPES.include?(base_class_name)
end
def supports_discussions?
DiscussionNote::NOTEABLE_TYPES.include?(base_class_name)
end
def discussion_notes
notes
end
delegate :find_discussion, to: :discussion_notes
def discussions
@discussions ||= discussion_notes
.inc_relations_for_view
.discussions(self)
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
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,
:first_note,
:first_note_to_resolve,
:last_resolved_note,
:last_note
)
delegate :potentially_resolvable?, to: :first_note
delegate :resolved_at,
:resolved_by,
to: :last_resolved_note,
allow_nil: true
end
def resolvable?
return @resolvable if @resolvable.present?
@resolvable = potentially_resolvable? && notes.any?(&:resolvable?)
end
def resolved?
return @resolved if @resolved.present?
@resolved = resolvable? && notes.none?(&:to_be_resolved?)
end
def first_note
@first_note ||= notes.first
end
def first_note_to_resolve
return unless resolvable?
@first_note_to_resolve ||= notes.find(&:to_be_resolved?)
end
def last_resolved_note
return unless resolved?
@last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
end
def resolved_notes
notes.select(&:resolved?)
end
def to_be_resolved?
resolvable? && !resolved?
end
def can_resolve?(current_user)
return false unless current_user
return false unless resolvable?
current_user == self.noteable.author ||
current_user.can?(:resolve_note, self.project)
end
def resolve!(current_user)
return unless resolvable?
update { |notes| notes.resolve!(current_user) }
end
def unresolve!
return unless resolvable?
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
module ResolvableNote
extend ActiveSupport::Concern
# Names of all subclasses of `Note` that can be resolvable.
RESOLVABLE_TYPES = %w(DiffNote DiscussionNote).freeze
included do
belongs_to :resolved_by, class_name: "User"
validates :resolved_by, presence: true, if: :resolved?
# Keep this scope in sync with `#potentially_resolvable?`
scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: Noteable::RESOLVABLE_TYPES) }
# Keep this scope in sync with `#resolvable?`
scope :resolvable, -> { potentially_resolvable.user }
scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
scope :unresolved, -> { resolvable.where(resolved_at: nil) }
end
module ClassMethods
# This method must be kept in sync with `#resolve!`
def resolve!(current_user)
unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
end
# This method must be kept in sync with `#unresolve!`
def unresolve!
resolved.update_all(resolved_at: nil, resolved_by_id: nil)
end
end
# Keep this method in sync with the `potentially_resolvable` scope
def potentially_resolvable?
RESOLVABLE_TYPES.include?(self.class.name) && noteable.supports_resolvable_notes?
end
# Keep this method in sync with the `resolvable` scope
def resolvable?
potentially_resolvable? && !system?
end
def resolved?
return false unless resolvable?
self.resolved_at.present?
end
def to_be_resolved?
resolvable? && !resolved?
end
# If you update this method remember to also update `.resolve!`
def resolve!(current_user)
return unless resolvable?
return if resolved?
self.resolved_at = Time.now
self.resolved_by = current_user
save!
end
# If you update this method remember to also update `.unresolve!`
def unresolve!
return unless resolvable?
return unless resolved?
self.resolved_at = nil
self.resolved_by = nil
save!
end
end
# A discussion on merge request or commit diffs consisting of `DiffNote` notes.
#
# A discussion of this type can be resolvable.
class DiffDiscussion < Discussion
include DiscussionOnDiff
def self.note_class
DiffNote
end
delegate :position,
:original_position,
to: :first_note
def legacy_diff_discussion?
false
end
def reply_attributes
super.merge(
original_position: original_position.to_json,
position: position.to_json,
)
end
end
# A note on merge request or commit diffs
#
# A note of this type can be resolvable.
class DiffNote < Note
include NoteOnDiff
NOTEABLE_TYPES = %w(MergeRequest Commit).freeze
serialize :original_position, Gitlab::Diff::Position
serialize :position, Gitlab::Diff::Position
......@@ -8,59 +13,31 @@ class DiffNote < Note
validates :position, presence: true
validates :diff_line, presence: true
validates :line_code, presence: true, line_code: true
validates :noteable_type, inclusion: { in: %w(Commit MergeRequest) }
validates :resolved_by, presence: true, if: :resolved?
validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
validate :positions_complete
validate :verify_supported
# Keep this scope in sync with the logic in `#resolvable?`
scope :resolvable, -> { user.where(noteable_type: 'MergeRequest') }
scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
scope :unresolved, -> { resolvable.where(resolved_at: nil) }
after_initialize :ensure_original_discussion_id
before_validation :set_original_position, :update_position, on: :create
before_validation :set_line_code, :set_original_discussion_id
# We need to do this again, because it's already in `Note`, but is affected by
# `update_position` and needs to run after that.
before_validation :set_discussion_id
before_validation :set_line_code
after_save :keep_around_commits
class << self
def build_discussion_id(noteable_type, noteable_id, position)
[super(noteable_type, noteable_id), *position.key].join("-")
end
# This method must be kept in sync with `#resolve!`
def resolve!(current_user)
unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
end
# This method must be kept in sync with `#unresolve!`
def unresolve!
resolved.update_all(resolved_at: nil, resolved_by_id: nil)
end
end
def new_diff_note?
true
def discussion_class(*)
DiffDiscussion
end
def diff_attributes
{ position: position.to_json }
end
%i(original_position position).each do |meth|
define_method "#{meth}=" do |new_position|
if new_position.is_a?(String)
new_position = JSON.parse(new_position) rescue nil
end
def position=(new_position)
if new_position.is_a?(String)
new_position = JSON.parse(new_position) rescue nil
end
if new_position.is_a?(Hash)
new_position = new_position.with_indifferent_access
new_position = Gitlab::Diff::Position.new(new_position)
end
if new_position.is_a?(Hash)
new_position = new_position.with_indifferent_access
new_position = Gitlab::Diff::Position.new(new_position)
super(new_position)
end
super(new_position)
end
def diff_file
......@@ -88,43 +65,6 @@ class DiffNote < Note
self.position.diff_refs == diff_refs
end
# If you update this method remember to also update the scope `resolvable`
def resolvable?
!system? && for_merge_request?
end
def resolved?
return false unless resolvable?
self.resolved_at.present?
end
# If you update this method remember to also update `.resolve!`
def resolve!(current_user)
return unless resolvable?
return if resolved?
self.resolved_at = Time.now
self.resolved_by = current_user
save!
end
# If you update this method remember to also update `.unresolve!`
def unresolve!
return unless resolvable?
return unless resolved?
self.resolved_at = nil
self.resolved_by = nil
save!
end
def discussion
return unless resolvable?
self.noteable.find_diff_discussion(self.discussion_id)
end
private
def supported?
......@@ -140,33 +80,13 @@ class DiffNote < Note
end
def set_original_position
self.original_position = self.position.dup
self.original_position = self.position.dup unless self.original_position&.complete?
end
def set_line_code
self.line_code = self.position.line_code(self.project.repository)
end
def ensure_original_discussion_id
return unless self.persisted?
return if self.original_discussion_id
set_original_discussion_id
update_column(:original_discussion_id, self.original_discussion_id)
end
def set_original_discussion_id
self.original_discussion_id = Digest::SHA1.hexdigest(build_original_discussion_id)
end
def build_discussion_id
self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position)
end
def build_original_discussion_id
self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position)
end
def update_position
return unless supported?
return if for_commit?
......
# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes.
#
# A discussion of this type can be resolvable.
class Discussion
NUMBER_OF_TRUNCATED_DIFF_LINES = 16
include ResolvableDiscussion
attr_reader :notes
attr_reader :notes, :context_noteable
delegate :created_at,
:project,
......@@ -11,43 +14,62 @@ class Discussion
:for_commit?,
:for_merge_request?,
:line_code,
:original_line_code,
:diff_file,
:for_line?,
:active?,
to: :first_note
delegate :resolved_at,
:resolved_by,
def self.build(notes, context_noteable = nil)
notes.first.discussion_class(context_noteable).new(notes, context_noteable)
end
to: :last_resolved_note,
allow_nil: true
def self.build_collection(notes, context_noteable = nil)
notes.group_by { |n| n.discussion_id(context_noteable) }.values.map { |notes| build(notes, context_noteable) }
end
delegate :blob,
:highlighted_diff_lines,
:diff_lines,
# Returns an alphanumeric discussion ID based on `build_discussion_id`
def self.discussion_id(note)
Digest::SHA1.hexdigest(build_discussion_id(note).join("-"))
end
to: :diff_file,
allow_nil: true
# Returns an array of discussion ID components
def self.build_discussion_id(note)
[*base_discussion_id(note), SecureRandom.hex]
end
def self.for_notes(notes)
notes.group_by(&:discussion_id).values.map { |notes| new(notes) }
def self.base_discussion_id(note)
noteable_id = note.noteable_id || note.commit_id
[:discussion, note.noteable_type.try(:underscore), noteable_id]
end
def self.for_diff_notes(notes)
notes.group_by(&:line_code).values.map { |notes| new(notes) }
# 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 turn a list of notes into a list of discussions, they are grouped by discussion ID, so 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)
@notes = notes
def self.note_class
DiscussionNote
end
def last_resolved_note
return unless resolved?
def initialize(notes, context_noteable = nil)
@notes = notes
@context_noteable = context_noteable
end
@last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
def ==(other)
other.class == self.class &&
other.context_noteable == self.context_noteable &&
other.id == self.id &&
other.notes == self.notes
end
def last_updated_at
......@@ -59,91 +81,29 @@ class Discussion
end
def id
first_note.discussion_id
first_note.discussion_id(context_noteable)
end
alias_method :to_param, :id
def diff_discussion?
first_note.diff_note?
end
def legacy_diff_discussion?
notes.any?(&:legacy_diff_note?)
false
end
def resolvable?
return @resolvable if @resolvable.present?
@resolvable = diff_discussion? && notes.any?(&:resolvable?)
def individual_note?
false
end
def resolved?
return @resolved if @resolved.present?
@resolved = resolvable? && notes.none?(&:to_be_resolved?)
end
def first_note
@first_note ||= @notes.first
end
def first_note_to_resolve
@first_note_to_resolve ||= notes.detect(&:to_be_resolved?)
def new_discussion?
notes.length == 1
end
def last_note
@last_note ||= @notes.last
end
def resolved_notes
notes.select(&:resolved?)
end
def to_be_resolved?
resolvable? && !resolved?
end
def can_resolve?(current_user)
return false unless current_user
return false unless resolvable?
current_user == self.noteable.author ||
current_user.can?(:resolve_note, self.project)
end
def resolve!(current_user)
return unless resolvable?
update { |notes| notes.resolve!(current_user) }
end
def unresolve!
return unless resolvable?
update { |notes| notes.unresolve! }
end
def for_target?(target)
self.noteable == target && !diff_discussion?
end
def active?
return @active if @active.present?
@active = first_note.active?
@last_note ||= notes.last
end
def collapsed?
return false unless diff_discussion?
if resolvable?
# New diff discussions only disappear once they are marked resolved
resolved?
else
# Old diff discussions disappear once they become outdated
!active?
end
resolved?
end
def expanded?
......@@ -151,52 +111,6 @@ class Discussion
end
def reply_attributes
data = {
noteable_type: first_note.noteable_type,
noteable_id: first_note.noteable_id,
commit_id: first_note.commit_id,
discussion_id: self.id,
}
if diff_discussion?
data[:note_type] = first_note.type
data.merge!(first_note.diff_attributes)
end
data
end
# Returns an array of at most 16 highlighted lines above a diff note
def truncated_diff_lines(highlight: true)
lines = highlight ? highlighted_diff_lines : diff_lines
prev_lines = []
lines.each do |line|
if line.meta?
prev_lines.clear
else
prev_lines << line
break if for_line?(line)
prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
end
end
prev_lines
end
private
def update
notes_relation = DiffNote.where(id: notes.map(&:id)).fresh
yield(notes_relation)
# Set the notes array to the updated notes
@notes = notes_relation.to_a
# Reset the memoized values
@last_resolved_note = @resolvable = @resolved = @first_note = @last_note = nil
first_note.slice(:type, :noteable_type, :noteable_id, :commit_id, :discussion_id)
end
end
# A note in a non-diff discussion on an issue, merge request, commit, or snippet.
#
# A note of this type can be resolvable.
class DiscussionNote < Note
# Names of all implementers of `Noteable` that support discussions.
NOTEABLE_TYPES = %w(MergeRequest Issue Commit Snippet).freeze
validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
def discussion_class(*)
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.
#
# A discussion of this type is never resolvable.
class IndividualNoteDiscussion < Discussion
def self.note_class
Note
end
def individual_note?
true
end
end
......@@ -3,6 +3,7 @@ require 'carrierwave/orm/activerecord'
class Issue < ActiveRecord::Base
include InternalId
include Issuable
include Noteable
include Referable
include Sortable
include Spammable
......
# 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`.
#
# A discussion of this type is never resolvable.
class LegacyDiffDiscussion < Discussion
include DiscussionOnDiff
def legacy_diff_discussion?
true
end
def self.note_class
LegacyDiffNote
end
def collapsed?
!active?
end
def reply_attributes
super.merge(line_code: line_code)
end
end
# 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`.
#
# A note of this type is never resolvable.
class LegacyDiffNote < Note
include NoteOnDiff
......@@ -7,18 +13,8 @@ class LegacyDiffNote < Note
before_create :set_diff
class << self
def build_discussion_id(noteable_type, noteable_id, line_code)
[super(noteable_type, noteable_id), line_code].join("-")
end
end
def legacy_diff_note?
true
end
def diff_attributes
{ line_code: line_code }
def discussion_class(*)
LegacyDiffDiscussion
end
def project_repository
......@@ -119,8 +115,4 @@ class LegacyDiffNote < Note
diffs = noteable.raw_diffs(Commit.max_diff_options)
diffs.find { |d| d.new_path == self.diff.new_path }
end
def build_discussion_id
self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code)
end
end
class MergeRequest < ActiveRecord::Base
include InternalId
include Issuable
include Noteable
include Referable
include Sortable
......@@ -475,43 +476,7 @@ class MergeRequest < ActiveRecord::Base
)
end
def discussions
@discussions ||= self.related_notes.
inc_relations_for_view.
fresh.
discussions
end
def diff_discussions
@diff_discussions ||= self.notes.diff_notes.discussions
end
def resolvable_discussions
@resolvable_discussions ||= diff_discussions.select(&:to_be_resolved?)
end
def discussions_can_be_resolved_by?(user)
resolvable_discussions.all? { |discussion| discussion.can_resolve?(user) }
end
def find_diff_discussion(discussion_id)
notes = self.notes.diff_notes.where(discussion_id: discussion_id).fresh.to_a
return if notes.empty?
Discussion.new(notes)
end
def discussions_resolvable?
diff_discussions.any?(&:resolvable?)
end
def discussions_resolved?
discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?)
end
def discussions_to_be_resolved?
discussions_resolvable? && !discussions_resolved?
end
alias_method :discussion_notes, :related_notes
def mergeable_discussions_state?
return true unless project.only_allow_merge_if_all_discussions_are_resolved?
......@@ -857,8 +822,8 @@ class MergeRequest < ActiveRecord::Base
return unless has_complete_diff_refs?
return if new_diff_refs == old_diff_refs
active_diff_notes = self.notes.diff_notes.select do |note|
note.new_diff_note? && note.active?(old_diff_refs)
active_diff_notes = self.notes.new_diff_notes.select do |note|
note.active?(old_diff_refs)
end
return if active_diff_notes.empty?
......
# A note on the root of an issue, merge request, commit, or snippet.
#
# A note of this type is never resolvable.
class Note < ActiveRecord::Base
extend ActiveModel::Naming
include Gitlab::CurrentSettings
......@@ -8,6 +11,10 @@ class Note < ActiveRecord::Base
include FasterCacheKeys
include CacheMarkdownField
include AfterCommitQueue
include ResolvableNote
include IgnorableColumn
ignore_column :original_discussion_id
cache_markdown_field :note, pipeline: :note
......@@ -32,9 +39,6 @@ class Note < ActiveRecord::Base
belongs_to :author, class_name: "User"
belongs_to :updated_by, class_name: "User"
# Only used by DiffNote, but defined here so that it can be used in `Note.includes`
belongs_to :resolved_by, class_name: "User"
has_many :todos, dependent: :destroy
has_many :events, as: :target, dependent: :destroy
has_one :system_note_metadata
......@@ -54,10 +58,11 @@ class Note < ActiveRecord::Base
validates :noteable_id, presence: true, unless: [:for_commit?, :importing?]
validates :commit_id, presence: true, if: :for_commit?
validates :author, presence: true
validates :discussion_id, presence: true, format: { with: /\A\h{40}\z/ }
validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note|
unless note.noteable.try(:project) == note.project
errors.add(:invalid_project, 'Note and noteable project mismatch')
errors.add(:project, 'does not match noteable project')
end
end
......@@ -69,6 +74,7 @@ class Note < ActiveRecord::Base
scope :user, ->{ where(system: false) }
scope :common, ->{ where(noteable_type: ["", nil]) }
scope :fresh, ->{ order(created_at: :asc, id: :asc) }
scope :updated_after, ->(time){ where('updated_at > ?', time) }
scope :inc_author_project, ->{ includes(:project, :author) }
scope :inc_author, ->{ includes(:author) }
scope :inc_relations_for_view, -> do
......@@ -76,7 +82,8 @@ class Note < ActiveRecord::Base
end
scope :diff_notes, ->{ where(type: %w(LegacyDiffNote DiffNote)) }
scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
scope :new_diff_notes, ->{ where(type: 'DiffNote') }
scope :non_diff_notes, ->{ where(type: ['Note', 'DiscussionNote', nil]) }
scope :with_associations, -> do
# FYI noteable cannot be loaded for LegacyDiffNote for commits
......@@ -86,7 +93,7 @@ class Note < ActiveRecord::Base
after_initialize :ensure_discussion_id
before_validation :nullify_blank_type, :nullify_blank_line_code
before_validation :set_discussion_id
before_validation :set_discussion_id, on: :create
after_save :keep_around_commit, unless: :for_personal_snippet?
after_save :expire_etag_cache
......@@ -95,22 +102,23 @@ class Note < ActiveRecord::Base
ActiveModel::Name.new(self, nil, 'note')
end
def build_discussion_id(noteable_type, noteable_id)
[:discussion, noteable_type.try(:underscore), noteable_id].join("-")
def discussions(context_noteable = nil)
Discussion.build_collection(fresh, context_noteable)
end
def discussion_id(*args)
Digest::SHA1.hexdigest(build_discussion_id(*args))
end
def find_discussion(discussion_id)
notes = where(discussion_id: discussion_id).fresh.to_a
return if notes.empty?
def discussions
Discussion.for_notes(fresh)
Discussion.build(notes)
end
def grouped_diff_discussions
active_notes = diff_notes.fresh.select(&:active?)
Discussion.for_diff_notes(active_notes).
map { |d| [d.line_code, d] }.to_h
diff_notes.
fresh.
discussions.
select(&:active?).
group_by(&:line_code)
end
def count_for_collection(ids, type)
......@@ -121,37 +129,17 @@ class Note < ActiveRecord::Base
end
def cross_reference?
system && SystemNoteService.cross_reference?(note)
system? && SystemNoteService.cross_reference?(note)
end
def diff_note?
false
end
def legacy_diff_note?
false
end
def new_diff_note?
false
end
def active?
true
end
def resolvable?
false
end
def resolved?
false
end
def to_be_resolved?
resolvable? && !resolved?
end
def max_attachment_size
current_application_settings.max_attachment_size.megabytes.to_i
end
......@@ -228,7 +216,7 @@ class Note < ActiveRecord::Base
end
def can_be_award_emoji?
noteable.is_a?(Awardable)
noteable.is_a?(Awardable) && !part_of_discussion?
end
def contains_emoji_only?
......@@ -239,6 +227,63 @@ class Note < ActiveRecord::Base
for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore
end
def can_be_discussion_note?
self.noteable.supports_discussions? && !part_of_discussion?
end
def discussion_class(noteable = nil)
# When commit notes are rendered on an MR's Discussion page, they are
# displayed in one discussion instead of individually.
# See also `#discussion_id` and `Discussion.override_discussion_id`.
if noteable && noteable != self.noteable
OutOfContextDiscussion
else
IndividualNoteDiscussion
end
end
# See `Discussion.override_discussion_id` for details.
def discussion_id(noteable = nil)
discussion_class(noteable).override_discussion_id(self) || super()
end
# Returns a discussion containing just this note.
# This method exists as an alternative to `#discussion` to use when the methods
# we intend to call on the Discussion object don't require it to have all of its notes,
# and just depend on the first note or the type of discussion. This saves us a DB query.
def to_discussion(noteable = nil)
Discussion.build([self], noteable)
end
# Returns the entire discussion this note is part of.
# Consider using `#to_discussion` if we do not need to render the discussion
# and all its notes and if we don't care about the discussion's resolvability status.
def discussion
full_discussion = self.noteable.notes.find_discussion(self.discussion_id) if part_of_discussion?
full_discussion || to_discussion
end
def part_of_discussion?
!to_discussion.individual_note?
end
def in_reply_to?(other)
case other
when Note
if part_of_discussion?
in_reply_to?(other.noteable) && in_reply_to?(other.to_discussion)
else
in_reply_to?(other.noteable)
end
when Discussion
self.discussion_id == other.id
when Noteable
self.noteable == other
else
false
end
end
private
def keep_around_commit
......@@ -264,17 +309,7 @@ class Note < ActiveRecord::Base
end
def set_discussion_id
self.discussion_id = Digest::SHA1.hexdigest(build_discussion_id)
end
def build_discussion_id
if for_merge_request?
# Notes on merge requests are always in a discussion of their own,
# so we generate a unique discussion ID.
[:discussion, :note, SecureRandom.hex].join("-")
else
self.class.build_discussion_id(noteable_type, noteable_id || commit_id)
end
self.discussion_id ||= discussion_class.discussion_id(self)
end
def expire_etag_cache
......
# 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.
#
# A discussion of this type is never resolvable.
class OutOfContextDiscussion < 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)
discussion_id(note)
end
def self.note_class
Note
end
end
......@@ -5,10 +5,11 @@ class SentNotification < ActiveRecord::Base
belongs_to :noteable, polymorphic: true
belongs_to :recipient, class_name: "User"
validates :project, :recipient, :reply_key, presence: true
validates :reply_key, uniqueness: true
validates :project, :recipient, presence: true
validates :reply_key, presence: true, uniqueness: true
validates :noteable_id, presence: true, unless: :for_commit?
validates :commit_id, presence: true, if: :for_commit?
validates :in_reply_to_discussion_id, format: { with: /\A\h{40}\z/, allow_nil: true }
validate :note_valid
after_save :keep_around_commit
......@@ -22,9 +23,7 @@ class SentNotification < ActiveRecord::Base
find_by(reply_key: reply_key)
end
def record(noteable, recipient_id, reply_key, attrs = {})
return unless reply_key
def record(noteable, recipient_id, reply_key = self.reply_key, attrs = {})
noteable_id = nil
commit_id = nil
if noteable.is_a?(Commit)
......@@ -34,23 +33,20 @@ class SentNotification < ActiveRecord::Base
end
attrs.reverse_merge!(
project: noteable.project,
noteable_type: noteable.class.name,
noteable_id: noteable_id,
commit_id: commit_id,
recipient_id: recipient_id,
reply_key: reply_key
project: noteable.project,
recipient_id: recipient_id,
reply_key: reply_key,
noteable_type: noteable.class.name,
noteable_id: noteable_id,
commit_id: commit_id,
)
create(attrs)
end
def record_note(note, recipient_id, reply_key, attrs = {})
if note.diff_note?
attrs[:note_type] = note.type
attrs.merge!(note.diff_attributes)
end
def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {})
attrs[:in_reply_to_discussion_id] = note.discussion_id
record(note.noteable, recipient_id, reply_key, attrs)
end
......@@ -89,31 +85,45 @@ class SentNotification < ActiveRecord::Base
self.reply_key
end
def note_attributes
{
project: self.project,
author: self.recipient,
type: self.note_type,
noteable_type: self.noteable_type,
noteable_id: self.noteable_id,
commit_id: self.commit_id,
line_code: self.line_code,
position: self.position.to_json
}
end
def create_note(note)
Notes::CreateService.new(
self.project,
self.recipient,
self.note_attributes.merge(note: note)
).execute
def create_reply(message, dryrun: false)
klass = dryrun ? Notes::BuildService : Notes::CreateService
klass.new(self.project, self.recipient, reply_params.merge(note: message)).execute
end
private
def reply_params
attrs = {
noteable_type: self.noteable_type,
noteable_id: self.noteable_id,
commit_id: self.commit_id
}
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,
# LegacyDiffNote
line_code: self.line_code,
# DiffNote
position: self.position.to_json
)
end
attrs
end
def note_valid
Note.new(note_attributes.merge(note: "Test")).valid?
note = create_reply('Test', dryrun: true)
unless note.valid?
self.errors.add(:base, "Note parameters are invalid: #{note.errors.full_messages.to_sentence}")
end
end
def keep_around_commit
......
......@@ -2,6 +2,7 @@ class Snippet < ActiveRecord::Base
include Gitlab::VisibilityLevel
include Linguist::BlobHelper
include CacheMarkdownField
include Noteable
include Participable
include Referable
include Sortable
......
......@@ -21,11 +21,11 @@ module Issues
@discussions_to_resolve ||=
if discussion_to_resolve_id
discussion_or_nil = merge_request_to_resolve_discussions_of
.find_diff_discussion(discussion_to_resolve_id)
.find_discussion(discussion_to_resolve_id)
Array(discussion_or_nil)
else
merge_request_to_resolve_discussions_of
.resolvable_discussions
.discussions_to_be_resolved
end
end
end
......
......@@ -35,14 +35,19 @@ module Issues
end
def item_for_discussion(discussion)
first_note = discussion.first_note_to_resolve || discussion.first_note
first_note_to_resolve = discussion.first_note_to_resolve || discussion.first_note
is_very_first_note = first_note_to_resolve == discussion.first_note
action = is_very_first_note ? "started" : "commented on"
note_url = Gitlab::UrlBuilder.build(first_note_to_resolve)
other_note_count = discussion.notes.size - 1
note_url = Gitlab::UrlBuilder.build(first_note)
discussion_info = "- [ ] #{first_note.author.to_reference} commented on a [discussion](#{note_url}): "
discussion_info = "- [ ] #{first_note_to_resolve.author.to_reference} #{action} a [discussion](#{note_url}): "
discussion_info << " (+#{other_note_count} #{'comment'.pluralize(other_note_count)})" if other_note_count > 0
note_without_block_quotes = Banzai::Filter::BlockquoteFenceFilter.new(first_note.note).call
note_without_block_quotes = Banzai::Filter::BlockquoteFenceFilter.new(first_note_to_resolve.note).call
spaces = ' ' * 4
quote = note_without_block_quotes.lines.map { |line| "#{spaces}> #{line}" }.join
......
module Notes
class BuildService < ::BaseService
def execute
in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
if project && in_reply_to_discussion_id.present?
discussion = project.notes.find_discussion(in_reply_to_discussion_id)
unless discussion
note = Note.new
note.errors.add(:base, 'Discussion to reply to cannot be found')
return note
end
params.merge!(discussion.reply_attributes)
end
note = Note.new(params)
note.project = project
note.author = current_user
note
end
end
end
module Notes
class CreateService < BaseService
class CreateService < ::BaseService
def execute
merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
note = Note.new(params)
note.project = project
note.author = current_user
note.system = false
note = Notes::BuildService.new(project, current_user, params).execute
return note unless note.valid?
# We execute commands (extracted from `params[:note]`) on the noteable
# **before** we save the note because if the note consists of commands
......
......@@ -228,12 +228,10 @@ module SystemNoteService
def discussion_continued_in_issue(discussion, project, author, issue)
body = "created #{issue.to_reference} to continue this discussion"
note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
note_params = discussion.reply_attributes.merge(project: project, author: author, note: body)
note_params[:type] = note_params.delete(:note_type)
note = Note.create(note_params.merge(system: true))
note.system_note_metadata = SystemNoteMetadata.new({ action: 'discussion' })
note = Note.create(note_attributes.merge(system: true))
note.system_note_metadata = SystemNoteMetadata.new(action: 'discussion')
note
end
......
......@@ -3,4 +3,4 @@
%td.notes_line{ colspan: 2 }
%td.notes_content
.content{ class: ('hide' unless expanded) }
= render "discussions/notes", discussion: discussion
= render partial: "discussions/notes", collection: discussions, as: :discussion
......@@ -7,7 +7,7 @@
.diff-content.code.js-syntax-highlight
%table
- discussions = { discussion.original_line_code => discussion }
- discussions = { discussion.original_line_code => [discussion] }
= render partial: "projects/diffs/line",
collection: discussion.truncated_diff_lines,
as: :line,
......
......@@ -5,7 +5,7 @@
= link_to user_path(discussion.author) do
= image_tag avatar_icon(discussion.author), class: "avatar s40"
.timeline-content
.discussion.js-toggle-container{ class: discussion.id, data: { discussion_id: discussion.id } }
.discussion.js-toggle-container{ data: { discussion_id: discussion.id } }
.discussion-header
.discussion-actions
%button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button" }
......@@ -18,19 +18,21 @@
.inline.discussion-headline-light
= discussion.author.to_reference
started a discussion on
started a discussion
- if discussion.for_commit?
- if discussion.for_commit? && @noteable != discussion.noteable
on
- commit = discussion.noteable
- if commit
commit
= link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code), class: 'monospace'
- anchor = discussion.line_code if discussion.diff_discussion?
= link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: anchor), class: 'monospace'
- else
a deleted commit
- else
- elsif discussion.diff_discussion?
on
- if discussion.active?
= link_to diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code) do
the diff
= link_to 'the diff', discussion_diff_path(discussion)
- else
an outdated diff
......
%ul.notes{ data: { discussion_id: discussion.id } }
= render partial: "projects/notes/note", collection: discussion.notes, as: :note
.discussion-notes
%ul.notes{ data: { discussion_id: discussion.id } }
= render partial: "projects/notes/note", collection: discussion.notes, as: :note
- if current_user
.discussion-reply-holder
- if discussion.diff_discussion?
- line_type = local_assigns.fetch(:line_type, nil)
- if current_user
.discussion-reply-holder
- if discussion.potentially_resolvable?
- line_type = local_assigns.fetch(:line_type, nil)
.btn-group-justified.discussion-with-resolve-btn{ role: "group" }
.btn-group{ role: "group" }
= link_to_reply_discussion(discussion, line_type)
= render "discussions/resolve_all", discussion: discussion
.btn-group-justified.discussion-with-resolve-btn{ role: "group" }
.btn-group{ role: "group" }
= link_to_reply_discussion(discussion, line_type)
= render "discussions/resolve_all", discussion: discussion
- if discussion.for_merge_request?
.btn-group.discussion-actions
= render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable
= render "discussions/jump_to_next", discussion: discussion
- else
= link_to_reply_discussion(discussion)
- else
= link_to_reply_discussion(discussion)
- expanded = discussion_left.try(:expanded?) || discussion_right.try(:expanded?)
- expanded = [*discussions_left, *discussions_right].any?(&:expanded?)
%tr.notes_holder{ class: ('hide' unless expanded) }
- if discussion_left
- if discussions_left
%td.notes_line.old
%td.notes_content.parallel.old
.content{ class: ('hide' unless discussion_left.expanded?) }
= render "discussions/notes", discussion: discussion_left, line_type: 'old'
.content{ class: ('hide' unless discussions_left.any?(&:expanded?)) }
= render partial: "discussions/notes", collection: discussions_left, as: :discussion, line_type: 'old'
- else
%td.notes_line.old= ("")
%td.notes_content.parallel.old
.content
- if discussion_right
- if discussions_right
%td.notes_line.new
%td.notes_content.parallel.new
.content{ class: ('hide' unless discussion_right.expanded?) }
= render "discussions/notes", discussion: discussion_right, line_type: 'new'
.content{ class: ('hide' unless discussions_right.any?(&:expanded?)) }
= render partial: "discussions/notes", collection: discussions_right, as: :discussion, line_type: 'new'
- else
%td.notes_line.new= ("")
%td.notes_content.parallel.new
......
- if discussion.for_merge_request?
%resolve-discussion-btn{ ":discussion-id" => "'#{discussion.id}'",
":merge-request-id" => discussion.noteable.iid,
":can-resolve" => discussion.can_resolve?(current_user),
"inline-template" => true }
.btn-group{ role: "group", "v-if" => "showButton" }
%button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading", "v-cloak" => "true" }
= icon("spinner spin", "v-show" => "loading")
{{ buttonText }}
%resolve-discussion-btn{ ":discussion-id" => "'#{discussion.id}'",
":merge-request-id" => discussion.noteable.iid,
":can-resolve" => discussion.can_resolve?(current_user),
"inline-template" => true }
.btn-group{ role: "group", "v-if" => "showButton" }
%button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading", "v-cloak" => "true" }
= icon("spinner spin", "v-show" => "loading")
{{ buttonText }}
<%= yield -%>
---
You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>.
= yield
You're receiving this email because of your account on #{Gitlab.config.gitlab.host}.
Manage all notifications: #{profile_notifications_url}
Help: #{help_url}
......@@ -25,8 +25,8 @@
- if @labels_url
adjust your #{link_to 'label subscriptions', @labels_url}.
- else
- if @sent_notification_url
= link_to "unsubscribe", @sent_notification_url
- if @unsubscribe_url
= link_to "unsubscribe", @unsubscribe_url
from this thread or
adjust your notification settings.
......
<%= yield -%>
---
<% if @target_url -%>
<% if @reply_by_email -%>
<%= "Reply to this email directly or view it on GitLab: #{@target_url}" -%>
<% else -%>
<%= "View it on GitLab: #{@target_url}" -%>
<% end -%>
<% end -%>
You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>.
- discussion = @note.discussion if @note.part_of_discussion?
- if discussion
%p.details
= succeed ':' do
= link_to @note.author_name, user_url(@note.author)
- if discussion.diff_discussion?
- if discussion.new_discussion?
started a new discussion
- else
commented on a discussion
on #{link_to discussion.file_path, @target_url}
- else
- if discussion.new_discussion?
started a new discussion
- else
commented on a #{link_to 'discussion', @target_url}
- elsif current_application_settings.email_author_in_body
%p.details
#{link_to @note.author_name, user_url(@note.author)} commented:
- if discussion&.diff_discussion?
= content_for :head do
= stylesheet_link_tag 'mailers/highlighted_diff_email'
%table
= render partial: "projects/diffs/line",
collection: discussion.truncated_diff_lines,
as: :line,
locals: { diff_file: discussion.diff_file,
plain: true,
email: true }
%div
= markdown(@note.note, pipeline: :email, author: @note.author)
<% discussion = @note.discussion if @note.part_of_discussion? -%>
<% if discussion && !discussion.individual_note? -%>
<%= @note.author_name -%>
<% if discussion.new_discussion? -%>
<%= " started a new discussion" -%>
<% else -%>
<%= " commented on a discussion" -%>
<% end -%>
<% if discussion.diff_discussion? -%>
<%= " on #{discussion.file_path}" -%>
<% end -%>
<%= ":" -%>
<% elsif current_application_settings.email_author_in_body -%>
<%= "#{@note.author_name} commented:" -%>
<% end -%>
<% if discussion&.diff_discussion? -%>
<% discussion.truncated_diff_lines(highlight: false).each do |line| -%>
<%= "> #{line.text}\n" -%>
<% end -%>
<% end -%>
<%= @note.note -%>
- if current_application_settings.email_author_in_body
%div
#{link_to @note.author_name, user_url(@note.author)} wrote:
%div
= markdown(@note.note, pipeline: :email, author: @note.author)
<% if current_application_settings.email_author_in_body %>
<%= @note.author_name %> wrote:
<% end -%>
<%= @note.note %>
= content_for :head do
= stylesheet_link_tag 'mailers/highlighted_diff_email'
New comment
- if @discussion && @discussion.diff_file
on
= link_to @note.diff_file.file_path, @target_url, class: 'details'
\:
%table
= render partial: "projects/diffs/line",
collection: @discussion.truncated_diff_lines,
as: :line,
locals: { diff_file: @note.diff_file,
plain: true,
email: true }
= render 'note_message'
<% if @discussion && @discussion.diff_file -%>
on <%= @note.diff_file.file_path -%>
<% end -%>:
<%= url %>
<%= render 'simple_diff' if @discussion -%>
<%= render 'note_message' %>
<% @discussion.truncated_diff_lines(highlight: false).each do |line| %>
> <%= line.text %>
<% end %>
- if current_application_settings.email_author_in_body
%div
#{link_to @issue.author_name, user_url(@issue.author)} wrote:
- if @issue.description
= markdown(@issue.description, pipeline: :email, author: @issue.author)
%p.details
#{link_to @issue.author_name, user_url(@issue.author)} created an issue:
- if @issue.assignee_id.present?
%p
Assignee: #{@issue.assignee_name}
- if @issue.description
%div
= markdown(@issue.description, pipeline: :email, author: @issue.author)
%p
You have been mentioned in an issue.
- if current_application_settings.email_author_in_body
%div
#{link_to @issue.author_name, user_url(@issue.author)} wrote:
- if @issue.description
= markdown(@issue.description, pipeline: :email, author: @issue.author)
- if @issue.assignee_id.present?
%p
Assignee: #{@issue.assignee_name}
= render template: 'notify/new_issue_email'
%p
You have been mentioned in Merge Request #{@merge_request.to_reference}
- if current_application_settings.email_author_in_body
%div
#{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote:
%p.details
!= merge_path_description(@merge_request, '&rarr;')
- if @merge_request.assignee_id.present?
%p
Assignee: #{@merge_request.author_name} &rarr; #{@merge_request.assignee_name}
- if @merge_request.description
= markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)
= render template: 'notify/new_merge_request_email'
- if current_application_settings.email_author_in_body
%div
#{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote:
%p.details
#{link_to @merge_request.author_name, user_url(@merge_request.author)} created a merge request:
%p.details
!= merge_path_description(@merge_request, '&rarr;')
- if @merge_request.assignee_id.present?
%p
Assignee: #{@merge_request.author_name} &rarr; #{@merge_request.assignee_name}
Assignee: #{@merge_request.assignee_name}
- if @merge_request.description
= markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)
%div
= markdown(@merge_request.description, pipeline: :email, author: @merge_request.author)
%p.details
= render 'note_mr_or_commit_email'
= render 'note_email'
New comment for Commit <%= @commit.short_id -%>
<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url } %>
<%= render 'note_email' %>
= render 'note_message'
= render 'note_email'
New comment for Issue <%= @issue.iid %>
<%= url_for(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue, anchor: "note_#{@note.id}")) %>
Author: <%= @note.author_name %>
<%= @note.note %>
<%= render 'note_email' %>
%p.details
= render 'note_mr_or_commit_email'
= render 'note_email'
New comment for Merge Request <%= @merge_request.to_reference -%>
<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url }%>
<%= render 'note_email' %>
New comment for Snippet <%= @snippet.id %>
<%= url_for(snippet_url(@snippet, anchor: "note_#{@note.id}")) %>
Author: <%= @note.author_name %>
<%= @note.note %>
<%= render 'note_email' %>
= render 'note_message'
= render 'note_email'
New comment for Snippet <%= @snippet.id %>
<%= url_for(namespace_project_snippet_url(@snippet.project.namespace, @snippet.project, @snippet, anchor: "note_#{@note.id}")) %>
Author: <%= @note.author_name %>
<%= @note.note %>
<%= render 'note_email' %>
......@@ -10,6 +10,7 @@
- else
.block-connector
= render "projects/diffs/diffs", diffs: @diffs, environment: @environment
= render "projects/notes/notes_with_form"
- if can_collaborate_with_project?
- %w(revert cherry-pick).each do |type|
......
......@@ -4,7 +4,7 @@
- type = line.type
- line_code = diff_file.line_code(line)
- if discussions && !line.meta?
- discussion = discussions[line_code]
- line_discussions = discussions[line_code]
%tr.line_holder{ class: type, id: (line_code unless plain) }
- case type
- when 'match'
......@@ -20,6 +20,7 @@
= link_text
- else
%a{ href: "##{line_code}", data: { linenumber: link_text } }
- discussion = line_discussions.try(:first)
- if discussion && discussion.resolvable? && !plain
%diff-note-avatars{ "discussion-id" => discussion.id }
%td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
......@@ -34,6 +35,6 @@
- else
= diff_line_content(line.text)
- if discussion
- discussion_expanded = local_assigns.fetch(:discussion_expanded, discussion.expanded?)
= render "discussions/diff_discussion", discussion: discussion, expanded: discussion_expanded
- if line_discussions
- discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?))
= render "discussions/diff_discussion", discussions: line_discussions, expanded: discussion_expanded
......@@ -6,7 +6,7 @@
- right = line[:right]
- last_line = right.new_pos if right
- unless @diff_notes_disabled
- discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file)
- discussions_left, discussions_right = parallel_diff_discussions(left, right, diff_file)
%tr.line_holder.parallel
- if left
- case left.type
......@@ -20,6 +20,7 @@
- left_position = diff_file.position(left)
%td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } }
%a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } }
- discussion_left = discussions_left.try(:first)
- if discussion_left && discussion_left.resolvable?
%diff-note-avatars{ "discussion-id" => discussion_left.id }
%td.line_content.parallel.noteable_line{ class: left.type, data: diff_view_line_data(left_line_code, left_position, 'old') }= diff_line_content(left.text)
......@@ -39,6 +40,7 @@
- right_position = diff_file.position(right)
%td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } }
%a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } }
- discussion_right = discussions_right.try(:first)
- if discussion_right && discussion_right.resolvable?
%diff-note-avatars{ "discussion-id" => discussion_right.id }
%td.line_content.parallel.noteable_line{ class: right.type, data: diff_view_line_data(right_line_code, right_position, 'new') }= diff_line_content(right.text)
......@@ -46,8 +48,8 @@
%td.old_line.diff-line-num.empty-cell
%td.line_content.parallel
- if discussion_left || discussion_right
= render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right
- if discussions_left || discussions_right
= render "discussions/parallel_diff_discussion", discussions_left: discussions_left, discussions_right: discussions_right
- if !diff_file.new_file && !diff_file.deleted_file && diff_file.diff_lines.any?
- last_line = diff_file.diff_lines.last
- if last_line.new_pos < total_lines
......
- content_for :note_actions do
- if can?(current_user, :update_merge_request, @merge_request)
- if @merge_request.open?
= link_to 'Close merge request', merge_request_path(@merge_request, merge_request: {state_event: :close }), method: :put, class: "btn btn-nr btn-comment btn-close close-mr-link js-note-target-close", title: "Close merge request", data: {original_text: "Close merge request", alternative_text: "Comment & close merge request"}
= link_to 'Close merge request', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, class: "btn btn-nr btn-comment btn-close close-mr-link js-note-target-close", title: "Close merge request", data: { original_text: "Close merge request", alternative_text: "Comment & close merge request"}
- if @merge_request.reopenable?
= link_to 'Reopen merge request', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: "btn btn-nr btn-comment btn-reopen reopen-mr-link js-note-target-reopen", title: "Reopen merge request", data: {original_text: "Reopen merge request", alternative_text: "Comment & reopen merge request"}
= link_to 'Reopen merge request', merge_request_path(@merge_request, merge_request: { state_event: :reopen }), method: :put, class: "btn btn-nr btn-comment btn-reopen reopen-mr-link js-note-target-close js-note-target-reopen", title: "Reopen merge request", data: { original_text: "Reopen merge request", alternative_text: "Comment & reopen merge request"}
%comment-and-resolve-btn{ "inline-template" => true, ":discussion-id" => "" }
%button.btn.btn-nr.btn-default.append-right-10.js-comment-resolve-button{ "v-if" => "showButton", type: "submit", data: { project_path: "#{project_path(@merge_request.project)}" } }
{{ buttonText }}
......
- noteable_name = @note.noteable.human_class_name
.pull-left.btn-group.append-right-10.comment-type-dropdown.js-comment-type-dropdown
%input.btn.btn-nr.btn-create.comment-btn.js-comment-button.js-comment-submit-button{ type: 'submit', value: 'Comment' }
- if @note.can_be_discussion_note?
= button_tag type: 'button', class: 'btn btn-nr dropdown-toggle comment-btn js-note-new-discussion js-disable-on-submit', data: { 'dropdown-trigger' => '#resolvable-comment-menu' }, 'aria-label' => 'Open comment type dropdown' do
= icon('caret-down', class: 'toggle-icon')
%ul#resolvable-comment-menu.dropdown-menu{ data: { dropdown: true } }
%li#comment.droplab-item-selected{ data: { value: '', 'submit-text' => 'Comment', 'close-text' => "Comment & close #{noteable_name}", 'reopen-text' => "Comment & reopen #{noteable_name}" } }
%a{ href: '#' }
= icon('check')
.description
%strong Comment
%p
Add a general comment to this #{noteable_name}.
%li.divider
%li#discussion{ data: { value: 'DiscussionNote', 'submit-text' => 'Start discussion', 'close-text' => "Start discussion & close #{noteable_name}", 'reopen-text' => "Start discussion & reopen #{noteable_name}" } }
%a{ href: '#' }
= icon('check')
.description
%strong Start discussion
%p
= succeed '.' do
Discuss a specific suggestion or question
- if @note.noteable.supports_resolvable_notes?
that needs to be resolved
......@@ -4,12 +4,18 @@
= hidden_field_tag :view, diff_view
= hidden_field_tag :line_type
= hidden_field_tag :merge_request_diff_head_sha, @note.noteable.try(:diff_head_sha)
= hidden_field_tag :in_reply_to_discussion_id
= note_target_fields(@note)
= f.hidden_field :commit_id
= f.hidden_field :line_code
= f.hidden_field :noteable_id
= f.hidden_field :noteable_type
= f.hidden_field :noteable_id
= f.hidden_field :commit_id
= f.hidden_field :type
-# LegacyDiffNote
= f.hidden_field :line_code
-# DiffNote
= f.hidden_field :position
= render layout: 'projects/md_preview', locals: { preview_class: "md-preview", referenced_users: true } do
......@@ -22,7 +28,9 @@
.error-alert
.note-form-actions.clearfix
= f.submit 'Comment', class: "btn btn-nr btn-create append-right-10 comment-btn js-comment-button"
= render partial: 'projects/notes/comment_button'
= yield(:note_actions)
%a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } }
Discard draft
......@@ -34,7 +34,7 @@
- if note.resolvable?
- can_resolve = can?(current_user, :resolve_note, note)
%resolve-btn{ "project-path" => project_path(note.project),
"discussion-id" => note.discussion_id,
"discussion-id" => note.discussion_id(@noteable),
":note-id" => note.id,
":resolved" => note.resolved?,
":can-resolve" => can_resolve,
......
- if @discussions.present?
- if defined?(@discussions)
- @discussions.each do |discussion|
- if discussion.for_target?(@noteable)
= render partial: "projects/notes/note", object: discussion.first_note, as: :note
- if discussion.individual_note?
= render partial: "projects/notes/note", collection: discussion.notes, as: :note
- else
= render 'discussions/discussion', discussion: discussion
- else
......
---
title: Add option to start a new resolvable discussion in an MR
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 AddInReplyToDiscussionIdToSentNotifications < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
# When a migration requires downtime you **must** uncomment the following
# constant and define a short and easy to understand explanation as to why the
# migration requires downtime.
# DOWNTIME_REASON = ''
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def change
add_column :sent_notifications, :in_reply_to_discussion_id, :string
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class RemoveNotesOriginalDiscussionId < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def change
remove_column :notes, :original_discussion_id, :string
end
end
......@@ -12,7 +12,6 @@
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20170407140450) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
enable_extension "pg_trgm"
......@@ -757,7 +756,6 @@ ActiveRecord::Schema.define(version: 20170407140450) do
t.datetime "resolved_at"
t.integer "resolved_by_id"
t.string "discussion_id"
t.string "original_discussion_id"
t.text "note_html"
end
......@@ -1055,6 +1053,7 @@ ActiveRecord::Schema.define(version: 20170407140450) do
t.string "line_code"
t.string "note_type"
t.text "position"
t.string "in_reply_to_discussion_id"
end
add_index "sent_notifications", ["reply_key"], name: "index_sent_notifications_on_reply_key", unique: true, using: :btree
......
......@@ -347,6 +347,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
end
step 'I should see a discussion by user "John Doe" has started on diff' do
# Trigger a refresh of notes
execute_script("$(document).trigger('visibilitychange');")
wait_for_ajax
page.within(".notes .discussion") do
page.should have_content "#{user_exists("John Doe").name} #{user_exists("John Doe").to_reference} started a discussion"
page.should have_content sample_commit.line_code_path
......
......@@ -33,6 +33,10 @@ module Gitlab
new_pos unless removed? || meta?
end
def line
new_line || old_line
end
def unchanged?
type.nil?
end
......
require 'gitlab/email/handler/base_handler'
require 'gitlab/email/handler/reply_processing'
......@@ -42,17 +41,7 @@ module Gitlab
end
def create_note
Notes::CreateService.new(
project,
author,
note: message,
noteable_type: sent_notification.noteable_type,
noteable_id: sent_notification.noteable_id,
commit_id: sent_notification.commit_id,
line_code: sent_notification.line_code,
position: sent_notification.position,
type: sent_notification.note_type
).execute
sent_notification.create_reply(message)
end
end
end
......
......@@ -266,8 +266,8 @@ describe Projects::CommitController do
diff_for_path(id: commit2.id, old_path: existing_path, new_path: existing_path)
expect(assigns(:diff_notes_disabled)).to be_falsey
expect(assigns(:comments_target)).to eq(noteable_type: 'Commit',
commit_id: commit2.id)
expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'Commit',
commit_id: commit2.id)
end
it 'only renders the diffs for the path given' do
......
......@@ -4,7 +4,7 @@ describe Projects::DiscussionsController do
let(:user) { create(:user) }
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.source_project }
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
let(:discussion) { note.discussion }
let(:request_params) do
......
......@@ -519,7 +519,7 @@ describe Projects::IssuesController do
end
context 'resolving discussions in MergeRequest' do
let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
let(:merge_request) { discussion.noteable }
let(:project) { merge_request.source_project }
......
......@@ -586,8 +586,8 @@ describe Projects::MergeRequestsController do
diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path)
expect(assigns(:diff_notes_disabled)).to be_falsey
expect(assigns(:comments_target)).to eq(noteable_type: 'MergeRequest',
noteable_id: merge_request.id)
expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'MergeRequest',
noteable_id: merge_request.id)
end
it 'only renders the diffs for the path given' do
......
......@@ -14,6 +14,109 @@ describe Projects::NotesController do
}
end
describe 'GET index' do
let(:request_params) do
{
namespace_id: project.namespace,
project_id: project,
target_type: 'issue',
target_id: issue.id,
format: 'json'
}
end
let(:parsed_response) { JSON.parse(response.body).with_indifferent_access }
let(:note_json) { parsed_response[:notes].first }
before do
sign_in(user)
project.team << [user, :developer]
end
it 'passes last_fetched_at from headers to NotesFinder' do
last_fetched_at = 3.hours.ago.to_i
request.headers['X-Last-Fetched-At'] = last_fetched_at
expect(NotesFinder).to receive(:new)
.with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
.and_call_original
get :index, request_params
end
context 'for a discussion note' do
let!(:note) { create(:discussion_note_on_issue, noteable: issue, project: project) }
it 'responds with the expected attributes' do
get :index, request_params
expect(note_json[:id]).to eq(note.id)
expect(note_json[:discussion_html]).not_to be_nil
expect(note_json[:diff_discussion_html]).to be_nil
end
end
context 'for a diff discussion note' do
let(:project) { create(:project, :repository) }
let!(:note) { create(:diff_note_on_merge_request, project: project) }
let(:params) { request_params.merge(target_type: 'merge_request', target_id: note.noteable_id) }
it 'responds with the expected attributes' do
get :index, params
expect(note_json[:id]).to eq(note.id)
expect(note_json[:discussion_html]).not_to be_nil
expect(note_json[:diff_discussion_html]).not_to be_nil
end
end
context 'for a commit note' do
let(:project) { create(:project, :repository) }
let!(:note) { create(:note_on_commit, project: project) }
context 'when displayed on a merge request' do
let(:merge_request) { create(:merge_request, source_project: project) }
let(:params) { request_params.merge(target_type: 'merge_request', target_id: merge_request.id) }
it 'responds with the expected attributes' do
get :index, params
expect(note_json[:id]).to eq(note.id)
expect(note_json[:discussion_html]).not_to be_nil
expect(note_json[:diff_discussion_html]).to be_nil
end
end
context 'when displayed on the commit' do
let(:params) { request_params.merge(target_type: 'commit', target_id: note.commit_id) }
it 'responds with the expected attributes' do
get :index, params
expect(note_json[:id]).to eq(note.id)
expect(note_json[:discussion_html]).to be_nil
expect(note_json[:diff_discussion_html]).to be_nil
end
end
end
context 'for a regular note' do
let!(:note) { create(:note, noteable: issue, project: project) }
it 'responds with the expected attributes' do
get :index, request_params
expect(note_json[:id]).to eq(note.id)
expect(note_json[:html]).not_to be_nil
expect(note_json[:discussion_html]).to be_nil
expect(note_json[:diff_discussion_html]).to be_nil
end
end
end
describe 'POST create' do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.source_project }
......@@ -49,7 +152,8 @@ describe Projects::NotesController do
note: 'some note',
noteable_id: merge_request.id.to_s,
noteable_type: 'MergeRequest',
merge_request_diff_head_sha: 'sha'
merge_request_diff_head_sha: 'sha',
in_reply_to_discussion_id: nil
}
expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true))
......@@ -200,31 +304,4 @@ describe Projects::NotesController do
end
end
end
describe 'GET index' do
let(:last_fetched_at) { '1487756246' }
let(:request_params) do
{
namespace_id: project.namespace,
project_id: project,
target_type: 'issue',
target_id: issue.id
}
end
before do
sign_in(user)
project.team << [user, :developer]
end
it 'passes last_fetched_at from headers to NotesFinder' do
request.headers['X-Last-Fetched-At'] = last_fetched_at
expect(NotesFinder).to receive(:new)
.with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
.and_call_original
get :index, request_params
end
end
end
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment