Skip to content
Snippets Groups Projects
Commit b79133ca authored by Douwe Maan's avatar Douwe Maan
Browse files

Add specs

parent 37f1ed05
No related branches found
No related tags found
Loading
Pipeline #
This commit is part of merge request !7527. Comments created here will be created in the context of that merge request.
Showing
with 259 additions and 121 deletions
class Projects::NotesController < Projects::ApplicationController
include NotesHelper
include ToggleAwardEmoji
# Authorize
......@@ -12,7 +13,8 @@ def index
notes_json = { notes: [], last_fetched_at: current_fetched_at }
@notes = notes_finder.execute.inc_author
@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)
......@@ -117,7 +119,7 @@ def note_html(note)
end
def discussion_html(discussion)
return if discussion.render_as_individual_notes?
return if discussion.individual_note?
render_to_string(
"discussions/_discussion",
......@@ -153,21 +155,20 @@ def diff_discussion_html(discussion)
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,
id: note.id,
discussion_id: note.discussion_id(noteable),
html: note_html(note),
note: note.note
)
discussion = note.to_discussion(noteable)
unless discussion.render_as_individual_notes?
unless discussion.individual_note?
attrs.merge!(
discussion_resolvable: discussion.resolvable?,
......@@ -187,7 +188,6 @@ def note_json(note)
)
end
attrs[:commands_changes] = note.commands_changes
attrs
end
......
class CommitDiscussion < Discussion
def self.override_discussion_id(note)
discussion_id(note)
end
def potentially_resolvable?
false
end
end
module ResolvableNote
extend ActiveSupport::Concern
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 the logic in `#resolvable?` in `Note` subclasses that are resolvable
scope :resolvable, -> { where(type: %w(DiffNote DiscussionNote)).user.where(noteable_type: 'MergeRequest') }
# Keep this scope in sync with the logic in `#potentially_resolvable?` in `Discussion` subclasses that are resolvable
scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: 'MergeRequest') }
# 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
......@@ -24,9 +29,11 @@ def unresolve!
end
end
# If you update this method remember to also update the scope `resolvable`
delegate :potentially_resolvable?, to: :to_discussion
# Keep this method in sync with the `resolvable` scope
def resolvable?
to_discussion.potentially_resolvable? && !system?
potentially_resolvable? && !system?
end
def resolved?
......
class DiffNote < Note
include NoteOnDiff
NOTEABLE_TYPES = %w(MergeRequest Commit).freeze
serialize :original_position, Gitlab::Diff::Position
serialize :position, Gitlab::Diff::Position
......@@ -8,7 +10,7 @@ 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 :noteable_type, inclusion: { in: NOTEABLE_TYPES }
validate :positions_complete
validate :verify_supported
......
class Discussion
MEMOIZED_VALUES = [] # rubocop:disable Style/MutableConstant
attr_reader :notes
attr_reader :notes, :noteable
delegate :created_at,
:project,
......@@ -61,6 +61,13 @@ def initialize(notes, noteable = nil)
@noteable = noteable
end
def ==(other)
other.class == self.class &&
other.noteable == self.noteable &&
other.id == self.id &&
other.notes == self.notes
end
def last_updated_at
last_note.created_at
end
......@@ -83,7 +90,7 @@ def diff_discussion?
false
end
def render_as_individual_notes?
def individual_note?
false
end
......@@ -91,8 +98,9 @@ def new_discussion?
notes.length == 1
end
# Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
def potentially_resolvable?
first_note.for_merge_request?
for_merge_request?
end
def resolvable?
......@@ -162,12 +170,7 @@ def unresolve!
end
def collapsed?
if resolvable?
# New diff discussions only disappear once they are marked resolved
resolved?
else
false
end
resolved?
end
def expanded?
......
......@@ -3,11 +3,12 @@ def self.build_discussion_id(note)
[*super(note), SecureRandom.hex]
end
# Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
def potentially_resolvable?
false
end
def render_as_individual_notes?
def individual_note?
true
end
end
......@@ -11,6 +11,7 @@ def legacy_diff_discussion?
true
end
# Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
def potentially_resolvable?
false
end
......
......@@ -55,7 +55,7 @@ class Note < ActiveRecord::Base
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
......@@ -233,14 +233,14 @@ def to_ability_name
end
def can_be_discussion_note?
DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type)
DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type) && !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
if noteable && noteable != self.noteable && for_commit?
CommitDiscussion
if noteable && noteable != self.noteable
OutOfContextDiscussion
else
IndividualNoteDiscussion
end
......@@ -265,7 +265,24 @@ def discussion
end
def part_of_discussion?
!to_discussion.render_as_individual_notes?
!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
......
class OutOfContextDiscussion < Discussion
# To make sure all out-of-context notes are displayed in 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
# Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
def potentially_resolvable?
false
end
end
......@@ -23,9 +23,7 @@ def for(reply_key)
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)
......@@ -47,7 +45,7 @@ def record(noteable, recipient_id, reply_key, attrs = {})
create(attrs)
end
def record_note(note, recipient_id, reply_key, attrs = {})
def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {})
attrs[:in_reply_to_discussion_id] = note.original_discussion_id
record(note.noteable, recipient_id, reply_key, attrs)
......@@ -87,7 +85,14 @@ def to_param
self.reply_key
end
def note_params
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,
......@@ -111,10 +116,12 @@ def note_params
attrs
end
private
def note_valid
Notes::BuildService.new(self.project, self.recipient, note_params.merge(note: 'Test')).execute.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
......
......@@ -21,7 +21,7 @@ def discussions_to_resolve
@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
......
module Notes
class BuildService < BaseService
def execute
# TODO: Remove when we use a selectbox instead of a submit button
params[:type] = DiscussionNote.name if params.delete(:new_discussion)
in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
if project && in_reply_to_discussion_id.present?
discussion =
......@@ -17,6 +14,9 @@ def execute
end
params.merge!(discussion.reply_attributes)
elsif params.delete(:new_discussion)
# TODO: Remove when we use a selectbox instead of a submit button
params[:type] = DiscussionNote.name
end
note = Note.new(params)
......
- 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: '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: '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)
- if defined?(@discussions)
- @discussions.each do |discussion|
- if discussion.render_as_individual_notes?
- if discussion.individual_note?
= render partial: "projects/notes/note", collection: discussion.notes, as: :note
- else
= render 'discussions/discussion', discussion: discussion
......
require 'gitlab/email/handler/base_handler'
require 'gitlab/email/handler/reply_processing'
......@@ -42,11 +41,7 @@ def sent_notification
end
def create_note
Notes::CreateService.new(
project,
author,
sent_notification.note_params.merge(note: message)
).execute
sent_notification.create_reply(message)
end
end
end
......
......@@ -15,14 +15,163 @@
end
describe 'GET index' do
# It renders the discussion partial for any threaded note
# TODO: Test
let(:last_fetched_at) { '1487756246' }
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
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 'includes the ID' do
get :index, request_params
expect(note_json[:id]).to eq(note.id)
end
it 'includes discussion_html' do
get :index, request_params
expect(note_json[:discussion_html]).not_to be_nil
end
it "doesn't include diff_discussion_html" do
get :index, request_params
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 'includes the ID' do
get :index, params
expect(note_json[:id]).to eq(note.id)
end
it 'includes discussion_html' do
get :index, params
expect(note_json[:discussion_html]).not_to be_nil
end
it 'includes diff_discussion_html' do
get :index, params
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 'includes the ID' do
get :index, params
expect(note_json[:id]).to eq(note.id)
end
it 'includes discussion_html' do
get :index, params
expect(note_json[:discussion_html]).not_to be_nil
end
it "doesn't include diff_discussion_html" do
get :index, params
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 'includes the ID' do
get :index, params
expect(note_json[:id]).to eq(note.id)
end
it "doesn't include discussion_html" do
get :index, params
expect(note_json[:discussion_html]).to be_nil
end
it "doesn't include diff_discussion_html" do
get :index, params
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 'includes the ID' do
get :index, request_params
expect(note_json[:id]).to eq(note.id)
end
it 'includes html' do
get :index, request_params
expect(note_json[:html]).not_to be_nil
end
it "doesn't include discussion_html" do
get :index, request_params
expect(note_json[:discussion_html]).to be_nil
end
it "doesn't include diff_discussion_html" do
get :index, request_params
expect(note_json[:diff_discussion_html]).to be_nil
end
end
end
describe 'POST create' do
# Test :type, :new_discussion, :in_reply_to_discussion_id (in_reply_to_id?)
# TODO: Test
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.source_project }
let(:request_params) do
......@@ -210,31 +359,4 @@
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
FactoryGirl.define do
factory :discussion do
# TODO: Implement
end
end
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment