Commit b406d63a authored by Fatih Acet's avatar Fatih Acet

Merge branch 'jprovazn-comment-thread' into 'master'

Refactor discussions/notes code

Closes #42148

See merge request gitlab-org/gitlab-ce!17926
parents 67ca10f3 65664c2e
...@@ -4,7 +4,7 @@ import $ from 'jquery'; ...@@ -4,7 +4,7 @@ import $ from 'jquery';
import _ from 'underscore'; import _ from 'underscore';
import Cookies from 'js-cookie'; import Cookies from 'js-cookie';
import { __ } from './locale'; import { __ } from './locale';
import { isInIssuePage, isInMRPage, hasVueMRDiscussionsCookie, updateTooltipTitle } from './lib/utils/common_utils'; import { isInIssuePage, isInMRPage, isInEpicPage, hasVueMRDiscussionsCookie, updateTooltipTitle } from './lib/utils/common_utils';
import flash from './flash'; import flash from './flash';
import axios from './lib/utils/axios_utils'; import axios from './lib/utils/axios_utils';
...@@ -300,7 +300,7 @@ class AwardsHandler { ...@@ -300,7 +300,7 @@ class AwardsHandler {
} }
isInVueNoteablePage() { isInVueNoteablePage() {
return isInIssuePage() || this.isVueMRDiscussions(); return isInIssuePage() || isInEpicPage() || this.isVueMRDiscussions();
} }
getVotesBlock() { getVotesBlock() {
......
...@@ -33,6 +33,7 @@ export const checkPageAndAction = (page, action) => { ...@@ -33,6 +33,7 @@ export const checkPageAndAction = (page, action) => {
export const isInIssuePage = () => checkPageAndAction('issues', 'show'); export const isInIssuePage = () => checkPageAndAction('issues', 'show');
export const isInMRPage = () => checkPageAndAction('merge_requests', 'show'); export const isInMRPage = () => checkPageAndAction('merge_requests', 'show');
export const isInEpicPage = () => checkPageAndAction('epics', 'show');
export const isInNoteablePage = () => isInIssuePage() || isInMRPage(); export const isInNoteablePage = () => isInIssuePage() || isInMRPage();
export const hasVueMRDiscussionsCookie = () => Cookies.get('vue_mr_discussions'); export const hasVueMRDiscussionsCookie = () => Cookies.get('vue_mr_discussions');
......
...@@ -99,6 +99,10 @@ export default { ...@@ -99,6 +99,10 @@ export default {
'js-note-target-reopen': !this.isOpen, 'js-note-target-reopen': !this.isOpen,
}; };
}, },
supportQuickActions() {
// Disable quick actions support for Epics
return this.noteableType !== constants.EPIC_NOTEABLE_TYPE;
},
markdownDocsPath() { markdownDocsPath() {
return this.getNotesData.markdownDocsPath; return this.getNotesData.markdownDocsPath;
}, },
...@@ -355,7 +359,7 @@ Please check your network connection and try again.`; ...@@ -355,7 +359,7 @@ Please check your network connection and try again.`;
name="note[note]" name="note[note]"
class="note-textarea js-vue-comment-form class="note-textarea js-vue-comment-form
js-gfm-input js-autosize markdown-area js-vue-textarea" js-gfm-input js-autosize markdown-area js-vue-textarea"
data-supports-quick-actions="true" :data-supports-quick-actions="supportQuickActions"
aria-label="Description" aria-label="Description"
v-model="note" v-model="note"
ref="textarea" ref="textarea"
......
...@@ -50,7 +50,11 @@ export default { ...@@ -50,7 +50,11 @@ export default {
...mapGetters(['notes', 'getNotesDataByProp', 'discussionCount']), ...mapGetters(['notes', 'getNotesDataByProp', 'discussionCount']),
noteableType() { noteableType() {
// FIXME -- @fatihacet Get this from JSON data. // FIXME -- @fatihacet Get this from JSON data.
const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE } = constants; const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants;
if (this.noteableData.noteableType === EPIC_NOTEABLE_TYPE) {
return EPIC_NOTEABLE_TYPE;
}
return this.noteableData.merge_params return this.noteableData.merge_params
? MERGE_REQUEST_NOTEABLE_TYPE ? MERGE_REQUEST_NOTEABLE_TYPE
......
...@@ -10,6 +10,7 @@ export const CLOSED = 'closed'; ...@@ -10,6 +10,7 @@ export const CLOSED = 'closed';
export const EMOJI_THUMBSUP = 'thumbsup'; export const EMOJI_THUMBSUP = 'thumbsup';
export const EMOJI_THUMBSDOWN = 'thumbsdown'; export const EMOJI_THUMBSDOWN = 'thumbsdown';
export const ISSUE_NOTEABLE_TYPE = 'issue'; export const ISSUE_NOTEABLE_TYPE = 'issue';
export const EPIC_NOTEABLE_TYPE = 'epic';
export const MERGE_REQUEST_NOTEABLE_TYPE = 'merge_request'; export const MERGE_REQUEST_NOTEABLE_TYPE = 'merge_request';
export const UNRESOLVE_NOTE_METHOD_NAME = 'delete'; export const UNRESOLVE_NOTE_METHOD_NAME = 'delete';
export const RESOLVE_NOTE_METHOD_NAME = 'post'; export const RESOLVE_NOTE_METHOD_NAME = 'post';
...@@ -12,8 +12,11 @@ document.addEventListener( ...@@ -12,8 +12,11 @@ document.addEventListener(
data() { data() {
const notesDataset = document.getElementById('js-vue-notes').dataset; const notesDataset = document.getElementById('js-vue-notes').dataset;
const parsedUserData = JSON.parse(notesDataset.currentUserData); const parsedUserData = JSON.parse(notesDataset.currentUserData);
const noteableData = JSON.parse(notesDataset.noteableData);
let currentUserData = {}; let currentUserData = {};
noteableData.noteableType = notesDataset.noteableType;
if (parsedUserData) { if (parsedUserData) {
currentUserData = { currentUserData = {
id: parsedUserData.id, id: parsedUserData.id,
...@@ -25,7 +28,7 @@ document.addEventListener( ...@@ -25,7 +28,7 @@ document.addEventListener(
} }
return { return {
noteableData: JSON.parse(notesDataset.noteableData), noteableData,
currentUserData, currentUserData,
notesData: JSON.parse(notesDataset.notesData), notesData: JSON.parse(notesDataset.notesData),
}; };
......
...@@ -14,6 +14,8 @@ export default { ...@@ -14,6 +14,8 @@ export default {
return constants.MERGE_REQUEST_NOTEABLE_TYPE; return constants.MERGE_REQUEST_NOTEABLE_TYPE;
case 'Issue': case 'Issue':
return constants.ISSUE_NOTEABLE_TYPE; return constants.ISSUE_NOTEABLE_TYPE;
case 'Epic':
return constants.EPIC_NOTEABLE_TYPE;
default: default:
return ''; return '';
} }
......
...@@ -88,11 +88,15 @@ module IssuableActions ...@@ -88,11 +88,15 @@ module IssuableActions
discussions = Discussion.build_collection(notes, issuable) discussions = Discussion.build_collection(notes, issuable)
render json: DiscussionSerializer.new(project: project, noteable: issuable, current_user: current_user).represent(discussions, context: self) render json: discussion_serializer.represent(discussions, context: self)
end end
private private
def discussion_serializer
DiscussionSerializer.new(project: project, noteable: issuable, current_user: current_user, note_entity: ProjectNoteEntity)
end
def recaptcha_check_if_spammable(should_redirect = true, &block) def recaptcha_check_if_spammable(should_redirect = true, &block)
return yield unless issuable.is_a? Spammable return yield unless issuable.is_a? Spammable
......
...@@ -212,7 +212,7 @@ module NotesActions ...@@ -212,7 +212,7 @@ module NotesActions
end end
def note_serializer def note_serializer
NoteSerializer.new(project: project, noteable: noteable, current_user: current_user) ProjectNoteSerializer.new(project: project, noteable: noteable, current_user: current_user)
end end
def note_project def note_project
......
...@@ -43,7 +43,7 @@ class Projects::DiscussionsController < Projects::ApplicationController ...@@ -43,7 +43,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
def render_json_with_discussions_serializer def render_json_with_discussions_serializer
render json: render json:
DiscussionSerializer.new(project: project, noteable: discussion.noteable, current_user: current_user) DiscussionSerializer.new(project: project, noteable: discussion.noteable, current_user: current_user, note_entity: ProjectNoteEntity)
.represent(discussion, context: self) .represent(discussion, context: self)
end end
......
...@@ -151,16 +151,17 @@ module NotesHelper ...@@ -151,16 +151,17 @@ module NotesHelper
} }
end end
def notes_data(issuable) def discussions_path(issuable)
discussions_path = if issuable.is_a?(Issue)
if issuable.is_a?(Issue) discussions_project_issue_path(@project, issuable, format: :json)
discussions_project_issue_path(@project, issuable, format: :json) else
else discussions_project_merge_request_path(@project, issuable, format: :json)
discussions_project_merge_request_path(@project, issuable, format: :json) end
end end
def notes_data(issuable)
{ {
discussionsPath: discussions_path, discussionsPath: discussions_path(issuable),
registerPath: new_session_path(:user, redirect_to_referer: 'yes', anchor: 'register-pane'), registerPath: new_session_path(:user, redirect_to_referer: 'yes', anchor: 'register-pane'),
newSessionPath: new_session_path(:user, redirect_to_referer: 'yes'), newSessionPath: new_session_path(:user, redirect_to_referer: 'yes'),
markdownDocsPath: help_page_path('user/markdown'), markdownDocsPath: help_page_path('user/markdown'),
...@@ -170,7 +171,6 @@ module NotesHelper ...@@ -170,7 +171,6 @@ module NotesHelper
notesPath: notes_url, notesPath: notes_url,
totalNotes: issuable.discussions.length, totalNotes: issuable.discussions.length,
lastFetchedAt: Time.now.to_i lastFetchedAt: Time.now.to_i
}.to_json }.to_json
end end
......
...@@ -379,12 +379,15 @@ class Note < ActiveRecord::Base ...@@ -379,12 +379,15 @@ class Note < ActiveRecord::Base
def expire_etag_cache def expire_etag_cache
return unless noteable&.discussions_rendered_on_frontend? return unless noteable&.discussions_rendered_on_frontend?
key = Gitlab::Routing.url_helpers.project_noteable_notes_path( Gitlab::EtagCaching::Store.new.touch(etag_key)
end
def etag_key
Gitlab::Routing.url_helpers.project_noteable_notes_path(
project, project,
target_type: noteable_type.underscore, target_type: noteable_type.underscore,
target_id: noteable_id target_id: noteable_id
) )
Gitlab::EtagCaching::Store.new.touch(key)
end end
def touch(*args) def touch(*args)
......
...@@ -4,7 +4,9 @@ class DiscussionEntity < Grape::Entity ...@@ -4,7 +4,9 @@ class DiscussionEntity < Grape::Entity
expose :id, :reply_id expose :id, :reply_id
expose :expanded?, as: :expanded expose :expanded?, as: :expanded
expose :notes, using: NoteEntity expose :notes do |discussion, opts|
request.note_entity.represent(discussion.notes, opts)
end
expose :individual_note?, as: :individual_note expose :individual_note?, as: :individual_note
expose :resolvable?, as: :resolvable expose :resolvable?, as: :resolvable
...@@ -12,7 +14,7 @@ class DiscussionEntity < Grape::Entity ...@@ -12,7 +14,7 @@ class DiscussionEntity < Grape::Entity
expose :resolve_path, if: -> (d, _) { d.resolvable? } do |discussion| expose :resolve_path, if: -> (d, _) { d.resolvable? } do |discussion|
resolve_project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion.id) resolve_project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion.id)
end end
expose :resolve_with_issue_path do |discussion| expose :resolve_with_issue_path, if: -> (d, _) { d.resolvable? } do |discussion|
new_project_issue_path(discussion.project, merge_request_to_resolve_discussions_of: discussion.noteable.iid, discussion_to_resolve: discussion.id) new_project_issue_path(discussion.project, merge_request_to_resolve_discussions_of: discussion.noteable.iid, discussion_to_resolve: discussion.id)
end end
......
...@@ -5,10 +5,6 @@ class NoteEntity < API::Entities::Note ...@@ -5,10 +5,6 @@ class NoteEntity < API::Entities::Note
expose :author, using: NoteUserEntity expose :author, using: NoteUserEntity
expose :human_access do |note|
note.project.team.human_max_access(note.author_id)
end
unexpose :note, as: :body unexpose :note, as: :body
expose :note expose :note
...@@ -37,36 +33,10 @@ class NoteEntity < API::Entities::Note ...@@ -37,36 +33,10 @@ class NoteEntity < API::Entities::Note
expose :emoji_awardable?, as: :emoji_awardable expose :emoji_awardable?, as: :emoji_awardable
expose :award_emoji, if: -> (note, _) { note.emoji_awardable? }, using: AwardEmojiEntity expose :award_emoji, if: -> (note, _) { note.emoji_awardable? }, using: AwardEmojiEntity
expose :toggle_award_path, if: -> (note, _) { note.emoji_awardable? } do |note|
if note.for_personal_snippet?
toggle_award_emoji_snippet_note_path(note.noteable, note)
else
toggle_award_emoji_project_note_path(note.project, note.id)
end
end
expose :report_abuse_path do |note| expose :report_abuse_path do |note|
new_abuse_report_path(user_id: note.author.id, ref_url: Gitlab::UrlBuilder.build(note)) new_abuse_report_path(user_id: note.author.id, ref_url: Gitlab::UrlBuilder.build(note))
end end
expose :path do |note|
if note.for_personal_snippet?
snippet_note_path(note.noteable, note)
else
project_note_path(note.project, note)
end
end
expose :resolve_path, if: -> (note, _) { note.part_of_discussion? && note.resolvable? } do |note|
resolve_project_merge_request_discussion_path(note.project, note.noteable, note.discussion_id)
end
expose :resolve_with_issue_path, if: -> (note, _) { note.part_of_discussion? && note.resolvable? } do |note|
new_project_issue_path(note.project, merge_request_to_resolve_discussions_of: note.noteable.iid, discussion_to_resolve: note.discussion_id)
end
expose :attachment, using: NoteAttachmentEntity, if: -> (note, _) { note.attachment? } expose :attachment, using: NoteAttachmentEntity, if: -> (note, _) { note.attachment? }
expose :delete_attachment_path, if: -> (note, _) { note.attachment? } do |note|
delete_attachment_project_note_path(note.project, note)
end
end end
class NoteSerializer < BaseSerializer
entity NoteEntity
end
class ProjectNoteEntity < NoteEntity
expose :human_access do |note|
note.project.team.human_max_access(note.author_id)
end
expose :toggle_award_path, if: -> (note, _) { note.emoji_awardable? } do |note|
toggle_award_emoji_project_note_path(note.project, note.id)
end
expose :path do |note|
project_note_path(note.project, note)
end
expose :resolve_path, if: -> (note, _) { note.part_of_discussion? && note.resolvable? } do |note|
resolve_project_merge_request_discussion_path(note.project, note.noteable, note.discussion_id)
end
expose :resolve_with_issue_path, if: -> (note, _) { note.part_of_discussion? && note.resolvable? } do |note|
new_project_issue_path(note.project, merge_request_to_resolve_discussions_of: note.noteable.iid, discussion_to_resolve: note.discussion_id)
end
expose :delete_attachment_path, if: -> (note, _) { note.attachment? } do |note|
delete_attachment_project_note_path(note.project, note)
end
end
class ProjectNoteSerializer < BaseSerializer
entity ProjectNoteEntity
end
...@@ -974,7 +974,7 @@ describe Projects::IssuesController do ...@@ -974,7 +974,7 @@ describe Projects::IssuesController do
it 'returns discussion json' do it 'returns discussion json' do
get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid get :discussions, namespace_id: project.namespace, project_id: project, id: issue.iid
expect(json_response.first.keys).to match_array(%w[id reply_id expanded notes diff_discussion individual_note resolvable resolve_with_issue_path resolved]) expect(json_response.first.keys).to match_array(%w[id reply_id expanded notes diff_discussion individual_note resolvable resolved])
end end
context 'with cross-reference system note', :request_store do context 'with cross-reference system note', :request_store do
......
...@@ -6,7 +6,7 @@ describe DiscussionEntity do ...@@ -6,7 +6,7 @@ describe DiscussionEntity do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:note) { create(:discussion_note_on_merge_request) } let(:note) { create(:discussion_note_on_merge_request) }
let(:discussion) { note.discussion } let(:discussion) { note.discussion }
let(:request) { double('request') } let(:request) { double('request', note_entity: ProjectNoteEntity) }
let(:controller) { double('controller') } let(:controller) { double('controller') }
let(:entity) { described_class.new(discussion, request: request, context: controller) } let(:entity) { described_class.new(discussion, request: request, context: controller) }
......
...@@ -10,53 +10,5 @@ describe NoteEntity do ...@@ -10,53 +10,5 @@ describe NoteEntity do
let(:user) { create(:user) } let(:user) { create(:user) }
subject { entity.as_json } subject { entity.as_json }
context 'basic note' do it_behaves_like 'note entity'
it 'exposes correct elements' do
expect(subject).to include(:type, :author, :human_access, :note, :note_html, :current_user,
:discussion_id, :emoji_awardable, :award_emoji, :toggle_award_path, :report_abuse_path, :path, :attachment)
end
it 'does not expose elements for specific notes cases' do
expect(subject).not_to include(:last_edited_by, :last_edited_at, :system_note_icon_name)
end
it 'exposes author correctly' do
expect(subject[:author]).to include(:id, :name, :username, :state, :avatar_url, :path)
end
it 'does not expose web_url for author' do
expect(subject[:author]).not_to include(:web_url)
end
end
context 'when note was edited' do
before do
note.update(updated_at: 1.minute.from_now, updated_by: user)
end
it 'exposes last_edited_at and last_edited_by elements' do
expect(subject).to include(:last_edited_at, :last_edited_by)
end
end
context 'when note is a system note' do
before do
note.update(system: true)
end
it 'exposes system_note_icon_name element' do
expect(subject).to include(:system_note_icon_name)
end
end
context 'when note is part of resolvable discussion' do
before do
allow(note).to receive(:part_of_discussion?).and_return(true)
allow(note).to receive(:resolvable?).and_return(true)
end
it 'exposes paths to resolve note' do
expect(subject).to include(:resolve_path, :resolve_with_issue_path)
end
end
end end
require 'spec_helper'
describe ProjectNoteEntity do
include Gitlab::Routing
let(:request) { double('request', current_user: user, noteable: note.noteable) }
let(:entity) { described_class.new(note, request: request) }
let(:note) { create(:note) }
let(:user) { create(:user) }
subject { entity.as_json }
it_behaves_like 'note entity'
it 'exposes project-specific elements' do
expect(subject).to include(:human_access, :toggle_award_path, :path)
end
context 'when note is part of resolvable discussion' do
before do
allow(note).to receive(:part_of_discussion?).and_return(true)
allow(note).to receive(:resolvable?).and_return(true)
end
it 'exposes paths to resolve note' do
expect(subject).to include(:resolve_path, :resolve_with_issue_path)
end
end
end
...@@ -81,7 +81,10 @@ shared_examples 'discussion comments' do |resource_name| ...@@ -81,7 +81,10 @@ shared_examples 'discussion comments' do |resource_name|
# on issues page, the menu closes when clicking anywhere, on other pages it will # on issues page, the menu closes when clicking anywhere, on other pages it will
# remain open if clicking divider or menu padding, but should not change button action # remain open if clicking divider or menu padding, but should not change button action
if resource_name == 'issue' #
# if dropdown menu is not toggled (and also not present),
# it's "issue-type" dropdown
if first(menu_selector).nil?
expect(find(dropdown_selector)).to have_content 'Comment' expect(find(dropdown_selector)).to have_content 'Comment'
find(toggle_selector).click find(toggle_selector).click
...@@ -107,8 +110,10 @@ shared_examples 'discussion comments' do |resource_name| ...@@ -107,8 +110,10 @@ shared_examples 'discussion comments' do |resource_name|
end end
it 'updates the submit button text and closes the dropdown' do it 'updates the submit button text and closes the dropdown' do
button = find(submit_selector)
# on issues page, the submit input is a <button>, on other pages it is <input> # on issues page, the submit input is a <button>, on other pages it is <input>
if resource_name == 'issue' if button.tag_name == 'button'
expect(find(submit_selector)).to have_content 'Start discussion' expect(find(submit_selector)).to have_content 'Start discussion'
else else
expect(find(submit_selector).value).to eq 'Start discussion' expect(find(submit_selector).value).to eq 'Start discussion'
...@@ -132,6 +137,8 @@ shared_examples 'discussion comments' do |resource_name| ...@@ -132,6 +137,8 @@ shared_examples 'discussion comments' do |resource_name|
describe 'creating a discussion' do describe 'creating a discussion' do
before do before do
find(submit_selector).click find(submit_selector).click
wait_for_requests
find(comments_selector, match: :first) find(comments_selector, match: :first)
end end
...@@ -197,11 +204,13 @@ shared_examples 'discussion comments' do |resource_name| ...@@ -197,11 +204,13 @@ shared_examples 'discussion comments' do |resource_name|
end end
it 'updates the submit button text and closes the dropdown' do it 'updates the submit button text and closes the dropdown' do
button = find(submit_selector)
# on issues page, the submit input is a <button>, on other pages it is <input> # on issues page, the submit input is a <button>, on other pages it is <input>
if resource_name == 'issue' if button.tag_name == 'button'
expect(find(submit_selector)).to have_content 'Comment' expect(button).to have_content 'Comment'
else else
expect(find(submit_selector).value).to eq 'Comment' expect(button.value).to eq 'Comment'
end end
expect(page).not_to have_selector menu_selector expect(page).not_to have_selector menu_selector
......
shared_examples 'note entity' do
subject { entity.as_json }
context 'basic note' do
it 'exposes correct elements' do
expect(subject).to include(:type, :author, :note, :note_html, :current_user,
:discussion_id, :emoji_awardable, :award_emoji, :report_abuse_path, :attachment)
end
it 'does not expose elements for specific notes cases' do
expect(subject).not_to include(:last_edited_by, :last_edited_at, :system_note_icon_name)
end
it 'exposes author correctly' do
expect(subject[:author]).to include(:id, :name, :username, :state, :avatar_url, :path)
end
it 'does not expose web_url for author' do
expect(subject[:author]).not_to include(:web_url)
end
end
context 'when note was edited' do
before do
note.update(updated_at: 1.minute.from_now, updated_by: user)
end
it 'exposes last_edited_at and last_edited_by elements' do
expect(subject).to include(:last_edited_at, :last_edited_by)
end
end
context 'when note is a system note' do
before do
note.update(system: true)
end
it 'exposes system_note_icon_name element' do
expect(subject).to include(:system_note_icon_name)
end
end
end
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