diff options
Diffstat (limited to 'spec')
35 files changed, 1886 insertions, 174 deletions
diff --git a/spec/controllers/projects/design_management/designs/resized_image_controller_spec.rb b/spec/controllers/projects/design_management/designs/resized_image_controller_spec.rb index 9a3fee5b43a..6bfec1b314e 100644 --- a/spec/controllers/projects/design_management/designs/resized_image_controller_spec.rb +++ b/spec/controllers/projects/design_management/designs/resized_image_controller_spec.rb @@ -14,16 +14,6 @@ describe Projects::DesignManagement::Designs::ResizedImageController do let(:sha) { design.versions.first.sha } before do - # TODO these tests are being temporarily skipped unless run in EE, - # as we are in the process of moving Design Management to FOSS in 13.0 - # in steps. In the current step the services have not yet been moved, - # and the `design` factory used in this test uses the `:with_smaller_image_versions` - # trait, which calls `GenerateImageVersionsService` to generate the - # smaller image versions. - # - # See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283. - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - enable_design_management end diff --git a/spec/frontend/alert_management/components/alert_management_list_spec.js b/spec/frontend/alert_management/components/alert_management_list_spec.js index 662c39fc84d..df845884b03 100644 --- a/spec/frontend/alert_management/components/alert_management_list_spec.js +++ b/spec/frontend/alert_management/components/alert_management_list_spec.js @@ -4,16 +4,20 @@ import { GlTable, GlAlert, GlLoadingIcon, - GlNewDropdown, + GlDropdown, GlBadge, GlTab, + GlDropdownItem, } from '@gitlab/ui'; import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue'; +import createFlash from '~/flash'; import AlertManagementList from '~/alert_management/components/alert_management_list.vue'; import { ALERTS_STATUS_TABS } from '../../../../app/assets/javascripts/alert_management/constants'; - +import updateAlertStatus from '~/alert_management/graphql/mutations/update_alert_status.graphql'; import mockAlerts from '../mocks/alerts.json'; +jest.mock('~/flash'); + describe('AlertManagementList', () => { let wrapper; @@ -21,10 +25,11 @@ describe('AlertManagementList', () => { const findAlerts = () => wrapper.findAll('table tbody tr'); const findAlert = () => wrapper.find(GlAlert); const findLoader = () => wrapper.find(GlLoadingIcon); - const findStatusDropdown = () => wrapper.find(GlNewDropdown); + const findStatusDropdown = () => wrapper.find(GlDropdown); const findStatusFilterTabs = () => wrapper.findAll(GlTab); const findNumberOfAlertsBadge = () => wrapper.findAll(GlBadge); const findDateFields = () => wrapper.findAll(TimeAgo); + const findFirstStatusOption = () => findStatusDropdown().find(GlDropdownItem); function mountComponent({ props = { @@ -51,6 +56,7 @@ describe('AlertManagementList', () => { }, mocks: { $apollo: { + mutate: jest.fn(), queries: { alerts: { loading, @@ -191,6 +197,7 @@ describe('AlertManagementList', () => { alerts: [ { iid: 1, + status: 'acknowledged', startedAt: '2020-03-17T23:18:14.996Z', endedAt: '2020-04-17T23:18:14.996Z', }, @@ -209,6 +216,7 @@ describe('AlertManagementList', () => { alerts: [ { iid: 1, + status: 'acknowledged', startedAt: null, endedAt: null, }, @@ -221,4 +229,52 @@ describe('AlertManagementList', () => { }); }); }); + + describe('updating the alert status', () => { + const iid = '1527542'; + const mockUpdatedMutationResult = { + data: { + updateAlertStatus: { + errors: [], + alert: { + iid, + status: 'acknowledged', + }, + }, + }, + }; + + beforeEach(() => { + mountComponent({ + props: { alertManagementEnabled: true, userCanEnableAlertManagement: true }, + data: { alerts: mockAlerts, errored: false }, + loading: false, + }); + }); + + it('calls `$apollo.mutate` with `updateAlertStatus` mutation and variables containing `iid`, `status`, & `projectPath`', () => { + jest.spyOn(wrapper.vm.$apollo, 'mutate').mockResolvedValue(mockUpdatedMutationResult); + findFirstStatusOption().vm.$emit('click'); + + expect(wrapper.vm.$apollo.mutate).toHaveBeenCalledWith({ + mutation: updateAlertStatus, + variables: { + iid, + status: 'TRIGGERED', + projectPath: 'gitlab-org/gitlab', + }, + }); + }); + + it('calls `createFlash` when request fails', () => { + jest.spyOn(wrapper.vm.$apollo, 'mutate').mockReturnValue(Promise.reject(new Error())); + findFirstStatusOption().vm.$emit('click'); + + setImmediate(() => { + expect(createFlash).toHaveBeenCalledWith( + 'There was an error while updating the status of the alert. Please try again.', + ); + }); + }); + }); }); diff --git a/spec/frontend/alert_management/mocks/alerts.json b/spec/frontend/alert_management/mocks/alerts.json index e0b8fa55507..5e6b96aed71 100644 --- a/spec/frontend/alert_management/mocks/alerts.json +++ b/spec/frontend/alert_management/mocks/alerts.json @@ -9,8 +9,8 @@ "status": "triggered" }, { - "iid": "1527542", - "title": "Some otherr alert Some otherr alert Some otherr alert Some otherr alert Some otherr alert Some otherr alert", + "iid": "1527543", + "title": "Some other alert Some other alert Some other alert Some other alert Some other alert Some other alert", "severity": "MEDIUM", "eventCount": 1, "startedAt": "2020-04-17T23:18:14.996Z", @@ -18,7 +18,7 @@ "status": "acknowledged" }, { - "iid": "1527542", + "iid": "1527544", "title": "SyntaxError: Invalid or unexpected token", "severity": "LOW", "eventCount": 4, diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 1a95f586344..57e3a93c6f4 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -28,8 +28,14 @@ describe('diffs/components/app', () => { let wrapper; let mock; - function createComponent(props = {}, extendStore = () => {}) { + function createComponent(props = {}, extendStore = () => {}, provisions = {}) { const localVue = createLocalVue(); + const provide = { + ...provisions, + glFeatures: { + ...(provisions.glFeatures || {}), + }, + }; localVue.use(Vuex); @@ -52,6 +58,7 @@ describe('diffs/components/app', () => { showSuggestPopover: true, ...props, }, + provide, store, methods: { isLatestVersion() { @@ -82,7 +89,10 @@ describe('diffs/components/app', () => { window.mrTabs = oldMrTabs; // reset component - wrapper.destroy(); + if (wrapper) { + wrapper.destroy(); + wrapper = null; + } mock.restore(); }); @@ -455,76 +465,109 @@ describe('diffs/components/app', () => { }); describe('keyboard shortcut navigation', () => { - const mappings = { - '[': -1, - k: -1, - ']': +1, - j: +1, - }; - let spy; + let spies = []; + let jumpSpy; + let moveSpy; + + function setup(componentProps, featureFlags) { + createComponent( + componentProps, + ({ state }) => { + state.diffs.commit = { id: 'SHA123' }; + }, + { glFeatures: { mrCommitNeighborNav: true, ...featureFlags } }, + ); + + moveSpy = jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {}); + jumpSpy = jest.fn(); + spies = [jumpSpy, moveSpy]; + wrapper.setMethods({ + jumpToFile: jumpSpy, + }); + } describe('visible app', () => { - beforeEach(() => { - spy = jest.fn(); + it.each` + key | name | spy | args | featureFlags + ${'['} | ${'jumpToFile'} | ${0} | ${[-1]} | ${{}} + ${'k'} | ${'jumpToFile'} | ${0} | ${[-1]} | ${{}} + ${']'} | ${'jumpToFile'} | ${0} | ${[+1]} | ${{}} + ${'j'} | ${'jumpToFile'} | ${0} | ${[+1]} | ${{}} + ${'x'} | ${'moveToNeighboringCommit'} | ${1} | ${[{ direction: 'previous' }]} | ${{ mrCommitNeighborNav: true }} + ${'c'} | ${'moveToNeighboringCommit'} | ${1} | ${[{ direction: 'next' }]} | ${{ mrCommitNeighborNav: true }} + `( + 'calls `$name()` with correct parameters whenever the "$key" key is pressed', + ({ key, spy, args, featureFlags }) => { + setup({ shouldShow: true }, featureFlags); - createComponent({ - shouldShow: true, - }); - wrapper.setMethods({ - jumpToFile: spy, - }); - }); + return wrapper.vm.$nextTick().then(() => { + expect(spies[spy]).not.toHaveBeenCalled(); + + Mousetrap.trigger(key); + + expect(spies[spy]).toHaveBeenCalledWith(...args); + }); + }, + ); + + it.each` + key | name | spy | featureFlags + ${'x'} | ${'moveToNeighboringCommit'} | ${1} | ${{ mrCommitNeighborNav: false }} + ${'c'} | ${'moveToNeighboringCommit'} | ${1} | ${{ mrCommitNeighborNav: false }} + `( + 'does not call `$name()` even when the correct key is pressed if the feature flag is disabled', + ({ key, spy, featureFlags }) => { + setup({ shouldShow: true }, featureFlags); - it.each(Object.keys(mappings))( - 'calls `jumpToFile()` with correct parameter whenever pre-defined %s is pressed', - key => { return wrapper.vm.$nextTick().then(() => { - expect(spy).not.toHaveBeenCalled(); + expect(spies[spy]).not.toHaveBeenCalled(); Mousetrap.trigger(key); - expect(spy).toHaveBeenCalledWith(mappings[key]); + expect(spies[spy]).not.toHaveBeenCalled(); }); }, ); - it('does not call `jumpToFile()` when unknown key is pressed', done => { - wrapper.vm - .$nextTick() - .then(() => { - Mousetrap.trigger('d'); + it.each` + key | name | spy | allowed + ${'d'} | ${'jumpToFile'} | ${0} | ${['[', ']', 'j', 'k']} + ${'r'} | ${'moveToNeighboringCommit'} | ${1} | ${['x', 'c']} + `( + `does not call \`$name()\` when a key that is not one of \`$allowed\` is pressed`, + ({ key, spy }) => { + setup({ shouldShow: true }, { mrCommitNeighborNav: true }); - expect(spy).not.toHaveBeenCalled(); - }) - .then(done) - .catch(done.fail); - }); + return wrapper.vm.$nextTick().then(() => { + Mousetrap.trigger(key); + + expect(spies[spy]).not.toHaveBeenCalled(); + }); + }, + ); }); - describe('hideen app', () => { + describe('hidden app', () => { beforeEach(() => { - spy = jest.fn(); + setup({ shouldShow: false }, { mrCommitNeighborNav: true }); - createComponent({ - shouldShow: false, - }); - wrapper.setMethods({ - jumpToFile: spy, + return wrapper.vm.$nextTick().then(() => { + Mousetrap.reset(); }); }); - it('stops calling `jumpToFile()` when application is hidden', done => { - wrapper.vm - .$nextTick() - .then(() => { - Object.keys(mappings).forEach(key => { - Mousetrap.trigger(key); + it.each` + key | name | spy + ${'['} | ${'jumpToFile'} | ${0} + ${'k'} | ${'jumpToFile'} | ${0} + ${']'} | ${'jumpToFile'} | ${0} + ${'j'} | ${'jumpToFile'} | ${0} + ${'x'} | ${'moveToNeighboringCommit'} | ${1} + ${'c'} | ${'moveToNeighboringCommit'} | ${1} + `('stops calling `$name()` when the app is hidden', ({ key, spy }) => { + Mousetrap.trigger(key); - expect(spy).not.toHaveBeenCalled(); - }); - }) - .then(done) - .catch(done.fail); + expect(spies[spy]).not.toHaveBeenCalled(); }); }); }); diff --git a/spec/frontend/diffs/components/commit_item_spec.js b/spec/frontend/diffs/components/commit_item_spec.js index 6bb3a0dcf21..0df951d43a7 100644 --- a/spec/frontend/diffs/components/commit_item_spec.js +++ b/spec/frontend/diffs/components/commit_item_spec.js @@ -13,6 +13,8 @@ const TEST_AUTHOR_EMAIL = 'test+test@gitlab.com'; const TEST_AUTHOR_GRAVATAR = `${TEST_HOST}/avatar/test?s=40`; const TEST_SIGNATURE_HTML = '<a>Legit commit</a>'; const TEST_PIPELINE_STATUS_PATH = `${TEST_HOST}/pipeline/status`; +const NEXT_COMMIT_URL = `${TEST_HOST}/?commit_id=next`; +const PREV_COMMIT_URL = `${TEST_HOST}/?commit_id=prev`; describe('diffs/components/commit_item', () => { let wrapper; @@ -30,12 +32,24 @@ describe('diffs/components/commit_item', () => { const getCommitActionsElement = () => wrapper.find('.commit-actions'); const getCommitPipelineStatus = () => wrapper.find(CommitPipelineStatus); - const mountComponent = propsData => { + const getCommitNavButtonsElement = () => wrapper.find('.commit-nav-buttons'); + const getNextCommitNavElement = () => + getCommitNavButtonsElement().find('.btn-group > *:last-child'); + const getPrevCommitNavElement = () => + getCommitNavButtonsElement().find('.btn-group > *:first-child'); + + const mountComponent = (propsData, featureFlags = {}) => { wrapper = mount(Component, { propsData: { commit, ...propsData, }, + provide: { + glFeatures: { + mrCommitNeighborNav: true, + ...featureFlags, + }, + }, stubs: { CommitPipelineStatus: true, }, @@ -173,4 +187,132 @@ describe('diffs/components/commit_item', () => { expect(getCommitPipelineStatus().exists()).toBe(true); }); }); + + describe('without neighbor commits', () => { + beforeEach(() => { + mountComponent({ commit: { ...commit, prev_commit_id: null, next_commit_id: null } }); + }); + + it('does not render any navigation buttons', () => { + expect(getCommitNavButtonsElement().exists()).toEqual(false); + }); + }); + + describe('with neighbor commits', () => { + let mrCommit; + + beforeEach(() => { + mrCommit = { + ...commit, + next_commit_id: 'next', + prev_commit_id: 'prev', + }; + + mountComponent({ commit: mrCommit }); + }); + + it('renders the commit navigation buttons', () => { + expect(getCommitNavButtonsElement().exists()).toEqual(true); + + mountComponent({ + commit: { ...mrCommit, next_commit_id: null }, + }); + expect(getCommitNavButtonsElement().exists()).toEqual(true); + + mountComponent({ + commit: { ...mrCommit, prev_commit_id: null }, + }); + expect(getCommitNavButtonsElement().exists()).toEqual(true); + }); + + it('does not render the commit navigation buttons if the `mrCommitNeighborNav` feature flag is disabled', () => { + mountComponent({ commit: mrCommit }, { mrCommitNeighborNav: false }); + + expect(getCommitNavButtonsElement().exists()).toEqual(false); + }); + + describe('prev commit', () => { + const { location } = window; + + beforeAll(() => { + delete window.location; + window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` }; + }); + + beforeEach(() => { + jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {}); + }); + + afterAll(() => { + window.location = location; + }); + + it('uses the correct href', () => { + const link = getPrevCommitNavElement(); + + expect(link.element.getAttribute('href')).toEqual(PREV_COMMIT_URL); + }); + + it('triggers the correct Vuex action on click', () => { + const link = getPrevCommitNavElement(); + + link.trigger('click'); + return wrapper.vm.$nextTick().then(() => { + expect(wrapper.vm.moveToNeighboringCommit).toHaveBeenCalledWith({ + direction: 'previous', + }); + }); + }); + + it('renders a disabled button when there is no prev commit', () => { + mountComponent({ commit: { ...mrCommit, prev_commit_id: null } }); + + const button = getPrevCommitNavElement(); + + expect(button.element.tagName).toEqual('BUTTON'); + expect(button.element.hasAttribute('disabled')).toEqual(true); + }); + }); + + describe('next commit', () => { + const { location } = window; + + beforeAll(() => { + delete window.location; + window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` }; + }); + + beforeEach(() => { + jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {}); + }); + + afterAll(() => { + window.location = location; + }); + + it('uses the correct href', () => { + const link = getNextCommitNavElement(); + + expect(link.element.getAttribute('href')).toEqual(NEXT_COMMIT_URL); + }); + + it('triggers the correct Vuex action on click', () => { + const link = getNextCommitNavElement(); + + link.trigger('click'); + return wrapper.vm.$nextTick().then(() => { + expect(wrapper.vm.moveToNeighboringCommit).toHaveBeenCalledWith({ direction: 'next' }); + }); + }); + + it('renders a disabled button when there is no next commit', () => { + mountComponent({ commit: { ...mrCommit, next_commit_id: null } }); + + const button = getNextCommitNavElement(); + + expect(button.element.tagName).toEqual('BUTTON'); + expect(button.element.hasAttribute('disabled')).toEqual(true); + }); + }); + }); }); diff --git a/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb b/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb index 53469159110..8cfd07df777 100644 --- a/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb @@ -124,5 +124,55 @@ describe Gitlab::Ci::Config::Entry::Artifacts do end end end + + describe 'excluded artifacts' do + context 'when configuration is valid and the feature is enabled' do + before do + stub_feature_flags(ci_artifacts_exclude: true) + end + + context 'when configuration is valid' do + let(:config) { { untracked: true, exclude: ['some/directory/'] } } + + it 'correctly parses the configuration' do + expect(entry).to be_valid + expect(entry.value).to eq config + end + end + + context 'when configuration is not valid' do + let(:config) { { untracked: true, exclude: 1234 } } + + it 'returns an error' do + expect(entry).not_to be_valid + expect(entry.errors) + .to include 'artifacts exclude should be an array of strings' + end + end + end + + context 'when artifacts/exclude feature is disabled' do + before do + stub_feature_flags(ci_artifacts_exclude: false) + end + + context 'when configuration has been provided' do + let(:config) { { untracked: true, exclude: ['some/directory/'] } } + + it 'returns an error' do + expect(entry).not_to be_valid + expect(entry.errors).to include 'artifacts exclude feature is disabled' + end + end + + context 'when configuration is not present' do + let(:config) { { untracked: true } } + + it 'is a valid configuration' do + expect(entry).to be_valid + end + end + end + end end end diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb index 969a674a308..c93bb901981 100644 --- a/spec/lib/gitlab/ci/yaml_processor_spec.rb +++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb @@ -1364,6 +1364,24 @@ module Gitlab expect { described_class.new(config) }.to raise_error(described_class::ValidationError) end + + it 'populates a build options with complete artifacts configuration' do + stub_feature_flags(ci_artifacts_exclude: true) + + config = <<~YAML + test: + script: echo "Hello World" + artifacts: + paths: + - my/test + exclude: + - my/test/something + YAML + + attributes = Gitlab::Ci::YamlProcessor.new(config).build_attributes('test') + + expect(attributes.dig(*%i[options artifacts exclude])).to eq(%w[my/test/something]) + end end describe "release" do diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 73b9dcc5bf9..9ed4db673a6 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -647,7 +647,7 @@ describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:update_column_in_batches) .with(:projects, :foo, 10) - expect(model).not_to receive(:add_not_null_constraint) + expect(model).not_to receive(:change_column_null) model.add_column_with_default(:projects, :foo, :integer, default: 10, @@ -658,8 +658,8 @@ describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:update_column_in_batches) .with(:projects, :foo, 10) - expect(model).to receive(:add_not_null_constraint) - .with(:projects, :foo) + expect(model).to receive(:change_column_null) + .with(:projects, :foo, false) model.add_column_with_default(:projects, :foo, :integer, default: 10) end @@ -678,8 +678,8 @@ describe Gitlab::Database::MigrationHelpers do end it 'removes the added column whenever changing a column NULL constraint fails' do - expect(model).to receive(:add_not_null_constraint) - .with(:projects, :foo) + expect(model).to receive(:change_column_null) + .with(:projects, :foo, false) .and_raise(ActiveRecord::ActiveRecordError) expect(model).to receive(:remove_column) @@ -699,7 +699,7 @@ describe Gitlab::Database::MigrationHelpers do allow(model).to receive(:transaction).and_yield allow(model).to receive(:column_for).with(:user_details, :foo).and_return(column) allow(model).to receive(:update_column_in_batches).with(:user_details, :foo, 10, batch_column_name: :user_id) - allow(model).to receive(:add_not_null_constraint).with(:user_details, :foo) + allow(model).to receive(:change_column_null).with(:user_details, :foo, false) allow(model).to receive(:change_column_default).with(:user_details, :foo, 10) expect(model).to receive(:add_column) @@ -721,7 +721,7 @@ describe Gitlab::Database::MigrationHelpers do allow(model).to receive(:transaction).and_yield allow(model).to receive(:column_for).with(:projects, :foo).and_return(column) allow(model).to receive(:update_column_in_batches).with(:projects, :foo, 10) - allow(model).to receive(:add_not_null_constraint).with(:projects, :foo) + allow(model).to receive(:change_column_null).with(:projects, :foo, false) allow(model).to receive(:change_column_default).with(:projects, :foo, 10) expect(model).to receive(:add_column) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 6e1dba6945d..3c66902bb2e 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -70,6 +70,7 @@ describe Notify do it_behaves_like 'an email starting a new thread with reply-by-email enabled' do let(:model) { issue } end + it_behaves_like 'it should show Gmail Actions View Issue link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -116,6 +117,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { issue } end + it_behaves_like 'it should show Gmail Actions View Issue link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -155,6 +157,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { issue } end + it_behaves_like 'it should show Gmail Actions View Issue link' it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'an email with a labels subscriptions link in its footer' @@ -200,6 +203,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { issue } end + it_behaves_like 'it should show Gmail Actions View Issue link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -214,6 +218,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { issue } end + it_behaves_like 'it should show Gmail Actions View Issue link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -248,6 +253,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { issue } end + it_behaves_like 'it should show Gmail Actions View Issue link' it_behaves_like 'an unsubscribeable thread' @@ -300,6 +306,7 @@ describe Notify do it_behaves_like 'an email starting a new thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -344,6 +351,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like "an unsubscribeable thread" it_behaves_like 'appearance header and footer enabled' @@ -409,6 +417,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'an email with a labels subscriptions link in its footer' @@ -436,6 +445,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -466,6 +476,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -502,6 +513,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -533,6 +545,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -694,6 +707,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { project_snippet } end + it_behaves_like 'a user cannot unsubscribe through footer link' it 'has the correct subject' do @@ -936,6 +950,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { commit } end + it_behaves_like 'it should show Gmail Actions View Commit link' it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'appearance header and footer enabled' @@ -962,6 +977,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -988,6 +1004,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { issue } end + it_behaves_like 'it should show Gmail Actions View Issue link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -1060,6 +1077,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { commit } end + it_behaves_like 'it should show Gmail Actions View Commit link' it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'appearance header and footer enabled' @@ -1092,6 +1110,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { merge_request } end + it_behaves_like 'it should show Gmail Actions View Merge request link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' @@ -1124,6 +1143,7 @@ describe Notify do it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do let(:model) { issue } end + it_behaves_like 'it should show Gmail Actions View Issue link' it_behaves_like 'an unsubscribeable thread' it_behaves_like 'appearance header and footer enabled' diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 985bb9d3194..02558ef21ee 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -4088,6 +4088,28 @@ describe Ci::Build do it { is_expected.to include(:upload_multiple_artifacts) } end + + context 'when artifacts exclude is defined and the is feature enabled' do + let(:options) do + { artifacts: { exclude: %w[something] } } + end + + context 'when a feature flag is enabled' do + before do + stub_feature_flags(ci_artifacts_exclude: true) + end + + it { is_expected.to include(:artifacts_exclude) } + end + + context 'when a feature flag is disabled' do + before do + stub_feature_flags(ci_artifacts_exclude: false) + end + + it { is_expected.not_to include(:artifacts_exclude) } + end + end end describe '#supported_runner?' do diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 4900743ccb5..6dd295ca915 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -105,6 +105,38 @@ describe Note do end end + describe 'callbacks' do + describe '#notify_after_create' do + it 'calls #after_note_created on the noteable' do + note = build(:note) + + expect(note).to receive(:notify_after_create).and_call_original + expect(note.noteable).to receive(:after_note_created).with(note) + + note.save! + end + end + + describe '#notify_after_destroy' do + it 'calls #after_note_destroyed on the noteable' do + note = create(:note) + + expect(note).to receive(:notify_after_destroy).and_call_original + expect(note.noteable).to receive(:after_note_destroyed).with(note) + + note.destroy + end + + it 'does not error if noteable is nil' do + note = create(:note) + + expect(note).to receive(:notify_after_destroy).and_call_original + expect(note).to receive(:noteable).at_least(:once).and_return(nil) + expect { note.destroy }.not_to raise_error + end + end + end + describe "Commit notes" do before do allow(Gitlab::Git::KeepAround).to receive(:execute).and_call_original diff --git a/spec/presenters/ci/build_runner_presenter_spec.rb b/spec/presenters/ci/build_runner_presenter_spec.rb index 6b2b031dbd0..cfa2e77e7cf 100644 --- a/spec/presenters/ci/build_runner_presenter_spec.rb +++ b/spec/presenters/ci/build_runner_presenter_spec.rb @@ -38,6 +38,47 @@ describe Ci::BuildRunnerPresenter do expect(presenter.artifacts).to be_empty end end + + context 'when artifacts exclude is defined' do + let(:build) do + create(:ci_build, options: { artifacts: { paths: %w[abc], exclude: %w[cde] } }) + end + + context 'when the feature is enabled' do + before do + stub_feature_flags(ci_artifacts_exclude: true) + end + + it 'includes the list of excluded paths' do + expect(presenter.artifacts.first).to include( + artifact_type: :archive, + artifact_format: :zip, + paths: %w[abc], + exclude: %w[cde] + ) + end + end + + context 'when the feature is disabled' do + before do + stub_feature_flags(ci_artifacts_exclude: false) + end + + it 'does not include the list of excluded paths' do + expect(presenter.artifacts.first).not_to have_key(:exclude) + end + end + end + + context 'when artifacts exclude is not defined' do + let(:build) do + create(:ci_build, options: { artifacts: { paths: %w[abc] } }) + end + + it 'does not include an empty list of excluded paths' do + expect(presenter.artifacts.first).not_to have_key(:exclude) + end + end end context "with reports" do diff --git a/spec/requests/api/metrics/user_starred_dashboards_spec.rb b/spec/requests/api/metrics/user_starred_dashboards_spec.rb new file mode 100644 index 00000000000..4b3c6407ef7 --- /dev/null +++ b/spec/requests/api/metrics/user_starred_dashboards_spec.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Metrics::UserStarredDashboards do + let_it_be(:user) { create(:user) } + let!(:project) { create(:project, :private, :repository, :custom_repo, namespace: user.namespace, files: { dashboard => dashboard_yml }) } + let(:dashboard_yml) { fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml') } + let(:dashboard) { '.gitlab/dashboards/find&seek.yml' } + let(:params) do + { + user: user, + project: project, + dashboard_path: CGI.escape(dashboard) + } + end + + describe 'POST /projects/:id/metrics/user_starred_dashboards' do + let(:url) { "/projects/#{project.id}/metrics/user_starred_dashboards" } + + before do + project.add_reporter(user) + end + + context 'with correct permissions' do + context 'with valid parameters' do + context 'dashboard_path as url param url escaped' do + it 'creates a new annotation', :aggregate_failures do + post api(url, user), params: params + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['project_id']).to eq(project.id) + expect(json_response['user_id']).to eq(user.id) + expect(json_response['dashboard_path']).to eq(dashboard) + end + end + + context 'dashboard_path in request body unescaped' do + let(:params) do + { + user: user, + project: project, + dashboard_path: dashboard + } + end + + it 'creates a new annotation', :aggregate_failures do + post api(url, user), params: params + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['project_id']).to eq(project.id) + expect(json_response['user_id']).to eq(user.id) + expect(json_response['dashboard_path']).to eq(dashboard) + end + end + end + + context 'with invalid parameters' do + it 'returns error message' do + post api(url, user), params: { dashboard_path: '' } + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('dashboard_path is empty') + end + + context 'user is missing' do + it 'returns 404 not found' do + post api(url, nil), params: params + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'project is missing' do + it 'returns 404 not found' do + post api("/projects/#{project.id + 1}/user_starred_dashboards", user), params: params + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end + + context 'without correct permissions' do + it 'returns 404 not found' do + post api(url, create(:user)), params: params + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end +end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index bc2da8a2b9a..5327a3a77c5 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -998,6 +998,53 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end end + describe 'a job with excluded artifacts' do + context 'when excluded paths are defined' do + let(:job) do + create(:ci_build, pipeline: pipeline, token: 'test-job-token', name: 'test', + stage: 'deploy', stage_idx: 1, + options: { artifacts: { paths: ['abc'], exclude: ['cde'] } }) + end + + context 'when a runner supports this feature' do + it 'exposes excluded paths when the feature is enabled' do + stub_feature_flags(ci_artifacts_exclude: true) + + request_job info: { features: { artifacts_exclude: true } } + + expect(response).to have_gitlab_http_status(:created) + expect(json_response.dig('artifacts').first).to include('exclude' => ['cde']) + end + + it 'does not expose excluded paths when the feature is disabled' do + stub_feature_flags(ci_artifacts_exclude: false) + + request_job info: { features: { artifacts_exclude: true } } + + expect(response).to have_gitlab_http_status(:created) + expect(json_response.dig('artifacts').first).not_to have_key('exclude') + end + end + + context 'when a runner does not support this feature' do + it 'does not expose the build at all' do + stub_feature_flags(ci_artifacts_exclude: true) + + request_job + + expect(response).to have_gitlab_http_status(:no_content) + end + end + end + + it 'does not expose excluded paths when these are empty' do + request_job + + expect(response).to have_gitlab_http_status(:created) + expect(json_response.dig('artifacts').first).not_to have_key('exclude') + end + end + def request_job(token = runner.token, **params) new_params = params.merge(token: token, last_update: last_update) post api('/jobs/request'), params: new_params, headers: { 'User-Agent' => user_agent } diff --git a/spec/rubocop/cop/rspec/empty_line_after_shared_example_spec.rb b/spec/rubocop/cop/rspec/empty_line_after_shared_example_spec.rb new file mode 100644 index 00000000000..cee593fe535 --- /dev/null +++ b/spec/rubocop/cop/rspec/empty_line_after_shared_example_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require_relative '../../../../rubocop/cop/rspec/empty_line_after_shared_example' + +describe RuboCop::Cop::RSpec::EmptyLineAfterSharedExample do + subject(:cop) { described_class.new } + + it 'flags a missing empty line after `it_behaves_like` block' do + expect_offense(<<-RUBY) + RSpec.describe Foo do + it_behaves_like 'does this' do + end + ^^^ Add an empty line after `it_behaves_like` block. + it_behaves_like 'does that' do + end + end + RUBY + + expect_correction(<<-RUBY) + RSpec.describe Foo do + it_behaves_like 'does this' do + end + + it_behaves_like 'does that' do + end + end + RUBY + end + + it 'ignores one-line shared examples before shared example blocks' do + expect_no_offenses(<<-RUBY) + RSpec.describe Foo do + it_behaves_like 'does this' + it_behaves_like 'does that' do + end + end + RUBY + end + + it 'flags a missing empty line after `shared_examples`' do + expect_offense(<<-RUBY) + RSpec.context 'foo' do + shared_examples do + end + ^^^ Add an empty line after `shared_examples` block. + shared_examples 'something gets done' do + end + end + RUBY + + expect_correction(<<-RUBY) + RSpec.context 'foo' do + shared_examples do + end + + shared_examples 'something gets done' do + end + end + RUBY + end + + it 'ignores consecutive one-liners' do + expect_no_offenses(<<-RUBY) + RSpec.describe Foo do + it_behaves_like 'do this' + it_behaves_like 'do that' + end + RUBY + end + + it 'flags mixed one-line and multi-line shared examples' do + expect_offense(<<-RUBY) + RSpec.context 'foo' do + it_behaves_like 'do this' + it_behaves_like 'do that' + it_behaves_like 'does this' do + end + ^^^ Add an empty line after `it_behaves_like` block. + it_behaves_like 'do this' + it_behaves_like 'do that' + end + RUBY + end +end diff --git a/spec/services/design_management/delete_designs_service_spec.rb b/spec/services/design_management/delete_designs_service_spec.rb new file mode 100644 index 00000000000..2c0c1570cb4 --- /dev/null +++ b/spec/services/design_management/delete_designs_service_spec.rb @@ -0,0 +1,195 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DesignManagement::DeleteDesignsService do + include DesignManagementTestHelpers + + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:user) { create(:user) } + let(:designs) { create_designs } + + subject(:service) { described_class.new(project, user, issue: issue, designs: designs) } + + # Defined as a method so that the reponse is not cached. We also construct + # a new service executor each time to avoid the intermediate cached values + # it constructs during its execution. + def run_service(delenda = nil) + service = described_class.new(project, user, issue: issue, designs: delenda || designs) + service.execute + end + + let(:response) { run_service } + + shared_examples 'a service error' do + it 'returns an error', :aggregate_failures do + expect(response).to include(status: :error) + end + end + + shared_examples 'a top-level error' do + let(:expected_error) { StandardError } + it 'raises an en expected error', :aggregate_failures do + expect { run_service }.to raise_error(expected_error) + end + end + + shared_examples 'a success' do + it 'returns successfully', :aggregate_failures do + expect(response).to include(status: :success) + end + + it 'saves the user as the author' do + version = response[:version] + + expect(version.author).to eq(user) + end + end + + before do + enable_design_management(enabled) + project.add_developer(user) + end + + describe "#execute" do + context "when the feature is not available" do + let(:enabled) { false } + + it_behaves_like "a service error" + end + + context "when the feature is available" do + let(:enabled) { true } + + it 'is able to delete designs' do + expect(service.send(:can_delete_designs?)).to be true + end + + context 'no designs were passed' do + let(:designs) { [] } + + it_behaves_like "a top-level error" + + it 'does not log any events' do + counter = ::Gitlab::UsageDataCounters::DesignsCounter + expect { run_service rescue nil }.not_to change { counter.totals } + end + end + + context 'one design is passed' do + before do + create_designs(2) + end + + let!(:designs) { create_designs(1) } + + it 'removes that design' do + expect { run_service }.to change { issue.designs.current.count }.from(3).to(2) + end + + it 'logs a deletion event' do + counter = ::Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:delete) }.by(1) + end + + it 'informs the new-version-worker' do + expect(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer) + + run_service + end + + it 'creates a new version' do + expect { run_service }.to change { DesignManagement::Version.where(issue: issue).count }.by(1) + end + + it 'returns the new version' do + version = response[:version] + + expect(version).to eq(DesignManagement::Version.for_issue(issue).ordered.first) + end + + it_behaves_like "a success" + + it 'removes the design from the current design list' do + run_service + + expect(issue.designs.current).not_to include(designs.first) + end + + it 'marks the design as deleted' do + expect { run_service } + .to change { designs.first.deleted? }.from(false).to(true) + end + end + + context 'more than one design is passed' do + before do + create_designs(1) + end + + let!(:designs) { create_designs(2) } + + it 'removes those designs' do + expect { run_service } + .to change { issue.designs.current.count }.from(3).to(1) + end + + it 'logs the correct number of deletion events' do + counter = ::Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:delete) }.by(2) + end + + it_behaves_like "a success" + + context 'after executing the service' do + let(:deleted_designs) { designs.map(&:reset) } + + let!(:version) { run_service[:version] } + + it 'removes the removed designs from the current design list' do + expect(issue.designs.current).not_to include(*deleted_designs) + end + + it 'does not make the designs impossible to find' do + expect(issue.designs).to include(*deleted_designs) + end + + it 'associates the new version with all the designs' do + current_versions = deleted_designs.map { |d| d.most_recent_action.version } + expect(current_versions).to all(eq version) + end + + it 'marks all deleted designs as deleted' do + expect(deleted_designs).to all(be_deleted) + end + + it 'marks all deleted designs with the same deletion version' do + expect(deleted_designs.map { |d| d.most_recent_action.version_id }.uniq) + .to have_attributes(size: 1) + end + end + end + + describe 'scalability' do + before do + run_service(create_designs(1)) # ensure project, issue, etc are created + end + + it 'makes the same number of DB requests for one design as for several' do + one = create_designs(1) + many = create_designs(5) + + baseline = ActiveRecord::QueryRecorder.new { run_service(one) } + + expect { run_service(many) }.not_to exceed_query_limit(baseline) + end + end + end + end + + private + + def create_designs(how_many = 2) + create_list(:design, how_many, :with_lfs_file, issue: issue) + end +end diff --git a/spec/services/design_management/design_user_notes_count_service_spec.rb b/spec/services/design_management/design_user_notes_count_service_spec.rb index f4808542995..62211a4dd0f 100644 --- a/spec/services/design_management/design_user_notes_count_service_spec.rb +++ b/spec/services/design_management/design_user_notes_count_service_spec.rb @@ -23,15 +23,8 @@ describe DesignManagement::DesignUserNotesCountService, :use_clean_rails_memory_ end end - # TODO These tests are being temporarily skipped unless run in EE, - # as we are in the process of moving Design Management to FOSS in 13.0 - # in steps. In the current step the services have not yet been moved. - # - # See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283. describe 'cache invalidation' do it 'changes when a new note is created' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - new_note_attrs = attributes_for(:diff_note_on_design, noteable: design) expect do @@ -40,8 +33,6 @@ describe DesignManagement::DesignUserNotesCountService, :use_clean_rails_memory_ end it 'changes when a note is destroyed' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - note = create(:diff_note_on_design, noteable: design) expect do diff --git a/spec/services/design_management/generate_image_versions_service_spec.rb b/spec/services/design_management/generate_image_versions_service_spec.rb new file mode 100644 index 00000000000..cd021c8d7d3 --- /dev/null +++ b/spec/services/design_management/generate_image_versions_service_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe DesignManagement::GenerateImageVersionsService do + let_it_be(:project) { create(:project) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:version) { create(:design, :with_lfs_file, issue: issue).versions.first } + let_it_be(:action) { version.actions.first } + + describe '#execute' do + it 'generates the image' do + expect { described_class.new(version).execute } + .to change { action.reload.image_v432x230.file } + .from(nil).to(CarrierWave::SanitizedFile) + end + + it 'skips generating image versions if the mime type is not whitelisted' do + stub_const('DesignManagement::DesignV432x230Uploader::MIME_TYPE_WHITELIST', []) + + described_class.new(version).execute + + expect(action.reload.image_v432x230.file).to eq(nil) + end + + it 'skips generating image versions if the design file size is too large' do + stub_const("#{described_class.name}::MAX_DESIGN_SIZE", 1.byte) + + described_class.new(version).execute + + expect(action.reload.image_v432x230.file).to eq(nil) + end + + it 'returns the status' do + result = described_class.new(version).execute + + expect(result[:status]).to eq(:success) + end + + it 'returns the version' do + result = described_class.new(version).execute + + expect(result[:version]).to eq(version) + end + + it 'logs if the raw image cannot be found' do + version.designs.first.update(filename: 'foo.png') + + expect(Gitlab::AppLogger).to receive(:error).with("No design file found for Action: #{action.id}") + + described_class.new(version).execute + end + + context 'when an error is encountered when generating the image versions' do + before do + expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader| + expect(uploader).to receive(:cache!).and_raise(CarrierWave::DownloadError, 'foo') + end + end + + it 'logs the error' do + expect(Gitlab::AppLogger).to receive(:error).with('foo') + + described_class.new(version).execute + end + + it 'tracks the error' do + expect(Gitlab::ErrorTracking).to receive(:track_exception).with( + instance_of(CarrierWave::DownloadError), + project_id: project.id, version_id: version.id, design_id: version.designs.first.id + ) + + described_class.new(version).execute + end + end + end +end diff --git a/spec/services/design_management/save_designs_service_spec.rb b/spec/services/design_management/save_designs_service_spec.rb new file mode 100644 index 00000000000..013d5473860 --- /dev/null +++ b/spec/services/design_management/save_designs_service_spec.rb @@ -0,0 +1,356 @@ +# frozen_string_literal: true +require 'spec_helper' + +describe DesignManagement::SaveDesignsService do + include DesignManagementTestHelpers + include ConcurrentHelpers + + let_it_be(:developer) { create(:user) } + let(:project) { issue.project } + let(:issue) { create(:issue) } + let(:user) { developer } + let(:files) { [rails_sample] } + let(:design_repository) { ::Gitlab::GlRepository::DESIGN.repository_resolver.call(project) } + let(:rails_sample_name) { 'rails_sample.jpg' } + let(:rails_sample) { sample_image(rails_sample_name) } + let(:dk_png) { sample_image('dk.png') } + + def sample_image(filename) + fixture_file_upload("spec/fixtures/#{filename}") + end + + before do + project.add_developer(developer) + end + + def run_service(files_to_upload = nil) + design_files = files_to_upload || files + design_files.each(&:rewind) + + service = described_class.new(project, user, + issue: issue, + files: design_files) + service.execute + end + + # Randomly alter the content of files. + # This allows the files to be updated by the service, as unmodified + # files are rejected. + def touch_files(files_to_touch = nil) + design_files = files_to_touch || files + + design_files.each do |f| + f.tempfile.write(SecureRandom.random_bytes) + end + end + + let(:response) { run_service } + + shared_examples 'a service error' do + it 'returns an error', :aggregate_failures do + expect(response).to match(a_hash_including(status: :error)) + end + end + + shared_examples 'an execution error' do + it 'returns an error', :aggregate_failures do + expect { service.execute }.to raise_error(some_error) + end + end + + describe '#execute' do + context 'when the feature is not available' do + before do + enable_design_management(false) + end + + it_behaves_like 'a service error' + end + + context 'when the feature is available' do + before do + enable_design_management(true) + end + + describe 'repository existence' do + def repository_exists + # Expire the memoized value as the service creates it's own instance + design_repository.expire_exists_cache + design_repository.exists? + end + + it 'creates a design repository when it did not exist' do + expect { run_service }.to change { repository_exists }.from(false).to(true) + end + end + + it 'updates the creation count' do + counter = Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:create) }.by(1) + end + + it 'creates a commit in the repository' do + run_service + + expect(design_repository.commit).to have_attributes( + author: user, + message: include(rails_sample_name) + ) + end + + it 'can run the same command in parallel' do + blocks = Array.new(10).map do + unique_files = %w(rails_sample.jpg dk.png) + .map { |name| RenameableUpload.unique_file(name) } + + -> { run_service(unique_files) } + end + + expect { run_parallel(blocks) }.to change(DesignManagement::Version, :count).by(10) + end + + it 'causes diff_refs not to be nil' do + expect(response).to include( + designs: all(have_attributes(diff_refs: be_present)) + ) + end + + it 'creates a design & a version for the filename if it did not exist' do + expect(issue.designs.size).to eq(0) + + updated_designs = response[:designs] + + expect(updated_designs.size).to eq(1) + expect(updated_designs.first.versions.size).to eq(1) + end + + it 'saves the user as the author' do + updated_designs = response[:designs] + + expect(updated_designs.first.versions.first.author).to eq(user) + end + + describe 'saving the file to LFS' do + before do + expect_next_instance_of(Lfs::FileTransformer) do |transformer| + expect(transformer).to receive(:lfs_file?).and_return(true) + end + end + + it 'saves the design to LFS' do + expect { run_service }.to change { LfsObject.count }.by(1) + end + + it 'saves the repository_type of the LfsObjectsProject as design' do + expect do + run_service + end.to change { project.lfs_objects_projects.count }.from(0).to(1) + + expect(project.lfs_objects_projects.first.repository_type).to eq('design') + end + end + + context 'when a design is being updated' do + before do + run_service + touch_files + end + + it 'creates a new version for the existing design and updates the file' do + expect(issue.designs.size).to eq(1) + expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1) + + updated_designs = response[:designs] + + expect(updated_designs.size).to eq(1) + expect(updated_designs.first.versions.size).to eq(2) + end + + it 'increments the update counter' do + counter = Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:update) }.by 1 + end + + context 'when uploading a new design' do + it 'does not link the new version to the existing design' do + existing_design = issue.designs.first + + updated_designs = run_service([dk_png])[:designs] + + expect(existing_design.versions.reload.size).to eq(1) + expect(updated_designs.size).to eq(1) + expect(updated_designs.first.versions.size).to eq(1) + end + end + end + + context 'when a design has not changed since its previous version' do + before do + run_service + end + + it 'does not create a new version' do + expect { run_service }.not_to change { issue.design_versions.count } + end + + it 'returns the design in `skipped_designs` instead of `designs`' do + response = run_service + + expect(response[:designs]).to be_empty + expect(response[:skipped_designs].size).to eq(1) + end + end + + context 'when doing a mixture of updates and creations' do + let(:files) { [rails_sample, dk_png] } + + before do + # Create just the first one, which we will later update. + run_service([files.first]) + touch_files([files.first]) + end + + it 'counts one creation and one update' do + counter = Gitlab::UsageDataCounters::DesignsCounter + expect { run_service } + .to change { counter.read(:create) }.by(1) + .and change { counter.read(:update) }.by(1) + end + + it 'creates a single commit' do + commit_count = -> do + design_repository.expire_all_method_caches + design_repository.commit_count + end + + expect { run_service }.to change { commit_count.call }.by(1) + end + + it 'enqueues just one new version worker' do + expect(::DesignManagement::NewVersionWorker) + .to receive(:perform_async).once.with(Integer) + + run_service + end + end + + context 'when uploading multiple files' do + let(:files) { [rails_sample, dk_png] } + + it 'returns information about both designs in the response' do + expect(response).to include(designs: have_attributes(size: 2), status: :success) + end + + it 'creates 2 designs with a single version' do + expect { run_service }.to change { issue.designs.count }.from(0).to(2) + + expect(DesignManagement::Version.for_designs(issue.designs).size).to eq(1) + end + + it 'increments the creation count by 2' do + counter = Gitlab::UsageDataCounters::DesignsCounter + expect { run_service }.to change { counter.read(:create) }.by 2 + end + + it 'enqueues a new version worker' do + expect(::DesignManagement::NewVersionWorker) + .to receive(:perform_async).once.with(Integer) + + run_service + end + + it 'creates a single commit' do + commit_count = -> do + design_repository.expire_all_method_caches + design_repository.commit_count + end + + expect { run_service }.to change { commit_count.call }.by(1) + end + + it 'only does 5 gitaly calls', :request_store, :sidekiq_might_not_need_inline do + allow(::DesignManagement::NewVersionWorker).to receive(:perform_async).with(Integer) + service = described_class.new(project, user, issue: issue, files: files) + # Some unrelated calls that are usually cached or happen only once + service.__send__(:repository).create_if_not_exists + service.__send__(:repository).has_visible_content? + + request_count = -> { Gitlab::GitalyClient.get_request_count } + + # An exists?, a check for existing blobs, default branch, an after_commit + # callback on LfsObjectsProject + expect { service.execute }.to change(&request_count).by(4) + end + + context 'when uploading too many files' do + let(:files) { Array.new(DesignManagement::SaveDesignsService::MAX_FILES + 1) { dk_png } } + + it 'returns the correct error' do + expect(response[:message]).to match(/only \d+ files are allowed simultaneously/i) + end + end + end + + context 'when the user is not allowed to upload designs' do + let(:user) { create(:user) } + + it_behaves_like 'a service error' + end + + describe 'failure modes' do + let(:service) { described_class.new(project, user, issue: issue, files: files) } + let(:response) { service.execute } + + before do + expect(service).to receive(:run_actions).and_raise(some_error) + end + + context 'when creating the commit fails' do + let(:some_error) { Gitlab::Git::BaseError } + + it_behaves_like 'an execution error' + end + + context 'when creating the versions fails' do + let(:some_error) { ActiveRecord::RecordInvalid } + + it_behaves_like 'a service error' + end + end + + context "when a design already existed in the repo but we didn't know about it in the database" do + let(:filename) { rails_sample_name } + + before do + path = File.join(build(:design, issue: issue, filename: filename).full_path) + design_repository.create_if_not_exists + design_repository.create_file(user, path, 'something fake', + branch_name: 'master', + message: 'Somehow created without being tracked in db') + end + + it 'creates the design and a new version for it' do + first_updated_design = response[:designs].first + + expect(first_updated_design.filename).to eq(filename) + expect(first_updated_design.versions.size).to eq(1) + end + end + + describe 'scalability', skip: 'See: https://gitlab.com/gitlab-org/gitlab/-/issues/213169' do + before do + run_service([sample_image('banana_sample.gif')]) # ensure project, issue, etc are created + end + + it 'runs the same queries for all requests, regardless of number of files' do + one = [dk_png] + two = [rails_sample, dk_png] + + baseline = ActiveRecord::QueryRecorder.new { run_service(one) } + + expect { run_service(two) }.not_to exceed_query_limit(baseline) + end + end + end + end +end diff --git a/spec/services/git/wiki_push_service_spec.rb b/spec/services/git/wiki_push_service_spec.rb index 2b2f8835d85..cdb1dc5a435 100644 --- a/spec/services/git/wiki_push_service_spec.rb +++ b/spec/services/git/wiki_push_service_spec.rb @@ -320,7 +320,7 @@ describe Git::WikiPushService, services: true do file_content: 'some stuff', branch_name: 'master' } - ::Wikis::CreateAttachmentService.new(project, project.owner, params).execute + ::Wikis::CreateAttachmentService.new(container: project, current_user: project.owner, params: params).execute end def update_page(title) diff --git a/spec/services/lfs/file_transformer_spec.rb b/spec/services/lfs/file_transformer_spec.rb index 9973d64930b..13d9c369c42 100644 --- a/spec/services/lfs/file_transformer_spec.rb +++ b/spec/services/lfs/file_transformer_spec.rb @@ -81,6 +81,23 @@ describe Lfs::FileTransformer do expect(LfsObject.last.file.read).to eq file_content end + + context 'when repository is a design repository' do + let(:file_path) { "/#{DesignManagement.designs_directory}/test_file.lfs" } + let(:repository) { project.design_repository } + + it "creates an LfsObject with the file's content" do + subject.new_file(file_path, file) + + expect(LfsObject.last.file.read).to eq(file_content) + end + + it 'saves the correct repository_type to LfsObjectsProject' do + subject.new_file(file_path, file) + + expect(project.lfs_objects_projects.first.repository_type).to eq('design') + end + end end context "when doesn't use LFS" do diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index c461dd700ec..2ae4fbeb900 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -342,6 +342,60 @@ describe Notes::CreateService do end end + context 'design note' do + subject(:service) { described_class.new(project, user, params) } + + let_it_be(:design) { create(:design, :with_file) } + let_it_be(:project) { design.project } + let_it_be(:user) { project.owner } + let_it_be(:params) do + { + type: 'DiffNote', + noteable: design, + note: "A message", + position: { + old_path: design.full_path, + new_path: design.full_path, + position_type: 'image', + width: '100', + height: '100', + x: '50', + y: '50', + base_sha: design.diff_refs.base_sha, + start_sha: design.diff_refs.base_sha, + head_sha: design.diff_refs.head_sha + } + } + end + + it 'can create diff notes for designs' do + note = service.execute + + expect(note).to be_a(DiffNote) + expect(note).to be_persisted + expect(note.noteable).to eq(design) + end + + it 'sends a notification about this note', :sidekiq_might_not_need_inline do + notifier = double + allow(::NotificationService).to receive(:new).and_return(notifier) + + expect(notifier) + .to receive(:new_note) + .with have_attributes(noteable: design) + + service.execute + end + + it 'correctly builds the position of the note' do + note = service.execute + + expect(note.position.new_path).to eq(design.full_path) + expect(note.position.old_path).to eq(design.full_path) + expect(note.position.diff_refs).to eq(design.diff_refs) + end + end + context 'note with emoji only' do it 'creates regular note' do opts = { diff --git a/spec/services/notes/post_process_service_spec.rb b/spec/services/notes/post_process_service_spec.rb index 99db7897664..d564cacd2d8 100644 --- a/spec/services/notes/post_process_service_spec.rb +++ b/spec/services/notes/post_process_service_spec.rb @@ -43,5 +43,32 @@ describe Notes::PostProcessService do described_class.new(@note).execute end end + + context 'when the noteable is a design' do + let_it_be(:noteable) { create(:design, :with_file) } + let_it_be(:discussion_note) { create_note } + + subject { described_class.new(note).execute } + + def create_note(in_reply_to: nil) + create(:diff_note_on_design, noteable: noteable, in_reply_to: in_reply_to) + end + + context 'when the note is the start of a new discussion' do + let(:note) { discussion_note } + + it 'creates a new system note' do + expect { subject }.to change { Note.system.count }.by(1) + end + end + + context 'when the note is a reply within a discussion' do + let_it_be(:note) { create_note(in_reply_to: discussion_note) } + + it 'does not create a new system note' do + expect { subject }.not_to change { Note.system.count } + end + end + end end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 2134166cd6d..7d6fd9430eb 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -709,6 +709,57 @@ describe NotificationService, :mailer do end end end + + context 'when notified of a new design diff note' do + include DesignManagementTestHelpers + + let_it_be(:design) { create(:design, :with_file) } + let_it_be(:project) { design.project } + let_it_be(:dev) { create(:user) } + let_it_be(:stranger) { create(:user) } + let_it_be(:note) do + create(:diff_note_on_design, + noteable: design, + note: "Hello #{dev.to_reference}, G'day #{stranger.to_reference}") + end + let(:mailer) { double(deliver_later: true) } + + context 'design management is enabled' do + before do + enable_design_management + project.add_developer(dev) + allow(Notify).to receive(:note_design_email) { mailer } + end + + it 'sends new note notifications' do + expect(subject).to receive(:send_new_note_notifications).with(note) + + subject.new_note(note) + end + + it 'sends a mail to the developer' do + expect(Notify) + .to receive(:note_design_email).with(dev.id, note.id, 'mentioned') + + subject.new_note(note) + end + + it 'does not notify non-developers' do + expect(Notify) + .not_to receive(:note_design_email).with(stranger.id, note.id) + + subject.new_note(note) + end + end + + context 'design management is disabled' do + it 'does not notify the user' do + expect(Notify).not_to receive(:note_design_email) + + subject.new_note(note) + end + end + end end describe '#send_new_release_notifications', :deliver_mails_inline, :sidekiq_inline do diff --git a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb index 71be335c11d..f1eaf8324e0 100644 --- a/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/migrate_repository_service_spec.rb @@ -6,7 +6,7 @@ describe Projects::HashedStorage::MigrateRepositoryService do include GitHelpers let(:gitlab_shell) { Gitlab::Shell.new } - let(:project) { create(:project, :legacy_storage, :repository, :wiki_repo) } + let(:project) { create(:project, :legacy_storage, :repository, :wiki_repo, :design_repo) } let(:legacy_storage) { Storage::LegacyProject.new(project) } let(:hashed_storage) { Storage::Hashed.new(project) } @@ -45,11 +45,12 @@ describe Projects::HashedStorage::MigrateRepositoryService do end context 'when succeeds' do - it 'renames project and wiki repositories' do + it 'renames project, wiki and design repositories' do service.execute expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.design.git")).to be_truthy end it 'updates project to be hashed and not read-only' do @@ -59,9 +60,10 @@ describe Projects::HashedStorage::MigrateRepositoryService do expect(project.repository_read_only).to be_falsey end - it 'move operation is called for both repositories' do + it 'move operation is called for all repositories' do expect_move_repository(old_disk_path, new_disk_path) expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki") + expect_move_repository("#{old_disk_path}.design", "#{new_disk_path}.design") service.execute end @@ -86,6 +88,7 @@ describe Projects::HashedStorage::MigrateRepositoryService do expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.design.git")).to be_falsey expect(project.repository_read_only?).to be_falsey end @@ -97,6 +100,7 @@ describe Projects::HashedStorage::MigrateRepositoryService do it 'does not try to move nil repository over existing' do expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, old_disk_path, new_disk_path) expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki") + expect_move_repository("#{old_disk_path}.design", "#{new_disk_path}.design") service.execute end diff --git a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb index 6dcd2ff4555..1c0f446d9cf 100644 --- a/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb +++ b/spec/services/projects/hashed_storage/rollback_repository_service_spec.rb @@ -6,7 +6,7 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis include GitHelpers let(:gitlab_shell) { Gitlab::Shell.new } - let(:project) { create(:project, :repository, :wiki_repo, storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository]) } + let(:project) { create(:project, :repository, :wiki_repo, :design_repo, storage_version: ::Project::HASHED_STORAGE_FEATURES[:repository]) } let(:legacy_storage) { Storage::LegacyProject.new(project) } let(:hashed_storage) { Storage::Hashed.new(project) } @@ -45,11 +45,12 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis end context 'when succeeds' do - it 'renames project and wiki repositories' do + it 'renames project, wiki and design repositories' do service.execute expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_truthy expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_truthy + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.design.git")).to be_truthy end it 'updates project to be legacy and not read-only' do @@ -62,6 +63,7 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis it 'move operation is called for both repositories' do expect_move_repository(old_disk_path, new_disk_path) expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki") + expect_move_repository("#{old_disk_path}.design", "#{new_disk_path}.design") service.execute end @@ -86,6 +88,7 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.git")).to be_falsey expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.wiki.git")).to be_falsey + expect(gitlab_shell.repository_exists?(project.repository_storage, "#{new_disk_path}.design.git")).to be_falsey expect(project.repository_read_only?).to be_falsey end @@ -97,6 +100,7 @@ describe Projects::HashedStorage::RollbackRepositoryService, :clean_gitlab_redis it 'does not try to move nil repository over existing' do expect(gitlab_shell).not_to receive(:mv_repository).with(project.repository_storage, old_disk_path, new_disk_path) expect_move_repository("#{old_disk_path}.wiki", "#{new_disk_path}.wiki") + expect_move_repository("#{old_disk_path}.design", "#{new_disk_path}.design") service.execute end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index f17ddb22d22..274af5f03bf 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -359,6 +359,90 @@ describe Projects::TransferService do end end + describe 'transferring a design repository' do + subject { described_class.new(project, user) } + + before do + group.add_owner(user) + end + + def design_repository + project.design_repository + end + + it 'does not create a design repository' do + expect(subject.execute(group)).to be true + + project.clear_memoization(:design_repository) + + expect(design_repository.exists?).to be false + end + + describe 'when the project has a design repository' do + let(:project_repo_path) { "#{project.path}#{::Gitlab::GlRepository::DESIGN.path_suffix}" } + let(:old_full_path) { "#{user.namespace.full_path}/#{project_repo_path}" } + let(:new_full_path) { "#{group.full_path}/#{project_repo_path}" } + + context 'with legacy storage' do + let(:project) { create(:project, :repository, :legacy_storage, :design_repo, namespace: user.namespace) } + + it 'moves the repository' do + expect(subject.execute(group)).to be true + + project.clear_memoization(:design_repository) + + expect(design_repository).to have_attributes( + disk_path: new_full_path, + full_path: new_full_path + ) + end + + it 'does not move the repository when an error occurs', :aggregate_failures do + allow(subject).to receive(:execute_system_hooks).and_raise('foo') + expect { subject.execute(group) }.to raise_error('foo') + + project.clear_memoization(:design_repository) + + expect(design_repository).to have_attributes( + disk_path: old_full_path, + full_path: old_full_path + ) + end + end + + context 'with hashed storage' do + let(:project) { create(:project, :repository, namespace: user.namespace) } + + it 'does not move the repository' do + old_disk_path = design_repository.disk_path + + expect(subject.execute(group)).to be true + + project.clear_memoization(:design_repository) + + expect(design_repository).to have_attributes( + disk_path: old_disk_path, + full_path: new_full_path + ) + end + + it 'does not move the repository when an error occurs' do + old_disk_path = design_repository.disk_path + + allow(subject).to receive(:execute_system_hooks).and_raise('foo') + expect { subject.execute(group) }.to raise_error('foo') + + project.clear_memoization(:design_repository) + + expect(design_repository).to have_attributes( + disk_path: old_disk_path, + full_path: old_full_path + ) + end + end + end + end + def rugged_config rugged_repo(project.repository).config end diff --git a/spec/services/projects/update_repository_storage_service_spec.rb b/spec/services/projects/update_repository_storage_service_spec.rb index b58dac9a6ce..4d63216d1c8 100644 --- a/spec/services/projects/update_repository_storage_service_spec.rb +++ b/spec/services/projects/update_repository_storage_service_spec.rb @@ -141,5 +141,18 @@ describe Projects::UpdateRepositoryStorageService do end end end + + context 'with design repository' do + include_examples 'moves repository to another storage', 'design' do + let(:project) { create(:project, :repository, repository_read_only: true) } + let(:repository) { project.design_repository } + let(:destination) { 'test_second_storage' } + let(:repository_storage_move) { create(:project_repository_storage_move, :scheduled, project: project, destination_storage_name: destination) } + + before do + project.design_repository.create_if_not_exists + end + end + end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index f3e6cb3cc52..66f9b5d092f 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -6,6 +6,7 @@ describe SystemNoteService do include Gitlab::Routing include RepoHelpers include AssetsHelpers + include DesignManagementTestHelpers let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, :repository, group: group) } @@ -636,4 +637,28 @@ describe SystemNoteService do described_class.auto_resolve_prometheus_alert(noteable, project, author) end end + + describe '.design_version_added' do + let(:version) { create(:design_version) } + + it 'calls DesignManagementService' do + expect_next_instance_of(SystemNotes::DesignManagementService) do |service| + expect(service).to receive(:design_version_added).with(version) + end + + described_class.design_version_added(version) + end + end + + describe '.design_discussion_added' do + let(:discussion_note) { create(:diff_note_on_design) } + + it 'calls DesignManagementService' do + expect_next_instance_of(SystemNotes::DesignManagementService) do |service| + expect(service).to receive(:design_discussion_added).with(discussion_note) + end + + described_class.design_discussion_added(discussion_note) + end + end end diff --git a/spec/services/system_notes/design_management_service_spec.rb b/spec/services/system_notes/design_management_service_spec.rb new file mode 100644 index 00000000000..08511e62341 --- /dev/null +++ b/spec/services/system_notes/design_management_service_spec.rb @@ -0,0 +1,155 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SystemNotes::DesignManagementService do + let(:project) { create(:project) } + let(:issue) { create(:issue, project: project) } + + let(:instance) { described_class.new(noteable: instance_noteable, project: instance_project, author: instance_author) } + + describe '#design_version_added' do + let(:instance_noteable) { version.issue } + let(:instance_project) { version.issue.project } + let(:instance_author) { version.author } + + subject { instance.design_version_added(version) } + + # default (valid) parameters: + let(:n_designs) { 3 } + let(:designs) { create_list(:design, n_designs, issue: issue) } + let(:user) { build(:user) } + let(:version) do + create(:design_version, issue: issue, designs: designs) + end + + before do + # Avoid needing to call into gitaly + allow(version).to receive(:author).and_return(user) + end + + context 'with one kind of event' do + before do + DesignManagement::Action + .where(design: designs).update_all(event: :modification) + end + + it 'makes just one note' do + expect(subject).to contain_exactly(Note) + end + + it 'adds a new system note' do + expect { subject }.to change { Note.system.count }.by(1) + end + end + + context 'with a mixture of events' do + let(:n_designs) { DesignManagement::Action.events.size } + + before do + designs.each_with_index do |design, i| + design.actions.update_all(event: i) + end + end + + it 'makes one note for each kind of event' do + expect(subject).to have_attributes(size: n_designs) + end + + it 'adds a system note for each kind of event' do + expect { subject }.to change { Note.system.count }.by(n_designs) + end + end + + describe 'icons' do + where(:action) do + [ + [:creation], + [:modification], + [:deletion] + ] + end + + with_them do + before do + version.actions.update_all(event: action) + end + + subject(:metadata) do + instance.design_version_added(version) + .first.system_note_metadata + end + + it 'has a valid action' do + expect(::SystemNoteHelper::ICON_NAMES_BY_ACTION) + .to include(metadata.action) + end + end + end + + context 'it succeeds' do + where(:action, :icon, :human_description) do + [ + [:creation, 'designs_added', 'added'], + [:modification, 'designs_modified', 'updated'], + [:deletion, 'designs_removed', 'removed'] + ] + end + + with_them do + before do + version.actions.update_all(event: action) + end + + let(:anchor_tag) { %r{ <a[^>]*>#{link}</a>} } + let(:href) { instance.send(:designs_path, { version: version.id }) } + let(:link) { "#{n_designs} designs" } + + subject(:note) { instance.design_version_added(version).first } + + it 'has the correct data' do + expect(note) + .to be_system + .and have_attributes( + system_note_metadata: have_attributes(action: icon), + note: include(human_description) + .and(include link) + .and(include href), + note_html: a_string_matching(anchor_tag) + ) + end + end + end + end + + describe '#design_discussion_added' do + let(:instance_noteable) { design.issue } + let(:instance_project) { design.issue.project } + let(:instance_author) { discussion_note.author } + + subject { instance.design_discussion_added(discussion_note) } + + let(:design) { create(:design, :with_file, issue: issue) } + let(:author) { create(:user) } + let(:discussion_note) do + create(:diff_note_on_design, noteable: design, author: author) + end + let(:action) { 'designs_discussion_added' } + + it_behaves_like 'a system note' do + let(:noteable) { discussion_note.noteable.issue } + end + + it 'adds a new system note' do + expect { subject }.to change { Note.system.count }.by(1) + end + + it 'has the correct note text' do + href = instance.send(:designs_path, + { vueroute: design.filename, anchor: ActionView::RecordIdentifier.dom_id(discussion_note) } + ) + + expect(subject.note).to eq("started a discussion on [#{design.filename}](#{href})") + end + end +end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 9b92590cb63..4894cf12372 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -895,6 +895,36 @@ describe TodoService do end end + describe 'Designs' do + include DesignManagementTestHelpers + + let(:issue) { create(:issue, project: project) } + let(:design) { create(:design, issue: issue) } + + before do + enable_design_management + + project.add_guest(author) + project.add_developer(john_doe) + end + + let(:note) do + build(:diff_note_on_design, + noteable: design, + author: author, + note: "Hey #{john_doe.to_reference}") + end + + it 'creates a todo for mentioned user on new diff note' do + service.new_note(note, author) + + should_create_todo(user: john_doe, + target: design, + action: Todo::MENTIONED, + note: note) + end + end + describe '#update_note' do let(:noteable) { create(:issue, project: project) } let(:note) { create(:note, project: project, note: mentions, noteable: noteable) } diff --git a/spec/services/wikis/create_attachment_service_spec.rb b/spec/services/wikis/create_attachment_service_spec.rb index 7a73a0a555f..4adfaa24874 100644 --- a/spec/services/wikis/create_attachment_service_spec.rb +++ b/spec/services/wikis/create_attachment_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Wikis::CreateAttachmentService do - let(:project) { create(:project, :wiki_repo) } + let(:container) { create(:project, :wiki_repo) } let(:user) { create(:user) } let(:file_name) { 'filename.txt' } let(:file_path_regex) { %r{#{described_class::ATTACHMENT_PATH}/\h{32}/#{file_name}} } @@ -15,25 +15,21 @@ describe Wikis::CreateAttachmentService do end let(:opts) { file_opts } - subject(:service) { described_class.new(project, user, opts) } + subject(:service) { described_class.new(container: container, current_user: user, params: opts) } before do - project.add_developer(user) + container.add_developer(user) end describe 'initialization' do context 'author commit info' do it 'does not raise error if user is nil' do - service = described_class.new(project, nil, opts) + service = described_class.new(container: container, current_user: nil, params: opts) expect(service.instance_variable_get(:@author_email)).to be_nil expect(service.instance_variable_get(:@author_name)).to be_nil end - it 'fills file_path from the repository uploads folder' do - expect(service.instance_variable_get(:@file_path)).to match(file_path_regex) - end - context 'when no author info provided' do it 'fills author_email and author_name from current_user info' do expect(service.instance_variable_get(:@author_email)).to eq user.email @@ -73,7 +69,7 @@ describe Wikis::CreateAttachmentService do context 'branch name' do context 'when no branch provided' do it 'sets the branch from the wiki default_branch' do - expect(service.instance_variable_get(:@branch_name)).to eq project.wiki.default_branch + expect(service.instance_variable_get(:@branch_name)).to eq container.wiki.default_branch end end @@ -151,7 +147,7 @@ describe Wikis::CreateAttachmentService do context 'when user' do shared_examples 'wiki attachment user validations' do it 'returns error' do - result = described_class.new(project, user2, opts).execute + result = described_class.new(container: container, current_user: user2, params: opts).execute expect(result[:status]).to eq :error expect(result[:message]).to eq 'You are not allowed to push to the wiki' @@ -172,54 +168,5 @@ describe Wikis::CreateAttachmentService do end end - describe '#execute' do - let(:wiki) { project.wiki } - - subject(:service_execute) { service.execute[:result] } - - context 'creates branch if it does not exists' do - let(:branch_name) { 'new_branch' } - let(:opts) { file_opts.merge(branch_name: branch_name) } - - it do - expect(wiki.repository.branches).to be_empty - expect { service.execute }.to change { wiki.repository.branches.count }.by(1) - expect(wiki.repository.branches.first.name).to eq branch_name - end - end - - it 'adds file to the repository' do - expect(wiki.repository.ls_files('HEAD')).to be_empty - - service.execute - - files = wiki.repository.ls_files('HEAD') - expect(files.count).to eq 1 - expect(files.first).to match(file_path_regex) - end - - context 'returns' do - before do - allow(SecureRandom).to receive(:hex).and_return('fixed_hex') - - service_execute - end - - it 'returns the file name' do - expect(service_execute[:file_name]).to eq file_name - end - - it 'returns the path where file was stored' do - expect(service_execute[:file_path]).to eq 'uploads/fixed_hex/filename.txt' - end - - it 'returns the branch where the file was pushed' do - expect(service_execute[:branch]).to eq wiki.default_branch - end - - it 'returns the commit id' do - expect(service_execute[:commit]).not_to be_empty - end - end - end + it_behaves_like 'Wikis::CreateAttachmentService#execute', :project end diff --git a/spec/support/helpers/wiki_helpers.rb b/spec/support/helpers/wiki_helpers.rb index 86eb1793707..e6818ff8f0c 100644 --- a/spec/support/helpers/wiki_helpers.rb +++ b/spec/support/helpers/wiki_helpers.rb @@ -14,7 +14,10 @@ module WikiHelpers file_content: File.read(expand_fixture_path(file_name)) } - ::Wikis::CreateAttachmentService.new(project, user, opts) - .execute[:result][:file_path] + ::Wikis::CreateAttachmentService.new( + container: project, + current_user: user, + params: opts + ).execute[:result][:file_path] end end diff --git a/spec/support/shared_examples/services/wikis/create_attachment_service_shared_examples.rb b/spec/support/shared_examples/services/wikis/create_attachment_service_shared_examples.rb new file mode 100644 index 00000000000..541e332e3a1 --- /dev/null +++ b/spec/support/shared_examples/services/wikis/create_attachment_service_shared_examples.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'Wikis::CreateAttachmentService#execute' do |container_type| + let(:container) { create(container_type, :wiki_repo) } + let(:wiki) { container.wiki } + + let(:user) { create(:user) } + let(:file_name) { 'filename.txt' } + let(:file_path_regex) { %r{#{described_class::ATTACHMENT_PATH}/\h{32}/#{file_name}} } + + let(:file_opts) do + { + file_name: file_name, + file_content: 'Content of attachment' + } + end + let(:opts) { file_opts } + + let(:service) { Wikis::CreateAttachmentService.new(container: container, current_user: user, params: opts) } + + subject(:service_execute) { service.execute[:result] } + + before do + container.add_developer(user) + end + + context 'creates branch if it does not exists' do + let(:branch_name) { 'new_branch' } + let(:opts) { file_opts.merge(branch_name: branch_name) } + + it do + expect(wiki.repository.branches).to be_empty + expect { service.execute }.to change { wiki.repository.branches.count }.by(1) + expect(wiki.repository.branches.first.name).to eq branch_name + end + end + + it 'adds file to the repository' do + expect(wiki.repository.ls_files('HEAD')).to be_empty + + service.execute + + files = wiki.repository.ls_files('HEAD') + expect(files.count).to eq 1 + expect(files.first).to match(file_path_regex) + end + + context 'returns' do + before do + allow(SecureRandom).to receive(:hex).and_return('fixed_hex') + + service_execute + end + + it 'returns related information', :aggregate_failures do + expect(service_execute[:file_name]).to eq file_name + expect(service_execute[:file_path]).to eq 'uploads/fixed_hex/filename.txt' + expect(service_execute[:branch]).to eq wiki.default_branch + expect(service_execute[:commit]).not_to be_empty + end + end +end diff --git a/spec/workers/design_management/new_version_worker_spec.rb b/spec/workers/design_management/new_version_worker_spec.rb index 76497dde464..ef7cd8de108 100644 --- a/spec/workers/design_management/new_version_worker_spec.rb +++ b/spec/workers/design_management/new_version_worker_spec.rb @@ -3,14 +3,6 @@ require 'spec_helper' describe DesignManagement::NewVersionWorker do - # TODO a number of these tests are being temporarily skipped unless run in EE, - # as we are in the process of moving Design Management to FOSS in 13.0 - # in steps. In the current step the services have not yet been moved, and - # certain services are called within these tests: - # - `SystemNoteService` - # - `DesignManagement::GenerateImageVersionsService` - # - # See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283. describe '#perform' do let(:worker) { described_class.new } @@ -18,16 +10,12 @@ describe DesignManagement::NewVersionWorker do let(:version_id) { -1 } it 'does not create system notes' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - expect(SystemNoteService).not_to receive(:design_version_added) worker.perform(version_id) end it 'does not invoke GenerateImageVersionsService' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - expect(DesignManagement::GenerateImageVersionsService).not_to receive(:new) worker.perform(version_id) @@ -45,14 +33,10 @@ describe DesignManagement::NewVersionWorker do let_it_be(:version) { create(:design_version, :with_lfs_file, designs_count: 2) } it 'creates a system note' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - expect { worker.perform(version.id) }.to change { Note.system.count }.by(1) end it 'invokes GenerateImageVersionsService' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - expect_next_instance_of(DesignManagement::GenerateImageVersionsService) do |service| expect(service).to receive(:execute) end @@ -61,8 +45,6 @@ describe DesignManagement::NewVersionWorker do end it 'does not log anything' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - expect(Sidekiq.logger).not_to receive(:warn) worker.perform(version.id) @@ -77,14 +59,10 @@ describe DesignManagement::NewVersionWorker do end it 'creates two system notes' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - expect { worker.perform(version.id) }.to change { Note.system.count }.by(2) end it 'calls design_version_added' do - skip 'See https://gitlab.com/gitlab-org/gitlab/-/issues/212566#note_327724283' unless Gitlab.ee? - expect(SystemNoteService).to receive(:design_version_added).with(version) worker.perform(version.id) |