Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(app): Track restart status in discovery state for better alerts #2639

Merged
merged 4 commits into from
Nov 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions app/src/components/RobotSettings/ConnectBanner.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,15 @@ export default class ConnectBanner extends React.Component<Robot, State> {
}

render () {
const {name, connected} = this.props
const {displayName, connected} = this.props
const isVisible = connected && !this.state.dismissed
const TITLE = `${name} successfully connected`

if (!isVisible) return null

return (
<AlertItem
type="success"
onCloseClick={() => this.setState({dismissed: true})}
title={TITLE}
title={`${displayName} successfully connected`}
/>
)
}
Expand Down
38 changes: 28 additions & 10 deletions app/src/components/RobotSettings/ReachableRobotBanner.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,21 @@ import type {ReachableRobot} from '../../discovery'

type State = {dismissed: boolean}

const TITLE = 'Unable to establish connection with robot'
const UNRESPONSIVE_TITLE = 'Unable to establish connection with robot'
const RESTARTING_TITLE = 'Robot restarting'

const LAST_RESORT = (
<p>
If your robot remains unresponsive, please reach out to our Support team.
</p>
)

const RESTARTING_MESSAGE = (
<div className={styles.banner}>
<p>Your robot is restarting, and should be back online shortly.</p>
{LAST_RESORT}
</div>
)

const NO_SERVER_MESSAGE = (
<div className={styles.banner}>
Expand All @@ -16,9 +30,7 @@ const NO_SERVER_MESSAGE = (
requests.
</p>
<p>We recommend power-cycling your robot.</p>
<p>
If your robot remains unresponsive, please reach out to our Support team.
</p>
{LAST_RESORT}
</div>
)

Expand All @@ -40,9 +52,7 @@ const SERVER_MESSAGE = (
</p>
</li>
</ol>
<p>
If your robot remains unresponsive, please reach out to our Support team.
</p>
{LAST_RESORT}
</div>
)

Expand All @@ -56,17 +66,25 @@ export default class ReachableRobotBanner extends React.Component<
}

render () {
const {serverOk} = this.props
const {serverOk, restartStatus} = this.props
const isVisible = !this.state.dismissed
const message = serverOk ? SERVER_MESSAGE : NO_SERVER_MESSAGE
let title = ''
let message = ''
if (restartStatus) {
title = RESTARTING_TITLE
message = RESTARTING_MESSAGE
} else {
title = UNRESPONSIVE_TITLE
message = serverOk ? SERVER_MESSAGE : NO_SERVER_MESSAGE
}

if (!isVisible) return null

return (
<AlertItem
type="warning"
onCloseClick={() => this.setState({dismissed: true})}
title={TITLE}
title={title}
>
{message}
</AlertItem>
Expand Down
11 changes: 11 additions & 0 deletions app/src/discovery/__tests__/actions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,15 @@ describe('discovery actions', () => {
jest.runTimersToTime(expectedTimeout)
expect(store.getActions()).toEqual([expectedStart, expectedFinish])
})

test('startDiscovery with timeout', () => {
const expectedTimeout = 60000
const expectedStart = {type: 'discovery:START', meta: {shell: true}}
const expectedFinish = {type: 'discovery:FINISH', meta: {shell: true}}

store.dispatch(startDiscovery(60000))
expect(store.getActions()).toEqual([expectedStart])
jest.runTimersToTime(expectedTimeout)
expect(store.getActions()).toEqual([expectedStart, expectedFinish])
})
})
49 changes: 46 additions & 3 deletions app/src/discovery/__tests__/reducer.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
import {discoveryReducer} from '..'

jest.mock('../../shell', () => ({
getShellRobots: () => ([
getShellRobots: () => [
{name: 'foo', ip: '192.168.1.1', port: 31950},
{name: 'bar', ip: '192.168.1.2', port: 31950},
]),
],
}))

describe('discoveryReducer', () => {
Expand All @@ -20,6 +20,7 @@ describe('discoveryReducer', () => {
initialState: undefined,
expectedState: {
scanning: false,
restartsByName: {},
robotsByName: {
foo: [{name: 'foo', ip: '192.168.1.1', port: 31950}],
bar: [{name: 'bar', ip: '192.168.1.2', port: 31950}],
Expand Down Expand Up @@ -49,12 +50,54 @@ describe('discoveryReducer', () => {
],
},
},
initialState: {robotsByName: {}},
initialState: {robotsByName: {}, restartsByName: {}},
expectedState: {
robotsByName: {
foo: [{name: 'foo', ip: '192.168.1.1', port: 31950}],
bar: [{name: 'bar', ip: '192.168.1.2', port: 31950}],
},
restartsByName: {},
},
},
{
name: 'api:SERVER_SUCCESS sets restart pending',
action: {
type: 'api:SERVER_SUCCESS',
payload: {path: 'restart', robot: {name: 'name'}},
},
initialState: {restartsByName: {}},
expectedState: {restartsByName: {name: 'pending'}},
},
{
name: 'discovery:UPDATE_LIST sets restart down if pending robot not ok',
action: {
type: 'discovery:UPDATE_LIST',
payload: {
robots: [{name: 'name', ip: '192.168.1.1', port: 31950, ok: false}],
},
},
initialState: {restartsByName: {name: 'pending'}},
expectedState: {
robotsByName: {
name: [{name: 'name', ip: '192.168.1.1', port: 31950, ok: false}],
},
restartsByName: {name: 'down'},
},
},
{
name: 'discovery:UPDATE_LIST clears restart if down robot ok',
action: {
type: 'discovery:UPDATE_LIST',
payload: {
robots: [{name: 'name', ip: '192.168.1.1', port: 31950, ok: true}],
},
},
initialState: {restartsByName: {name: 'down'}},
expectedState: {
robotsByName: {
name: [{name: 'name', ip: '192.168.1.1', port: 31950, ok: true}],
},
restartsByName: {name: null},
},
},
]
Expand Down
91 changes: 76 additions & 15 deletions app/src/discovery/__tests__/selectors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ const makeFullyUp = (
ip,
status = null,
connected = null,
displayName = null
displayName = null,
restartStatus = null
) => ({
name,
ip,
Expand All @@ -19,14 +20,16 @@ const makeFullyUp = (
status,
connected,
displayName,
restartStatus,
})

const makeConnectable = (
name,
ip,
status = null,
connected = null,
displayName = null
displayName = null,
restartStatus = null
) => ({
name,
ip,
Expand All @@ -37,9 +40,16 @@ const makeConnectable = (
status,
connected,
displayName,
restartStatus,
})

const makeAdvertising = (name, ip, status = null, displayName = null) => ({
const makeAdvertising = (
name,
ip,
status = null,
displayName = null,
restartStatus = null
) => ({
name,
ip,
local: false,
Expand All @@ -48,14 +58,16 @@ const makeAdvertising = (name, ip, status = null, displayName = null) => ({
advertising: true,
status,
displayName,
restartStatus,
})

const makeServerUp = (
name,
ip,
advertising,
status = null,
displayName = null
displayName = null,
restartStatus = null
) => ({
name,
ip,
Expand All @@ -66,9 +78,16 @@ const makeServerUp = (
serverHealth: {},
status,
displayName,
restartStatus,
})

const makeUnreachable = (name, ip, status = null, displayName = null) => ({
const makeUnreachable = (
name,
ip,
status = null,
displayName = null,
restartStatus = null
) => ({
name,
ip,
local: false,
Expand All @@ -77,6 +96,7 @@ const makeUnreachable = (name, ip, status = null, displayName = null) => ({
advertising: false,
status,
displayName,
restartStatus,
})

describe('discovery selectors', () => {
Expand Down Expand Up @@ -130,6 +150,20 @@ describe('discovery selectors', () => {
makeConnectable('foo', '10.0.0.1', 'connectable', true, 'foo'),
],
},
{
name: 'getConnectableRobots adds restartStatus if it exists',
state: {
discovery: {
robotsByName: {foo: [makeFullyUp('foo', '10.0.0.2')]},
restartsByName: {foo: 'pending'},
},
robot: {connection: {connectedTo: 'foo'}},
},
selector: discovery.getConnectableRobots,
expected: [
makeFullyUp('foo', '10.0.0.2', 'connectable', true, 'foo', 'pending'),
],
},
{
name: 'getReachableRobots grabs robots with serverUp or advertising',
selector: discovery.getReachableRobots,
Expand Down Expand Up @@ -186,6 +220,19 @@ describe('discovery selectors', () => {
},
expected: [],
},
{
name: 'getReachableRobots adds restartStatus if it exists',
state: {
discovery: {
robotsByName: {foo: [makeServerUp('foo', '10.0.0.1', false)]},
restartsByName: {foo: 'down'},
},
},
selector: discovery.getReachableRobots,
expected: [
makeServerUp('foo', '10.0.0.1', false, 'reachable', 'foo', 'down'),
],
},
{
name: 'getUnreachableRobots grabs robots with no ip',
selector: discovery.getUnreachableRobots,
Expand All @@ -195,7 +242,13 @@ describe('discovery selectors', () => {
},
},
expected: [
{name: 'foo', ip: null, status: 'unreachable', displayName: 'foo'},
{
name: 'foo',
ip: null,
status: 'unreachable',
displayName: 'foo',
restartStatus: null,
},
],
},
{
Expand Down Expand Up @@ -237,6 +290,19 @@ describe('discovery selectors', () => {
},
expected: [],
},
{
name: 'getUnreachableRobots adds restartStatus if it exists',
state: {
discovery: {
robotsByName: {foo: [makeUnreachable('foo', '10.0.0.1')]},
restartsByName: {foo: 'down'},
},
},
selector: discovery.getUnreachableRobots,
expected: [
makeUnreachable('foo', '10.0.0.1', 'unreachable', 'foo', 'down'),
],
},
{
name: 'display name removes opentrons- from connectable robot names',
selector: discovery.getConnectableRobots,
Expand Down Expand Up @@ -281,17 +347,12 @@ describe('discovery selectors', () => {
selector: discovery.getUnreachableRobots,
state: {
discovery: {
robotsByName: {'opentrons-foo': [{name: 'opentrons-foo', ip: null}]},
robotsByName: {
'opentrons-foo': [makeUnreachable('opentrons-foo', null)],
},
},
},
expected: [
{
name: 'opentrons-foo',
ip: null,
status: 'unreachable',
displayName: 'foo',
},
],
expected: [makeUnreachable('opentrons-foo', null, 'unreachable', 'foo')],
},
{
name: 'getAllRobots returns all robots',
Expand Down
Loading