From 10a0e4a4df06c28c50949ec9a76302b383c18b2a Mon Sep 17 00:00:00 2001 From: tvand7093 Date: Tue, 21 Nov 2017 00:04:46 -0800 Subject: [PATCH 01/14] Sign-in button is now disabled when token is empty --- lib/popover-component.js | 4 +- lib/sign-in-component.js | 33 +++++++++++++- test/teletype-package.test.js | 82 +++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 3 deletions(-) diff --git a/lib/popover-component.js b/lib/popover-component.js index bee307f1..ad450b8b 100644 --- a/lib/popover-component.js +++ b/lib/popover-component.js @@ -49,7 +49,9 @@ class PopoverComponent { activeComponent = $(SignInComponent, { ref: 'signInComponent', authenticationProvider, - commandRegistry + commandRegistry, + workspace, + notificationManager }) } diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js index 0273b640..2f94c31d 100644 --- a/lib/sign-in-component.js +++ b/lib/sign-in-component.js @@ -14,6 +14,9 @@ class SignInComponent { this.disposables.add(this.props.commandRegistry.add(this.element, { 'core:confirm': this.signIn.bind(this) })) + this.props.workspace.observeTextEditors(editor => { + this.attachChangeEvent() + }) } destroy () { @@ -26,6 +29,10 @@ class SignInComponent { etch.update(this) } + readAfterUpdate () { + this.attachChangeEvent() + } + render () { return $.div({className: 'SignInComponent', tabIndex: -1}, $.span({className: 'SignInComponent-GitHubLogo'}), @@ -56,7 +63,8 @@ class SignInComponent { ref: 'loginButton', type: 'button', className: 'btn btn-primary btn-sm inline-block-tight', - onClick: this.signIn + onClick: this.signIn, + disabled: true }, 'Sign in' ) @@ -70,10 +78,31 @@ class SignInComponent { : null } + attachChangeEvent () { + const previousTokenEditor = this.editor + this.editor = this.refs.editor + + if (!previousTokenEditor && this.editor) { + this.editor.onDidChange(() => { + const token = this.refs.editor.getText().trim() + this.refs.loginButton.disabled = !token + }) + } + } + async signIn () { + const token = this.refs.editor.getText().trim() + + if (!token) { + this.props.notificationManager.addError('Invalid login token', { + description: 'The login token must not be empty. Please insert a valid token and try again.', + dismissable: true + }) + return + } + await this.update({invalidToken: false}) - const token = this.refs.editor.getText() const signedIn = await this.props.authenticationProvider.signIn(token) if (!signedIn) await this.update({invalidToken: true}) } diff --git a/test/teletype-package.test.js b/test/teletype-package.test.js index 8ef6683a..dec990dd 100644 --- a/test/teletype-package.test.js +++ b/test/teletype-package.test.js @@ -955,6 +955,88 @@ suite('TeletypePackage', function () { assert(description.includes('some error')) }) + test('disables button when empty token specified', async () => { + { + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + //this is to trigger the on change event in the token input + const editor = await env.workspace.open() + + await pack.consumeStatusBar(new FakeStatusBar()) + + const {popoverComponent} = pack.portalStatusBarIndicator + + //it should be disabled by default + assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) + } + + { + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + //this is to trigger the on change event in the token input + const editor = await env.workspace.open() + + await pack.consumeStatusBar(new FakeStatusBar()) + + const {popoverComponent} = pack.portalStatusBarIndicator + + // whitespace should also leave the button disabled + popoverComponent.refs.signInComponent.refs.editor.setText(' ') + assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) + } + + { + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + //this is to trigger the on change event in the token input + const editor = await env.workspace.open() + + await pack.consumeStatusBar(new FakeStatusBar()) + + const {popoverComponent} = pack.portalStatusBarIndicator + + //it should be disabled when set to empty + popoverComponent.refs.signInComponent.refs.editor.setText('') + assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) + } + }) + + test('enables button when non-empty token specified', async () => { + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + + //this is to trigger the on change event in the token input + const editor = await env.workspace.open() + + await pack.consumeStatusBar(new FakeStatusBar()) + + const {popoverComponent} = pack.portalStatusBarIndicator + //set empty text first + + popoverComponent.refs.signInComponent.refs.editor.setText('some-token') + + //it should be enabled when not empty + assert(!popoverComponent.refs.signInComponent.refs.loginButton.disabled) + }) + + test('shows empty token error if submitted', async () => { + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + await pack.consumeStatusBar(new FakeStatusBar()) + + const {popoverComponent} = pack.portalStatusBarIndicator + + await popoverComponent.refs.signInComponent.signIn() + + //it should show an error notification about an empty token + assert.equal(env.notifications.getNotifications().length, 1) + const {type, message, options} = env.notifications.getNotifications()[0] + const {description} = options + assert.equal(type, 'error') + assert.equal(message, 'Invalid login token') + assert(description.includes('The login token must not be empty. Please insert a valid token and try again.')) + }) + test('client connection errors', async () => { const env = buildAtomEnvironment() const pack = await buildPackage(env) From ba4b879f5f5ab88e9a8af778c702d148edfb4fd7 Mon Sep 17 00:00:00 2001 From: tvand7093 Date: Tue, 21 Nov 2017 18:15:35 -0800 Subject: [PATCH 02/14] Replaced 4 spaces with 2 as per PR comments. --- test/teletype-package.test.js | 52 +++++++++++++++++------------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/test/teletype-package.test.js b/test/teletype-package.test.js index dec990dd..34e5256f 100644 --- a/test/teletype-package.test.js +++ b/test/teletype-package.test.js @@ -957,47 +957,47 @@ suite('TeletypePackage', function () { test('disables button when empty token specified', async () => { { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - //this is to trigger the on change event in the token input - const editor = await env.workspace.open() + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + //this is to trigger the on change event in the token input + const editor = await env.workspace.open() - await pack.consumeStatusBar(new FakeStatusBar()) + await pack.consumeStatusBar(new FakeStatusBar()) - const {popoverComponent} = pack.portalStatusBarIndicator + const {popoverComponent} = pack.portalStatusBarIndicator - //it should be disabled by default - assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) + //it should be disabled by default + assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) } { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - //this is to trigger the on change event in the token input - const editor = await env.workspace.open() + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + //this is to trigger the on change event in the token input + const editor = await env.workspace.open() - await pack.consumeStatusBar(new FakeStatusBar()) + await pack.consumeStatusBar(new FakeStatusBar()) - const {popoverComponent} = pack.portalStatusBarIndicator + const {popoverComponent} = pack.portalStatusBarIndicator - // whitespace should also leave the button disabled - popoverComponent.refs.signInComponent.refs.editor.setText(' ') - assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) + // whitespace should also leave the button disabled + popoverComponent.refs.signInComponent.refs.editor.setText(' ') + assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) } { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - //this is to trigger the on change event in the token input - const editor = await env.workspace.open() + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + //this is to trigger the on change event in the token input + const editor = await env.workspace.open() - await pack.consumeStatusBar(new FakeStatusBar()) + await pack.consumeStatusBar(new FakeStatusBar()) - const {popoverComponent} = pack.portalStatusBarIndicator + const {popoverComponent} = pack.portalStatusBarIndicator - //it should be disabled when set to empty - popoverComponent.refs.signInComponent.refs.editor.setText('') - assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) + //it should be disabled when set to empty + popoverComponent.refs.signInComponent.refs.editor.setText('') + assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) } }) From 7f04e09f7437c7de3ff607308e5ce0e53db06833 Mon Sep 17 00:00:00 2001 From: tvand7093 Date: Fri, 24 Nov 2017 10:55:06 -0800 Subject: [PATCH 03/14] Fixed linter errors. --- lib/sign-in-component.js | 2 +- test/teletype-package.test.js | 30 +++++++++++++++--------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js index 2f94c31d..fda19fa9 100644 --- a/lib/sign-in-component.js +++ b/lib/sign-in-component.js @@ -92,7 +92,7 @@ class SignInComponent { async signIn () { const token = this.refs.editor.getText().trim() - + if (!token) { this.props.notificationManager.addError('Invalid login token', { description: 'The login token must not be empty. Please insert a valid token and try again.', diff --git a/test/teletype-package.test.js b/test/teletype-package.test.js index e62c0e2b..752b3679 100644 --- a/test/teletype-package.test.js +++ b/test/teletype-package.test.js @@ -959,22 +959,22 @@ suite('TeletypePackage', function () { { const env = buildAtomEnvironment() const pack = await buildPackage(env, {signIn: false}) - //this is to trigger the on change event in the token input - const editor = await env.workspace.open() + // this is to trigger the on change event in the token input + await env.workspace.open() await pack.consumeStatusBar(new FakeStatusBar()) const {popoverComponent} = pack.portalStatusBarIndicator - //it should be disabled by default + // it should be disabled by default assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) } { const env = buildAtomEnvironment() const pack = await buildPackage(env, {signIn: false}) - //this is to trigger the on change event in the token input - const editor = await env.workspace.open() + // this is to trigger the on change event in the token input + await env.workspace.open() await pack.consumeStatusBar(new FakeStatusBar()) @@ -988,14 +988,14 @@ suite('TeletypePackage', function () { { const env = buildAtomEnvironment() const pack = await buildPackage(env, {signIn: false}) - //this is to trigger the on change event in the token input - const editor = await env.workspace.open() + // this is to trigger the on change event in the token input + await env.workspace.open() await pack.consumeStatusBar(new FakeStatusBar()) const {popoverComponent} = pack.portalStatusBarIndicator - //it should be disabled when set to empty + // it should be disabled when set to empty popoverComponent.refs.signInComponent.refs.editor.setText('') assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) } @@ -1005,17 +1005,17 @@ suite('TeletypePackage', function () { const env = buildAtomEnvironment() const pack = await buildPackage(env, {signIn: false}) - //this is to trigger the on change event in the token input - const editor = await env.workspace.open() - + // this is to trigger the on change event in the token input + await env.workspace.open() + await pack.consumeStatusBar(new FakeStatusBar()) const {popoverComponent} = pack.portalStatusBarIndicator - //set empty text first + // set empty text first popoverComponent.refs.signInComponent.refs.editor.setText('some-token') - //it should be enabled when not empty + // it should be enabled when not empty assert(!popoverComponent.refs.signInComponent.refs.loginButton.disabled) }) @@ -1027,8 +1027,8 @@ suite('TeletypePackage', function () { const {popoverComponent} = pack.portalStatusBarIndicator await popoverComponent.refs.signInComponent.signIn() - - //it should show an error notification about an empty token + + // it should show an error notification about an empty token assert.equal(env.notifications.getNotifications().length, 1) const {type, message, options} = env.notifications.getNotifications()[0] const {description} = options From dea240d0b33da5976923d5c1427794d1e6a629c8 Mon Sep 17 00:00:00 2001 From: tvand7093 Date: Fri, 24 Nov 2017 14:56:21 -0800 Subject: [PATCH 04/14] Moved sign-in unit tests to a different file. Also refactored some of the fakes into their own files as well. --- lib/popover-component.js | 4 +- lib/sign-in-component.js | 33 ++---- test/helpers/fake-authentication-provider.js | 32 ++++++ test/helpers/fake-command-registry.js | 17 ++++ test/helpers/fake-notification-manager.js | 14 +++ test/helpers/fake-workspace.js | 14 +++ test/portal-list-component.test.js | 34 +------ test/sign-in-component.test.js | 80 +++++++++++++++ test/teletype-package.test.js | 102 ------------------- 9 files changed, 169 insertions(+), 161 deletions(-) create mode 100644 test/helpers/fake-authentication-provider.js create mode 100644 test/helpers/fake-command-registry.js create mode 100644 test/helpers/fake-notification-manager.js create mode 100644 test/helpers/fake-workspace.js create mode 100644 test/sign-in-component.test.js diff --git a/lib/popover-component.js b/lib/popover-component.js index be5f8cbe..ad216606 100644 --- a/lib/popover-component.js +++ b/lib/popover-component.js @@ -49,9 +49,7 @@ class PopoverComponent { activeComponent = $(SignInComponent, { ref: 'signInComponent', authenticationProvider, - commandRegistry, - workspace, - notificationManager + commandRegistry }) } diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js index fda19fa9..8f9f62ff 100644 --- a/lib/sign-in-component.js +++ b/lib/sign-in-component.js @@ -7,6 +7,11 @@ class SignInComponent { constructor (props) { this.props = props etch.initialize(this) + + this.refs.editor.onDidChange(() => { + const token = this.refs.editor.getText().trim() + this.refs.loginButton.disabled = !token + }) this.disposables = new CompositeDisposable() this.disposables.add(this.props.authenticationProvider.onDidChange(() => { etch.update(this) @@ -14,9 +19,6 @@ class SignInComponent { this.disposables.add(this.props.commandRegistry.add(this.element, { 'core:confirm': this.signIn.bind(this) })) - this.props.workspace.observeTextEditors(editor => { - this.attachChangeEvent() - }) } destroy () { @@ -26,11 +28,7 @@ class SignInComponent { update (props) { Object.assign(this.props, props) - etch.update(this) - } - - readAfterUpdate () { - this.attachChangeEvent() + return etch.update(this) } render () { @@ -74,30 +72,15 @@ class SignInComponent { renderErrorMessage () { return this.props.invalidToken - ? $.p({className: 'error-messages'}, 'That token does not appear to be valid.') + ? $.p({ref: 'errorMessage', className: 'error-messages'}, 'That token does not appear to be valid.') : null } - attachChangeEvent () { - const previousTokenEditor = this.editor - this.editor = this.refs.editor - - if (!previousTokenEditor && this.editor) { - this.editor.onDidChange(() => { - const token = this.refs.editor.getText().trim() - this.refs.loginButton.disabled = !token - }) - } - } - async signIn () { const token = this.refs.editor.getText().trim() if (!token) { - this.props.notificationManager.addError('Invalid login token', { - description: 'The login token must not be empty. Please insert a valid token and try again.', - dismissable: true - }) + await this.update({invalidToken: true}) return } diff --git a/test/helpers/fake-authentication-provider.js b/test/helpers/fake-authentication-provider.js new file mode 100644 index 00000000..88e38b5e --- /dev/null +++ b/test/helpers/fake-authentication-provider.js @@ -0,0 +1,32 @@ +const {Disposable} = require('atom') + +module.exports = +class FakeAuthenticationProvider { + constructor ({shouldFailSignIn, notificationManager}) { + this.shouldFailSignIn = shouldFailSignIn + this.notificationManager = notificationManager + } + + onDidChange (callback) { + return new Disposable(callback) + } + + async signIn (token) { + if (this.shouldFailSignIn) { + this.notificationManager.addError() + } + return !this.shouldFailSignIn + } + + isSigningIn () { + return false + } + + async signOut () { + + } + + dispose () { + + } +} diff --git a/test/helpers/fake-command-registry.js b/test/helpers/fake-command-registry.js new file mode 100644 index 00000000..1ad977f7 --- /dev/null +++ b/test/helpers/fake-command-registry.js @@ -0,0 +1,17 @@ +module.exports = +class FakeCommandRegistry { + constructor () { + this.items = new Set() + } + + add (elem, commands) { + this.items.add({ elem, commands }) + return this + } + + dispose () { + this.items.forEach(({elem, command}) => { + elem.dispose() + }) + } +} diff --git a/test/helpers/fake-notification-manager.js b/test/helpers/fake-notification-manager.js new file mode 100644 index 00000000..a1a30994 --- /dev/null +++ b/test/helpers/fake-notification-manager.js @@ -0,0 +1,14 @@ +module.exports = +class FakeNotificationManager { + constructor () { + this.errorCount = 0 + } + + addInfo () {} + + addSuccess () {} + + addError () { + this.errorCount++ + } +} diff --git a/test/helpers/fake-workspace.js b/test/helpers/fake-workspace.js new file mode 100644 index 00000000..0f0618ee --- /dev/null +++ b/test/helpers/fake-workspace.js @@ -0,0 +1,14 @@ +const {Disposable} = require('atom') + +module.exports = +class FakeWorkspace { + async open () {} + + getElement () { + return document.createElement('div') + } + + observeActiveTextEditor () { + return new Disposable(() => {}) + } +} diff --git a/test/portal-list-component.test.js b/test/portal-list-component.test.js index a666a14c..4594d890 100644 --- a/test/portal-list-component.test.js +++ b/test/portal-list-component.test.js @@ -1,11 +1,13 @@ const assert = require('assert') const condition = require('./helpers/condition') -const {Disposable} = require('atom') const FakeClipboard = require('./helpers/fake-clipboard') const {TeletypeClient} = require('@atom/teletype-client') const {startTestServer} = require('@atom/teletype-server') const PortalBindingManager = require('../lib/portal-binding-manager') const PortalListComponent = require('../lib/portal-list-component') +const FakeNotificationManager = require('./helpers/fake-notification-manager') +const FakeWorkspace = require('./helpers/fake-workspace') +const FakeCommandRegistry = require('./helpers/fake-command-registry') suite('PortalListComponent', function () { if (process.env.CI) this.timeout(process.env.TEST_TIMEOUT_IN_MS) @@ -243,33 +245,3 @@ suite('PortalListComponent', function () { return portalBindingManager } }) - -class FakeWorkspace { - async open () {} - - getElement () { - return document.createElement('div') - } - - observeActiveTextEditor () { - return new Disposable(() => {}) - } -} - -class FakeNotificationManager { - constructor () { - this.errorCount = 0 - } - - addInfo () {} - - addSuccess () {} - - addError () { - this.errorCount++ - } -} - -class FakeCommandRegistry { - add () { return new Set() } -} diff --git a/test/sign-in-component.test.js b/test/sign-in-component.test.js new file mode 100644 index 00000000..662815c4 --- /dev/null +++ b/test/sign-in-component.test.js @@ -0,0 +1,80 @@ +const assert = require('assert') +const FakeAuthenticationProvider = require('./helpers/fake-authentication-provider') +const SignInComponent = require('../lib/sign-in-component') +const FakeNotificationManager = require('./helpers/fake-notification-manager') +const FakeCommandRegistry = require('./helpers/fake-command-registry') + +suite('SignInComponent', function () { + test('has correct fields', () => { + const component = buildComponent() + assert(component.refs.editor) + assert(component.refs.loginButton) + assert(!component.refs.errorMessage) + }) + + test('disables button when empty token specified', () => { + { + const component = buildComponent() + + // it should be disabled by default + assert(component.refs.loginButton.disabled) + } + + { + const component = buildComponent() + + // whitespace should also leave the button disabled + component.refs.editor.setText(' ') + assert(component.refs.loginButton.disabled) + } + + { + const component = buildComponent() + + // it should be disabled when set to empty + component.refs.editor.setText('') + assert(component.refs.loginButton.disabled) + } + }) + + test('enables button when non-empty token specified', () => { + const component = buildComponent() + + component.refs.editor.setText('some-token') + + // it should be enabled when not empty + assert(!component.refs.loginButton.disabled) + }) + + test('reports errors attempting to sign in', async () => { + { + const component = buildComponent() + + component.refs.editor.setText('some-token') + await component.signIn() + + // it should prompt an error message when the login attempt fails + assert.equal(component.props.authenticationProvider.notificationManager.errorCount, 1) + } + + { + const component = buildComponent() + + await component.signIn() + + // it should show an error message about an invalid token + assert(component.refs.errorMessage) + assert.equal(component.refs.errorMessage.innerHTML, 'That token does not appear to be valid.') + } + }) + + function buildComponent () { + return new SignInComponent({ + commandRegistry: new FakeCommandRegistry(), + authenticationProvider: new FakeAuthenticationProvider({ + shouldFailSignIn: true, + notificationManager: new FakeNotificationManager() + }) + }) + } +}) diff --git a/test/teletype-package.test.js b/test/teletype-package.test.js index 752b3679..d3d32226 100644 --- a/test/teletype-package.test.js +++ b/test/teletype-package.test.js @@ -935,108 +935,6 @@ suite('TeletypePackage', function () { } }) - test('reports errors attempting to sign in', async () => { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - await pack.consumeStatusBar(new FakeStatusBar()) - pack.client.signIn = async function () { - throw new Error('some error') - } - - const {popoverComponent} = pack.portalStatusBarIndicator - popoverComponent.refs.signInComponent.refs.editor.setText('some-token') - await popoverComponent.refs.signInComponent.signIn() - - assert.equal(env.notifications.getNotifications().length, 1) - const {type, message, options} = env.notifications.getNotifications()[0] - const {description} = options - assert.equal(type, 'error') - assert.equal(message, 'Failed to authenticate to teletype') - assert(description.includes('some error')) - }) - - test('disables button when empty token specified', async () => { - { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - // this is to trigger the on change event in the token input - await env.workspace.open() - - await pack.consumeStatusBar(new FakeStatusBar()) - - const {popoverComponent} = pack.portalStatusBarIndicator - - // it should be disabled by default - assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) - } - - { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - // this is to trigger the on change event in the token input - await env.workspace.open() - - await pack.consumeStatusBar(new FakeStatusBar()) - - const {popoverComponent} = pack.portalStatusBarIndicator - - // whitespace should also leave the button disabled - popoverComponent.refs.signInComponent.refs.editor.setText(' ') - assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) - } - - { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - // this is to trigger the on change event in the token input - await env.workspace.open() - - await pack.consumeStatusBar(new FakeStatusBar()) - - const {popoverComponent} = pack.portalStatusBarIndicator - - // it should be disabled when set to empty - popoverComponent.refs.signInComponent.refs.editor.setText('') - assert(popoverComponent.refs.signInComponent.refs.loginButton.disabled) - } - }) - - test('enables button when non-empty token specified', async () => { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - - // this is to trigger the on change event in the token input - await env.workspace.open() - - await pack.consumeStatusBar(new FakeStatusBar()) - - const {popoverComponent} = pack.portalStatusBarIndicator - // set empty text first - - popoverComponent.refs.signInComponent.refs.editor.setText('some-token') - - // it should be enabled when not empty - assert(!popoverComponent.refs.signInComponent.refs.loginButton.disabled) - }) - - test('shows empty token error if submitted', async () => { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - await pack.consumeStatusBar(new FakeStatusBar()) - - const {popoverComponent} = pack.portalStatusBarIndicator - - await popoverComponent.refs.signInComponent.signIn() - - // it should show an error notification about an empty token - assert.equal(env.notifications.getNotifications().length, 1) - const {type, message, options} = env.notifications.getNotifications()[0] - const {description} = options - assert.equal(type, 'error') - assert.equal(message, 'Invalid login token') - assert(description.includes('The login token must not be empty. Please insert a valid token and try again.')) - }) - test('client connection errors', async () => { const env = buildAtomEnvironment() const pack = await buildPackage(env) From e79c0bc806678ad65f35884f23550ac61ebb6c1e Mon Sep 17 00:00:00 2001 From: tvand7093 Date: Fri, 24 Nov 2017 15:24:44 -0800 Subject: [PATCH 05/14] Added integration test to ensure sign-in elements are visible. --- test/helpers/fake-authentication-provider.js | 8 ++--- test/sign-in-component.test.js | 8 +++-- test/teletype-package.test.js | 38 ++++++++++++++++++++ 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/test/helpers/fake-authentication-provider.js b/test/helpers/fake-authentication-provider.js index 88e38b5e..5e847dad 100644 --- a/test/helpers/fake-authentication-provider.js +++ b/test/helpers/fake-authentication-provider.js @@ -2,8 +2,7 @@ const {Disposable} = require('atom') module.exports = class FakeAuthenticationProvider { - constructor ({shouldFailSignIn, notificationManager}) { - this.shouldFailSignIn = shouldFailSignIn + constructor ({notificationManager}) { this.notificationManager = notificationManager } @@ -12,10 +11,7 @@ class FakeAuthenticationProvider { } async signIn (token) { - if (this.shouldFailSignIn) { - this.notificationManager.addError() - } - return !this.shouldFailSignIn + return true } isSigningIn () { diff --git a/test/sign-in-component.test.js b/test/sign-in-component.test.js index 662815c4..a48d86a2 100644 --- a/test/sign-in-component.test.js +++ b/test/sign-in-component.test.js @@ -49,12 +49,17 @@ suite('SignInComponent', function () { test('reports errors attempting to sign in', async () => { { const component = buildComponent() + const {authenticationProvider} = component.props + const notifications = authenticationProvider.notificationManager + authenticationProvider.signIn = (token) => { + notifications.addError() + } component.refs.editor.setText('some-token') await component.signIn() // it should prompt an error message when the login attempt fails - assert.equal(component.props.authenticationProvider.notificationManager.errorCount, 1) + assert.equal(notifications.errorCount, 1) } { @@ -72,7 +77,6 @@ suite('SignInComponent', function () { return new SignInComponent({ commandRegistry: new FakeCommandRegistry(), authenticationProvider: new FakeAuthenticationProvider({ - shouldFailSignIn: true, notificationManager: new FakeNotificationManager() }) }) diff --git a/test/teletype-package.test.js b/test/teletype-package.test.js index d3d32226..25aabcf4 100644 --- a/test/teletype-package.test.js +++ b/test/teletype-package.test.js @@ -950,6 +950,44 @@ suite('TeletypePackage', function () { assert(description.includes('connection-error')) }) + test('prompting for a token when signing in', async () => { + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + await pack.consumeStatusBar(new FakeStatusBar()) + + assert(!pack.portalStatusBarIndicator.isPopoverVisible()) + await pack.joinPortal() + assert(pack.portalStatusBarIndicator.isPopoverVisible()) + + const {popoverComponent} = pack.portalStatusBarIndicator + const {signInComponent} = popoverComponent.refs + const {editor, loginButton, errorMessage} = signInComponent.refs + + assert(editor) + assert(loginButton) + assert(!errorMessage) + }) + + test('reports errors attempting to sign in', async () => { + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + await pack.consumeStatusBar(new FakeStatusBar()) + pack.client.signIn = async function () { + throw new Error('some error') + } + + const {popoverComponent} = pack.portalStatusBarIndicator + popoverComponent.refs.signInComponent.refs.editor.setText('some-token') + await popoverComponent.refs.signInComponent.signIn() + + assert.equal(env.notifications.getNotifications().length, 1) + const {type, message, options} = env.notifications.getNotifications()[0] + const {description} = options + assert.equal(type, 'error') + assert.equal(message, 'Failed to authenticate to teletype') + assert(description.includes('some error')) + }) + let nextTokenId = 0 async function buildPackage (env, options = {}) { const credentialCache = new FakeCredentialCache() From 8c1795267c78b956c58ab9e8507b699b46d1ae84 Mon Sep 17 00:00:00 2001 From: tvand7093 Date: Fri, 24 Nov 2017 15:30:36 -0800 Subject: [PATCH 06/14] Added explicit return value to signIn function in test. --- test/sign-in-component.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/sign-in-component.test.js b/test/sign-in-component.test.js index a48d86a2..d0356b25 100644 --- a/test/sign-in-component.test.js +++ b/test/sign-in-component.test.js @@ -54,6 +54,7 @@ suite('SignInComponent', function () { authenticationProvider.signIn = (token) => { notifications.addError() + return false } component.refs.editor.setText('some-token') await component.signIn() From d901437def2ca2d6d889354cc855f70a024a8ceb Mon Sep 17 00:00:00 2001 From: tvand7093 Date: Sun, 26 Nov 2017 12:22:30 -0800 Subject: [PATCH 07/14] Fixed bug where onDidChange no longer fires after login attempt. --- lib/sign-in-component.js | 28 ++++++++++++++++++++++++---- test/teletype-package.test.js | 23 +++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js index 8f9f62ff..bf1eb32b 100644 --- a/lib/sign-in-component.js +++ b/lib/sign-in-component.js @@ -8,10 +8,9 @@ class SignInComponent { this.props = props etch.initialize(this) - this.refs.editor.onDidChange(() => { - const token = this.refs.editor.getText().trim() - this.refs.loginButton.disabled = !token - }) + // We call this here to initially wire up the + // editor for change events. + this.attachChangeEvent() this.disposables = new CompositeDisposable() this.disposables.add(this.props.authenticationProvider.onDidChange(() => { etch.update(this) @@ -31,6 +30,13 @@ class SignInComponent { return etch.update(this) } + readAfterUpdate () { + // Since the sign in process changes the view, + // we need to rewire the onDidChange event on the + // token input field. + this.attachChangeEvent() + } + render () { return $.div({className: 'SignInComponent', tabIndex: -1}, $.span({className: 'SignInComponent-GitHubLogo'}), @@ -89,4 +95,18 @@ class SignInComponent { const signedIn = await this.props.authenticationProvider.signIn(token) if (!signedIn) await this.update({invalidToken: true}) } + + attachChangeEvent () { + // This enables/disables the login button + // based on the current input in the editor. + const previousTokenEditor = this.editor + this.editor = this.refs.editor + + if (!previousTokenEditor && this.editor) { + this.editor.onDidChange(() => { + const token = this.refs.editor.getText().trim() + this.refs.loginButton.disabled = !token + }) + } + } } diff --git a/test/teletype-package.test.js b/test/teletype-package.test.js index 25aabcf4..22f7bc5e 100644 --- a/test/teletype-package.test.js +++ b/test/teletype-package.test.js @@ -988,6 +988,29 @@ suite('TeletypePackage', function () { assert(description.includes('some error')) }) + test('resets editor and button after failed sign-in attempt', async () => { + const env = buildAtomEnvironment() + const pack = await buildPackage(env, {signIn: false}) + await pack.consumeStatusBar(new FakeStatusBar()) + pack.client.signIn = async function () { + throw new Error('some error') + } + + const {popoverComponent} = pack.portalStatusBarIndicator + const {signInComponent} = popoverComponent.refs + + signInComponent.refs.editor.setText('some-token') + + await signInComponent.signIn() + + // TODO: Find out why these refs are not updating. + // by now, the button should be disabled, + // and the text should be reset + assert(signInComponent.refs.loginButton.disabled) + assert(!signInComponent.refs.editor.getText()) + assert(signInComponent.refs.errorMessage) + }) + let nextTokenId = 0 async function buildPackage (env, options = {}) { const credentialCache = new FakeCredentialCache() From b008eca2caa47792c34cfa6fcd231c189c3873e3 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Tue, 12 Dec 2017 10:29:52 -0500 Subject: [PATCH 08/14] Use alternative strategy for disabling sign-in button To eliminate the need to wire up an observer multiple times [1], always render the login element, but set it to `display: none` when the signing in. Also, fix the failing test [2] by clearing the text when sign-in fails. [1]: https://github.com/atom/teletype/pull/248#discussion_r153080880 [2]: https://github.com/atom/teletype/pull/248#discussion_r153080893 --- lib/sign-in-component.js | 59 +++++++++++++++++------------------ test/teletype-package.test.js | 3 -- 2 files changed, 29 insertions(+), 33 deletions(-) diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js index bf1eb32b..0bbecdd8 100644 --- a/lib/sign-in-component.js +++ b/lib/sign-in-component.js @@ -8,9 +8,11 @@ class SignInComponent { this.props = props etch.initialize(this) - // We call this here to initially wire up the - // editor for change events. - this.attachChangeEvent() + this.refs.editor.onDidChange(() => { + const token = this.refs.editor.getText().trim() + this.refs.loginButton.disabled = !token + }) + this.disposables = new CompositeDisposable() this.disposables.add(this.props.authenticationProvider.onDidChange(() => { etch.update(this) @@ -30,29 +32,30 @@ class SignInComponent { return etch.update(this) } - readAfterUpdate () { - // Since the sign in process changes the view, - // we need to rewire the onDidChange event on the - // token input field. - this.attachChangeEvent() - } - render () { return $.div({className: 'SignInComponent', tabIndex: -1}, $.span({className: 'SignInComponent-GitHubLogo'}), $.h3(null, 'Sign in with GitHub'), - this.props.authenticationProvider.isSigningIn() - ? this.renderSigningInIndicator() - : this.renderTokenPrompt() + this.renderSigningInIndicator(), + this.renderTokenPrompt() ) } renderSigningInIndicator () { - return $.span({className: 'loading loading-spinner-tiny inline-block'}) + const element = $.span({}) + + if (this.props.authenticationProvider.isSigningIn()) { + element.props.className = 'loading loading-spinner-tiny inline-block' + } + else { + element.props.display = 'none' + } + + return element } renderTokenPrompt () { - return $.div(null, + const element = $.div({}, $.p(null, 'Visit ', $.a({href: 'https://teletype.atom.io/login', className: 'text-info'}, 'teletype.atom.io/login'), @@ -74,6 +77,12 @@ class SignInComponent { ) ) ) + + if (this.props.authenticationProvider.isSigningIn()) { + element.props.display = 'none' + } + + return element } renderErrorMessage () { @@ -83,7 +92,8 @@ class SignInComponent { } async signIn () { - const token = this.refs.editor.getText().trim() + const {editor} = this.refs + const token = editor.getText().trim() if (!token) { await this.update({invalidToken: true}) @@ -93,20 +103,9 @@ class SignInComponent { await this.update({invalidToken: false}) const signedIn = await this.props.authenticationProvider.signIn(token) - if (!signedIn) await this.update({invalidToken: true}) - } - - attachChangeEvent () { - // This enables/disables the login button - // based on the current input in the editor. - const previousTokenEditor = this.editor - this.editor = this.refs.editor - - if (!previousTokenEditor && this.editor) { - this.editor.onDidChange(() => { - const token = this.refs.editor.getText().trim() - this.refs.loginButton.disabled = !token - }) + if (!signedIn) { + editor.setText('') + await this.update({invalidToken: true}) } } } diff --git a/test/teletype-package.test.js b/test/teletype-package.test.js index 96f9a65f..fc538dd8 100644 --- a/test/teletype-package.test.js +++ b/test/teletype-package.test.js @@ -1162,9 +1162,6 @@ suite('TeletypePackage', function () { await signInComponent.signIn() - // TODO: Find out why these refs are not updating. - // by now, the button should be disabled, - // and the text should be reset assert(signInComponent.refs.loginButton.disabled) assert(!signInComponent.refs.editor.getText()) assert(signInComponent.refs.errorMessage) From fb6cd24c22d6247fad58299aa08bd334581c0cac Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Tue, 12 Dec 2017 10:31:52 -0500 Subject: [PATCH 09/14] :art: --- lib/sign-in-component.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js index 0bbecdd8..7d8ec43a 100644 --- a/lib/sign-in-component.js +++ b/lib/sign-in-component.js @@ -95,16 +95,15 @@ class SignInComponent { const {editor} = this.refs const token = editor.getText().trim() - if (!token) { - await this.update({invalidToken: true}) - return - } - - await this.update({invalidToken: false}) - - const signedIn = await this.props.authenticationProvider.signIn(token) - if (!signedIn) { - editor.setText('') + if (token) { + const signedIn = await this.props.authenticationProvider.signIn(token) + if (signedIn) { + await this.update({invalidToken: false}) + } else { + editor.setText('') + await this.update({invalidToken: true}) + } + } else { await this.update({invalidToken: true}) } } From cbcb72ba1b61884e3c60818b01bc9eca70be7893 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Tue, 12 Dec 2017 10:46:40 -0500 Subject: [PATCH 10/14] Merge a couple integration tests Use the existing integration test to verify that signing in with an invalid token will disable the sign-in button and clear the token text. --- test/teletype-package.test.js | 41 ++--------------------------------- 1 file changed, 2 insertions(+), 39 deletions(-) diff --git a/test/teletype-package.test.js b/test/teletype-package.test.js index fc538dd8..14b997b2 100644 --- a/test/teletype-package.test.js +++ b/test/teletype-package.test.js @@ -267,7 +267,8 @@ suite('TeletypePackage', function () { popoverComponent.refs.signInComponent.signIn() await condition(() => ( popoverComponent.refs.signInComponent.props.invalidToken && - popoverComponent.refs.signInComponent.refs.editor + popoverComponent.refs.signInComponent.refs.editor.getText() === '' && + popoverComponent.refs.signInComponent.refs.loginButton.disabled )) assert.equal(await pack.credentialCache.get('oauth-token'), null) assert(!env.workspace.element.classList.contains('teletype-Authenticated')) @@ -1109,24 +1110,6 @@ suite('TeletypePackage', function () { assert(description.includes('connection-error')) }) - test('prompting for a token when signing in', async () => { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - await pack.consumeStatusBar(new FakeStatusBar()) - - assert(!pack.portalStatusBarIndicator.isPopoverVisible()) - await pack.joinPortal() - assert(pack.portalStatusBarIndicator.isPopoverVisible()) - - const {popoverComponent} = pack.portalStatusBarIndicator - const {signInComponent} = popoverComponent.refs - const {editor, loginButton, errorMessage} = signInComponent.refs - - assert(editor) - assert(loginButton) - assert(!errorMessage) - }) - test('reports errors attempting to sign in', async () => { const env = buildAtomEnvironment() const pack = await buildPackage(env, {signIn: false}) @@ -1147,26 +1130,6 @@ suite('TeletypePackage', function () { assert(description.includes('some error')) }) - test('resets editor and button after failed sign-in attempt', async () => { - const env = buildAtomEnvironment() - const pack = await buildPackage(env, {signIn: false}) - await pack.consumeStatusBar(new FakeStatusBar()) - pack.client.signIn = async function () { - throw new Error('some error') - } - - const {popoverComponent} = pack.portalStatusBarIndicator - const {signInComponent} = popoverComponent.refs - - signInComponent.refs.editor.setText('some-token') - - await signInComponent.signIn() - - assert(signInComponent.refs.loginButton.disabled) - assert(!signInComponent.refs.editor.getText()) - assert(signInComponent.refs.errorMessage) - }) - let nextTokenId = 0 async function buildPackage (env, options = {}) { const credentialCache = new FakeCredentialCache() From 550815f59d7bf0701fb52c42847c26f705e277d1 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Tue, 12 Dec 2017 10:50:49 -0500 Subject: [PATCH 11/14] :art: --- test/sign-in-component.test.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/test/sign-in-component.test.js b/test/sign-in-component.test.js index d0356b25..68b9a587 100644 --- a/test/sign-in-component.test.js +++ b/test/sign-in-component.test.js @@ -14,24 +14,21 @@ suite('SignInComponent', function () { test('disables button when empty token specified', () => { { + // It should be disabled by default const component = buildComponent() - - // it should be disabled by default assert(component.refs.loginButton.disabled) } { + // Whitespace should also leave the button disabled const component = buildComponent() - - // whitespace should also leave the button disabled component.refs.editor.setText(' ') assert(component.refs.loginButton.disabled) } { + // It should be disabled when set to an empty string const component = buildComponent() - - // it should be disabled when set to empty component.refs.editor.setText('') assert(component.refs.loginButton.disabled) } @@ -39,10 +36,7 @@ suite('SignInComponent', function () { test('enables button when non-empty token specified', () => { const component = buildComponent() - component.refs.editor.setText('some-token') - - // it should be enabled when not empty assert(!component.refs.loginButton.disabled) }) @@ -59,7 +53,7 @@ suite('SignInComponent', function () { component.refs.editor.setText('some-token') await component.signIn() - // it should prompt an error message when the login attempt fails + // It should display an error message when the login attempt fails assert.equal(notifications.errorCount, 1) } @@ -68,7 +62,7 @@ suite('SignInComponent', function () { await component.signIn() - // it should show an error message about an invalid token + // It should show an error message about an invalid token assert(component.refs.errorMessage) assert.equal(component.refs.errorMessage.innerHTML, 'That token does not appear to be valid.') } From 72b971047d809a495b5cd1cac0c35baddc0174bb Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Tue, 12 Dec 2017 10:52:32 -0500 Subject: [PATCH 12/14] :art: --- test/helpers/fake-authentication-provider.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test/helpers/fake-authentication-provider.js b/test/helpers/fake-authentication-provider.js index 5e847dad..84394e83 100644 --- a/test/helpers/fake-authentication-provider.js +++ b/test/helpers/fake-authentication-provider.js @@ -18,11 +18,7 @@ class FakeAuthenticationProvider { return false } - async signOut () { + async signOut () {} - } - - dispose () { - - } + dispose () {} } From e56ea1b7220bee928a931ffef934d3d3aff672d5 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Tue, 12 Dec 2017 11:44:34 -0500 Subject: [PATCH 13/14] :art: --- lib/sign-in-component.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js index 7d8ec43a..115d0d5f 100644 --- a/lib/sign-in-component.js +++ b/lib/sign-in-component.js @@ -94,16 +94,12 @@ class SignInComponent { async signIn () { const {editor} = this.refs const token = editor.getText().trim() + const signedIn = token ? await this.props.authenticationProvider.signIn(token) : false - if (token) { - const signedIn = await this.props.authenticationProvider.signIn(token) - if (signedIn) { - await this.update({invalidToken: false}) - } else { - editor.setText('') - await this.update({invalidToken: true}) - } + if (signedIn) { + await this.update({invalidToken: false}) } else { + editor.setText('') await this.update({invalidToken: true}) } } From 09be2169e22353c80bf4047f7e76b685655b2771 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Tue, 12 Dec 2017 11:46:11 -0500 Subject: [PATCH 14/14] Put `display: none` inside a `style` property --- lib/sign-in-component.js | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/sign-in-component.js b/lib/sign-in-component.js index 115d0d5f..5d0f7a37 100644 --- a/lib/sign-in-component.js +++ b/lib/sign-in-component.js @@ -42,20 +42,20 @@ class SignInComponent { } renderSigningInIndicator () { - const element = $.span({}) - + let props = {} if (this.props.authenticationProvider.isSigningIn()) { - element.props.className = 'loading loading-spinner-tiny inline-block' - } - else { - element.props.display = 'none' + props.className = 'loading loading-spinner-tiny inline-block' + } else { + props.style = {display: 'none'} } - return element + return $.span(props) } renderTokenPrompt () { - const element = $.div({}, + const props = this.props.authenticationProvider.isSigningIn() ? {style: {display: 'none'}} : null + + return $.div(props, $.p(null, 'Visit ', $.a({href: 'https://teletype.atom.io/login', className: 'text-info'}, 'teletype.atom.io/login'), @@ -77,12 +77,6 @@ class SignInComponent { ) ) ) - - if (this.props.authenticationProvider.isSigningIn()) { - element.props.display = 'none' - } - - return element } renderErrorMessage () {