fix(401/403): Unify error bars, query /account, improve reconnect flow

Summary: See https://paper.dropbox.com/doc/Sync-disabling-for-N1-URZmjVpSSxWFvjC62TiFI

Test Plan: Tests incoming

Reviewers: juan, evan

Reviewed By: evan

Differential Revision: https://phab.nylas.com/D2959
This commit is contained in:
Ben Gotow 2016-05-13 14:16:54 -07:00
parent 4145a60130
commit 17ed847240
13 changed files with 263 additions and 122 deletions

View file

@ -11,9 +11,18 @@ export default class AccountErrorHeader extends React.Component {
}
componentDidMount() {
this.mounted = true;
this.unsubscribe = AccountStore.listen(() => this._onAccountsChanged());
}
componentWillUnmount() {
this.mounted = false;
if (this.unsubscribe) {
this.unsubscribe();
this.unsubscribe = null;
}
}
getStateFromStores() {
return {accounts: AccountStore.accounts()}
}
@ -22,9 +31,9 @@ export default class AccountErrorHeader extends React.Component {
this.setState(this.getStateFromStores())
}
_reconnect(account) {
_reconnect(existingAccount) {
const ipc = require('electron').ipcRenderer;
ipc.send('command', 'application:add-account', account.provider);
ipc.send('command', 'application:add-account', {existingAccount});
}
_openPreferences() {
@ -37,6 +46,18 @@ export default class AccountErrorHeader extends React.Component {
shell.openExternal("https://support.nylas.com/hc/en-us/requests/new");
}
_onCheckAgain = (event) => {
const errorAccounts = this.state.accounts.filter(a => a.hasSyncStateError());
this.setState({refreshing: true});
event.stopPropagation();
AccountStore.refreshHealthOfAccounts(errorAccounts.map(a => a.id)).finally(() => {
if (!this.mounted) { return; }
this.setState({refreshing: false});
});
}
renderErrorHeader(message, buttonName, actionCallback) {
return (
<div className="account-error-header notifications-sticky">
@ -52,11 +73,15 @@ export default class AccountErrorHeader extends React.Component {
<div className="message">
{message}
</div>
<a className="action refresh" onClick={this._onCheckAgain}>
{this.state.refreshing ? "Checking..." : "Check Again"}
</a>
<a className="action default" onClick={actionCallback}>
{buttonName}
</a>
</div>
</div>)
</div>
)
}
render() {
@ -93,6 +118,6 @@ export default class AccountErrorHeader extends React.Component {
"Open preferences",
() => this._openPreferences());
}
return false;
return <span />;
}
}

View file

@ -0,0 +1,81 @@
import {mount} from 'enzyme';
import AccountErrorHeader from '../lib/headers/account-error-header';
import {AccountStore, Account, Actions, React} from 'nylas-exports'
import {ipcRenderer} from 'electron';
describe("AccountErrorHeader", function AccountErrorHeaderTests() {
describe("when one account is in the `invalid` state", () => {
beforeEach(() => {
spyOn(AccountStore, 'accounts').andReturn([
new Account({id: 'A', syncState: 'invalid', emailAddress: '123@gmail.com'}),
new Account({id: 'B', syncState: 'running', emailAddress: 'other@gmail.com'}),
])
});
it("renders an error bar that mentions the account email", () => {
const header = mount(<AccountErrorHeader />);
expect(header.find('.notifications-sticky-item')).toBeDefined();
expect(header.find('.message').text().indexOf('123@gmail.com') > 0).toBe(true);
});
it("allows the user to refresh the account", () => {
const header = mount(<AccountErrorHeader />);
spyOn(AccountStore, 'refreshHealthOfAccounts').andReturn(Promise.resolve());
header.find('.action.refresh').simulate('click');
expect(AccountStore.refreshHealthOfAccounts).toHaveBeenCalledWith(['A']);
});
it("allows the user to reconnect the account", () => {
const header = mount(<AccountErrorHeader />);
spyOn(ipcRenderer, 'send');
header.find('.action.default').simulate('click');
expect(ipcRenderer.send).toHaveBeenCalledWith('command', 'application:add-account', {
existingAccount: AccountStore.accounts()[0],
});
});
});
describe("when more than one account is in the `invalid` state", () => {
beforeEach(() => {
spyOn(AccountStore, 'accounts').andReturn([
new Account({id: 'A', syncState: 'invalid', emailAddress: '123@gmail.com'}),
new Account({id: 'B', syncState: 'invalid', emailAddress: 'other@gmail.com'}),
])
});
it("renders an error bar", () => {
const header = mount(<AccountErrorHeader />);
expect(header.find('.notifications-sticky-item')).toBeDefined();
});
it("allows the user to refresh the accounts", () => {
const header = mount(<AccountErrorHeader />);
spyOn(AccountStore, 'refreshHealthOfAccounts').andReturn(Promise.resolve());
header.find('.action.refresh').simulate('click');
expect(AccountStore.refreshHealthOfAccounts).toHaveBeenCalledWith(['A', 'B']);
});
it("allows the user to open preferences", () => {
spyOn(Actions, 'switchPreferencesTab')
spyOn(Actions, 'openPreferences')
const header = mount(<AccountErrorHeader />);
header.find('.action.default').simulate('click');
expect(Actions.openPreferences).toHaveBeenCalled();
expect(Actions.switchPreferencesTab).toHaveBeenCalledWith('Accounts');
});
});
describe("when all accounts are fine", () => {
beforeEach(() => {
spyOn(AccountStore, 'accounts').andReturn([
new Account({id: 'A', syncState: 'running', emailAddress: '123@gmail.com'}),
new Account({id: 'B', syncState: 'running', emailAddress: 'other@gmail.com'}),
])
});
it("renders nothing", () => {
const header = mount(<AccountErrorHeader />);
expect(header.html()).toEqual('<span></span>');
});
});
});

View file

@ -10,17 +10,16 @@ url = require 'url'
class AccountChoosePage extends React.Component
@displayName: "AccountChoosePage"
constructor: (@props) ->
@state =
email: ""
provider: ""
componentWillUnmount: ->
@_usub?()
componentDidMount: ->
if @props.pageData.provider
providerData = _.findWhere(Providers, name: @props.pageData.provider)
{existingAccount} = @props.pageData
if existingAccount and not existingAccount.routed
# Hack to prevent coming back to this page from drilling you back in.
# This should all get re-written soon.
existingAccount.routed = true
providerName = existingAccount.provider
providerName = 'exchange' if providerName is 'eas'
providerData = _.findWhere(Providers, {name: providerName})
if providerData
@_onChooseProvider(providerData)
@ -46,16 +45,6 @@ class AccountChoosePage extends React.Component
<span className="provider-name">{provider.displayName}</span>
</div>
_renderError: ->
if @state.error
<div className="alert alert-danger" role="alert">
{@state.error}
</div>
else <div></div>
_onEmailChange: (event) =>
@setState email: event.target.value
_onChooseProvider: (provider) =>
Actions.recordUserEvent('Auth Flow Started', {
provider: provider.name
@ -67,7 +56,7 @@ class AccountChoosePage extends React.Component
_.delay =>
@_onBounceToGmail(provider)
, 600
OnboardingActions.moveToPage("account-settings", {provider})
OnboardingActions.moveToPage("account-settings", Object.assign({provider}, @props.pageData))
_base64url: (buf) ->
# Python-style urlsafe_b64encode

View file

@ -26,6 +26,10 @@ class AccountSettingsPage extends React.Component
if field.default?
@state.settings[field.name] = field.default
if @props.pageData.existingAccount
@state.fields.name = @props.pageData.existingAccount.name
@state.fields.email = @props.pageData.existingAccount.emailAddress
# Special case for gmail. Rather than showing a form, we poll in the
# background for completion of the gmail auth on the server.
if @state.provider.name is 'gmail'
@ -56,6 +60,10 @@ class AccountSettingsPage extends React.Component
)
poll(pollAttemptId,5000)
componentWillUnmount: =>
clearTimeout(@_resizeTimer) if @_resizeTimer
@_resizeTimer = null
render: ->
<div className="page account-setup">
<div className="logo-container">
@ -67,7 +75,7 @@ class AccountSettingsPage extends React.Component
{@_renderTitle()}
<div className="back" onClick={@_fireMoveToPrevPage}>
<div className="back" onClick={@_onMoveToPrevPage}>
<RetinaImg
name="onboarding-back.png"
mode={RetinaImg.Mode.ContentPreserve}/>
@ -447,11 +455,12 @@ class AccountSettingsPage extends React.Component
callback()
_resize: =>
setTimeout( =>
clearTimeout(@_resizeTimer) if @_resizeTimer
@_resizeTimer = setTimeout( =>
@props.onResize?()
, 10)
_fireMoveToPrevPage: =>
_onMoveToPrevPage: =>
if @state.pageNumber > 0
@setState(pageNumber: @state.pageNumber - 1)
@_resize()

View file

@ -119,13 +119,13 @@ Providers = [
}
]
settings: [{
name: 'password'
type: 'password'
placeholder: 'Password'
label: 'Password'
required: true
page: 0
}]
name: 'password'
type: 'password'
placeholder: 'Password'
label: 'Password'
required: true
page: 0
}]
}, {
name: 'yahoo'
displayName: 'Yahoo'
@ -158,7 +158,7 @@ Providers = [
required: true
}]
}, {
name: 'imap'
name: 'custom'
displayName: 'IMAP / SMTP Setup'
icon: 'ic-settings-account-imap.png'
header_icon: 'setup-icon-provider-imap.png'
@ -260,37 +260,6 @@ Providers = [
required: true
}
]
# }, {
# name: 'default'
# displayName: ''
# icon: ''
# color: ''
# fields: [
# {
# name: 'name'
# type: 'text'
# placeholder: 'Ashton Letterman'
# label: 'Name'
# }, {
# name: 'email'
# type: 'text'
# placeholder: 'you@example.com'
# label: 'Email'
# }
# ]
# settings: [
# {
# name: 'username'
# type: 'text'
# placeholder: 'Username'
# label: 'Username'
# }, {
# name: 'password'
# type: 'password'
# placeholder: 'Password'
# label: 'Password'
# }
# ]
}
]

View file

@ -1,5 +1,4 @@
/* eslint global-require: 0 */
import _ from 'underscore';
import React, {Component, PropTypes} from 'react';
import {EditableList, NewsletterSignup} from 'nylas-component-kit';
import {RegExpUtils, Account} from 'nylas-exports';
@ -57,8 +56,8 @@ class PreferencesAccountDetails extends Component {
};
_setState = (updates, callback = () => {}) => {
const updated = _.extend({}, this.state.account, updates);
this.setState({account: updated}, callback);
const account = Object.assign(this.state.account.clone(), updates);
this.setState({account}, callback);
};
_setStateAndSave = (updates) => {
@ -106,12 +105,12 @@ class PreferencesAccountDetails extends Component {
this._setStateAndSave({defaultAlias});
};
_reconnect() {
const {account} = this.state;
_onReconnect = () => {
const ipc = require('electron').ipcRenderer;
ipc.send('command', 'application:add-account', account.provider);
ipc.send('command', 'application:add-account', {existingAccount: this.state.account});
}
_contactSupport() {
_onContactSupport = () => {
const {shell} = require("electron");
shell.openExternal("https://support.nylas.com/hc/en-us/requests/new");
}
@ -152,17 +151,17 @@ class PreferencesAccountDetails extends Component {
`Nylas N1 can no longer authenticate with ${account.emailAddress}. The password or
authentication may have changed.`,
"Reconnect",
() => this._reconnect());
this._onReconnect);
case Account.SYNC_STATE_STOPPED:
return this._renderErrorDetail(
`The cloud sync for ${account.emailAddress} has been disabled. Please contact Nylas support.`,
"Contact support",
() => this._contactSupport());
this._onContactSupport);
default:
return this._renderErrorDetail(
`Nylas encountered an error while syncing mail for ${account.emailAddress}. Contact Nylas support for details.`,
"Contact support",
() => this._contactSupport());
this._onContactSupport);
}
}
return null;
@ -176,7 +175,7 @@ class PreferencesAccountDetails extends Component {
return (
<div className="account-details">
{this._renderSyncErrorDetails(account)}
{this._renderSyncErrorDetails()}
<h3>Account Label</h3>
<input
type="text"
@ -185,6 +184,12 @@ class PreferencesAccountDetails extends Component {
onChange={this._onAccountLabelUpdated}
/>
<h3>Account Settings</h3>
<div className="btn" onClick={this._onReconnect}>
{account.provider === 'gmail' ? 'Re-authenticate with Gmail...' : 'Update Connection Settings...'}
</div>
<h3>Aliases</h3>
<div className="platform-note">
@ -207,7 +212,6 @@ class PreferencesAccountDetails extends Component {
<NewsletterSignup emailAddress={account.emailAddress} name={account.name} />
</div>
</div>
);
}

View file

@ -62,17 +62,16 @@ class PreferencesAccountList extends Component {
return <div className="account-list"></div>;
}
return (
<div className="account-list">
<EditableList
items={this.props.accounts}
itemContent={this._renderAccount}
selected={this.props.selected}
onReorderItem={this.props.onReorderAccount}
onCreateItem={this.props.onAddAccount}
onSelectItem={this.props.onSelectAccount}
onDeleteItem={this.props.onRemoveAccount}
/>
</div>
<EditableList
className="account-list"
items={this.props.accounts}
itemContent={this._renderAccount}
selected={this.props.selected}
onReorderItem={this.props.onReorderAccount}
onCreateItem={this.props.onAddAccount}
onSelectItem={this.props.onSelectAccount}
onDeleteItem={this.props.onRemoveAccount}
/>
);
}

View file

@ -11,10 +11,13 @@
justify-content: center;
.account-list {
display: flex;
flex-direction: column;
height: auto;
width: 400px;
.items-wrapper {
height: 440px;
flex: 1;
}
.account {
@ -49,7 +52,7 @@
.account-details {
width: 400px;
padding-top: 20px;
padding: 20px;
padding-left: @spacing-standard * 2.25;
padding-right: @spacing-standard * 2.25;
background-color: @gray-lighter;

View file

@ -152,18 +152,17 @@ describe "NylasAPI", ->
expect(DatabaseTransaction.prototype.unpersistModel).not.toHaveBeenCalled()
describe "handleAuthenticationFailure", ->
it "should post a notification", ->
spyOn(Actions, 'postNotification')
NylasAPI._handleAuthenticationFailure('/threads/1234', 'token')
expect(Actions.postNotification).toHaveBeenCalled()
expect(Actions.postNotification.mostRecentCall.args[0].message.trim()).toEqual("A mailbox action for your mail provider could not be completed. You may not be able to send or receive mail.")
it "should include the email address if possible", ->
it "should put the account in an `invalid` state", ->
spyOn(Actions, 'updateAccount')
spyOn(AccountStore, 'tokenForAccountId').andReturn('token')
spyOn(Actions, 'postNotification')
NylasAPI._handleAuthenticationFailure('/threads/1234', 'token')
expect(Actions.postNotification).toHaveBeenCalled()
expect(Actions.postNotification.mostRecentCall.args[0].message.trim()).toEqual("A mailbox action for #{AccountStore.accounts()[0].emailAddress} could not be completed. You may not be able to send or receive mail.")
expect(Actions.updateAccount).toHaveBeenCalled()
expect(Actions.updateAccount.mostRecentCall.args).toEqual([AccountStore.accounts()[0].id, {syncState: 'invalid'}])
it "should not throw an exception if the account cannot be found", ->
spyOn(Actions, 'updateAccount')
NylasAPI._handleAuthenticationFailure('/threads/1234', 'token')
expect(Actions.updateAccount).not.toHaveBeenCalled()
describe "handleModelResponse", ->
beforeEach ->

View file

@ -1,5 +1,6 @@
_ = require 'underscore'
keytar = require 'keytar'
NylasAPI = require '../../src/flux/nylas-api'
AccountStore = require '../../src/flux/stores/account-store'
Account = require('../../src/flux/models/account').default
Actions = require '../../src/flux/actions'
@ -38,6 +39,7 @@ describe "AccountStore", ->
}]
spyOn(NylasEnv.config, 'get').andCallFake (key) =>
return 'production' if key is 'env'
return @configAccounts if key is 'nylas.accounts'
return @configVersion if key is 'nylas.accountsVersion'
return @configTokens if key is 'nylas.accountTokens'
@ -77,6 +79,21 @@ describe "AccountStore", ->
expect(@instance.tokenForAccountId('A')).toEqual('A-TOKEN')
expect(@instance.tokenForAccountId('B')).toEqual('B-TOKEN')
describe "in the work window and running on production", ->
it "should refresh the accounts", ->
spyOn(NylasEnv, 'isWorkWindow').andReturn(true)
@instance = new @constructor
spyOn(@instance, 'refreshHealthOfAccounts')
advanceClock(10000)
expect(@instance.refreshHealthOfAccounts).toHaveBeenCalledWith(['A', 'B'])
describe "in the main window", ->
it "should not refresh the accounts", ->
@instance = new @constructor
spyOn(@instance, 'refreshHealthOfAccounts')
advanceClock(10000)
expect(@instance.refreshHealthOfAccounts).not.toHaveBeenCalled()
describe "accountForEmail", ->
beforeEach ->
@instance = new @constructor
@ -164,3 +181,45 @@ describe "AccountStore", ->
expect(@instance._accounts.length).toBe 2
expect(@instance.accountForId('B')).toBe(undefined)
expect(@instance.accountForId('NEVER SEEN BEFORE')).not.toBe(undefined)
describe "refreshHealthOfAccounts", ->
beforeEach ->
@spyOnConfig()
spyOn(NylasAPI, 'makeRequest').andCallFake (options) =>
if options.accountId is 'return-api-error'
Promise.reject(new Error("API ERROR"))
else
Promise.resolve({
sync_state: 'running',
id: options.accountId,
account_id: options.accountId
})
@instance = new @constructor
spyOn(@instance, '_save')
it "should GET /account for each of the provided account IDs", ->
@instance.refreshHealthOfAccounts(['A', 'B'])
expect(NylasAPI.makeRequest.callCount).toBe(2)
expect(NylasAPI.makeRequest.calls[0].args).toEqual([{path: '/account', accountId: 'A'}])
expect(NylasAPI.makeRequest.calls[1].args).toEqual([{path: '/account', accountId: 'B'}])
it "should update existing account objects and call save exactly once", ->
@instance.accountForId('A').syncState = 'invalid'
@instance.refreshHealthOfAccounts(['A', 'B'])
advanceClock()
expect(@instance.accountForId('A').syncState).toEqual('running')
expect(@instance._save.callCount).toBe(1)
it "should ignore accountIds which do not exist locally when the request completes", ->
@instance.accountForId('A').syncState = 'invalid'
@instance.refreshHealthOfAccounts(['gone', 'A', 'B'])
advanceClock()
expect(@instance.accountForId('A').syncState).toEqual('running')
expect(@instance._save.callCount).toBe(1)
it "should not stop if a single GET /account fails", ->
@instance.accountForId('B').syncState = 'invalid'
@instance.refreshHealthOfAccounts(['return-api-error', 'B']).catch (e) =>
advanceClock()
expect(@instance.accountForId('B').syncState).toEqual('running')
expect(@instance._save.callCount).toBe(1)

View file

@ -300,12 +300,12 @@ export default class Application extends EventEmitter {
win.browserWindow.inspectElement(x, y);
});
this.on('application:add-account', (provider) => {
this.on('application:add-account', ({existingAccount} = {}) => {
this.windowManager.ensureWindow(WindowManager.ONBOARDING_WINDOW, {
title: "Add an Account",
windowProps: {
page: "account-choose",
pageData: {provider},
pageData: {existingAccount},
},
})
});

View file

@ -250,24 +250,8 @@ class NylasAPI
account = AccountStore.accounts().find (account) ->
AccountStore.tokenForAccountId(account.id) is apiToken
email = "your mail provider"
email = account.emailAddress if account
Actions.postNotification
type: 'error'
tag: '401'
sticky: true
message: "A mailbox action for #{email} could not be completed. You
may not be able to send or receive mail.",
icon: 'fa-sign-out'
actions: [{
default: true
dismisses: true
label: 'Dismiss'
provider: account?.provider ? ""
id: '401:dismiss'
}]
if account
Actions.updateAccount(account.id, {syncState: Account.SYNC_STATE_AUTH_FAILED})
return Promise.resolve()
# Returns a Promise that resolves when any parsed out models (if any)

View file

@ -5,6 +5,7 @@ Account = require('../models/account').default
Utils = require '../models/utils'
DatabaseStore = require './database-store'
keytar = require 'keytar'
NylasAPI = null
configAccountsKey = "nylas.accounts"
configVersionKey = "nylas.accountsVersion"
@ -25,6 +26,11 @@ class AccountStore extends NylasStore
@listenTo Actions.updateAccount, @_onUpdateAccount
@listenTo Actions.reorderAccount, @_onReorderAccount
if NylasEnv.isWorkWindow() and ['staging', 'production'].includes(NylasEnv.config.get('env'))
setTimeout( =>
@refreshHealthOfAccounts(@_accounts.map((a) -> a.id))
, 2000)
NylasEnv.config.onDidChange configVersionKey, (change) =>
# If we already have this version of the accounts config, it means we
# are the ones who saved the change, and we don't need to reload.
@ -168,6 +174,20 @@ class AccountStore extends NylasStore
@_save()
Actions.focusDefaultMailboxPerspectiveForAccounts([account.id])
refreshHealthOfAccounts: (accountIds) =>
NylasAPI ?= require '../nylas-api'
Promise.all(accountIds.map (accountId) =>
return NylasAPI.makeRequest({
path: '/account',
accountId: accountId,
}).then (json) =>
existing = @accountForId(accountId)
return unless existing # user may have deleted
existing.fromJSON(json)
).finally =>
@_caches = {}
@_save()
# Exposed Data
# Private: Helper which caches the results of getter functions often needed