diff options
32 files changed, 671 insertions, 74 deletions
diff --git a/.gitlab/issue_templates/Feature proposal.md b/.gitlab/issue_templates/Feature proposal.md index 0b22c7bc26b..1bb8d33ff63 100644 --- a/.gitlab/issue_templates/Feature proposal.md +++ b/.gitlab/issue_templates/Feature proposal.md @@ -39,7 +39,7 @@ Existing personas are: (copy relevant personas out of this comment, and delete a ### What does success look like, and how can we measure that? -<!--- Define both the success metrics and acceptance criteria. Note thet success metrics indicate the desired business outcomes, while acceptance criteria indicate when the solution is working correctly. If there is no way to measure success, link to an issue that will implement a way to measure this --> +<!--- Define both the success metrics and acceptance criteria. Note that success metrics indicate the desired business outcomes, while acceptance criteria indicate when the solution is working correctly. If there is no way to measure success, link to an issue that will implement a way to measure this --> ### Links / references diff --git a/app/assets/javascripts/notes/components/note_actions.vue b/app/assets/javascripts/notes/components/note_actions.vue index d99694b06e9..394f2a80a67 100644 --- a/app/assets/javascripts/notes/components/note_actions.vue +++ b/app/assets/javascripts/notes/components/note_actions.vue @@ -2,11 +2,13 @@ import { mapGetters } from 'vuex'; import Icon from '~/vue_shared/components/icon.vue'; import { GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui'; +import ReplyButton from './note_actions/reply_button.vue'; export default { name: 'NoteActions', components: { Icon, + ReplyButton, GlLoadingIcon, }, directives: { @@ -21,6 +23,11 @@ export default { type: [String, Number], required: true, }, + discussionId: { + type: String, + required: false, + default: '', + }, noteUrl: { type: String, required: false, @@ -36,6 +43,10 @@ export default { required: false, default: null, }, + showReply: { + type: Boolean, + required: true, + }, canEdit: { type: Boolean, required: true, @@ -80,6 +91,9 @@ export default { }, computed: { ...mapGetters(['getUserDataByProp']), + showReplyButton() { + return gon.features && gon.features.replyToIndividualNotes && this.showReply; + }, shouldShowActionsDropdown() { return this.currentUserId && (this.canEdit || this.canReportAsAbuse); }, @@ -153,6 +167,12 @@ export default { <icon css-classes="link-highlight award-control-icon-super-positive" name="emoji_smiley" /> </a> </div> + <reply-button + v-if="showReplyButton" + ref="replyButton" + class="js-reply-button" + :note-id="discussionId" + /> <div v-if="canEdit" class="note-actions-item"> <button v-gl-tooltip.bottom diff --git a/app/assets/javascripts/notes/components/note_actions/reply_button.vue b/app/assets/javascripts/notes/components/note_actions/reply_button.vue new file mode 100644 index 00000000000..b2f9d7f128a --- /dev/null +++ b/app/assets/javascripts/notes/components/note_actions/reply_button.vue @@ -0,0 +1,40 @@ +<script> +import { mapActions } from 'vuex'; +import { GlTooltipDirective, GlButton } from '@gitlab/ui'; +import Icon from '~/vue_shared/components/icon.vue'; + +export default { + name: 'ReplyButton', + components: { + Icon, + GlButton, + }, + directives: { + GlTooltip: GlTooltipDirective, + }, + props: { + noteId: { + type: String, + required: true, + }, + }, + methods: { + ...mapActions(['convertToDiscussion']), + }, +}; +</script> + +<template> + <div class="note-actions-item"> + <gl-button + ref="button" + v-gl-tooltip.bottom + class="note-action-button" + variant="transparent" + :title="__('Reply to comment')" + @click="convertToDiscussion(noteId)" + > + <icon name="comment" css-classes="link-highlight" /> + </gl-button> + </div> +</template> diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index e26cce1c47f..b7e9f7c2028 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -398,6 +398,7 @@ Please check your network connection and try again.`; :line="line" :commit="commit" :help-page-path="helpPagePath" + :show-reply-button="canReply" @handleDeleteNote="deleteNoteHandler" > <note-edited-text diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue index 3c48d81ed05..56108a58010 100644 --- a/app/assets/javascripts/notes/components/noteable_note.vue +++ b/app/assets/javascripts/notes/components/noteable_note.vue @@ -29,6 +29,11 @@ export default { type: Object, required: true, }, + discussion: { + type: Object, + required: false, + default: null, + }, line: { type: Object, required: false, @@ -54,7 +59,7 @@ export default { }; }, computed: { - ...mapGetters(['targetNoteHash', 'getNoteableData', 'getUserData']), + ...mapGetters(['targetNoteHash', 'getNoteableData', 'getUserData', 'commentsDisabled']), author() { return this.note.author; }, @@ -80,6 +85,19 @@ export default { isTarget() { return this.targetNoteHash === this.noteAnchorId; }, + discussionId() { + if (this.discussion) { + return this.discussion.id; + } + return ''; + }, + showReplyButton() { + if (!this.discussion || !this.getNoteableData.current_user.can_create_note) { + return false; + } + + return this.discussion.individual_note && !this.commentsDisabled; + }, actionText() { if (!this.commit) { return ''; @@ -231,6 +249,7 @@ export default { :note-id="note.id" :note-url="note.noteable_note_url" :access-level="note.human_access" + :show-reply="showReplyButton" :can-edit="note.current_user.can_edit" :can-award-emoji="note.current_user.can_award_emoji" :can-delete="note.current_user.can_edit" @@ -241,6 +260,7 @@ export default { :is-resolved="note.resolved" :is-resolving="isResolving" :resolved-by="note.resolved_by" + :discussion-id="discussionId" @handleEdit="editHandler" @handleDelete="deleteHandler" @handleResolve="resolveHandler" diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue index 5edceea043c..6d72b72e628 100644 --- a/app/assets/javascripts/notes/components/notes_app.vue +++ b/app/assets/javascripts/notes/components/notes_app.vue @@ -199,7 +199,12 @@ export default { :key="discussion.id" :note="discussion.notes[0]" /> - <noteable-note v-else :key="discussion.id" :note="discussion.notes[0]" /> + <noteable-note + v-else + :key="discussion.id" + :note="discussion.notes[0]" + :discussion="discussion" + /> </template> <noteable-discussion v-else diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 2105a62cecb..ff65f14d529 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -426,5 +426,8 @@ export const submitSuggestion = ( }); }; +export const convertToDiscussion = ({ commit }, noteId) => + commit(types.CONVERT_TO_DISCUSSION, noteId); + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js index df943c155f4..2bffedad336 100644 --- a/app/assets/javascripts/notes/stores/mutation_types.js +++ b/app/assets/javascripts/notes/stores/mutation_types.js @@ -17,6 +17,7 @@ export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE'; export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE'; export const DISABLE_COMMENTS = 'DISABLE_COMMENTS'; export const APPLY_SUGGESTION = 'APPLY_SUGGESTION'; +export const CONVERT_TO_DISCUSSION = 'CONVERT_TO_DISCUSSION'; // DISCUSSION export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION'; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index 33d39ad2ec9..d167f8ef421 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -264,4 +264,9 @@ export default { ).length; state.hasUnresolvedDiscussions = state.unresolvedDiscussionsCount > 1; }, + + [types.CONVERT_TO_DISCUSSION](state, discussionId) { + const discussion = utils.findNoteObjectById(state.discussions, discussionId); + Object.assign(discussion, { individual_note: false }); + }, }; diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 8ef3b6502df..cd3fa641e89 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -7,6 +7,9 @@ module IssuableActions included do before_action :authorize_destroy_issuable!, only: :destroy before_action :authorize_admin_issuable!, only: :bulk_update + before_action only: :show do + push_frontend_feature_flag(:reply_to_individual_notes) + end end def permitted_keys diff --git a/app/controllers/projects/environments_controller.rb b/app/controllers/projects/environments_controller.rb index 4e85de25c6b..79685e8b675 100644 --- a/app/controllers/projects/environments_controller.rb +++ b/app/controllers/projects/environments_controller.rb @@ -158,6 +158,16 @@ class Projects::EnvironmentsController < Projects::ApplicationController end end + def search + respond_to do |format| + format.json do + environment_names = search_environment_names + + render json: environment_names, status: environment_names.any? ? :ok : :no_content + end + end + end + private def verify_api_request! @@ -181,6 +191,12 @@ class Projects::EnvironmentsController < Projects::ApplicationController @environment ||= project.environments.find(params[:id]) end + def search_environment_names + return [] unless params[:query] + + project.environments.for_name_like(params[:query]).pluck_names + end + def serialize_environments(request, response, nested = false) EnvironmentSerializer .new(project: @project, current_user: @current_user) diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index 29476654bf7..3c74034b527 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -1,9 +1,18 @@ # frozen_string_literal: true module Noteable - # Names of all implementers of `Noteable` that support resolvable notes. + extend ActiveSupport::Concern + + # `Noteable` class names that support resolvable notes. RESOLVABLE_TYPES = %w(MergeRequest).freeze + class_methods do + # `Noteable` class names that support replying to individual notes. + def replyable_types + %w(Issue MergeRequest) + end + end + def base_class_name self.class.base_class.name end @@ -26,6 +35,10 @@ module Noteable DiscussionNote.noteable_types.include?(base_class_name) end + def supports_replying_to_individual_notes? + supports_discussions? && self.class.replyable_types.include?(base_class_name) + end + def supports_suggestion? false end diff --git a/app/models/discussion.rb b/app/models/discussion.rb index dbc7b6e67be..f2678e0597d 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -17,6 +17,8 @@ class Discussion :for_commit?, :for_merge_request?, + :save, + to: :first_note def project_id @@ -116,6 +118,10 @@ class Discussion false end + def can_convert_to_discussion? + false + end + def new_discussion? notes.length == 1 end diff --git a/app/models/environment.rb b/app/models/environment.rb index cdfe3b7c023..1fc088b12ae 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -50,6 +50,14 @@ class Environment < ActiveRecord::Base end scope :in_review_folder, -> { where(environment_type: "review") } scope :for_name, -> (name) { where(name: name) } + + ## + # Search environments which have names like the given query. + # Do not set a large limit unless you've confirmed that it works on gitlab.com scale. + scope :for_name_like, -> (query, limit: 5) do + where('name LIKE ?', "#{sanitize_sql_like(query)}%").limit(limit) + end + scope :for_project, -> (project) { where(project_id: project) } scope :with_deployment, -> (sha) { where('EXISTS (?)', Deployment.select(1).where('deployments.environment_id = environments.id').where(sha: sha)) } @@ -70,6 +78,10 @@ class Environment < ActiveRecord::Base end end + def self.pluck_names + pluck(:name) + end + def predefined_variables Gitlab::Ci::Variables::Collection.new .append(key: 'CI_ENVIRONMENT_NAME', value: name) diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb index 07ee7470ea2..aab0ff93468 100644 --- a/app/models/individual_note_discussion.rb +++ b/app/models/individual_note_discussion.rb @@ -13,6 +13,14 @@ class IndividualNoteDiscussion < Discussion true end + def can_convert_to_discussion? + noteable.supports_replying_to_individual_notes? && Feature.enabled?(:reply_to_individual_notes) + end + + def convert_to_discussion! + first_note.becomes!(Discussion.note_class).to_discussion + end + def reply_attributes super.tap { |attrs| attrs.delete(:discussion_id) } end diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index e65b3df0fb6..6caab24143b 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -48,7 +48,7 @@ class SentNotification < ActiveRecord::Base end def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {}) - attrs[:in_reply_to_discussion_id] = note.discussion_id + attrs[:in_reply_to_discussion_id] = note.discussion_id if note.part_of_discussion? record(note.noteable, recipient_id, reply_key, attrs) end @@ -99,29 +99,12 @@ class SentNotification < ActiveRecord::Base private def reply_params - attrs = { + { noteable_type: self.noteable_type, noteable_id: self.noteable_id, - commit_id: self.commit_id + commit_id: self.commit_id, + in_reply_to_discussion_id: self.in_reply_to_discussion_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 diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb index bae98ede561..541f3e0d23c 100644 --- a/app/services/notes/build_service.rb +++ b/app/services/notes/build_service.rb @@ -15,6 +15,8 @@ module Notes return note end + discussion = discussion.convert_to_discussion! if discussion.can_convert_to_discussion? + params.merge!(discussion.reply_attributes) should_resolve = discussion.resolved? end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index c4546f30235..b975c3a8cb6 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -34,6 +34,10 @@ module Notes end if !only_commands && note.save + if note.part_of_discussion? && note.discussion.can_convert_to_discussion? + note.discussion.convert_to_discussion!.save(touch: false) + end + todo_service.new_note(note, current_user) clear_noteable_diffs_cache(note) Suggestions::CreateService.new(note).execute diff --git a/changelogs/unreleased/introduce-environment-search-endpoint.yml b/changelogs/unreleased/introduce-environment-search-endpoint.yml new file mode 100644 index 00000000000..01851ba7d27 --- /dev/null +++ b/changelogs/unreleased/introduce-environment-search-endpoint.yml @@ -0,0 +1,5 @@ +--- +title: Introduce Internal API for searching environment names +merge_request: 24923 +author: +type: added diff --git a/config/routes/project.rb b/config/routes/project.rb index 21793e7756a..d730479cf2b 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -224,6 +224,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do collection do get :metrics, action: :metrics_redirect get :folder, path: 'folders/*id', constraints: { format: /(html|json)/ } + get :search end resources :deployments, only: [:index] do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bdb2ed1e26e..1caf263b9f3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -5959,6 +5959,9 @@ msgstr "" msgid "Reopen milestone" msgstr "" +msgid "Reply to comment" +msgstr "" + msgid "Reply to this email directly or %{view_it_on_gitlab}." msgstr "" diff --git a/spec/controllers/projects/environments_controller_spec.rb b/spec/controllers/projects/environments_controller_spec.rb index a4d494a820f..aa97a417a98 100644 --- a/spec/controllers/projects/environments_controller_spec.rb +++ b/spec/controllers/projects/environments_controller_spec.rb @@ -422,6 +422,79 @@ describe Projects::EnvironmentsController do end end + describe 'GET #search' do + before do + create(:environment, name: 'staging', project: project) + create(:environment, name: 'review/patch-1', project: project) + create(:environment, name: 'review/patch-2', project: project) + end + + let(:query) { 'pro' } + + it 'responds with status code 200' do + get :search, params: environment_params(format: :json, query: query) + + expect(response).to have_gitlab_http_status(:ok) + end + + it 'returns matched results' do + get :search, params: environment_params(format: :json, query: query) + + expect(json_response).to contain_exactly('production') + end + + context 'when query is review' do + let(:query) { 'review' } + + it 'returns matched results' do + get :search, params: environment_params(format: :json, query: query) + + expect(json_response).to contain_exactly('review/patch-1', 'review/patch-2') + end + end + + context 'when query is empty' do + let(:query) { '' } + + it 'returns matched results' do + get :search, params: environment_params(format: :json, query: query) + + expect(json_response) + .to contain_exactly('production', 'staging', 'review/patch-1', 'review/patch-2') + end + end + + context 'when query is review/patch-3' do + let(:query) { 'review/patch-3' } + + it 'responds with status code 204' do + get :search, params: environment_params(format: :json, query: query) + + expect(response).to have_gitlab_http_status(:no_content) + end + end + + context 'when query is partially matched in the middle of environment name' do + let(:query) { 'patch' } + + it 'responds with status code 204' do + get :search, params: environment_params(format: :json, query: query) + + expect(response).to have_gitlab_http_status(:no_content) + end + end + + context 'when query contains a wildcard character' do + let(:query) { 'review%' } + + it 'prevents wildcard injection' do + get :search, params: environment_params(format: :json, query: query) + + expect(response).to have_gitlab_http_status(:no_content) + end + end + end + def environment_params(opts = {}) opts.reverse_merge(namespace_id: project.namespace, project_id: project, diff --git a/spec/features/merge_request/user_posts_notes_spec.rb b/spec/features/merge_request/user_posts_notes_spec.rb index ee5f5377ca6..1bbcf455ac7 100644 --- a/spec/features/merge_request/user_posts_notes_spec.rb +++ b/spec/features/merge_request/user_posts_notes_spec.rb @@ -66,6 +66,38 @@ describe 'Merge request > User posts notes', :js do is_expected.to have_css('.js-note-text', visible: true) end end + + describe 'when reply_to_individual_notes feature flag is not set' do + before do + stub_feature_flags(reply_to_individual_notes: false) + visit project_merge_request_path(project, merge_request) + end + + it 'does not show a reply button' do + expect(page).to have_no_selector('.js-reply-button') + end + end + + describe 'when reply_to_individual_notes feature flag is set' do + before do + stub_feature_flags(reply_to_individual_notes: true) + visit project_merge_request_path(project, merge_request) + end + + it 'shows a reply button' do + reply_button = find('.js-reply-button', match: :first) + + expect(reply_button).to have_selector('.ic-comment') + end + + it 'shows reply placeholder when clicking reply button' do + reply_button = find('.js-reply-button', match: :first) + + reply_button.click + + expect(page).to have_selector('.discussion-reply-holder') + end + end end describe 'when previewing a note' do diff --git a/spec/javascripts/notes/components/note_actions/reply_button_spec.js b/spec/javascripts/notes/components/note_actions/reply_button_spec.js new file mode 100644 index 00000000000..11e1664a3f4 --- /dev/null +++ b/spec/javascripts/notes/components/note_actions/reply_button_spec.js @@ -0,0 +1,46 @@ +import Vuex from 'vuex'; +import { createLocalVue, mount } from '@vue/test-utils'; +import ReplyButton from '~/notes/components/note_actions/reply_button.vue'; + +describe('ReplyButton', () => { + const noteId = 'dummy-note-id'; + + let wrapper; + let convertToDiscussion; + + beforeEach(() => { + const localVue = createLocalVue(); + convertToDiscussion = jasmine.createSpy('convertToDiscussion'); + + localVue.use(Vuex); + const store = new Vuex.Store({ + actions: { + convertToDiscussion, + }, + }); + + wrapper = mount(ReplyButton, { + propsData: { + noteId, + }, + store, + sync: false, + localVue, + }); + }); + + afterEach(() => { + wrapper.destroy(); + }); + + it('dispatches convertToDiscussion with note ID on click', () => { + const button = wrapper.find({ ref: 'button' }); + + button.trigger('click'); + + expect(convertToDiscussion).toHaveBeenCalledTimes(1); + const [, payload] = convertToDiscussion.calls.argsFor(0); + + expect(payload).toBe(noteId); + }); +}); diff --git a/spec/javascripts/notes/components/note_actions_spec.js b/spec/javascripts/notes/components/note_actions_spec.js index b102b7aecf7..0c1962912b4 100644 --- a/spec/javascripts/notes/components/note_actions_spec.js +++ b/spec/javascripts/notes/components/note_actions_spec.js @@ -2,14 +2,38 @@ import Vue from 'vue'; import { shallowMount, createLocalVue } from '@vue/test-utils'; import createStore from '~/notes/stores'; import noteActions from '~/notes/components/note_actions.vue'; +import { TEST_HOST } from 'spec/test_constants'; import { userDataMock } from '../mock_data'; describe('noteActions', () => { let wrapper; let store; + let props; + + const createWrapper = propsData => { + const localVue = createLocalVue(); + return shallowMount(noteActions, { + store, + propsData, + localVue, + sync: false, + }); + }; beforeEach(() => { store = createStore(); + props = { + accessLevel: 'Maintainer', + authorId: 26, + canDelete: true, + canEdit: true, + canAwardEmoji: true, + canReportAsAbuse: true, + noteId: '539', + noteUrl: `${TEST_HOST}/group/project/merge_requests/1#note_1`, + reportAbusePath: `${TEST_HOST}/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26`, + showReply: false, + }; }); afterEach(() => { @@ -17,31 +41,10 @@ describe('noteActions', () => { }); describe('user is logged in', () => { - let props; - beforeEach(() => { - props = { - accessLevel: 'Maintainer', - authorId: 26, - canDelete: true, - canEdit: true, - canAwardEmoji: true, - canReportAsAbuse: true, - noteId: '539', - noteUrl: 'https://localhost:3000/group/project/merge_requests/1#note_1', - reportAbusePath: - '/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26', - }; - store.dispatch('setUserData', userDataMock); - const localVue = createLocalVue(); - wrapper = shallowMount(noteActions, { - store, - propsData: props, - localVue, - sync: false, - }); + wrapper = createWrapper(props); }); it('should render access level badge', () => { @@ -91,28 +94,14 @@ describe('noteActions', () => { }); describe('user is not logged in', () => { - let props; - beforeEach(() => { store.dispatch('setUserData', {}); - props = { - accessLevel: 'Maintainer', - authorId: 26, + wrapper = createWrapper({ + ...props, canDelete: false, canEdit: false, canAwardEmoji: false, canReportAsAbuse: false, - noteId: '539', - noteUrl: 'https://localhost:3000/group/project/merge_requests/1#note_1', - reportAbusePath: - '/abuse_reports/new?ref_url=http%3A%2F%2Flocalhost%3A3000%2Fgitlab-org%2Fgitlab-ce%2Fissues%2F7%23note_539&user_id=26', - }; - const localVue = createLocalVue(); - wrapper = shallowMount(noteActions, { - store, - propsData: props, - localVue, - sync: false, }); }); @@ -124,4 +113,88 @@ describe('noteActions', () => { expect(wrapper.find('.more-actions').exists()).toBe(false); }); }); + + describe('with feature flag replyToIndividualNotes enabled', () => { + beforeEach(() => { + gon.features = { + replyToIndividualNotes: true, + }; + }); + + afterEach(() => { + gon.features = {}; + }); + + describe('for showReply = true', () => { + beforeEach(() => { + wrapper = createWrapper({ + ...props, + showReply: true, + }); + }); + + it('shows a reply button', () => { + const replyButton = wrapper.find({ ref: 'replyButton' }); + + expect(replyButton.exists()).toBe(true); + }); + }); + + describe('for showReply = false', () => { + beforeEach(() => { + wrapper = createWrapper({ + ...props, + showReply: false, + }); + }); + + it('does not show a reply button', () => { + const replyButton = wrapper.find({ ref: 'replyButton' }); + + expect(replyButton.exists()).toBe(false); + }); + }); + }); + + describe('with feature flag replyToIndividualNotes disabled', () => { + beforeEach(() => { + gon.features = { + replyToIndividualNotes: false, + }; + }); + + afterEach(() => { + gon.features = {}; + }); + + describe('for showReply = true', () => { + beforeEach(() => { + wrapper = createWrapper({ + ...props, + showReply: true, + }); + }); + + it('does not show a reply button', () => { + const replyButton = wrapper.find({ ref: 'replyButton' }); + + expect(replyButton.exists()).toBe(false); + }); + }); + + describe('for showReply = false', () => { + beforeEach(() => { + wrapper = createWrapper({ + ...props, + showReply: false, + }); + }); + + it('does not show a reply button', () => { + const replyButton = wrapper.find({ ref: 'replyButton' }); + + expect(replyButton.exists()).toBe(false); + }); + }); + }); }); diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index 2e3cd5e8f36..73f960dd21e 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/spec/javascripts/notes/stores/actions_spec.js @@ -585,4 +585,18 @@ describe('Actions Notes Store', () => { ); }); }); + + describe('convertToDiscussion', () => { + it('commits CONVERT_TO_DISCUSSION with noteId', done => { + const noteId = 'dummy-note-id'; + testAction( + actions.convertToDiscussion, + noteId, + {}, + [{ type: 'CONVERT_TO_DISCUSSION', payload: noteId }], + [], + done, + ); + }); + }); }); diff --git a/spec/javascripts/notes/stores/mutation_spec.js b/spec/javascripts/notes/stores/mutation_spec.js index b6b2c7d60a5..4f8d3069bb5 100644 --- a/spec/javascripts/notes/stores/mutation_spec.js +++ b/spec/javascripts/notes/stores/mutation_spec.js @@ -517,4 +517,27 @@ describe('Notes Store mutations', () => { ); }); }); + + describe('CONVERT_TO_DISCUSSION', () => { + let discussion; + let state; + + beforeEach(() => { + discussion = { + id: 42, + individual_note: true, + }; + state = { discussions: [discussion] }; + }); + + it('toggles individual_note', () => { + mutations.CONVERT_TO_DISCUSSION(state, discussion.id); + + expect(discussion.individual_note).toBe(false); + }); + + it('throws if discussion was not found', () => { + expect(() => mutations.CONVERT_TO_DISCUSSION(state, 99)).toThrow(); + }); + }); }); diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb index e5420ea6bea..50e473c459e 100644 --- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb @@ -155,11 +155,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler do it_behaves_like "checks permissions on noteable" end - context "when everything is fine" do - before do - setup_attachment - end - + shared_examples 'a reply to existing comment' do it "creates a comment" do expect { receiver.execute }.to change { noteable.notes.count }.by(1) new_note = noteable.notes.last @@ -168,7 +164,21 @@ describe Gitlab::Email::Handler::CreateNoteHandler do expect(new_note.position).to eq(note.position) expect(new_note.note).to include("I could not disagree more.") expect(new_note.in_reply_to?(note)).to be_truthy + + if note.part_of_discussion? + expect(new_note.discussion_id).to eq(note.discussion_id) + else + expect(new_note.discussion_id).not_to eq(note.discussion_id) + end end + end + + context "when everything is fine" do + before do + setup_attachment + end + + it_behaves_like 'a reply to existing comment' it "adds all attachments" do receiver.execute @@ -207,4 +217,10 @@ describe Gitlab::Email::Handler::CreateNoteHandler do end end end + + context "when note is not a discussion" do + let(:note) { create(:note_on_merge_request, project: project) } + + it_behaves_like 'a reply to existing comment' + end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 9a3f1f1c5a1..2d554326f05 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -41,6 +41,76 @@ describe Environment do end end + describe '.for_name_like' do + subject { project.environments.for_name_like(query, limit: limit) } + + let!(:environment) { create(:environment, name: 'production', project: project) } + let(:query) { 'pro' } + let(:limit) { 5 } + + it 'returns a found name' do + is_expected.to include(environment) + end + + context 'when query is production' do + let(:query) { 'production' } + + it 'returns a found name' do + is_expected.to include(environment) + end + end + + context 'when query is productionA' do + let(:query) { 'productionA' } + + it 'returns empty array' do + is_expected.to be_empty + end + end + + context 'when query is empty' do + let(:query) { '' } + + it 'returns a found name' do + is_expected.to include(environment) + end + end + + context 'when query is nil' do + let(:query) { } + + it 'raises an error' do + expect { subject }.to raise_error(NoMethodError) + end + end + + context 'when query is partially matched in the middle of environment name' do + let(:query) { 'duction' } + + it 'returns empty array' do + is_expected.to be_empty + end + end + + context 'when query contains a wildcard character' do + let(:query) { 'produc%' } + + it 'prevents wildcard injection' do + is_expected.to be_empty + end + end + end + + describe '.pluck_names' do + subject { described_class.pluck_names } + + let!(:environment) { create(:environment, name: 'production', project: project) } + + it 'plucks names' do + is_expected.to eq(%w[production]) + end + end + describe '#expire_etag_cache' do let(:store) { Gitlab::EtagCaching::Store.new } diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb index 677613b7980..6c35ed8f649 100644 --- a/spec/models/sent_notification_spec.rb +++ b/spec/models/sent_notification_spec.rb @@ -36,19 +36,41 @@ describe SentNotification do end end + shared_examples 'a successful sent notification' do + it 'creates a new SentNotification' do + expect { subject }.to change { described_class.count }.by(1) + end + end + describe '.record' do let(:issue) { create(:issue) } - it 'creates a new SentNotification' do - expect { described_class.record(issue, user.id) }.to change { described_class.count }.by(1) - end + subject { described_class.record(issue, user.id) } + + it_behaves_like 'a successful sent notification' end describe '.record_note' do - let(:note) { create(:diff_note_on_merge_request) } + subject { described_class.record_note(note, note.author.id) } - it 'creates a new SentNotification' do - expect { described_class.record_note(note, note.author.id) }.to change { described_class.count }.by(1) + context 'for a discussion note' do + let(:note) { create(:diff_note_on_merge_request) } + + it_behaves_like 'a successful sent notification' + + it 'sets in_reply_to_discussion_id' do + expect(subject.in_reply_to_discussion_id).to eq(note.discussion_id) + end + end + + context 'for an individual note' do + let(:note) { create(:note_on_merge_request) } + + it_behaves_like 'a successful sent notification' + + it 'does not set in_reply_to_discussion_id' do + expect(subject.in_reply_to_discussion_id).to be_nil + end end end diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb index 9aaccb4bffe..af4daff336b 100644 --- a/spec/services/notes/build_service_spec.rb +++ b/spec/services/notes/build_service_spec.rb @@ -123,6 +123,46 @@ describe Notes::BuildService do end end + context 'when replying to individual note' do + let(:note) { create(:note_on_issue) } + + subject { described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute } + + shared_examples 'an individual note reply' do + it 'builds another individual note' do + expect(subject).to be_valid + expect(subject).to be_a(Note) + expect(subject.discussion_id).not_to eq(note.discussion_id) + end + end + + context 'when reply_to_individual_notes is disabled' do + before do + stub_feature_flags(reply_to_individual_notes: false) + end + + it_behaves_like 'an individual note reply' + end + + context 'when reply_to_individual_notes is enabled' do + before do + stub_feature_flags(reply_to_individual_notes: true) + end + + it 'sets the note up to be in reply to that note' do + expect(subject).to be_valid + expect(subject).to be_a(DiscussionNote) + expect(subject.discussion_id).to eq(note.discussion_id) + end + + context 'when noteable does not support replies' do + let(:note) { create(:note_on_commit) } + + it_behaves_like 'an individual note reply' + end + end + end + it 'builds a note without saving it' do new_note = described_class.new(project, author, diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 1b9ba42cfd6..48f1d696ff6 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -278,5 +278,42 @@ describe Notes::CreateService do expect(note.note).to eq(':smile:') end end + + context 'reply to individual note' do + let(:existing_note) { create(:note_on_issue, noteable: issue, project: project) } + let(:reply_opts) { opts.merge(in_reply_to_discussion_id: existing_note.discussion_id) } + + subject { described_class.new(project, user, reply_opts).execute } + + context 'when reply_to_individual_notes is disabled' do + before do + stub_feature_flags(reply_to_individual_notes: false) + end + + it 'creates an individual note' do + expect(subject.type).to eq(nil) + expect(subject.discussion_id).not_to eq(existing_note.discussion_id) + end + + it 'does not convert existing note' do + expect { subject }.not_to change { existing_note.reload.type } + end + end + + context 'when reply_to_individual_notes is enabled' do + before do + stub_feature_flags(reply_to_individual_notes: true) + end + + it 'creates a DiscussionNote in reply to existing note' do + expect(subject).to be_a(DiscussionNote) + expect(subject.discussion_id).to eq(existing_note.discussion_id) + end + + it 'converts existing note to DiscussionNote' do + expect { subject }.to change { existing_note.reload.type }.from(nil).to('DiscussionNote') + end + end + end end end |