From d637f87f88b40704291d7a01032af09853afb4e4 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 9 Feb 2018 17:07:20 +0000 Subject: Makes close/reopen issue request to inside the vue app --- app/assets/javascripts/issue.js | 75 ++++++++++++++-------- .../javascripts/notes/components/comment_form.vue | 28 ++++---- app/assets/javascripts/notes/index.js | 2 + .../javascripts/notes/services/notes_service.js | 3 + app/assets/javascripts/notes/stores/actions.js | 33 ++++++++++ app/assets/javascripts/notes/stores/getters.js | 1 + .../javascripts/notes/stores/mutation_types.js | 4 ++ app/assets/javascripts/notes/stores/mutations.js | 8 +++ app/views/projects/issues/_discussion.html.haml | 2 + changelogs/unreleased/42923-close-issue.yml | 5 ++ spec/javascripts/notes/helpers.js | 12 ++++ spec/javascripts/notes/mock_data.js | 2 + spec/javascripts/notes/stores/actions_spec.js | 71 ++++++++++++++++++++ 13 files changed, 206 insertions(+), 40 deletions(-) create mode 100644 changelogs/unreleased/42923-close-issue.yml create mode 100644 spec/javascripts/notes/helpers.js diff --git a/app/assets/javascripts/issue.js b/app/assets/javascripts/issue.js index ff65ea99e9a..1bf3dc1da91 100644 --- a/app/assets/javascripts/issue.js +++ b/app/assets/javascripts/issue.js @@ -25,6 +25,51 @@ export default class Issue { if (Issue.createMrDropdownWrap) { this.createMergeRequestDropdown = new CreateMergeRequestDropdown(Issue.createMrDropdownWrap); } + + // Listen to state changes in the Vue app + document.addEventListener('issuable_vue_app:change', (event) => { + this.updateTopState(event.detail.isClosed, event.detail.data); + }); + } + + /** + * This method updates the top area of the issue. + * + * Once the issue state changes, either through a click on the top area (jquery) + * or a click on the bottom area (Vue) we need to update the top area. + * + * @param {Boolean} isClosed + * @param {Array} data + * @param {String} issueFailMessage + */ + updateTopState(isClosed, data, issueFailMessage = 'Unable to update this issue at this time.') { + if ('id' in data) { + const isClosedBadge = $('div.status-box-issue-closed'); + const isOpenBadge = $('div.status-box-open'); + const projectIssuesCounter = $('.issue_counter'); + + isClosedBadge.toggleClass('hidden', !isClosed); + isOpenBadge.toggleClass('hidden', isClosed); + + $(document).trigger('issuable:change', isClosed); + this.toggleCloseReopenButton(isClosed); + + let numProjectIssues = Number(projectIssuesCounter.first().text().trim().replace(/[^\d]/, '')); + numProjectIssues = isClosed ? numProjectIssues - 1 : numProjectIssues + 1; + projectIssuesCounter.text(addDelimiter(numProjectIssues)); + + if (this.createMergeRequestDropdown) { + if (isClosed) { + this.createMergeRequestDropdown.unavailable(); + this.createMergeRequestDropdown.disable(); + } else { + // We should check in case a branch was created in another tab + this.createMergeRequestDropdown.checkAbilityToCreateBranch(); + } + } + } else { + flash(issueFailMessage); + } } initIssueBtnEventListeners() { @@ -45,34 +90,8 @@ export default class Issue { url = $button.attr('href'); return axios.put(url) .then(({ data }) => { - const isClosedBadge = $('div.status-box-issue-closed'); - const isOpenBadge = $('div.status-box-open'); - const projectIssuesCounter = $('.issue_counter'); - - if ('id' in data) { - const isClosed = $button.hasClass('btn-close'); - isClosedBadge.toggleClass('hidden', !isClosed); - isOpenBadge.toggleClass('hidden', isClosed); - - $(document).trigger('issuable:change', isClosed); - this.toggleCloseReopenButton(isClosed); - - let numProjectIssues = Number(projectIssuesCounter.first().text().trim().replace(/[^\d]/, '')); - numProjectIssues = isClosed ? numProjectIssues - 1 : numProjectIssues + 1; - projectIssuesCounter.text(addDelimiter(numProjectIssues)); - - if (this.createMergeRequestDropdown) { - if (isClosed) { - this.createMergeRequestDropdown.unavailable(); - this.createMergeRequestDropdown.disable(); - } else { - // We should check in case a branch was created in another tab - this.createMergeRequestDropdown.checkAbilityToCreateBranch(); - } - } - } else { - flash(issueFailMessage); - } + const isClosed = $button.hasClass('btn-close'); + this.updateTopState(isClosed, data); }) .catch(() => flash(issueFailMessage)) .then(() => { diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue index 3c8452ac808..7b54edb0c2a 100644 --- a/app/assets/javascripts/notes/components/comment_form.vue +++ b/app/assets/javascripts/notes/components/comment_form.vue @@ -2,6 +2,7 @@ import { mapActions, mapGetters } from 'vuex'; import _ from 'underscore'; import Autosize from 'autosize'; + import { __ } from '~/locale'; import Flash from '../../flash'; import Autosave from '../../autosave'; import TaskList from '../../task_list'; @@ -30,9 +31,6 @@ return { note: '', noteType: constants.COMMENT, - // Can't use mapGetters, - // this needs to be in the data object because it belongs to the state - issueState: this.$store.getters.getNoteableData.state, isSubmitting: false, isSubmitButtonDisabled: true, }; @@ -43,7 +41,11 @@ 'getUserData', 'getNoteableData', 'getNotesData', + 'getIssueState', ]), + issueState() { + return this.getIssueState; + }, isLoggedIn() { return this.getUserData.id; }, @@ -71,8 +73,6 @@ return { 'btn-reopen': !this.isIssueOpen, 'btn-close': this.isIssueOpen, - 'js-note-target-close': this.isIssueOpen, - 'js-note-target-reopen': !this.isIssueOpen, }; }, markdownDocsPath() { @@ -105,7 +105,7 @@ mounted() { // jQuery is needed here because it is a custom event being dispatched with jQuery. $(document).on('issuable:change', (e, isClosed) => { - this.issueState = isClosed ? constants.CLOSED : constants.REOPENED; + this.toggleIssueLocalState(isClosed ? constants.CLOSED : constants.REOPENED); }); this.initAutoSave(); @@ -117,6 +117,9 @@ 'stopPolling', 'restartPolling', 'removePlaceholderNotes', + 'closeIssue', + 'reopenIssue', + 'toggleIssueLocalState', ]), setIsSubmitButtonDisabled(note, isSubmitting) { if (!_.isEmpty(note) && !isSubmitting) { @@ -185,12 +188,13 @@ Please check your network connection and try again.`; } }, toggleIssueState() { - this.issueState = this.isIssueOpen ? constants.CLOSED : constants.REOPENED; - - // This is out of scope for the Notes Vue component. - // It was the shortest path to update the issue state and relevant places. - const btnClass = this.isIssueOpen ? 'btn-reopen' : 'btn-close'; - $(`.js-btn-issue-action.${btnClass}:visible`).trigger('click'); + if (this.isIssueOpen) { + this.closeIssue() + .catch(() => Flash(__('Something went wrong while closing the issue. Please try again later'))); + } else { + this.reopenIssue() + .catch(() => Flash(__('Something went wrong while reopening the issue. Please try again later'))); + } }, discard(shouldClear = true) { // `blur` is needed to clear slash commands autocomplete cache if event fired. diff --git a/app/assets/javascripts/notes/index.js b/app/assets/javascripts/notes/index.js index d250dd8d25b..48e7cfddb63 100644 --- a/app/assets/javascripts/notes/index.js +++ b/app/assets/javascripts/notes/index.js @@ -28,6 +28,8 @@ document.addEventListener('DOMContentLoaded', () => new Vue({ notesPath: notesDataset.notesPath, markdownDocsPath: notesDataset.markdownDocsPath, quickActionsDocsPath: notesDataset.quickActionsDocsPath, + closeIssuePath: notesDataset.closeIssuePath, + reopenIssuePath: notesDataset.reopenIssuePath, }, }; }, diff --git a/app/assets/javascripts/notes/services/notes_service.js b/app/assets/javascripts/notes/services/notes_service.js index b51b0cb2013..b8e7ffc8c46 100644 --- a/app/assets/javascripts/notes/services/notes_service.js +++ b/app/assets/javascripts/notes/services/notes_service.js @@ -32,4 +32,7 @@ export default { toggleAward(endpoint, data) { return Vue.http.post(endpoint, data, { emulateJSON: true }); }, + toggleIssueState(endpoint, data) { + return Vue.http.put(endpoint, data); + }, }; diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 085b18642ba..1877d5bf959 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -61,6 +61,39 @@ export const createNewNote = ({ commit }, { endpoint, data }) => service export const removePlaceholderNotes = ({ commit }) => commit(types.REMOVE_PLACEHOLDER_NOTES); +export const closeIssue = ({ commit, dispatch, state }) => service + .toggleIssueState(state.notesData.closeIssuePath) + .then(res => res.json()) + .then((data) => { + commit(types.CLOSE_ISSUE); + dispatch('emitStateChangedEvent', data); + }); + +export const reopenIssue = ({ commit, dispatch, state }) => service + .toggleIssueState(state.notesData.reopenIssuePath) + .then(res => res.json()) + .then((data) => { + commit(types.REOPEN_ISSUE); + dispatch('emitStateChangedEvent', data); + }); + +export const emitStateChangedEvent = ({ commit }, data) => { + const event = new CustomEvent('issuable_vue_app:change', { detail: { + data, + isClosed: data.state === constants.CLOSED, + } }); + + document.dispatchEvent(event); +}; + +export const toggleIssueLocalState = ({ commit }, newState) => { + if (newState === constants.CLOSED) { + commit(types.CLOSE_ISSUE); + } else if (newState === constants.REOPENED) { + commit(types.REOPEN_ISSUE); + } +}; + export const saveNote = ({ commit, dispatch }, noteData) => { const { note } = noteData.data.note; let placeholderText = note; diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index e18b277119e..38b48c3bbe4 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -8,6 +8,7 @@ export const getNotesDataByProp = state => prop => state.notesData[prop]; export const getNoteableData = state => state.noteableData; export const getNoteableDataByProp = state => prop => state.noteableData[prop]; +export const getIssueState = state => state.noteableData.state; export const getUserData = state => state.userData || {}; export const getUserDataByProp = state => prop => state.userData && state.userData[prop]; diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js index d520c197407..6d7c3bbae0f 100644 --- a/app/assets/javascripts/notes/stores/mutation_types.js +++ b/app/assets/javascripts/notes/stores/mutation_types.js @@ -12,3 +12,7 @@ export const SHOW_PLACEHOLDER_NOTE = 'SHOW_PLACEHOLDER_NOTE'; export const TOGGLE_AWARD = 'TOGGLE_AWARD'; export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION'; export const UPDATE_NOTE = 'UPDATE_NOTE'; + +// Issue +export const CLOSE_ISSUE = 'CLOSE_ISSUE'; +export const REOPEN_ISSUE = 'REOPEN_ISSUE'; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index 20f81a430c2..b3f66578c9a 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -152,4 +152,12 @@ export default { noteObj.notes.splice(noteObj.notes.indexOf(comment), 1, note); } }, + + [types.CLOSE_ISSUE](state) { + Object.assign(state.noteableData, { state: constants.CLOSED }); + }, + + [types.REOPEN_ISSUE](state) { + Object.assign(state.noteableData, { state: constants.REOPENED }); + }, }; diff --git a/app/views/projects/issues/_discussion.html.haml b/app/views/projects/issues/_discussion.html.haml index 9779c1985d5..11b5e02f1e0 100644 --- a/app/views/projects/issues/_discussion.html.haml +++ b/app/views/projects/issues/_discussion.html.haml @@ -12,6 +12,8 @@ markdown_docs_path: help_page_path('user/markdown'), quick_actions_docs_path: help_page_path('user/project/quick_actions'), notes_path: notes_url, + close_issue_path: issue_path(@issue, issue: { state_event: :close }, format: 'json'), + reopen_issue_path: issue_path(@issue, issue: { state_event: :reopen }, format: 'json'), last_fetched_at: Time.now.to_i, noteable_data: serialize_issuable(@issue), current_user_data: UserSerializer.new.represent(current_user, only_path: true).to_json } } diff --git a/changelogs/unreleased/42923-close-issue.yml b/changelogs/unreleased/42923-close-issue.yml new file mode 100644 index 00000000000..e332bbf5dec --- /dev/null +++ b/changelogs/unreleased/42923-close-issue.yml @@ -0,0 +1,5 @@ +--- +title: Fix close button on issues not working on mobile +merge_request: +author: +type: fixed diff --git a/spec/javascripts/notes/helpers.js b/spec/javascripts/notes/helpers.js new file mode 100644 index 00000000000..a7663710a56 --- /dev/null +++ b/spec/javascripts/notes/helpers.js @@ -0,0 +1,12 @@ +// eslint-disable-next-line import/prefer-default-export +export const resetStore = (store) => { + store.replaceState({ + notes: [], + targetNoteHash: null, + lastFetchedAt: null, + + notesData: {}, + userData: {}, + noteableData: {}, + }); +}; diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index f0c800c759d..ccf4bd070c2 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/spec/javascripts/notes/mock_data.js @@ -7,6 +7,8 @@ export const notesDataMock = { notesPath: '/gitlab-org/gitlab-ce/noteable/issue/98/notes', quickActionsDocsPath: '/help/user/project/quick_actions', registerPath: '/users/sign_in?redirect_to_referer=yes#register-pane', + closeIssuePath: '/twitter/flight/issues/9.json?issue%5Bstate_event%5D=close', + reopenIssuePath: '/twitter/flight/issues/9.json?issue%5Bstate_event%5D=reopen', }; export const userDataMock = { diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index e092320f9a3..6508f4772d2 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/spec/javascripts/notes/stores/actions_spec.js @@ -1,8 +1,16 @@ +import Vue from 'vue'; +import _ from 'underscore'; import * as actions from '~/notes/stores/actions'; +import store from '~/notes/stores'; import testAction from '../../helpers/vuex_action_helper'; +import { resetStore } from '../helpers'; import { discussionMock, notesDataMock, userDataMock, noteableDataMock, individualNote } from '../mock_data'; describe('Actions Notes Store', () => { + afterEach(() => { + resetStore(store); + }); + describe('setNotesData', () => { it('should set received notes data', (done) => { testAction(actions.setNotesData, null, { notesData: {} }, [ @@ -58,4 +66,67 @@ describe('Actions Notes Store', () => { ], done); }); }); + + describe('async methods', () => { + const interceptor = (request, next) => { + next(request.respondWith(JSON.stringify({}), { + status: 200, + })); + }; + + beforeEach(() => { + Vue.http.interceptors.push(interceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); + }); + + describe('closeIssue', () => { + it('sets state as closed', (done) => { + store.dispatch('closeIssue', { notesData: { closeIssuePath: '' } }) + .then(() => { + expect(store.state.noteableData.state).toEqual('closed'); + done(); + }) + .catch(done.fail); + }); + }); + + describe('reopenIssue', () => { + it('sets state as reopened', (done) => { + store.dispatch('reopenIssue', { notesData: { reopenIssuePath: '' } }) + .then(() => { + expect(store.state.noteableData.state).toEqual('reopened'); + done(); + }) + .catch(done.fail); + }); + }); + }); + + describe('emitStateChangedEvent', () => { + it('emits an event on the document', () => { + document.addEventListener('issuable_vue_app:change', (event) => { + expect(event.detail.data).toEqual({ id: '1', state: 'closed' }); + expect(event.detail.isClosed).toEqual(true); + }); + + store.dispatch('emitStateChangedEvent', { id: '1', state: 'closed' }); + }); + }); + + describe('toggleIssueLocalState', () => { + it('sets issue state as closed', (done) => { + testAction(actions.toggleIssueLocalState, 'closed', {}, [ + { type: 'CLOSE_ISSUE', payload: 'closed' }, + ], done); + }); + + it('sets issue state as reopened', (done) => { + testAction(actions.toggleIssueLocalState, 'reopened', {}, [ + { type: 'REOPEN_ISSUE', payload: 'reopened' }, + ], done); + }); + }); }); -- cgit v1.2.1 From 853c80a9f72219d327fba1b92b539871086a08c9 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 12 Feb 2018 08:54:31 +0000 Subject: Put back class names used in shared test --- app/assets/javascripts/notes/components/comment_form.vue | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue index 7b54edb0c2a..c96f3336d28 100644 --- a/app/assets/javascripts/notes/components/comment_form.vue +++ b/app/assets/javascripts/notes/components/comment_form.vue @@ -73,6 +73,8 @@ return { 'btn-reopen': !this.isIssueOpen, 'btn-close': this.isIssueOpen, + 'js-note-target-close': this.isIssueOpen, + 'js-note-target-reopen': !this.isIssueOpen, }; }, markdownDocsPath() { -- cgit v1.2.1 From c33245e9999e0d00fd1da985fcc18af1618b211d Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 12 Feb 2018 16:49:41 +0000 Subject: Reuse getter Add loading button for better UX --- .../javascripts/notes/components/comment_form.vue | 47 ++++++++++++++-------- app/assets/javascripts/notes/stores/actions.js | 4 +- app/assets/javascripts/notes/stores/getters.js | 2 +- .../vue_shared/components/loading_button.vue | 2 +- spec/javascripts/notes/stores/actions_spec.js | 2 +- spec/javascripts/notes/stores/getters_spec.js | 6 +++ 6 files changed, 42 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/notes/components/comment_form.vue b/app/assets/javascripts/notes/components/comment_form.vue index c96f3336d28..df796050e0d 100644 --- a/app/assets/javascripts/notes/components/comment_form.vue +++ b/app/assets/javascripts/notes/components/comment_form.vue @@ -9,10 +9,11 @@ import * as constants from '../constants'; import eventHub from '../event_hub'; import issueWarning from '../../vue_shared/components/issue/issue_warning.vue'; - import noteSignedOutWidget from './note_signed_out_widget.vue'; - import discussionLockedWidget from './discussion_locked_widget.vue'; import markdownField from '../../vue_shared/components/markdown/field.vue'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; + import loadingButton from '../../vue_shared/components/loading_button.vue'; + import noteSignedOutWidget from './note_signed_out_widget.vue'; + import discussionLockedWidget from './discussion_locked_widget.vue'; import issuableStateMixin from '../mixins/issuable_state'; export default { @@ -23,6 +24,7 @@ discussionLockedWidget, markdownField, userAvatarLink, + loadingButton, }, mixins: [ issuableStateMixin, @@ -41,11 +43,8 @@ 'getUserData', 'getNoteableData', 'getNotesData', - 'getIssueState', + 'issueState', ]), - issueState() { - return this.getIssueState; - }, isLoggedIn() { return this.getUserData.id; }, @@ -131,6 +130,8 @@ } }, handleSave(withIssueAction) { + this.isSubmitting = true; + if (this.note.length) { const noteData = { endpoint: this.endpoint, @@ -147,7 +148,6 @@ if (this.noteType === constants.DISCUSSION) { noteData.data.note.type = constants.DISCUSSION_NOTE; } - this.isSubmitting = true; this.note = ''; // Empty textarea while being requested. Repopulate in catch this.resizeTextarea(); this.stopPolling(); @@ -189,13 +189,24 @@ Please check your network connection and try again.`; this.toggleIssueState(); } }, + enableButton() { + this.isSubmitting = false; + }, toggleIssueState() { if (this.isIssueOpen) { this.closeIssue() - .catch(() => Flash(__('Something went wrong while closing the issue. Please try again later'))); + .then(() => this.enableButton()) + .catch(() => { + this.enableButton(); + Flash(__('Something went wrong while closing the issue. Please try again later')); + }); } else { this.reopenIssue() - .catch(() => Flash(__('Something went wrong while reopening the issue. Please try again later'))); + .then(() => this.enableButton()) + .catch(() => { + this.enableButton(); + Flash(__('Something went wrong while reopening the issue. Please try again later')); + }); } }, discard(shouldClear = true) { @@ -373,15 +384,19 @@ append-right-10 comment-type-dropdown js-comment-type-dropdown droplab-dropdown" - + :label="issueActionButtonTitle" + /> +