This is a unpublished comment
') + end + + it 'creates a draft note with quick actions' do + create_draft_note(draft_overrides: { note: "#{user2.to_reference}\n/assign #{user.to_reference}" }) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['note_html']).to match(/#{user2.to_reference}/) + expect(json_response['references']['commands']).to match(/Assigns/) + expect(json_response['references']['users']).to include(user2.username) + end + + context 'in a thread' do + let(:discussion) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project).discussion } + + it 'creates draft note as a reply' do + expect do + create_draft_note(overrides: { in_reply_to_discussion_id: discussion.reply_id }) + end.to change { DraftNote.count }.by(1) + + draft_note = DraftNote.last + + expect(draft_note).to be_valid + expect(draft_note.discussion_id).to eq(discussion.reply_id) + end + + it 'creates a draft note that will resolve a thread' do + expect do + create_draft_note( + overrides: { in_reply_to_discussion_id: discussion.reply_id }, + draft_overrides: { resolve_discussion: true } + ) + end.to change { DraftNote.count }.by(1) + + draft_note = DraftNote.last + + expect(draft_note).to be_valid + expect(draft_note.discussion_id).to eq(discussion.reply_id) + expect(draft_note.resolve_discussion).to eq(true) + end + + it 'cannot create more than one draft note per thread' do + expect do + create_draft_note( + overrides: { in_reply_to_discussion_id: discussion.reply_id }, + draft_overrides: { resolve_discussion: true } + ) + end.to change { DraftNote.count }.by(1) + + expect do + create_draft_note( + overrides: { in_reply_to_discussion_id: discussion.reply_id }, + draft_overrides: { resolve_discussion: true, note: 'A note' } + ) + end.to change { DraftNote.count }.by(0) + end + end + + context 'commit_id is present' do + let(:commit) { project.commit(sample_commit.id) } + + let(:position) do + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: commit.diff_refs + ) + end + + before do + create_draft_note(draft_overrides: { commit_id: commit_id, position: position.to_json }) + end + + context 'value is a commit sha' do + let(:commit_id) { commit.id } + + it 'creates the draft note with commit ID' do + expect(DraftNote.last.commit_id).to eq(commit_id) + end + end + + context 'value is "undefined"' do + let(:commit_id) { 'undefined' } + + it 'creates the draft note with nil commit ID' do + expect(DraftNote.last.commit_id).to be_nil + end + end + end + end + + describe 'PUT #update' do + let(:draft) { create(:draft_note, merge_request: merge_request, author: user) } + + def update_draft_note(overrides = {}) + put_params = params.merge({ + id: draft.id, + draft_note: { + note: 'This is an updated unpublished comment' + }.merge(overrides) + }) + + put :update, params: put_params + end + + context 'without permissions' do + before do + sign_in(user2) + project.add_developer(user2) + end + + it 'does not allow editing draft note belonging to someone else' do + update_draft_note + + expect(response).to have_gitlab_http_status(:not_found) + expect(draft.reload.note).not_to eq('This is an updated unpublished comment') + end + end + + it 'updates the draft' do + expect(draft.note).not_to be_empty + + expect { update_draft_note }.not_to change { DraftNote.count } + + draft.reload + + expect(draft.note).to eq('This is an updated unpublished comment') + expect(json_response['note_html']).not_to be_empty + end + end + + describe 'POST #publish' do + context 'without permissions' do + shared_examples_for 'action that does not allow publishing draft note' do + it 'does not allow publishing draft note' do + expect { action } + .to not_change { Note.count } + .and not_change { DraftNote.count } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + before do + sign_in(user2) + end + + context 'when note belongs to someone else' do + before do + project.add_developer(user2) + end + + it_behaves_like 'action that does not allow publishing draft note' do + let!(:draft) { create(:draft_note, merge_request: merge_request, author: user) } + let(:action) { post :publish, params: params.merge(id: draft.id) } + end + end + + context 'when merge request discussion is locked' do + let(:project) { create(:project, :public, :merge_requests_public, :repository) } + + before do + create(:draft_note, merge_request: merge_request, author: user2) + merge_request.update!(discussion_locked: true) + end + + it_behaves_like 'action that does not allow publishing draft note' do + let(:action) { post :publish, params: params } + end + end + end + + context 'when PublishService errors' do + it 'returns message and 500 response' do + create(:draft_note, merge_request: merge_request, author: user) + error_message = "Something went wrong" + + expect_next_instance_of(DraftNotes::PublishService) do |service| + allow(service).to receive(:execute).and_return({ message: error_message, status: :error }) + end + + post :publish, params: params + + expect(response).to have_gitlab_http_status(:error) + expect(json_response["message"]).to include(error_message) + end + end + + it 'publishes draft notes with position' do + diff_refs = project.commit(sample_commit.id).try(:diff_refs) + + position = Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: diff_refs + ) + + draft = create(:draft_note_on_text_diff, merge_request: merge_request, author: user, position: position) + + expect { post :publish, params: params }.to change { Note.count }.by(1) + .and change { DraftNote.count }.by(-1) + + note = merge_request.notes.reload.last + + expect(note.note).to eq(draft.note) + expect(note.position).to eq(draft.position) + end + + it 'does nothing if there are no draft notes' do + expect { post :publish, params: params }.to change { Note.count }.by(0).and change { DraftNote.count }.by(0) + end + + it 'publishes a draft note with quick actions and applies them' do + project.add_developer(user2) + create(:draft_note, merge_request: merge_request, author: user, + note: "/assign #{user2.to_reference}") + + expect(merge_request.assignees).to be_empty + + expect { post :publish, params: params }.to change { Note.count }.by(1) + .and change { DraftNote.count }.by(-1) + + expect(response).to have_gitlab_http_status(:ok) + expect(merge_request.reload.assignee_ids).to match_array([user2.id]) + expect(Note.last.system?).to be true + end + + it 'publishes all draft notes for an MR' do + draft_params = { merge_request: merge_request, author: user } + + drafts = create_list(:draft_note, 4, draft_params) + + note = create(:discussion_note_on_merge_request, noteable: merge_request, project: project) + draft_reply = create(:draft_note, draft_params.merge(discussion_id: note.discussion_id)) + + diff_note = create(:diff_note_on_merge_request, noteable: merge_request, project: project) + diff_draft_reply = create(:draft_note, draft_params.merge(discussion_id: diff_note.discussion_id)) + + expect { post :publish, params: params }.to change { Note.count }.by(6) + .and change { DraftNote.count }.by(-6) + + expect(response).to have_gitlab_http_status(:ok) + + notes = merge_request.notes.reload + + expect(notes.pluck(:note)).to include(*drafts.map(&:note)) + expect(note.discussion.notes.last.note).to eq(draft_reply.note) + expect(diff_note.discussion.notes.last.note).to eq(diff_draft_reply.note) + end + + it 'can publish just a single draft note' do + draft_params = { merge_request: merge_request, author: user } + + drafts = create_list(:draft_note, 4, draft_params) + + expect { post :publish, params: params.merge(id: drafts.first.id) }.to change { Note.count }.by(1) + .and change { DraftNote.count }.by(-1) + end + + context 'when publishing drafts in a thread' do + let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) } + + def create_reply(discussion_id, resolves: false) + create(:draft_note, + merge_request: merge_request, + author: user, + discussion_id: discussion_id, + resolve_discussion: resolves + ) + end + + it 'resolves a thread if the draft note resolves it' do + draft_reply = create_reply(note.discussion_id, resolves: true) + + post :publish, params: params + + discussion = note.discussion + + expect(discussion.notes.last.note).to eq(draft_reply.note) + expect(discussion.resolved?).to eq(true) + expect(discussion.resolved_by.id).to eq(user.id) + end + + it 'unresolves a thread if the draft note unresolves it' do + note.discussion.resolve!(user) + expect(note.discussion.resolved?).to eq(true) + + draft_reply = create_reply(note.discussion_id, resolves: false) + + post :publish, params: params + + discussion = note.discussion + + expect(discussion.notes.last.note).to eq(draft_reply.note) + expect(discussion.resolved?).to eq(false) + end + end + end + + describe 'DELETE #destroy' do + let(:draft) { create(:draft_note, merge_request: merge_request, author: user) } + + def create_draft + create(:draft_note, merge_request: merge_request, author: user) + end + + context 'without permissions' do + before do + sign_in(user2) + project.add_developer(user2) + end + + it 'does not allow destroying a draft note belonging to someone else' do + draft = create(:draft_note, merge_request: merge_request, author: user) + + expect { post :destroy, params: params.merge(id: draft.id) } + .not_to change { DraftNote.count } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + it 'destroys the draft note when ID is given' do + draft = create_draft + + expect { delete :destroy, params: params.merge(id: draft.id) }.to change { DraftNote.count }.by(-1) + expect(response).to have_gitlab_http_status(:ok) + end + + context 'without permissions' do + before do + sign_in(user2) + end + + it 'does not allow editing draft note belonging to someone else' do + draft = create_draft + + expect { delete :destroy, params: params.merge(id: draft.id) }.to change { DraftNote.count }.by(0) + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe 'DELETE #discard' do + it 'deletes all DraftNotes belonging to a user in a Merge Request' do + create_list(:draft_note, 6, merge_request: merge_request, author: user) + + expect { delete :discard, params: params }.to change { DraftNote.count }.by(-6) + expect(response).to have_gitlab_http_status(:ok) + end + + context 'without permissions' do + before do + sign_in(user2) + project.add_developer(user2) + end + + it 'does not destroys a draft note belonging to someone else' do + create(:draft_note, merge_request: merge_request, author: user) + + expect { post :discard, params: params } + .not_to change { DraftNote.count } + + expect(response).to have_gitlab_http_status(:ok) + end + end + end +end diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 7c3ba122b5a..a7d4b4eb57a 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -183,6 +183,10 @@ FactoryBot.define do confidential { true } end + trait :with_review do + review + end + transient do in_reply_to { nil } end diff --git a/spec/frontend/logs/stores/actions_spec.js b/spec/frontend/logs/stores/actions_spec.js index 6199c400e16..e2e3c3d23c6 100644 --- a/spec/frontend/logs/stores/actions_spec.js +++ b/spec/frontend/logs/stores/actions_spec.js @@ -1,6 +1,6 @@ import MockAdapter from 'axios-mock-adapter'; - import testAction from 'helpers/vuex_action_helper'; +import Tracking from '~/tracking'; import * as types from '~/logs/stores/mutation_types'; import { convertToFixedRange } from '~/lib/utils/datetime_range'; import logsPageState from '~/logs/stores/state'; @@ -104,7 +104,7 @@ describe('Logs Store actions', () => { { type: types.SET_CURRENT_POD_NAME, payload: null }, { type: types.SET_SEARCH, payload: '' }, ], - [{ type: 'fetchLogs' }], + [{ type: 'fetchLogs', payload: 'used_search_bar' }], )); it('text search should filter with a search term', () => @@ -116,7 +116,7 @@ describe('Logs Store actions', () => { { type: types.SET_CURRENT_POD_NAME, payload: null }, { type: types.SET_SEARCH, payload: mockSearch }, ], - [{ type: 'fetchLogs' }], + [{ type: 'fetchLogs', payload: 'used_search_bar' }], )); it('pod search should filter with a search term', () => @@ -128,7 +128,7 @@ describe('Logs Store actions', () => { { type: types.SET_CURRENT_POD_NAME, payload: mockPodName }, { type: types.SET_SEARCH, payload: '' }, ], - [{ type: 'fetchLogs' }], + [{ type: 'fetchLogs', payload: 'used_search_bar' }], )); it('pod search should filter with a pod selection and a search term', () => @@ -140,7 +140,7 @@ describe('Logs Store actions', () => { { type: types.SET_CURRENT_POD_NAME, payload: mockPodName }, { type: types.SET_SEARCH, payload: mockSearch }, ], - [{ type: 'fetchLogs' }], + [{ type: 'fetchLogs', payload: 'used_search_bar' }], )); it('pod search should filter with a pod selection and two search terms', () => @@ -152,7 +152,7 @@ describe('Logs Store actions', () => { { type: types.SET_CURRENT_POD_NAME, payload: null }, { type: types.SET_SEARCH, payload: `term1 term2` }, ], - [{ type: 'fetchLogs' }], + [{ type: 'fetchLogs', payload: 'used_search_bar' }], )); it('pod search should filter with a pod selection and a search terms before and after', () => @@ -168,7 +168,7 @@ describe('Logs Store actions', () => { { type: types.SET_CURRENT_POD_NAME, payload: mockPodName }, { type: types.SET_SEARCH, payload: `term1 term2` }, ], - [{ type: 'fetchLogs' }], + [{ type: 'fetchLogs', payload: 'used_search_bar' }], )); }); @@ -179,7 +179,7 @@ describe('Logs Store actions', () => { mockPodName, state, [{ type: types.SET_CURRENT_POD_NAME, payload: mockPodName }], - [{ type: 'fetchLogs' }], + [{ type: 'fetchLogs', payload: 'pod_log_changed' }], )); }); @@ -198,7 +198,7 @@ describe('Logs Store actions', () => { { type: types.REQUEST_ENVIRONMENTS_DATA }, { type: types.RECEIVE_ENVIRONMENTS_DATA_SUCCESS, payload: mockEnvironments }, ], - [{ type: 'fetchLogs' }], + [{ type: 'fetchLogs', payload: 'environment_selected' }], ); }); @@ -471,3 +471,58 @@ describe('Logs Store actions', () => { }); }); }); + +describe('Tracking user interaction', () => { + let commit; + let dispatch; + let state; + let mock; + + beforeEach(() => { + jest.spyOn(Tracking, 'event'); + commit = jest.fn(); + dispatch = jest.fn(); + state = logsPageState(); + state.environments.options = mockEnvironments; + state.environments.current = mockEnvName; + + mock = new MockAdapter(axios); + }); + + afterEach(() => { + mock.reset(); + }); + + describe('Logs with data', () => { + beforeEach(() => { + mock.onGet(mockLogsEndpoint).reply(200, mockResponse); + mock.onGet(mockLogsEndpoint).replyOnce(202); // mock reactive cache + }); + + it('tracks fetched logs with data', () => { + return fetchLogs({ state, commit, dispatch }, 'environment_selected').then(() => { + expect(Tracking.event).toHaveBeenCalledWith(document.body.dataset.page, 'logs_view', { + label: 'environment_selected', + property: 'count', + value: 1, + }); + }); + }); + }); + + describe('Logs without data', () => { + beforeEach(() => { + mock.onGet(mockLogsEndpoint).reply(200, { + ...mockResponse, + logs: [], + }); + mock.onGet(mockLogsEndpoint).replyOnce(202); // mock reactive cache + }); + + it('does not track empty log responses', () => { + return fetchLogs({ state, commit, dispatch }).then(() => { + expect(Tracking.event).not.toHaveBeenCalled(); + }); + }); + }); +}); diff --git a/spec/frontend/registry/explorer/components/image_list_row_spec.js b/spec/frontend/registry/explorer/components/image_list_row_spec.js new file mode 100644 index 00000000000..a6c5d485051 --- /dev/null +++ b/spec/frontend/registry/explorer/components/image_list_row_spec.js @@ -0,0 +1,140 @@ +import { shallowMount } from '@vue/test-utils'; +import { GlIcon, GlSprintf } from '@gitlab/ui'; +import { createMockDirective, getBinding } from 'helpers/vue_mock_directive'; +import Component from '~/registry/explorer/components/image_list_row.vue'; +import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; +import { + ROW_SCHEDULED_FOR_DELETION, + LIST_DELETE_BUTTON_DISABLED, +} from '~/registry/explorer/constants'; +import { RouterLink } from '../stubs'; +import { imagesListResponse } from '../mock_data'; + +describe('Image List Row', () => { + let wrapper; + const item = imagesListResponse.data[0]; + const findDeleteBtn = () => wrapper.find('[data-testid="deleteImageButton"]'); + const findDetailsLink = () => wrapper.find('[data-testid="detailsLink"]'); + const findTagsCount = () => wrapper.find('[data-testid="tagsCount"]'); + const findDeleteButtonWrapper = () => wrapper.find('[data-testid="deleteButtonWrapper"]'); + const findClipboardButton = () => wrapper.find(ClipboardButton); + + const mountComponent = props => { + wrapper = shallowMount(Component, { + stubs: { + RouterLink, + GlSprintf, + }, + propsData: { + item, + ...props, + }, + directives: { + GlTooltip: createMockDirective(), + }, + }); + }; + + afterEach(() => { + wrapper.destroy(); + wrapper = null; + }); + + describe('main tooltip', () => { + it(`the title is ${ROW_SCHEDULED_FOR_DELETION}`, () => { + mountComponent(); + const tooltip = getBinding(wrapper.element, 'gl-tooltip'); + expect(tooltip).toBeDefined(); + expect(tooltip.value.title).toBe(ROW_SCHEDULED_FOR_DELETION); + }); + + it('is disabled when item is being deleted', () => { + mountComponent({ item: { ...item, deleting: true } }); + const tooltip = getBinding(wrapper.element, 'gl-tooltip'); + expect(tooltip.value.disabled).toBe(false); + }); + }); + + describe('image title and path', () => { + it('contains a link to the details page', () => { + mountComponent(); + const link = findDetailsLink(); + expect(link.html()).toContain(item.path); + expect(link.props('to').name).toBe('details'); + }); + + it('contains a clipboard button', () => { + mountComponent(); + const button = findClipboardButton(); + expect(button.exists()).toBe(true); + expect(button.props('text')).toBe(item.location); + expect(button.props('title')).toBe(item.location); + }); + }); + + describe('delete button wrapper', () => { + it('has a tooltip', () => { + mountComponent(); + const tooltip = getBinding(findDeleteButtonWrapper().element, 'gl-tooltip'); + expect(tooltip).toBeDefined(); + expect(tooltip.value.title).toBe(LIST_DELETE_BUTTON_DISABLED); + }); + it('tooltip is enabled when destroy_path is falsy', () => { + mountComponent({ item: { ...item, destroy_path: null } }); + const tooltip = getBinding(findDeleteButtonWrapper().element, 'gl-tooltip'); + expect(tooltip.value.disabled).toBeFalsy(); + }); + }); + + describe('delete button', () => { + it('exists', () => { + mountComponent(); + expect(findDeleteBtn().exists()).toBe(true); + }); + + it('emits a delete event', () => { + mountComponent(); + findDeleteBtn().vm.$emit('click'); + expect(wrapper.emitted('delete')).toEqual([[item]]); + }); + + it.each` + destroy_path | deleting | state + ${null} | ${null} | ${'true'} + ${null} | ${true} | ${'true'} + ${'foo'} | ${true} | ${'true'} + ${'foo'} | ${false} | ${undefined} + `( + 'disabled is $state when destroy_path is $destroy_path and deleting is $deleting', + ({ destroy_path, deleting, state }) => { + mountComponent({ item: { ...item, destroy_path, deleting } }); + expect(findDeleteBtn().attributes('disabled')).toBe(state); + }, + ); + }); + + describe('tags count', () => { + it('exists', () => { + mountComponent(); + expect(findTagsCount().exists()).toBe(true); + }); + + it('contains a tag icon', () => { + mountComponent(); + const icon = findTagsCount().find(GlIcon); + expect(icon.exists()).toBe(true); + expect(icon.props('name')).toBe('tag'); + }); + + describe('tags count text', () => { + it('with one tag in the image', () => { + mountComponent({ item: { ...item, tags_count: 1 } }); + expect(findTagsCount().text()).toMatchInterpolatedText('1 Tag'); + }); + it('with more than one tag in the image', () => { + mountComponent({ item: { ...item, tags_count: 3 } }); + expect(findTagsCount().text()).toMatchInterpolatedText('3 Tags'); + }); + }); + }); +}); diff --git a/spec/frontend/registry/explorer/components/image_list_spec.js b/spec/frontend/registry/explorer/components/image_list_spec.js index 12f0fbe0c87..f849e60a749 100644 --- a/spec/frontend/registry/explorer/components/image_list_spec.js +++ b/spec/frontend/registry/explorer/components/image_list_spec.js @@ -1,26 +1,18 @@ import { shallowMount } from '@vue/test-utils'; import { GlPagination } from '@gitlab/ui'; import Component from '~/registry/explorer/components/image_list.vue'; -import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; -import { RouterLink } from '../stubs'; +import ImageListRow from '~/registry/explorer/components/image_list_row.vue'; + import { imagesListResponse, imagePagination } from '../mock_data'; describe('Image List', () => { let wrapper; - const firstElement = imagesListResponse.data[0]; - - const findDeleteBtn = () => wrapper.find('[data-testid="deleteImageButton"]'); - const findRowItems = () => wrapper.findAll('[data-testid="rowItem"]'); - const findDetailsLink = () => wrapper.find('[data-testid="detailsLink"]'); - const findClipboardButton = () => wrapper.find(ClipboardButton); + const findRow = () => wrapper.findAll(ImageListRow); const findPagination = () => wrapper.find(GlPagination); const mountComponent = () => { wrapper = shallowMount(Component, { - stubs: { - RouterLink, - }, propsData: { images: imagesListResponse.data, pagination: imagePagination, @@ -32,26 +24,17 @@ describe('Image List', () => { mountComponent(); }); - it('contains one list element for each image', () => { - expect(findRowItems().length).toBe(imagesListResponse.data.length); - }); + describe('list', () => { + it('contains one list element for each image', () => { + expect(findRow().length).toBe(imagesListResponse.data.length); + }); - it('contains a link to the details page', () => { - const link = findDetailsLink(); - expect(link.html()).toContain(firstElement.path); - expect(link.props('to').name).toBe('details'); - }); - - it('contains a clipboard button', () => { - const button = findClipboardButton(); - expect(button.exists()).toBe(true); - expect(button.props('text')).toBe(firstElement.location); - expect(button.props('title')).toBe(firstElement.location); - }); - - it('should be possible to delete a repo', () => { - const deleteBtn = findDeleteBtn(); - expect(deleteBtn.exists()).toBe(true); + it('when delete event is emitted on the row it emits up a delete event', () => { + findRow() + .at(0) + .vm.$emit('delete', 'foo'); + expect(wrapper.emitted('delete')).toEqual([['foo']]); + }); }); describe('pagination', () => { diff --git a/spec/lib/gitlab/gl_repository/identifier_spec.rb b/spec/lib/gitlab/gl_repository/identifier_spec.rb new file mode 100644 index 00000000000..c36f296702e --- /dev/null +++ b/spec/lib/gitlab/gl_repository/identifier_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::GlRepository::Identifier do + let_it_be(:project) { create(:project) } + let_it_be(:personal_snippet) { create(:personal_snippet, author: project.owner) } + let_it_be(:project_snippet) { create(:project_snippet, project: project, author: project.owner) } + + describe 'project repository' do + it_behaves_like 'parsing gl_repository identifier' do + let(:record_id) { project.id } + let(:identifier) { "project-#{record_id}" } + let(:expected_container) { project } + let(:expected_type) { Gitlab::GlRepository::PROJECT } + end + end + + describe 'wiki' do + it_behaves_like 'parsing gl_repository identifier' do + let(:record_id) { project.id } + let(:identifier) { "wiki-#{record_id}" } + let(:expected_container) { project } + let(:expected_type) { Gitlab::GlRepository::WIKI } + end + end + + describe 'snippet' do + context 'when PersonalSnippet' do + it_behaves_like 'parsing gl_repository identifier' do + let(:record_id) { personal_snippet.id } + let(:identifier) { "snippet-#{record_id}" } + let(:expected_container) { personal_snippet } + let(:expected_type) { Gitlab::GlRepository::SNIPPET } + end + end + + context 'when ProjectSnippet' do + it_behaves_like 'parsing gl_repository identifier' do + let(:record_id) { project_snippet.id } + let(:identifier) { "snippet-#{record_id}" } + let(:expected_container) { project_snippet } + let(:expected_type) { Gitlab::GlRepository::SNIPPET } + end + end + end + + describe 'design' do + it_behaves_like 'parsing gl_repository identifier' do + let(:record_id) { project.id } + let(:identifier) { "design-#{project.id}" } + let(:expected_container) { project } + let(:expected_type) { Gitlab::GlRepository::DESIGN } + end + end + + describe 'incorrect format' do + def expect_error_raised_for(identifier) + expect { described_class.new(identifier) }.to raise_error(ArgumentError) + end + + it 'raises error for incorrect id' do + expect_error_raised_for('wiki-noid') + end + + it 'raises error for incorrect type' do + expect_error_raised_for('foo-2') + end + + it 'raises error for incorrect three-segment container' do + expect_error_raised_for('snippet-2-wiki') + end + + it 'raises error for one segment' do + expect_error_raised_for('snippet') + end + + it 'raises error for more than three segments' do + expect_error_raised_for('project-1-wiki-bar') + end + end +end diff --git a/spec/lib/gitlab/gl_repository/repo_type_spec.rb b/spec/lib/gitlab/gl_repository/repo_type_spec.rb index bf6df55b71e..f5270104d2f 100644 --- a/spec/lib/gitlab/gl_repository/repo_type_spec.rb +++ b/spec/lib/gitlab/gl_repository/repo_type_spec.rb @@ -13,7 +13,7 @@ describe Gitlab::GlRepository::RepoType do describe Gitlab::GlRepository::PROJECT do it_behaves_like 'a repo type' do - let(:expected_id) { project.id.to_s } + let(:expected_id) { project.id } let(:expected_identifier) { "project-#{expected_id}" } let(:expected_suffix) { '' } let(:expected_container) { project } @@ -42,7 +42,7 @@ describe Gitlab::GlRepository::RepoType do describe Gitlab::GlRepository::WIKI do it_behaves_like 'a repo type' do - let(:expected_id) { project.id.to_s } + let(:expected_id) { project.id } let(:expected_identifier) { "wiki-#{expected_id}" } let(:expected_suffix) { '.wiki' } let(:expected_container) { project } @@ -72,7 +72,7 @@ describe Gitlab::GlRepository::RepoType do describe Gitlab::GlRepository::SNIPPET do context 'when PersonalSnippet' do it_behaves_like 'a repo type' do - let(:expected_id) { personal_snippet.id.to_s } + let(:expected_id) { personal_snippet.id } let(:expected_identifier) { "snippet-#{expected_id}" } let(:expected_suffix) { '' } let(:expected_repository) { personal_snippet.repository } @@ -101,7 +101,7 @@ describe Gitlab::GlRepository::RepoType do context 'when ProjectSnippet' do it_behaves_like 'a repo type' do - let(:expected_id) { project_snippet.id.to_s } + let(:expected_id) { project_snippet.id } let(:expected_identifier) { "snippet-#{expected_id}" } let(:expected_suffix) { '' } let(:expected_repository) { project_snippet.repository } @@ -131,7 +131,7 @@ describe Gitlab::GlRepository::RepoType do describe Gitlab::GlRepository::DESIGN do it_behaves_like 'a repo type' do let(:expected_identifier) { "design-#{project.id}" } - let(:expected_id) { project.id.to_s } + let(:expected_id) { project.id } let(:expected_suffix) { '.design' } let(:expected_repository) { project.design_repository } let(:expected_container) { project } diff --git a/spec/lib/gitlab/gl_repository_spec.rb b/spec/lib/gitlab/gl_repository_spec.rb index 5f5244b7116..413540b4db8 100644 --- a/spec/lib/gitlab/gl_repository_spec.rb +++ b/spec/lib/gitlab/gl_repository_spec.rb @@ -11,7 +11,7 @@ describe ::Gitlab::GlRepository do expect(described_class.parse("project-#{project.id}")).to eq([project, project, Gitlab::GlRepository::PROJECT]) end - it 'parses a wiki gl_repository' do + it 'parses a project wiki gl_repository' do expect(described_class.parse("wiki-#{project.id}")).to eq([project, project, Gitlab::GlRepository::WIKI]) end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 4a0f0eea088..05d2e4bb5a2 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -15,6 +15,177 @@ describe API::Users, :do_not_mock_admin_mode do let(:not_existing_pat_id) { (PersonalAccessToken.maximum('id') || 0 ) + 10 } let(:private_user) { create(:user, private_profile: true) } + context 'admin notes' do + let(:admin) { create(:admin, note: '2019-10-06 | 2FA added | user requested | www.gitlab.com') } + let(:user) { create(:user, note: '2018-11-05 | 2FA removed | user requested | www.gitlab.com') } + + describe 'POST /users' do + context 'when unauthenticated' do + it 'return authentication error' do + post api('/users') + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + + context 'when authenticated' do + context 'as an admin' do + it 'contains the note of the user' do + optional_attributes = { note: 'Awesome Note' } + attributes = attributes_for(:user).merge(optional_attributes) + + post api('/users', admin), params: attributes + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['note']).to eq(optional_attributes[:note]) + end + end + + context 'as a regular user' do + it 'does not allow creating new user' do + post api('/users', user), params: attributes_for(:user) + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end + end + + describe 'GET /users/:id' do + context 'when unauthenticated' do + it 'does not contain the note of the user' do + get api("/users/#{user.id}") + + expect(json_response).not_to have_key('note') + end + end + + context 'when authenticated' do + context 'as an admin' do + it 'contains the note of the user' do + get api("/users/#{user.id}", admin) + + expect(json_response).to have_key('note') + expect(json_response['note']).to eq(user.note) + end + end + + context 'as a regular user' do + it 'does not contain the note of the user' do + get api("/users/#{user.id}", user) + + expect(json_response).not_to have_key('note') + end + end + end + end + + describe "PUT /users/:id" do + context 'when user is an admin' do + it "updates note of the user" do + new_note = '2019-07-07 | Email changed | user requested | www.gitlab.com' + + expect do + put api("/users/#{user.id}", admin), params: { note: new_note } + end.to change { user.reload.note } + .from('2018-11-05 | 2FA removed | user requested | www.gitlab.com') + .to(new_note) + + expect(response).to have_gitlab_http_status(:success) + expect(json_response['note']).to eq(new_note) + end + end + + context 'when user is not an admin' do + it "cannot update their own note" do + expect do + put api("/users/#{user.id}", user), params: { note: 'new note' } + end.not_to change { user.reload.note } + + expect(response).to have_gitlab_http_status(:forbidden) + end + end + end + + describe 'GET /users/' do + context 'when unauthenticated' do + it "does not contain the note of users" do + get api("/users"), params: { username: user.username } + + expect(json_response.first).not_to have_key('note') + end + end + + context 'when authenticated' do + context 'as a regular user' do + it 'does not contain the note of users' do + get api("/users", user), params: { username: user.username } + + expect(json_response.first).not_to have_key('note') + end + end + + context 'as an admin' do + it 'contains the note of users' do + get api("/users", admin), params: { username: user.username } + + expect(response).to have_gitlab_http_status(:success) + expect(json_response.first).to have_key('note') + expect(json_response.first['note']).to eq '2018-11-05 | 2FA removed | user requested | www.gitlab.com' + end + end + end + end + + describe 'GET /user' do + context 'when authenticated' do + context 'as an admin' do + context 'accesses their own profile' do + it 'contains the note of the user' do + get api("/user", admin) + + expect(json_response).to have_key('note') + expect(json_response['note']).to eq(admin.note) + end + end + + context 'sudo' do + let(:admin_personal_access_token) { create(:personal_access_token, user: admin, scopes: %w[api sudo]).token } + + context 'accesses the profile of another regular user' do + it 'does not contain the note of the user' do + get api("/user?private_token=#{admin_personal_access_token}&sudo=#{user.id}") + + expect(json_response['id']).to eq(user.id) + expect(json_response).not_to have_key('note') + end + end + + context 'accesses the profile of another admin' do + let(:admin_2) {create(:admin, note: '2010-10-10 | 2FA added | admin requested | www.gitlab.com')} + + it 'contains the note of the user' do + get api("/user?private_token=#{admin_personal_access_token}&sudo=#{admin_2.id}") + + expect(json_response['id']).to eq(admin_2.id) + expect(json_response).to have_key('note') + expect(json_response['note']).to eq(admin_2.note) + end + end + end + end + + context 'as a regular user' do + it 'does not contain the note of the user' do + get api("/user", user) + + expect(json_response).not_to have_key('note') + end + end + end + end + end + shared_examples 'rendering user status' do it 'returns the status if there was one' do create(:user_status, user: user) diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index a9de0a747f6..1bd402e38be 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -1621,6 +1621,29 @@ describe QuickActions::InterpretService do expect(message).to eq("Created branch '#{branch_name}' and a merge request to resolve this issue.") end end + + context 'submit_review command' do + using RSpec::Parameterized::TableSyntax + + where(:note) do + [ + 'I like it', + '/submit_review' + ] + end + + with_them do + let(:content) { '/submit_review' } + let!(:draft_note) { create(:draft_note, note: note, merge_request: merge_request, author: developer) } + + it 'submits the users current review' do + _, _, message = service.execute(content, merge_request) + + expect { draft_note.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(message).to eq('Submitted the current review.') + end + end + end end describe '#explain' do diff --git a/spec/support/shared_examples/lib/gitlab/gl_repository_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/gl_repository_shared_examples.rb new file mode 100644 index 00000000000..97f4341340d --- /dev/null +++ b/spec/support/shared_examples/lib/gitlab/gl_repository_shared_examples.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'parsing gl_repository identifier' do + subject { described_class.new(identifier) } + + it 'returns correct information' do + aggregate_failures do + expect(subject.repo_type).to eq(expected_type) + expect(subject.fetch_container!).to eq(expected_container) + end + end +end diff --git a/spec/support/shared_examples/lib/gitlab/repo_type_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/repo_type_shared_examples.rb index 69ae9339f10..4aeae788114 100644 --- a/spec/support/shared_examples/lib/gitlab/repo_type_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/repo_type_shared_examples.rb @@ -7,26 +7,6 @@ RSpec.shared_examples 'a repo type' do it { is_expected.to eq(expected_identifier) } end - describe '#fetch_id' do - it 'finds an id match in the identifier' do - expect(described_class.fetch_id(expected_identifier)).to eq(expected_id) - end - - it 'does not break on other identifiers' do - expect(described_class.fetch_id('wiki-noid')).to eq(nil) - end - end - - describe '#fetch_container!' do - it 'returns the container' do - expect(described_class.fetch_container!(expected_identifier)).to eq expected_container - end - - it 'raises an exception if the identifier is invalid' do - expect { described_class.fetch_container!('project-noid') }.to raise_error ArgumentError - end - end - describe '#path_suffix' do subject { described_class.path_suffix } diff --git a/spec/workers/new_note_worker_spec.rb b/spec/workers/new_note_worker_spec.rb index cf350fbcf2a..57269355180 100644 --- a/spec/workers/new_note_worker_spec.rb +++ b/spec/workers/new_note_worker_spec.rb @@ -49,4 +49,14 @@ describe NewNoteWorker do described_class.new.perform(unexistent_note_id) end end + + context 'when note is with review' do + it 'does not create a new note notification' do + note = create(:note, :with_review) + + expect_any_instance_of(NotificationService).not_to receive(:new_note) + + subject.perform(note.id) + end + end end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index aab7a36189a..18e06332eb3 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -355,7 +355,7 @@ describe PostReceive do context "webhook" do it "fetches the correct project" do - expect(Project).to receive(:find_by).with(id: project.id.to_s) + expect(Project).to receive(:find_by).with(id: project.id) perform end