From 371e1ed3716d25f48c6458373e64ea81e0a45946 Mon Sep 17 00:00:00 2001 From: Denys Mishunov Date: Thu, 22 Aug 2019 23:23:06 +0000 Subject: [PATCH] Always pre-select "Start a new merge request" One exception: there is an existing MR for the current branch and the branch is non-default and non-protected. Extended mock_data for ide/stores to have different types of branches: default, protected and regular Cleaned new MR checkbox view --- .../ide/components/commit_sidebar/actions.vue | 12 +- .../new_merge_request_option.vue | 33 ++- app/assets/javascripts/ide/stores/getters.js | 3 + .../ide/stores/modules/commit/actions.js | 23 +-- .../ide/stores/modules/commit/getters.js | 12 +- .../stores/modules/commit/mutation_types.js | 1 - .../ide/stores/modules/commit/mutations.js | 3 - .../ide/stores/modules/commit/state.js | 3 +- .../51470-webide-default-commit.yml | 5 + qa/qa/page/project/web_ide/edit.rb | 5 + .../projects/tree/create_directory_spec.rb | 13 +- .../projects/tree/create_file_spec.rb | 7 +- .../stores/modules/commit/mutations_spec.js | 8 - .../components/commit_sidebar/actions_spec.js | 188 ++++++++++++++++-- .../new_merge_request_option_spec.js | 160 ++++++++++++--- spec/javascripts/ide/mock_data.js | 34 +++- spec/javascripts/ide/stores/getters_spec.js | 32 +++ .../ide/stores/modules/commit/actions_spec.js | 177 ++++------------- .../ide/stores/modules/commit/getters_spec.js | 162 ++++++++++++++- 19 files changed, 619 insertions(+), 262 deletions(-) create mode 100644 changelogs/unreleased/51470-webide-default-commit.yml diff --git a/app/assets/javascripts/ide/components/commit_sidebar/actions.vue b/app/assets/javascripts/ide/components/commit_sidebar/actions.vue index 685d8a6b245..8b356ee6e97 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/actions.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/actions.vue @@ -41,10 +41,16 @@ export default { methods: { ...mapCommitActions(['updateCommitAction']), updateSelectedCommitAction() { - if (this.currentBranch && !this.currentBranch.can_push) { - this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH); - } else if (this.containsStagedChanges) { + if (!this.currentBranch) { + return; + } + + const { can_push: canPush = false, default: isDefault = false } = this.currentBranch; + + if (canPush && !isDefault) { this.updateCommitAction(consts.COMMIT_TO_CURRENT_BRANCH); + } else { + this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH); } }, }, diff --git a/app/assets/javascripts/ide/components/commit_sidebar/new_merge_request_option.vue b/app/assets/javascripts/ide/components/commit_sidebar/new_merge_request_option.vue index b2e7b15089c..daa44a42765 100644 --- a/app/assets/javascripts/ide/components/commit_sidebar/new_merge_request_option.vue +++ b/app/assets/javascripts/ide/components/commit_sidebar/new_merge_request_option.vue @@ -1,43 +1,36 @@ diff --git a/app/assets/javascripts/ide/stores/getters.js b/app/assets/javascripts/ide/stores/getters.js index 406903129db..85fd45358be 100644 --- a/app/assets/javascripts/ide/stores/getters.js +++ b/app/assets/javascripts/ide/stores/getters.js @@ -104,5 +104,8 @@ export const packageJson = state => state.entries[packageJsonPath]; export const isOnDefaultBranch = (_state, getters) => getters.currentProject && getters.currentProject.default_branch === getters.branchName; +export const canPushToBranch = (_state, getters) => + getters.currentBranch && getters.currentBranch.can_push; + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/ide/stores/modules/commit/actions.js b/app/assets/javascripts/ide/stores/modules/commit/actions.js index ac34491c1ad..23caf2d48ed 100644 --- a/app/assets/javascripts/ide/stores/modules/commit/actions.js +++ b/app/assets/javascripts/ide/stores/modules/commit/actions.js @@ -18,34 +18,15 @@ export const discardDraft = ({ commit }) => { commit(types.UPDATE_COMMIT_MESSAGE, ''); }; -export const updateCommitAction = ({ commit, dispatch }, commitAction) => { +export const updateCommitAction = ({ commit, getters }, commitAction) => { commit(types.UPDATE_COMMIT_ACTION, { commitAction, }); - dispatch('setShouldCreateMR'); + commit(types.TOGGLE_SHOULD_CREATE_MR, !getters.shouldHideNewMrOption); }; export const toggleShouldCreateMR = ({ commit }) => { commit(types.TOGGLE_SHOULD_CREATE_MR); - commit(types.INTERACT_WITH_NEW_MR); -}; - -export const setShouldCreateMR = ({ - commit, - getters, - rootGetters, - state: { interactedWithNewMR }, -}) => { - const committingToExistingMR = - getters.isCommittingToCurrentBranch && - rootGetters.hasMergeRequest && - !rootGetters.isOnDefaultBranch; - - if ((getters.isCommittingToDefaultBranch && !interactedWithNewMR) || committingToExistingMR) { - commit(types.TOGGLE_SHOULD_CREATE_MR, false); - } else if (!interactedWithNewMR) { - commit(types.TOGGLE_SHOULD_CREATE_MR, true); - } }; export const updateBranchName = ({ commit }, branchName) => { diff --git a/app/assets/javascripts/ide/stores/modules/commit/getters.js b/app/assets/javascripts/ide/stores/modules/commit/getters.js index 64779e9e4df..de289e27199 100644 --- a/app/assets/javascripts/ide/stores/modules/commit/getters.js +++ b/app/assets/javascripts/ide/stores/modules/commit/getters.js @@ -20,7 +20,7 @@ export const placeholderBranchName = (state, _, rootState) => )}`; export const branchName = (state, getters, rootState) => { - if (state.commitAction === consts.COMMIT_TO_NEW_BRANCH) { + if (getters.isCreatingNewBranch) { if (state.newBranchName === '') { return getters.placeholderBranchName; } @@ -48,11 +48,11 @@ export const preBuiltCommitMessage = (state, _, rootState) => { export const isCreatingNewBranch = state => state.commitAction === consts.COMMIT_TO_NEW_BRANCH; -export const isCommittingToCurrentBranch = state => - state.commitAction === consts.COMMIT_TO_CURRENT_BRANCH; - -export const isCommittingToDefaultBranch = (_state, getters, _rootState, rootGetters) => - getters.isCommittingToCurrentBranch && rootGetters.isOnDefaultBranch; +export const shouldHideNewMrOption = (_state, getters, _rootState, rootGetters) => + !getters.isCreatingNewBranch && + (rootGetters.hasMergeRequest || + (!rootGetters.hasMergeRequest && rootGetters.isOnDefaultBranch)) && + rootGetters.canPushToBranch; // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/ide/stores/modules/commit/mutation_types.js b/app/assets/javascripts/ide/stores/modules/commit/mutation_types.js index b81918156b0..7ad8f3570b7 100644 --- a/app/assets/javascripts/ide/stores/modules/commit/mutation_types.js +++ b/app/assets/javascripts/ide/stores/modules/commit/mutation_types.js @@ -3,4 +3,3 @@ export const UPDATE_COMMIT_ACTION = 'UPDATE_COMMIT_ACTION'; export const UPDATE_NEW_BRANCH_NAME = 'UPDATE_NEW_BRANCH_NAME'; export const UPDATE_LOADING = 'UPDATE_LOADING'; export const TOGGLE_SHOULD_CREATE_MR = 'TOGGLE_SHOULD_CREATE_MR'; -export const INTERACT_WITH_NEW_MR = 'INTERACT_WITH_NEW_MR'; diff --git a/app/assets/javascripts/ide/stores/modules/commit/mutations.js b/app/assets/javascripts/ide/stores/modules/commit/mutations.js index 14957d283bb..73b618e250f 100644 --- a/app/assets/javascripts/ide/stores/modules/commit/mutations.js +++ b/app/assets/javascripts/ide/stores/modules/commit/mutations.js @@ -24,7 +24,4 @@ export default { shouldCreateMR: shouldCreateMR === undefined ? !state.shouldCreateMR : shouldCreateMR, }); }, - [types.INTERACT_WITH_NEW_MR](state) { - Object.assign(state, { interactedWithNewMR: true }); - }, }; diff --git a/app/assets/javascripts/ide/stores/modules/commit/state.js b/app/assets/javascripts/ide/stores/modules/commit/state.js index 53647a7e3e3..259577e48e0 100644 --- a/app/assets/javascripts/ide/stores/modules/commit/state.js +++ b/app/assets/javascripts/ide/stores/modules/commit/state.js @@ -3,6 +3,5 @@ export default () => ({ commitAction: '1', newBranchName: '', submitCommitLoading: false, - shouldCreateMR: false, - interactedWithNewMR: false, + shouldCreateMR: true, }); diff --git a/changelogs/unreleased/51470-webide-default-commit.yml b/changelogs/unreleased/51470-webide-default-commit.yml new file mode 100644 index 00000000000..d64111e7ec3 --- /dev/null +++ b/changelogs/unreleased/51470-webide-default-commit.yml @@ -0,0 +1,5 @@ +--- +title: Updated WebIDE default commit options +merge_request: 31449 +author: +type: changed diff --git a/qa/qa/page/project/web_ide/edit.rb b/qa/qa/page/project/web_ide/edit.rb index b5a36862389..37bca97fec7 100644 --- a/qa/qa/page/project/web_ide/edit.rb +++ b/qa/qa/page/project/web_ide/edit.rb @@ -39,6 +39,10 @@ module QA element :commit_button end + view 'app/assets/javascripts/ide/components/commit_sidebar/new_merge_request_option.vue' do + element :start_new_mr_checkbox + end + def has_file?(file_name) within_element(:file_list) do page.has_content? file_name @@ -100,6 +104,7 @@ module QA # animation is still in process even when the buttons have the # expected visibility. commit_success_msg_shown = retry_until do + uncheck_element :start_new_mr_checkbox click_element :commit_button wait(reload: false) do diff --git a/spec/features/projects/tree/create_directory_spec.rb b/spec/features/projects/tree/create_directory_spec.rb index 7ac5da86702..99285011405 100644 --- a/spec/features/projects/tree/create_directory_spec.rb +++ b/spec/features/projects/tree/create_directory_spec.rb @@ -32,10 +32,12 @@ describe 'Multi-file editor new directory', :js do click_button('Create directory') end + expect(page).to have_content('folder name') + first('.ide-tree-actions button').click - page.within('.modal-dialog') do - find('.form-control').set('file name') + page.within('.modal') do + find('.form-control').set('folder name/file name') click_button('Create file') end @@ -44,13 +46,18 @@ describe 'Multi-file editor new directory', :js do find('.js-ide-commit-mode').click - find('.multi-file-commit-list-item').hover click_button 'Stage' fill_in('commit-message', with: 'commit message ide') + find(:css, ".js-ide-commit-new-mr input").set(false) + + wait_for_requests + page.within '.multi-file-commit-form' do click_button('Commit') + + wait_for_requests end find('.js-ide-edit-mode').click diff --git a/spec/features/projects/tree/create_file_spec.rb b/spec/features/projects/tree/create_file_spec.rb index 00eefe9db42..780575a5975 100644 --- a/spec/features/projects/tree/create_file_spec.rb +++ b/spec/features/projects/tree/create_file_spec.rb @@ -36,15 +36,20 @@ describe 'Multi-file editor new file', :js do find('.js-ide-commit-mode').click - find('.multi-file-commit-list-item').hover click_button 'Stage' fill_in('commit-message', with: 'commit message ide') + find(:css, ".js-ide-commit-new-mr input").set(false) + page.within '.multi-file-commit-form' do click_button('Commit') + + wait_for_requests end + find('.js-ide-edit-mode').click + expect(page).to have_content('file name') end end diff --git a/spec/frontend/ide/stores/modules/commit/mutations_spec.js b/spec/frontend/ide/stores/modules/commit/mutations_spec.js index 246500a2f34..45ac1a86ab3 100644 --- a/spec/frontend/ide/stores/modules/commit/mutations_spec.js +++ b/spec/frontend/ide/stores/modules/commit/mutations_spec.js @@ -62,12 +62,4 @@ describe('IDE commit module mutations', () => { expect(state.shouldCreateMR).toBe(false); }); }); - - describe('INTERACT_WITH_NEW_MR', () => { - it('sets interactedWithNewMR to true', () => { - mutations.INTERACT_WITH_NEW_MR(state); - - expect(state.interactedWithNewMR).toBe(true); - }); - }); }); diff --git a/spec/javascripts/ide/components/commit_sidebar/actions_spec.js b/spec/javascripts/ide/components/commit_sidebar/actions_spec.js index b903abe63fc..a3db3ee1b18 100644 --- a/spec/javascripts/ide/components/commit_sidebar/actions_spec.js +++ b/spec/javascripts/ide/components/commit_sidebar/actions_spec.js @@ -1,30 +1,28 @@ import Vue from 'vue'; -import store from '~/ide/stores'; -import consts from '~/ide/stores/modules/commit/constants'; +import { createStore } from '~/ide/stores'; import commitActions from '~/ide/components/commit_sidebar/actions.vue'; +import consts from '~/ide/stores/modules/commit/constants'; import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; -import { resetStore } from 'spec/ide/helpers'; -import { projectData } from 'spec/ide/mock_data'; +import { projectData, branches } from 'spec/ide/mock_data'; + +const ACTION_UPDATE_COMMIT_ACTION = 'commit/updateCommitAction'; describe('IDE commit sidebar actions', () => { + let store; let vm; - const createComponent = ({ - hasMR = false, - commitAction = consts.COMMIT_TO_NEW_BRANCH, - mergeRequestsEnabled = true, - currentBranchId = 'master', - shouldCreateMR = false, - } = {}) => { + + const createComponent = ({ hasMR = false, currentBranchId = 'master' } = {}) => { const Component = Vue.extend(commitActions); vm = createComponentWithStore(Component, store); vm.$store.state.currentBranchId = currentBranchId; vm.$store.state.currentProjectId = 'abcproject'; - vm.$store.state.commit.commitAction = commitAction; - Vue.set(vm.$store.state.projects, 'abcproject', { ...projectData }); - vm.$store.state.projects.abcproject.merge_requests_enabled = mergeRequestsEnabled; - vm.$store.state.commit.shouldCreateMR = shouldCreateMR; + + const proj = { ...projectData }; + proj.branches[currentBranchId] = branches.find(branch => branch.name === currentBranchId); + + Vue.set(vm.$store.state.projects, 'abcproject', proj); if (hasMR) { vm.$store.state.currentMergeRequestId = '1'; @@ -33,13 +31,19 @@ describe('IDE commit sidebar actions', () => { ] = { foo: 'bar' }; } - return vm.$mount(); + vm.$mount(); + + return vm; }; + beforeEach(() => { + store = createStore(); + spyOn(store, 'dispatch'); + }); + afterEach(() => { vm.$destroy(); - - resetStore(vm.$store); + vm = null; }); it('renders 2 groups', () => { @@ -73,4 +77,152 @@ describe('IDE commit sidebar actions', () => { expect(vm.commitToCurrentBranchText).not.toContain(injectedSrc); }); }); + + describe('updateSelectedCommitAction', () => { + it('does not return anything if currentBranch does not exist', () => { + createComponent({ currentBranchId: null }); + + expect(vm.$store.dispatch).not.toHaveBeenCalled(); + }); + + it('calls again after staged changes', done => { + createComponent({ currentBranchId: null }); + + vm.$store.state.currentBranchId = 'master'; + vm.$store.state.changedFiles.push({}); + vm.$store.state.stagedFiles.push({}); + + vm.$nextTick() + .then(() => { + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + jasmine.anything(), + ); + }) + .then(done) + .catch(done.fail); + }); + + describe('default branch', () => { + it('dispatches correct action for default branch', () => { + createComponent({ + currentBranchId: 'master', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledTimes(1); + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_NEW_BRANCH, + ); + }); + }); + + describe('protected branch', () => { + describe('with write access', () => { + it('dispatches correct action when MR exists', () => { + createComponent({ + hasMR: true, + currentBranchId: 'protected/access', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_CURRENT_BRANCH, + ); + }); + + it('dispatches correct action when MR does not exists', () => { + createComponent({ + hasMR: false, + currentBranchId: 'protected/access', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_CURRENT_BRANCH, + ); + }); + }); + + describe('without write access', () => { + it('dispatches correct action when MR exists', () => { + createComponent({ + hasMR: true, + currentBranchId: 'protected/no-access', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_NEW_BRANCH, + ); + }); + + it('dispatches correct action when MR does not exists', () => { + createComponent({ + hasMR: false, + currentBranchId: 'protected/no-access', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_NEW_BRANCH, + ); + }); + }); + }); + + describe('regular branch', () => { + describe('with write access', () => { + it('dispatches correct action when MR exists', () => { + createComponent({ + hasMR: true, + currentBranchId: 'regular', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_CURRENT_BRANCH, + ); + }); + + it('dispatches correct action when MR does not exists', () => { + createComponent({ + hasMR: false, + currentBranchId: 'regular', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_CURRENT_BRANCH, + ); + }); + }); + + describe('without write access', () => { + it('dispatches correct action when MR exists', () => { + createComponent({ + hasMR: true, + currentBranchId: 'regular/no-access', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_NEW_BRANCH, + ); + }); + + it('dispatches correct action when MR does not exists', () => { + createComponent({ + hasMR: false, + currentBranchId: 'regular/no-access', + }); + + expect(vm.$store.dispatch).toHaveBeenCalledWith( + ACTION_UPDATE_COMMIT_ACTION, + consts.COMMIT_TO_NEW_BRANCH, + ); + }); + }); + }); + }); }); diff --git a/spec/javascripts/ide/components/commit_sidebar/new_merge_request_option_spec.js b/spec/javascripts/ide/components/commit_sidebar/new_merge_request_option_spec.js index 7017bfcd6a6..5f2db695241 100644 --- a/spec/javascripts/ide/components/commit_sidebar/new_merge_request_option_spec.js +++ b/spec/javascripts/ide/components/commit_sidebar/new_merge_request_option_spec.js @@ -1,33 +1,36 @@ import Vue from 'vue'; import store from '~/ide/stores'; -import consts from '~/ide/stores/modules/commit/constants'; import NewMergeRequestOption from '~/ide/components/commit_sidebar/new_merge_request_option.vue'; import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; -import { projectData } from 'spec/ide/mock_data'; +import { projectData, branches } from 'spec/ide/mock_data'; import { resetStore } from 'spec/ide/helpers'; +import consts from '../../../../../app/assets/javascripts/ide/stores/modules/commit/constants'; describe('create new MR checkbox', () => { let vm; - const createComponent = ({ - hasMR = false, - commitAction = consts.COMMIT_TO_NEW_BRANCH, - currentBranchId = 'master', - } = {}) => { + const setMR = () => { + vm.$store.state.currentMergeRequestId = '1'; + vm.$store.state.projects[store.state.currentProjectId].mergeRequests[ + store.state.currentMergeRequestId + ] = { foo: 'bar' }; + }; + + const createComponent = ({ currentBranchId = 'master', createNewBranch = false } = {}) => { const Component = Vue.extend(NewMergeRequestOption); vm = createComponentWithStore(Component, store); + vm.$store.state.commit.commitAction = createNewBranch + ? consts.COMMIT_TO_NEW_BRANCH + : consts.COMMIT_TO_CURRENT_BRANCH; + vm.$store.state.currentBranchId = currentBranchId; vm.$store.state.currentProjectId = 'abcproject'; - vm.$store.state.commit.commitAction = commitAction; - Vue.set(vm.$store.state.projects, 'abcproject', { ...projectData }); - if (hasMR) { - vm.$store.state.currentMergeRequestId = '1'; - vm.$store.state.projects[store.state.currentProjectId].mergeRequests[ - store.state.currentMergeRequestId - ] = { foo: 'bar' }; - } + const proj = JSON.parse(JSON.stringify(projectData)); + proj.branches[currentBranchId] = branches.find(branch => branch.name === currentBranchId); + + Vue.set(vm.$store.state.projects, 'abcproject', proj); return vm.$mount(); }; @@ -38,30 +41,131 @@ describe('create new MR checkbox', () => { resetStore(vm.$store); }); - it('is hidden when an MR already exists and committing to current branch', () => { - createComponent({ - hasMR: true, - commitAction: consts.COMMIT_TO_CURRENT_BRANCH, - currentBranchId: 'feature', + describe('for default branch', () => { + describe('is rendered when pushing to a new branch', () => { + beforeEach(() => { + createComponent({ + currentBranchId: 'master', + createNewBranch: true, + }); + }); + + it('has NO new MR', () => { + expect(vm.$el.textContent).not.toBe(''); + }); + + it('has new MR', done => { + setMR(); + + vm.$nextTick() + .then(() => { + expect(vm.$el.textContent).not.toBe(''); + }) + .then(done) + .catch(done.fail); + }); }); - expect(vm.$el.textContent).toBe(''); + describe('is NOT rendered when pushing to the same branch', () => { + beforeEach(() => { + createComponent({ + currentBranchId: 'master', + createNewBranch: false, + }); + }); + + it('has NO new MR', () => { + expect(vm.$el.textContent).toBe(''); + }); + + it('has new MR', done => { + setMR(); + + vm.$nextTick() + .then(() => { + expect(vm.$el.textContent).toBe(''); + }) + .then(done) + .catch(done.fail); + }); + }); }); - it('does not hide checkbox if MR does not exist', () => { - createComponent({ hasMR: false }); + describe('for protected branch', () => { + describe('when user does not have the write access', () => { + beforeEach(() => { + createComponent({ + currentBranchId: 'protected/no-access', + }); + }); - expect(vm.$el.querySelector('input[type="checkbox"]').hidden).toBe(false); + it('is rendered if MR does not exists', () => { + expect(vm.$el.textContent).not.toBe(''); + }); + + it('is rendered if MR exists', done => { + setMR(); + + vm.$nextTick() + .then(() => { + expect(vm.$el.textContent).not.toBe(''); + }) + .then(done) + .catch(done.fail); + }); + }); + + describe('when user has the write access', () => { + beforeEach(() => { + createComponent({ + currentBranchId: 'protected/access', + }); + }); + + it('is rendered if MR does not exist', () => { + expect(vm.$el.textContent).not.toBe(''); + }); + + it('is hidden if MR exists', done => { + setMR(); + + vm.$nextTick() + .then(() => { + expect(vm.$el.textContent).toBe(''); + }) + .then(done) + .catch(done.fail); + }); + }); }); - it('does not hide checkbox when creating a new branch', () => { - createComponent({ commitAction: consts.COMMIT_TO_NEW_BRANCH }); + describe('for regular branch', () => { + beforeEach(() => { + createComponent({ + currentBranchId: 'regular', + }); + }); - expect(vm.$el.querySelector('input[type="checkbox"]').hidden).toBe(false); + it('is rendered if no MR exists', () => { + expect(vm.$el.textContent).not.toBe(''); + }); + + it('is hidden if MR exists', done => { + setMR(); + + vm.$nextTick() + .then(() => { + expect(vm.$el.textContent).toBe(''); + }) + .then(done) + .catch(done.fail); + }); }); it('dispatches toggleShouldCreateMR when clicking checkbox', () => { - createComponent(); + createComponent({ + currentBranchId: 'regular', + }); const el = vm.$el.querySelector('input[type="checkbox"]'); spyOn(vm.$store, 'dispatch'); el.dispatchEvent(new Event('change')); diff --git a/spec/javascripts/ide/mock_data.js b/spec/javascripts/ide/mock_data.js index 570a396c5e3..c02c7e5d45e 100644 --- a/spec/javascripts/ide/mock_data.js +++ b/spec/javascripts/ide/mock_data.js @@ -176,23 +176,51 @@ export const branches = [ committed_date: '2018-08-01T00:20:05Z', }, can_push: true, + protected: true, + default: true, }, { id: 2, - name: 'feature/lorem-ipsum', + name: 'protected/no-access', + commit: { + message: 'Update some stuff', + committed_date: '2018-08-02T00:00:05Z', + }, + can_push: false, + protected: true, + default: false, + }, + { + id: 3, + name: 'protected/access', commit: { message: 'Update some stuff', committed_date: '2018-08-02T00:00:05Z', }, can_push: true, + protected: true, + default: false, }, { - id: 3, - name: 'feature/dolar-amit', + id: 4, + name: 'regular', commit: { message: 'Update some more stuff', committed_date: '2018-06-30T00:20:05Z', }, can_push: true, + protected: false, + default: false, + }, + { + id: 5, + name: 'regular/no-access', + commit: { + message: 'Update some more stuff', + committed_date: '2018-06-30T00:20:05Z', + }, + can_push: false, + protected: false, + default: false, }, ]; diff --git a/spec/javascripts/ide/stores/getters_spec.js b/spec/javascripts/ide/stores/getters_spec.js index 735bbd47f55..73a8d993a13 100644 --- a/spec/javascripts/ide/stores/getters_spec.js +++ b/spec/javascripts/ide/stores/getters_spec.js @@ -221,4 +221,36 @@ describe('IDE store getters', () => { }); }); }); + + describe('canPushToBranch', () => { + it('returns false when no currentBranch exists', () => { + const localGetters = { + currentProject: undefined, + }; + + expect(getters.canPushToBranch({}, localGetters)).toBeFalsy(); + }); + + it('returns true when can_push to currentBranch', () => { + const localGetters = { + currentProject: { + default_branch: 'master', + }, + currentBranch: { can_push: true }, + }; + + expect(getters.canPushToBranch({}, localGetters)).toBeTruthy(); + }); + + it('returns false when !can_push to currentBranch', () => { + const localGetters = { + currentProject: { + default_branch: 'master', + }, + currentBranch: { can_push: false }, + }; + + expect(getters.canPushToBranch({}, localGetters)).toBeFalsy(); + }); + }); }); diff --git a/spec/javascripts/ide/stores/modules/commit/actions_spec.js b/spec/javascripts/ide/stores/modules/commit/actions_spec.js index 14d861f21d2..091b454c0d2 100644 --- a/spec/javascripts/ide/stores/modules/commit/actions_spec.js +++ b/spec/javascripts/ide/stores/modules/commit/actions_spec.js @@ -57,6 +57,44 @@ describe('IDE commit module actions', () => { .then(done) .catch(done.fail); }); + + it('sets shouldCreateMR to true if "Create new MR" option is visible', done => { + store.state.shouldHideNewMrOption = false; + + testAction( + actions.updateCommitAction, + {}, + store.state, + [ + { + type: mutationTypes.UPDATE_COMMIT_ACTION, + payload: { commitAction: jasmine.anything() }, + }, + { type: mutationTypes.TOGGLE_SHOULD_CREATE_MR, payload: true }, + ], + [], + done, + ); + }); + + it('sets shouldCreateMR to false if "Create new MR" option is hidden', done => { + store.state.shouldHideNewMrOption = true; + + testAction( + actions.updateCommitAction, + {}, + store.state, + [ + { + type: mutationTypes.UPDATE_COMMIT_ACTION, + payload: { commitAction: jasmine.anything() }, + }, + { type: mutationTypes.TOGGLE_SHOULD_CREATE_MR, payload: false }, + ], + [], + done, + ); + }); }); describe('updateBranchName', () => { @@ -541,147 +579,10 @@ describe('IDE commit module actions', () => { actions.toggleShouldCreateMR, {}, store.state, - [ - { type: mutationTypes.TOGGLE_SHOULD_CREATE_MR }, - { type: mutationTypes.INTERACT_WITH_NEW_MR }, - ], + [{ type: mutationTypes.TOGGLE_SHOULD_CREATE_MR }], [], done, ); }); }); - - describe('setShouldCreateMR', () => { - beforeEach(() => { - store.state.projects = { - project: { - default_branch: 'master', - branches: { - master: { - name: 'master', - }, - feature: { - name: 'feature', - }, - }, - }, - }; - - store.state.currentProjectId = 'project'; - }); - - it('sets to false when the current branch already has an MR', done => { - store.state.commit.currentMergeRequestId = 1; - store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH; - store.state.currentMergeRequestId = '1'; - store.state.currentBranchId = 'feature'; - spyOn(store, 'commit').and.callThrough(); - - store - .dispatch('commit/setShouldCreateMR') - .then(() => { - expect(store.commit.calls.allArgs()[0]).toEqual( - jasmine.arrayContaining([`commit/${mutationTypes.TOGGLE_SHOULD_CREATE_MR}`, false]), - ); - done(); - }) - .catch(done.fail); - }); - - it('changes to false when current branch is the default branch and user has not interacted', done => { - store.state.commit.interactedWithNewMR = false; - store.state.currentBranchId = 'master'; - store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH; - spyOn(store, 'commit').and.callThrough(); - - store - .dispatch('commit/setShouldCreateMR') - .then(() => { - expect(store.commit.calls.allArgs()[0]).toEqual( - jasmine.arrayContaining([`commit/${mutationTypes.TOGGLE_SHOULD_CREATE_MR}`, false]), - ); - done(); - }) - .catch(done.fail); - }); - - it('changes to true when "create new branch" is selected and user has not interacted', done => { - store.state.commit.commitAction = consts.COMMIT_TO_NEW_BRANCH; - store.state.commit.interactedWithNewMR = false; - spyOn(store, 'commit').and.callThrough(); - - store - .dispatch('commit/setShouldCreateMR') - .then(() => { - expect(store.commit.calls.allArgs()[0]).toEqual( - jasmine.arrayContaining([`commit/${mutationTypes.TOGGLE_SHOULD_CREATE_MR}`, true]), - ); - done(); - }) - .catch(done.fail); - }); - - it('does not change anything if user has interacted and comitting to new branch', done => { - store.state.commit.commitAction = consts.COMMIT_TO_NEW_BRANCH; - store.state.commit.interactedWithNewMR = true; - spyOn(store, 'commit').and.callThrough(); - - store - .dispatch('commit/setShouldCreateMR') - .then(() => { - expect(store.commit).not.toHaveBeenCalled(); - done(); - }) - .catch(done.fail); - }); - - it('does not change anything if user has interacted and comitting to branch without MR', done => { - store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH; - store.state.commit.currentMergeRequestId = null; - store.state.commit.interactedWithNewMR = true; - spyOn(store, 'commit').and.callThrough(); - - store - .dispatch('commit/setShouldCreateMR') - .then(() => { - expect(store.commit).not.toHaveBeenCalled(); - done(); - }) - .catch(done.fail); - }); - - it('still changes to false if hiding the checkbox', done => { - store.state.currentBranchId = 'feature'; - store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH; - store.state.currentMergeRequestId = '1'; - store.state.commit.interactedWithNewMR = true; - spyOn(store, 'commit').and.callThrough(); - - store - .dispatch('commit/setShouldCreateMR') - .then(() => { - expect(store.commit.calls.allArgs()[0]).toEqual( - jasmine.arrayContaining([`commit/${mutationTypes.TOGGLE_SHOULD_CREATE_MR}`, false]), - ); - done(); - }) - .catch(done.fail); - }); - - it('does not change to false when on master and user has interacted even if MR exists', done => { - store.state.currentBranchId = 'master'; - store.state.commit.commitAction = consts.COMMIT_TO_CURRENT_BRANCH; - store.state.currentMergeRequestId = '1'; - store.state.commit.interactedWithNewMR = true; - spyOn(store, 'commit').and.callThrough(); - - store - .dispatch('commit/setShouldCreateMR') - .then(() => { - expect(store.commit).not.toHaveBeenCalled(); - done(); - }) - .catch(done.fail); - }); - }); }); diff --git a/spec/javascripts/ide/stores/modules/commit/getters_spec.js b/spec/javascripts/ide/stores/modules/commit/getters_spec.js index 6e71a790deb..07445c22917 100644 --- a/spec/javascripts/ide/stores/modules/commit/getters_spec.js +++ b/spec/javascripts/ide/stores/modules/commit/getters_spec.js @@ -1,6 +1,6 @@ import commitState from '~/ide/stores/modules/commit/state'; -import consts from '~/ide/stores/modules/commit/constants'; import * as getters from '~/ide/stores/modules/commit/getters'; +import consts from '~/ide/stores/modules/commit/constants'; describe('IDE commit module getters', () => { let state; @@ -55,15 +55,15 @@ describe('IDE commit module getters', () => { }); }); - it('defualts to currentBranchId', () => { - expect(getters.branchName(state, null, rootState)).toBe('master'); + it('defaults to currentBranchId when not committing to a new branch', () => { + localGetters.isCreatingNewBranch = false; + + expect(getters.branchName(state, localGetters, rootState)).toBe('master'); }); - describe('COMMIT_TO_NEW_BRANCH', () => { + describe('commit to a new branch', () => { beforeEach(() => { - Object.assign(state, { - commitAction: consts.COMMIT_TO_NEW_BRANCH, - }); + localGetters.isCreatingNewBranch = true; }); it('uses newBranchName when not empty', () => { @@ -144,4 +144,152 @@ describe('IDE commit module getters', () => { }); }); }); + + describe('isCreatingNewBranch', () => { + it('returns false if NOT creating a new branch', () => { + state.commitAction = consts.COMMIT_TO_CURRENT_BRANCH; + + expect(getters.isCreatingNewBranch(state)).toBeFalsy(); + }); + + it('returns true if creating a new branch', () => { + state.commitAction = consts.COMMIT_TO_NEW_BRANCH; + + expect(getters.isCreatingNewBranch(state)).toBeTruthy(); + }); + }); + + describe('shouldHideNewMrOption', () => { + let localGetters = {}; + let rootGetters = {}; + + beforeEach(() => { + localGetters = { + isCreatingNewBranch: null, + }; + rootGetters = { + isOnDefaultBranch: null, + hasMergeRequest: null, + canPushToBranch: null, + }; + }); + + describe('NO existing MR for the branch', () => { + beforeEach(() => { + rootGetters.hasMergeRequest = false; + }); + + it('should never hide "New MR" option', () => { + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); + }); + }); + + describe('existing MR for the branch', () => { + beforeEach(() => { + rootGetters.hasMergeRequest = true; + }); + + it('should NOT hide "New MR" option if user can NOT push to the current branch', () => { + rootGetters.canPushToBranch = false; + + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); + }); + + it('should hide "New MR" option if user can push to the current branch', () => { + rootGetters.canPushToBranch = true; + + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeTruthy(); + }); + }); + + describe('user can NOT push the branch', () => { + beforeEach(() => { + rootGetters.canPushToBranch = false; + }); + + it('should never hide "New MR" option', () => { + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); + }); + }); + + describe('user can push to the branch', () => { + beforeEach(() => { + rootGetters.canPushToBranch = true; + }); + + it('should NOT hide "New MR" option if there is NO existing MR for the current branch', () => { + rootGetters.hasMergeRequest = false; + + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); + }); + + it('should hide "New MR" option if there is existing MR for the current branch', () => { + rootGetters.hasMergeRequest = true; + + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeTruthy(); + }); + }); + + describe('default branch', () => { + beforeEach(() => { + rootGetters.isOnDefaultBranch = true; + }); + + describe('committing to the same branch', () => { + beforeEach(() => { + localGetters.isCreatingNewBranch = false; + rootGetters.canPushToBranch = true; + }); + + it('should hide "New MR" when there is an existing MR', () => { + rootGetters.hasMergeRequest = true; + + expect( + getters.shouldHideNewMrOption(state, localGetters, null, rootGetters), + ).toBeTruthy(); + }); + + it('should hide "New MR" when there is no existing MR', () => { + rootGetters.hasMergeRequest = false; + + expect( + getters.shouldHideNewMrOption(state, localGetters, null, rootGetters), + ).toBeTruthy(); + }); + }); + + describe('creating a new branch', () => { + beforeEach(() => { + localGetters.isCreatingNewBranch = true; + }); + + it('should NOT hide "New MR" option no matter existence of an MR or write access', () => { + rootGetters.hasMergeRequest = false; + rootGetters.canPushToBranch = true; + + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); + + rootGetters.hasMergeRequest = true; + rootGetters.canPushToBranch = true; + + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); + + rootGetters.hasMergeRequest = false; + rootGetters.canPushToBranch = false; + + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); + }); + }); + }); + + it('should never hide "New MR" option when creating a new branch', () => { + localGetters.isCreatingNewBranch = true; + + rootGetters.isOnDefaultBranch = false; + rootGetters.hasMergeRequest = true; + rootGetters.canPushToBranch = true; + + expect(getters.shouldHideNewMrOption(state, localGetters, null, rootGetters)).toBeFalsy(); + }); + }); });