From ad21205a8ec9bdab86e77d1db005439105c85872 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 16 Dec 2016 15:55:07 -0500 Subject: [PATCH 01/19] Unit tests for commit command --- test/controllers/git-panel-controller.test.js | 155 ++++++++++++------ 1 file changed, 101 insertions(+), 54 deletions(-) diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index eafa62d29e..8315ec1862 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -2,6 +2,7 @@ import fs from 'fs'; import path from 'path'; +import etch from 'etch'; import dedent from 'dedent-js'; @@ -11,13 +12,15 @@ import {cloneRepository, buildRepository} from '../helpers'; import {AbortMergeError, CommitError} from '../../lib/models/repository'; describe('GitPanelController', () => { - let atomEnvironment, workspace, commandRegistry, notificationManager; + let atomEnvironment, workspace, workspaceElement, commandRegistry, notificationManager; beforeEach(() => { atomEnvironment = global.buildAtomEnvironment(); workspace = atomEnvironment.workspace; commandRegistry = atomEnvironment.commands; notificationManager = atomEnvironment.notifications; + + workspaceElement = atomEnvironment.views.getView(workspace); }); afterEach(() => { @@ -196,29 +199,7 @@ describe('GitPanelController', () => { describe('keyboard navigation commands', () => { let controller, gitPanel, stagingView, commitView, commitViewController, focusElement; - beforeEach(async () => { - const workdirPath = await cloneRepository('each-staging-group'); - const repository = await buildRepository(workdirPath); - - // Merge with conflicts - assert.isRejected(repository.git.merge('origin/branch')); - - // Three unstaged files - fs.writeFileSync(path.join(workdirPath, 'unstaged-1.txt'), 'This is an unstaged file.'); - fs.writeFileSync(path.join(workdirPath, 'unstaged-2.txt'), 'This is an unstaged file.'); - fs.writeFileSync(path.join(workdirPath, 'unstaged-3.txt'), 'This is an unstaged file.'); - - // Three staged files - fs.writeFileSync(path.join(workdirPath, 'staged-1.txt'), 'This is a file with some changes staged for commit.'); - fs.writeFileSync(path.join(workdirPath, 'staged-2.txt'), 'This is another file staged for commit.'); - fs.writeFileSync(path.join(workdirPath, 'staged-3.txt'), 'This is a third file staged for commit.'); - await repository.stageFiles(['staged-1.txt', 'staged-2.txt', 'staged-3.txt']); - - await repository.refresh(); - - controller = new GitPanelController({workspace, commandRegistry, repository}); - await controller.getLastModelDataRefreshPromise(); - + const extractReferences = () => { gitPanel = controller.refs.gitPanel; stagingView = gitPanel.refs.stagingView; commitViewController = gitPanel.refs.commitViewController; @@ -228,55 +209,121 @@ describe('GitPanelController', () => { sinon.stub(commitView, 'focus', () => { focusElement = commitView; }); sinon.stub(commitView, 'isFocused', () => focusElement === commitView); sinon.stub(stagingView, 'focus', () => { focusElement = stagingView; }); - }); + }; const assertSelected = paths => { const selectionPaths = Array.from(stagingView.selection.getSelectedItems()).map(item => item.filePath); assert.deepEqual(selectionPaths, paths); }; - it('blurs on tool-panel:unfocus', () => { - sinon.spy(workspace.getActivePane(), 'activate'); + describe('with conflicts and staged files', () => { + beforeEach(async () => { + const workdirPath = await cloneRepository('each-staging-group'); + const repository = await buildRepository(workdirPath); - commandRegistry.dispatch(controller.element, 'tool-panel:unfocus'); + // Merge with conflicts + assert.isRejected(repository.git.merge('origin/branch')); - assert.isTrue(workspace.getActivePane().activate.called); - }); + fs.writeFileSync(path.join(workdirPath, 'unstaged-1.txt'), 'This is an unstaged file.'); + fs.writeFileSync(path.join(workdirPath, 'unstaged-2.txt'), 'This is an unstaged file.'); + fs.writeFileSync(path.join(workdirPath, 'unstaged-3.txt'), 'This is an unstaged file.'); + + // Three staged files + fs.writeFileSync(path.join(workdirPath, 'staged-1.txt'), 'This is a file with some changes staged for commit.'); + fs.writeFileSync(path.join(workdirPath, 'staged-2.txt'), 'This is another file staged for commit.'); + fs.writeFileSync(path.join(workdirPath, 'staged-3.txt'), 'This is a third file staged for commit.'); + await repository.stageFiles(['staged-1.txt', 'staged-2.txt', 'staged-3.txt']); + await repository.refresh(); + + const didChangeAmending = () => {}; + + controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending}); + await controller.getLastModelDataRefreshPromise(); + + extractReferences(); + }); - it('advances focus through StagingView groups and CommitView, but does not cycle', () => { - assertSelected(['unstaged-1.txt']); + it('blurs on tool-panel:unfocus', () => { + sinon.spy(workspace.getActivePane(), 'activate'); - commandRegistry.dispatch(controller.element, 'core:focus-next'); - assertSelected(['conflict-1.txt']); + commandRegistry.dispatch(controller.element, 'tool-panel:unfocus'); - commandRegistry.dispatch(controller.element, 'core:focus-next'); - assertSelected(['staged-1.txt']); + assert.isTrue(workspace.getActivePane().activate.called); + }); + + it('advances focus through StagingView groups and CommitView, but does not cycle', () => { + assertSelected(['unstaged-1.txt']); + + commandRegistry.dispatch(controller.element, 'core:focus-next'); + assertSelected(['conflict-1.txt']); + + commandRegistry.dispatch(controller.element, 'core:focus-next'); + assertSelected(['staged-1.txt']); + + commandRegistry.dispatch(controller.element, 'core:focus-next'); + assertSelected(['staged-1.txt']); + assert.strictEqual(focusElement, commitView); + + // This should be a no-op. (Actually, it'll insert a tab in the CommitView editor.) + commandRegistry.dispatch(controller.element, 'core:focus-next'); + assertSelected(['staged-1.txt']); + assert.strictEqual(focusElement, commitView); + }); + + it('retreats focus from the CommitView through StagingView groups, but does not cycle', () => { + commitView.focus(); + + commandRegistry.dispatch(controller.element, 'core:focus-previous'); + assertSelected(['staged-1.txt']); - commandRegistry.dispatch(controller.element, 'core:focus-next'); - assertSelected(['staged-1.txt']); - assert.strictEqual(focusElement, commitView); + commandRegistry.dispatch(controller.element, 'core:focus-previous'); + assertSelected(['conflict-1.txt']); - // This should be a no-op. (Actually, it'll insert a tab in the CommitView editor.) - commandRegistry.dispatch(controller.element, 'core:focus-next'); - assertSelected(['staged-1.txt']); - assert.strictEqual(focusElement, commitView); + commandRegistry.dispatch(controller.element, 'core:focus-previous'); + assertSelected(['unstaged-1.txt']); + + // This should be a no-op. + commandRegistry.dispatch(controller.element, 'core:focus-previous'); + assertSelected(['unstaged-1.txt']); + }); }); - it('retreats focus from the CommitView through StagingView groups, but does not cycle', () => { - commitView.focus(); + describe('with staged changes', () => { + beforeEach(async () => { + const workdirPath = await cloneRepository('each-staging-group'); + const repository = await buildRepository(workdirPath); + + // A staged file + fs.writeFileSync(path.join(workdirPath, 'staged-1.txt'), 'This is a file with some changes staged for commit.'); + await repository.stageFiles(['staged-1.txt']); + await repository.refresh(); - commandRegistry.dispatch(controller.element, 'core:focus-previous'); - assertSelected(['staged-1.txt']); + const didChangeAmending = () => {}; - commandRegistry.dispatch(controller.element, 'core:focus-previous'); - assertSelected(['conflict-1.txt']); + controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending}); + await controller.getLastModelDataRefreshPromise(); + + extractReferences(); + }); - commandRegistry.dispatch(controller.element, 'core:focus-previous'); - assertSelected(['unstaged-1.txt']); + it('focuses the CommitView on github:commit with an empty commit message', async () => { + commitView.editor.setText(''); + sinon.spy(controller, 'commit'); + await etch.update(controller); // Ensure that the spy is passed to child components in props - // This should be a no-op. - commandRegistry.dispatch(controller.element, 'core:focus-previous'); - assertSelected(['unstaged-1.txt']); + commandRegistry.dispatch(workspaceElement, 'github:commit'); + assert.strictEqual(focusElement, commitView); + assert.isFalse(controller.commit.called); + }); + + it.only('creates a commit on github:commit with a nonempty commit message', async () => { + commitView.editor.setText('I fixed the things'); + sinon.spy(controller, 'commit'); + await etch.update(controller); // Ensure that the spy is passed to child components in props + + commandRegistry.dispatch(workspaceElement, 'github:commit'); + assert.isTrue(controller.commit.calledWith('I fixed the things')); + }); }); }); From 3c4c8f9c4cbedeccd1466594fb9a36e00a1c4b05 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 16 Dec 2016 15:55:35 -0500 Subject: [PATCH 02/19] Install github:commit in the full workspace --- lib/views/commit-view.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index d9d4712200..dcb87a709b 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -26,7 +26,7 @@ export default class CommitView { this.subscriptions = new CompositeDisposable( this.editor.onDidChange(() => this.props.onChangeMessage && this.props.onChangeMessage(this.editor.getText())), this.editor.onDidChangeCursorPosition(() => { etch.update(this); }), - props.commandRegistry.add(this.element, {'github:commit': this.commit}), + props.commandRegistry.add('atom-workspace', {'github:commit': this.commit}), ); const grammar = atom.grammars.grammarForScopeName(COMMIT_GRAMMAR_SCOPE); @@ -118,6 +118,8 @@ export default class CommitView { commit() { if (this.isCommitButtonEnabled()) { this.props.commit(this.editor.getText()); + } else { + this.focus(); } } From 6b6f6813f8e8f8bbf7dac60a66bca67e149d60fa Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 16 Dec 2016 15:57:41 -0500 Subject: [PATCH 03/19] Keymap entries for github:commit --- keymaps/git.cson | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/keymaps/git.cson b/keymaps/git.cson index 014ba5453f..64a14287a2 100644 --- a/keymaps/git.cson +++ b/keymaps/git.cson @@ -2,14 +2,16 @@ 'ctrl-9': 'github:toggle-git-panel' 'ctrl-(': 'github:toggle-git-panel-focus' # ctrl-shift-9 +'.github-Panel': + 'cmd-enter': 'github:commit' + 'ctrl-enter': 'github:commit' + '.github-StagingView': 'left': 'github:focus-diff-view' 'tab': 'core:focus-next' 'shift-tab': 'core:focus-previous' '.github-CommitView-editor atom-text-editor:not([mini])': - 'cmd-enter': 'github:commit' - 'ctrl-enter': 'github:commit' 'shift-tab': 'core:focus-previous' '.github-FilePatchView': From 495860c713fc5a9bbf39a1442ede0d946271bfb6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 16 Dec 2016 15:59:55 -0500 Subject: [PATCH 04/19] Fixup: remove .only --- test/controllers/git-panel-controller.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 8315ec1862..64c4016f81 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -316,7 +316,7 @@ describe('GitPanelController', () => { assert.isFalse(controller.commit.called); }); - it.only('creates a commit on github:commit with a nonempty commit message', async () => { + it('creates a commit on github:commit with a nonempty commit message', async () => { commitView.editor.setText('I fixed the things'); sinon.spy(controller, 'commit'); await etch.update(controller); // Ensure that the spy is passed to child components in props From 9983fc9cc5a17e7fa363367ad0a2c9a23e86ca4d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 16 Dec 2016 16:00:53 -0500 Subject: [PATCH 05/19] Fixup: restore atom-text-editor keymap entries --- keymaps/git.cson | 2 ++ 1 file changed, 2 insertions(+) diff --git a/keymaps/git.cson b/keymaps/git.cson index 64a14287a2..2ee2bdcfcd 100644 --- a/keymaps/git.cson +++ b/keymaps/git.cson @@ -12,6 +12,8 @@ 'shift-tab': 'core:focus-previous' '.github-CommitView-editor atom-text-editor:not([mini])': + 'cmd-enter': 'github:commit' + 'ctrl-enter': 'github:commit' 'shift-tab': 'core:focus-previous' '.github-FilePatchView': From 4adc0aae349cf24902ca04e91de85447687a0bb5 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 16 Dec 2016 16:02:32 -0500 Subject: [PATCH 06/19] Temporary --- lib/controllers/git-controller.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/controllers/git-controller.js b/lib/controllers/git-controller.js index 543c67df22..b560784abb 100644 --- a/lib/controllers/git-controller.js +++ b/lib/controllers/git-controller.js @@ -76,6 +76,7 @@ export default class GitController extends React.Component { props.commandRegistry.add('atom-workspace', { 'github:toggle-git-panel': this.toggleGitPanel, 'github:toggle-git-panel-focus': this.toggleGitPanelFocus, + 'github:commit': () => console.log('GitController callback fired'), }), ); From 906a4f17f8dfea706d334c2d2bd9d2c179bee202 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 16 Dec 2016 16:19:49 -0500 Subject: [PATCH 07/19] Ensure the GitPanel is visible when github:commit is fired --- lib/controllers/git-controller.js | 9 ++++++++- test/controllers/git-controller.test.js | 22 +++++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/lib/controllers/git-controller.js b/lib/controllers/git-controller.js index b560784abb..72f583099d 100644 --- a/lib/controllers/git-controller.js +++ b/lib/controllers/git-controller.js @@ -70,13 +70,14 @@ export default class GitController extends React.Component { this.toggleGitPanelFocus = this.toggleGitPanelFocus.bind(this); this.focusFilePatchView = this.focusFilePatchView.bind(this); this.focusGitPanel = this.focusGitPanel.bind(this); + this.ensureGitPanel = this.ensureGitPanel.bind(this); this.subscriptions = new CompositeDisposable(); this.subscriptions.add( props.commandRegistry.add('atom-workspace', { 'github:toggle-git-panel': this.toggleGitPanel, 'github:toggle-git-panel-focus': this.toggleGitPanelFocus, - 'github:commit': () => console.log('GitController callback fired'), + 'github:commit': this.ensureGitPanel, }), ); @@ -258,6 +259,12 @@ export default class GitController extends React.Component { return this.gitPanelController.getWrappedComponent().isFocused(); } + ensureGitPanel(event) { + if (!this.state.gitPanelActive) { + this.setState({gitPanelActive: true}); + } + } + focusFilePatchView() { this.filePatchController.getWrappedComponent().focus(); } diff --git a/test/controllers/git-controller.test.js b/test/controllers/git-controller.test.js index 417a22e0cb..817e645b00 100644 --- a/test/controllers/git-controller.test.js +++ b/test/controllers/git-controller.test.js @@ -11,13 +11,16 @@ import {cloneRepository, buildRepository} from '../helpers'; import GitController from '../../lib/controllers/git-controller'; describe('GitController', () => { - let atomEnv, workspace, commandRegistry, notificationManager, app; + let atomEnv, workspace, workspaceElement, commandRegistry, notificationManager, app; beforeEach(() => { atomEnv = global.buildAtomEnvironment(); workspace = atomEnv.workspace; commandRegistry = atomEnv.commands; notificationManager = atomEnv.notifications; + + workspaceElement = atomEnv.views.getView(workspace); + app = ( { wrapper.setProps({repository: repository1}); assert.equal(wrapper.state('amending'), true); }); + + it('ensures that the panel is visible with github:commit is sent', async () => { + const workdirPath = await cloneRepository('three-files'); + const repository = await buildRepository(workdirPath); + + sinon.stub(repository, 'commit'); + + app = React.cloneElement(app, {repository}); + const wrapper = shallow(app); + + assert.isFalse(wrapper.find('Panel').prop('visible')); + + commandRegistry.dispatch(workspaceElement, 'github:commit'); + + assert.isTrue(wrapper.find('Panel').prop('visible')); + assert.isFalse(repository.commit.called); + }); }); From d901392c36ff5d43cbe11ebeab17afa13ba81215 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 16 Dec 2016 16:23:45 -0500 Subject: [PATCH 08/19] Update CommitView spec --- test/views/commit-view.test.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/views/commit-view.test.js b/test/views/commit-view.test.js index b4c9aa978b..08749582ef 100644 --- a/test/views/commit-view.test.js +++ b/test/views/commit-view.test.js @@ -106,11 +106,12 @@ describe('CommitView', () => { const commit = sinon.spy(); const view = new CommitView({commandRegistry, stagedChangesExist: false, commit, message: ''}); const {editor, commitButton} = view.refs; + const workspaceElement = atomEnv.views.getView(atomEnv.workspace); // commit by clicking the commit button await view.update({stagedChangesExist: true, message: 'Commit 1'}); commitButton.dispatchEvent(new MouseEvent('click')); - assert.equal(commit.args[0][0], 'Commit 1'); + assert.isTrue(commit.calledWith('Commit 1')); // undo history is cleared commandRegistry.dispatch(editor.element, 'core:undo'); @@ -119,19 +120,19 @@ describe('CommitView', () => { // commit via the github:commit command commit.reset(); await view.update({stagedChangesExist: true, message: 'Commit 2'}); - commandRegistry.dispatch(editor.element, 'github:commit'); - assert.equal(commit.args[0][0], 'Commit 2'); + commandRegistry.dispatch(workspaceElement, 'github:commit'); + assert.isTrue(commit.calledWith('Commit 2')); // disable github:commit when there are no staged changes... commit.reset(); await view.update({stagedChangesExist: false, message: 'Commit 4'}); - commandRegistry.dispatch(editor.element, 'github:commit'); + commandRegistry.dispatch(workspaceElement, 'github:commit'); assert.equal(commit.callCount, 0); // ...or the commit message is empty commit.reset(); await view.update({stagedChangesExist: true, message: ''}); - commandRegistry.dispatch(editor.element, 'github:commit'); + commandRegistry.dispatch(workspaceElement, 'github:commit'); assert.equal(commit.callCount, 0); }); From e1740345cf616c76ed8b5be2dd09da85c4f58a7a Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 19 Dec 2016 12:55:53 -0500 Subject: [PATCH 09/19] Call onChangeMessage from CommitViewController --- lib/controllers/commit-view-controller.js | 25 +++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js index 155a7dfbe8..995adfdcc6 100644 --- a/lib/controllers/commit-view-controller.js +++ b/lib/controllers/commit-view-controller.js @@ -37,15 +37,11 @@ export default class CommitViewController { } render() { - let message = this.regularCommitMessage; - if (this.props.isAmending) { - message = this.amendingCommitMessage; - if (!message.length && this.props.lastCommit) { - message = this.props.lastCommit.message; - } - } else if (!message.length && this.props.mergeMessage) { - message = this.props.mergeMessage; + const message = this.getCommitMessage(); + if (this.props.onChangeMessage) { + this.props.onChangeMessage(message); } + return ( Date: Mon, 19 Dec 2016 14:53:12 -0500 Subject: [PATCH 10/19] Ensure that the git panel is shown Resolve a Promise as `true` if it had been previously hidden or `false` if it was already shown. --- lib/controllers/git-controller.js | 12 +++++++++--- test/controllers/git-controller.test.js | 24 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/lib/controllers/git-controller.js b/lib/controllers/git-controller.js index 72f583099d..c439123b62 100644 --- a/lib/controllers/git-controller.js +++ b/lib/controllers/git-controller.js @@ -77,7 +77,6 @@ export default class GitController extends React.Component { props.commandRegistry.add('atom-workspace', { 'github:toggle-git-panel': this.toggleGitPanel, 'github:toggle-git-panel-focus': this.toggleGitPanelFocus, - 'github:commit': this.ensureGitPanel, }), ); @@ -144,6 +143,7 @@ export default class GitController extends React.Component { didDiveIntoMergeConflictPath={this.diveIntoMergeConflictFileForPath} didChangeAmending={this.didChangeAmending} focusFilePatchView={this.focusFilePatchView} + ensureGitPanel={this.ensureGitPanel} /> @@ -259,10 +259,16 @@ export default class GitController extends React.Component { return this.gitPanelController.getWrappedComponent().isFocused(); } - ensureGitPanel(event) { + // Ensure that the Git panel is visible. Returns a Promise that resolves to `true` if the panel was initially + // hidden or `false` if it was already shown. + ensureGitPanel() { if (!this.state.gitPanelActive) { - this.setState({gitPanelActive: true}); + return new Promise((resolve, reject) => { + this.setState({gitPanelActive: true}, () => resolve(true)); + }); } + + return Promise.resolve(false); } focusFilePatchView() { diff --git a/test/controllers/git-controller.test.js b/test/controllers/git-controller.test.js index 817e645b00..2cb5d4660f 100644 --- a/test/controllers/git-controller.test.js +++ b/test/controllers/git-controller.test.js @@ -324,6 +324,30 @@ describe('GitController', () => { }); }); + describe('ensureGitPanel()', () => { + let wrapper; + + beforeEach(async () => { + const workdirPath = await cloneRepository('multiple-commits'); + const repository = await buildRepository(workdirPath); + + app = React.cloneElement(app, {repository}); + wrapper = shallow(app); + }); + + it('opens the Git panel when it is initially closed', async () => { + assert.isFalse(wrapper.find('Panel').prop('visible')); + assert.isTrue(await wrapper.instance().ensureGitPanel()); + }); + + it('does nothing when the Git panel is already open', async () => { + wrapper.instance().toggleGitPanel(); + assert.isTrue(wrapper.find('Panel').prop('visible')); + assert.isFalse(await wrapper.instance().ensureGitPanel()); + assert.isTrue(wrapper.find('Panel').prop('visible')); + }); + }); + it('correctly updates state when switching repos', async () => { const workdirPath1 = await cloneRepository('three-files'); const repository1 = await buildRepository(workdirPath1); From d3c2acea1eeaef0639dda3cd0863f8eae249946d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 19 Dec 2016 15:11:07 -0500 Subject: [PATCH 11/19] Ensure the Git panel is visible before committing --- lib/controllers/git-panel-controller.js | 5 +++++ test/controllers/git-panel-controller.test.js | 19 +++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index 4324c118b9..f15e4e5a62 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -173,6 +173,11 @@ export default class GitPanelController { } async commit(message) { + if (await this.props.ensureGitPanel()) { + this.focus(); + return; + } + try { await this.getActiveRepository().commit(message, {amend: this.props.isAmending}); this.setAmending(false); diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 64c4016f81..64b5f57e34 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -144,15 +144,29 @@ describe('GitPanelController', () => { }); describe('commit(message)', () => { + it('shows the git panel and takes no further action if it was hidden', async () => { + const workdirPath = await cloneRepository('three-files'); + const repository = await buildRepository(workdirPath); + sinon.stub(repository, 'commit', () => Promise.resolve()); + + const ensureGitPanel = () => Promise.resolve(true); + const controller = new GitPanelController({workspace, commandRegistry, repository, ensureGitPanel}); + + await controller.commit('message'); + + assert.equal(repository.commit.callCount, 0); + }); + it('shows an error notification when committing throws an ECONFLICT exception', async () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); + const ensureGitPanel = () => Promise.resolve(false); sinon.stub(repository, 'commit', async () => { await Promise.resolve(); throw new CommitError('ECONFLICT'); }); - const controller = new GitPanelController({workspace, commandRegistry, notificationManager, repository}); + const controller = new GitPanelController({workspace, commandRegistry, notificationManager, repository, ensureGitPanel}); assert.equal(notificationManager.getNotifications().length, 0); await controller.commit(); assert.equal(notificationManager.getNotifications().length, 1); @@ -161,9 +175,10 @@ describe('GitPanelController', () => { it('sets amending to false', async () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); + const ensureGitPanel = () => Promise.resolve(false); sinon.stub(repository, 'commit', () => Promise.resolve()); const didChangeAmending = sinon.stub(); - const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending}); + const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending, ensureGitPanel}); await controller.commit('message'); assert.equal(didChangeAmending.callCount, 1); From b0dc042857f591e71d855170d6c1992766ddc333 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 19 Dec 2016 15:14:26 -0500 Subject: [PATCH 12/19] Remove test for old behavior --- test/controllers/git-controller.test.js | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/test/controllers/git-controller.test.js b/test/controllers/git-controller.test.js index 2cb5d4660f..2b96cdf1cc 100644 --- a/test/controllers/git-controller.test.js +++ b/test/controllers/git-controller.test.js @@ -366,21 +366,4 @@ describe('GitController', () => { wrapper.setProps({repository: repository1}); assert.equal(wrapper.state('amending'), true); }); - - it('ensures that the panel is visible with github:commit is sent', async () => { - const workdirPath = await cloneRepository('three-files'); - const repository = await buildRepository(workdirPath); - - sinon.stub(repository, 'commit'); - - app = React.cloneElement(app, {repository}); - const wrapper = shallow(app); - - assert.isFalse(wrapper.find('Panel').prop('visible')); - - commandRegistry.dispatch(workspaceElement, 'github:commit'); - - assert.isTrue(wrapper.find('Panel').prop('visible')); - assert.isFalse(repository.commit.called); - }); }); From 6d62ae2a7821c17ac3e10bc40851d708fdab01c1 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Mon, 19 Dec 2016 15:24:40 -0500 Subject: [PATCH 13/19] Stub ensureGitPanel in other specs --- test/controllers/git-panel-controller.test.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 64b5f57e34..d1861a3ad5 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -79,11 +79,12 @@ describe('GitPanelController', () => { assert.deepEqual(controller.refs.gitPanel.props.unstagedChanges, await repository2.getUnstagedChanges()); }); - it('displays the staged changes since the parent commmit when amending', async () => { + it('displays the staged changes since the parent commit when amending', async () => { const didChangeAmending = sinon.spy(); const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); - const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending, isAmending: false}); + const ensureGitPanel = () => Promise.resolve(false); + const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending, ensureGitPanel, isAmending: false}); await controller.getLastModelDataRefreshPromise(); assert.deepEqual(controller.refs.gitPanel.props.stagedChanges, []); assert.equal(didChangeAmending.callCount, 0); @@ -348,7 +349,8 @@ describe('GitPanelController', () => { const repository = await buildRepository(workdirPath); fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'a change\n'); fs.unlinkSync(path.join(workdirPath, 'b.txt')); - const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending: sinon.stub()}); + const ensureGitPanel = () => Promise.resolve(false); + const controller = new GitPanelController({workspace, commandRegistry, repository, ensureGitPanel, didChangeAmending: sinon.stub()}); await controller.getLastModelDataRefreshPromise(); const stagingView = controller.refs.gitPanel.refs.stagingView; const commitView = controller.refs.gitPanel.refs.commitViewController.refs.commitView; From a6b5506a100bf9c410900ab12a2256784f2c9b9b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 20 Dec 2016 11:53:16 -0500 Subject: [PATCH 14/19] Give an async prepareToCommit() function a chance to cancel commits --- lib/views/commit-view.js | 7 +- test/views/commit-view.test.js | 144 +++++++++++++++++++++------------ 2 files changed, 95 insertions(+), 56 deletions(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index dcb87a709b..421abb5e55 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -12,11 +12,10 @@ export default class CommitView { constructor(props) { this.props = props; - this.commit = this.commit.bind(this); - this.abortMerge = this.abortMerge.bind(this); this.handleAmendBoxClick = this.handleAmendBoxClick.bind(this); this.commit = this.commit.bind(this); this.abortMerge = this.abortMerge.bind(this); + etch.initialize(this); this.editor = this.refs.editor; @@ -115,8 +114,8 @@ export default class CommitView { this.props.setAmending(this.refs.amend.checked); } - commit() { - if (this.isCommitButtonEnabled()) { + async commit() { + if (await this.props.prepareToCommit() && this.isCommitButtonEnabled()) { this.props.commit(this.editor.getText()); } else { this.focus(); diff --git a/test/views/commit-view.test.js b/test/views/commit-view.test.js index 08749582ef..d38a6f376d 100644 --- a/test/views/commit-view.test.js +++ b/test/views/commit-view.test.js @@ -1,6 +1,6 @@ /** @babel */ -import {cloneRepository, buildRepository} from '../helpers'; +import {cloneRepository, buildRepository, until} from '../helpers'; import etch from 'etch'; import CommitView from '../../lib/views/commit-view'; @@ -76,64 +76,104 @@ describe('CommitView', () => { assert.equal(view.editor.getGrammar().scopeName, 'text.git-commit'); }); - it('disables the commit button when no changes are staged, there are merge conflict files, or the commit message is empty', async () => { - const workdirPath = await cloneRepository('three-files'); - const repository = await buildRepository(workdirPath); - const viewState = {}; - const view = new CommitView({repository, commandRegistry, stagedChangesExist: false, viewState}); - const {editor, commitButton} = view.refs; - assert.isTrue(commitButton.disabled); + describe('the commit button', () => { + let view, editor, commitButton; - editor.setText('something'); - await etch.getScheduler().getNextUpdatePromise(); - assert.isTrue(commitButton.disabled); + beforeEach(async () => { + const workdirPath = await cloneRepository('three-files'); + const repository = await buildRepository(workdirPath); + const viewState = {}; + view = new CommitView({repository, commandRegistry, stagedChangesExist: true, mergeConflictsExist: false, viewState}); + editor = view.refs.editor; + commitButton = view.refs.commitButton; - await view.update({stagedChangesExist: true}); - assert.isFalse(commitButton.disabled); + editor.setText('something'); + await etch.getScheduler().getNextUpdatePromise(); + }); - await view.update({mergeConflictsExist: true}); - assert.isTrue(commitButton.disabled); + it('is disabled when no changes are staged', async () => { + await view.update({stagedChangesExist: false}); + assert.isTrue(commitButton.disabled); - await view.update({mergeConflictsExist: false}); - assert.isFalse(commitButton.disabled); + await view.update({stagedChangesExist: true}); + assert.isFalse(commitButton.disabled); + }); - editor.setText(''); - await etch.getScheduler().getNextUpdatePromise(); - assert.isTrue(commitButton.disabled); + it('is disabled when there are merge conflicts', async () => { + await view.update({mergeConflictsExist: false}); + assert.isFalse(commitButton.disabled); + + await view.update({mergeConflictsExist: true}); + assert.isTrue(commitButton.disabled); + }); + + it('is disabled when the commit message is empty', async () => { + editor.setText(''); + await etch.getScheduler().getNextUpdatePromise(); + assert.isTrue(commitButton.disabled); + + editor.setText('Not empty'); + await etch.getScheduler().getNextUpdatePromise(); + assert.isFalse(commitButton.disabled); + }); }); - it('calls props.commit(message) when the commit button is clicked or github:commit is dispatched', async () => { - const commit = sinon.spy(); - const view = new CommitView({commandRegistry, stagedChangesExist: false, commit, message: ''}); - const {editor, commitButton} = view.refs; - const workspaceElement = atomEnv.views.getView(atomEnv.workspace); - - // commit by clicking the commit button - await view.update({stagedChangesExist: true, message: 'Commit 1'}); - commitButton.dispatchEvent(new MouseEvent('click')); - assert.isTrue(commit.calledWith('Commit 1')); - - // undo history is cleared - commandRegistry.dispatch(editor.element, 'core:undo'); - assert.equal(editor.getText(), ''); - - // commit via the github:commit command - commit.reset(); - await view.update({stagedChangesExist: true, message: 'Commit 2'}); - commandRegistry.dispatch(workspaceElement, 'github:commit'); - assert.isTrue(commit.calledWith('Commit 2')); - - // disable github:commit when there are no staged changes... - commit.reset(); - await view.update({stagedChangesExist: false, message: 'Commit 4'}); - commandRegistry.dispatch(workspaceElement, 'github:commit'); - assert.equal(commit.callCount, 0); - - // ...or the commit message is empty - commit.reset(); - await view.update({stagedChangesExist: true, message: ''}); - commandRegistry.dispatch(workspaceElement, 'github:commit'); - assert.equal(commit.callCount, 0); + describe('committing', () => { + let view, commit, prepareToCommitResolution; + let editor, commitButton, workspaceElement; + + beforeEach(async () => { + const prepareToCommit = () => Promise.resolve(prepareToCommitResolution); + + commit = sinon.spy(); + view = new CommitView({commandRegistry, stagedChangesExist: true, prepareToCommit, commit, message: 'Something'}); + sinon.spy(view, 'focus'); + + editor = view.refs.editor; + commitButton = view.refs.commitButton; + + workspaceElement = atomEnv.views.getView(atomEnv.workspace); + + await view.update(); + }); + + describe('when props.prepareToCommit() resolves true', () => { + beforeEach(() => { prepareToCommitResolution = true; }); + + it('calls props.commit(message) when the commit button is clicked', async () => { + commitButton.dispatchEvent(new MouseEvent('click')); + + await until('props.commit() is called', () => commit.calledWith('Something')); + + // undo history is cleared + commandRegistry.dispatch(editor.element, 'core:undo'); + assert.equal(editor.getText(), ''); + }); + + it('calls props.commit(message) when github:commit is dispatched', async () => { + commandRegistry.dispatch(workspaceElement, 'github:commit'); + + await until('props.commit() is called', () => commit.calledWith('Something')); + }); + }); + + describe('when props.prepareToCommit() resolves false', () => { + beforeEach(() => { prepareToCommitResolution = false; }); + + it('takes no further action when the commit button is clicked', async () => { + commitButton.dispatchEvent(new MouseEvent('click')); + + await until('focus() is called', () => view.focus.called); + assert.isFalse(commit.called); + }); + + it('takes no further action when github:commit is dispatched', async () => { + commandRegistry.dispatch(workspaceElement, 'github:commit'); + + await until('focus() is called', () => view.focus.called); + assert.isFalse(commit.called); + }); + }); }); it('shows the "Abort Merge" button when props.isMerging is true', async () => { From 31c0fd34f7a8c6262053b37851c555d558f3b425 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 20 Dec 2016 11:53:52 -0500 Subject: [PATCH 15/19] Pass `this.props.prepareToCommit` through the component hierarchy --- lib/controllers/commit-view-controller.js | 1 + lib/views/git-panel-view.js | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js index 995adfdcc6..374aa4ec11 100644 --- a/lib/controllers/commit-view-controller.js +++ b/lib/controllers/commit-view-controller.js @@ -47,6 +47,7 @@ export default class CommitViewController { ref="commitView" stagedChangesExist={this.props.stagedChangesExist} mergeConflictsExist={this.props.mergeConflictsExist} + prepareToCommit={this.props.prepareToCommit} commit={this.commit} setAmending={this.props.setAmending} abortMerge={this.props.abortMerge} diff --git a/lib/views/git-panel-view.js b/lib/views/git-panel-view.js index 64ec419be3..4e74a47d16 100644 --- a/lib/views/git-panel-view.js +++ b/lib/views/git-panel-view.js @@ -68,6 +68,7 @@ export default class GitPanelView { ref="commitViewController" stagedChangesExist={this.props.stagedChanges.length > 0} mergeConflictsExist={this.props.mergeConflicts.length > 0} + prepareToCommit={this.props.prepareToCommit} commit={this.props.commit} amending={this.props.amending} setAmending={this.props.setAmending} From c7215669bad2fc3c0c406b91cc50b88f26d2ed84 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 20 Dec 2016 11:57:18 -0500 Subject: [PATCH 16/19] If the git panel is hidden, show and focus it and abort the commit --- lib/controllers/git-panel-controller.js | 9 ++++- test/controllers/git-panel-controller.test.js | 37 ++++++++++++------- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index f15e4e5a62..7dd9085593 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -14,6 +14,7 @@ export default class GitPanelController { this.unstageFilePatch = this.unstageFilePatch.bind(this); this.attemptFileStageOperation = this.attemptFileStageOperation.bind(this); + this.prepareToCommit = this.prepareToCommit.bind(this); this.commit = this.commit.bind(this); this.setAmending = this.setAmending.bind(this); this.checkout = this.checkout.bind(this); @@ -44,6 +45,7 @@ export default class GitPanelController { stageFilePatch={this.stageFilePatch} unstageFilePatch={this.unstageFilePatch} attemptFileStageOperation={this.attemptFileStageOperation} + prepareToCommit={this.prepareToCommit} commit={this.commit} setAmending={this.setAmending} isAmending={this.props.isAmending} @@ -172,12 +174,15 @@ export default class GitPanelController { } } - async commit(message) { + async prepareToCommit() { if (await this.props.ensureGitPanel()) { this.focus(); - return; + return false; } + return true; + } + async commit(message) { try { await this.getActiveRepository().commit(message, {amend: this.props.isAmending}); this.setAmending(false); diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index d1861a3ad5..54e587e61b 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -8,7 +8,7 @@ import dedent from 'dedent-js'; import GitPanelController from '../../lib/controllers/git-panel-controller'; -import {cloneRepository, buildRepository} from '../helpers'; +import {cloneRepository, buildRepository, until} from '../helpers'; import {AbortMergeError, CommitError} from '../../lib/models/repository'; describe('GitPanelController', () => { @@ -144,30 +144,38 @@ describe('GitPanelController', () => { }); }); - describe('commit(message)', () => { - it('shows the git panel and takes no further action if it was hidden', async () => { + describe('prepareToCommit', () => { + it('shows the git panel and returns false if it was hidden', async () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); - sinon.stub(repository, 'commit', () => Promise.resolve()); const ensureGitPanel = () => Promise.resolve(true); const controller = new GitPanelController({workspace, commandRegistry, repository, ensureGitPanel}); - await controller.commit('message'); + assert.isFalse(await controller.prepareToCommit()); + }); + + it('returns true if the git panel was already visible', async () => { + const workdirPath = await cloneRepository('three-files'); + const repository = await buildRepository(workdirPath); + + const ensureGitPanel = () => Promise.resolve(false); + const controller = new GitPanelController({workspace, commandRegistry, repository, ensureGitPanel}); - assert.equal(repository.commit.callCount, 0); + assert.isTrue(await controller.prepareToCommit()); }); + }); + describe('commit(message)', () => { it('shows an error notification when committing throws an ECONFLICT exception', async () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); - const ensureGitPanel = () => Promise.resolve(false); sinon.stub(repository, 'commit', async () => { await Promise.resolve(); throw new CommitError('ECONFLICT'); }); - const controller = new GitPanelController({workspace, commandRegistry, notificationManager, repository, ensureGitPanel}); + const controller = new GitPanelController({workspace, commandRegistry, notificationManager, repository}); assert.equal(notificationManager.getNotifications().length, 0); await controller.commit(); assert.equal(notificationManager.getNotifications().length, 1); @@ -176,10 +184,9 @@ describe('GitPanelController', () => { it('sets amending to false', async () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); - const ensureGitPanel = () => Promise.resolve(false); sinon.stub(repository, 'commit', () => Promise.resolve()); const didChangeAmending = sinon.stub(); - const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending, ensureGitPanel}); + const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending}); await controller.commit('message'); assert.equal(didChangeAmending.callCount, 1); @@ -315,8 +322,10 @@ describe('GitPanelController', () => { await repository.refresh(); const didChangeAmending = () => {}; + const prepareToCommit = () => Promise.resolve(true); + const ensureGitPanel = () => Promise.resolve(false); - controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending}); + controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending, prepareToCommit, ensureGitPanel}); await controller.getLastModelDataRefreshPromise(); extractReferences(); @@ -328,7 +337,8 @@ describe('GitPanelController', () => { await etch.update(controller); // Ensure that the spy is passed to child components in props commandRegistry.dispatch(workspaceElement, 'github:commit'); - assert.strictEqual(focusElement, commitView); + + await until('CommitView is focused', () => focusElement === commitView); assert.isFalse(controller.commit.called); }); @@ -338,7 +348,8 @@ describe('GitPanelController', () => { await etch.update(controller); // Ensure that the spy is passed to child components in props commandRegistry.dispatch(workspaceElement, 'github:commit'); - assert.isTrue(controller.commit.calledWith('I fixed the things')); + + await until('Commit method called', () => controller.commit.calledWith('I fixed the things')); }); }); }); From 40b492af61ec29534f7fb9af57e834b71700f59f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 20 Dec 2016 12:33:34 -0500 Subject: [PATCH 17/19] Shhh, eslint. Shhh --- test/controllers/git-controller.test.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/controllers/git-controller.test.js b/test/controllers/git-controller.test.js index 2b96cdf1cc..46cce443ff 100644 --- a/test/controllers/git-controller.test.js +++ b/test/controllers/git-controller.test.js @@ -11,7 +11,7 @@ import {cloneRepository, buildRepository} from '../helpers'; import GitController from '../../lib/controllers/git-controller'; describe('GitController', () => { - let atomEnv, workspace, workspaceElement, commandRegistry, notificationManager, app; + let atomEnv, workspace, commandRegistry, notificationManager, app; beforeEach(() => { atomEnv = global.buildAtomEnvironment(); @@ -19,8 +19,6 @@ describe('GitController', () => { commandRegistry = atomEnv.commands; notificationManager = atomEnv.notifications; - workspaceElement = atomEnv.views.getView(workspace); - app = ( Date: Tue, 20 Dec 2016 13:24:09 -0500 Subject: [PATCH 18/19] :fire: Code I thought I'd reverted <_< >_> --- lib/controllers/commit-view-controller.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js index 374aa4ec11..4136ba8904 100644 --- a/lib/controllers/commit-view-controller.js +++ b/lib/controllers/commit-view-controller.js @@ -38,9 +38,6 @@ export default class CommitViewController { render() { const message = this.getCommitMessage(); - if (this.props.onChangeMessage) { - this.props.onChangeMessage(message); - } return ( Date: Tue, 20 Dec 2016 13:49:00 -0500 Subject: [PATCH 19/19] Remove unnecessary .focus() call --- lib/controllers/git-panel-controller.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index 7dd9085593..50a1e3af38 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -175,11 +175,7 @@ export default class GitPanelController { } async prepareToCommit() { - if (await this.props.ensureGitPanel()) { - this.focus(); - return false; - } - return true; + return !await this.props.ensureGitPanel(); } async commit(message) {