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.

Sign up for GitHub

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

fix(app): properly enable/disable robot update from file button #6483

Merged
merged 8 commits into from
Sep 14, 2020
Merged

fix(app): properly enable/disable robot update from file button #6483

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
011a35b
fix(app): properly enable/disable robot update from file button
mcous Sep 3, 2020
b421743
fixup: add some missing changes
mcous Sep 3, 2020
544a8ff
Merge branch 'edge' into app_update-from-file-tooltip
mcous Sep 8, 2020
65b3d19
fixup: revert some selector changes and tweak copy
mcous Sep 8, 2020
18c7fe7
Merge branch 'edge' into app_update-from-file-tooltip
mcous Sep 9, 2020
95ad791
fixup: remove console.log
mcous Sep 10, 2020
3fed4cd
fixup: refactor buildroot selector tests for correctness and readability
mcous Sep 14, 2020
9492dff
fixup: tweak update type selector test names
mcous Sep 14, 2020
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
125 changes: 113 additions & 12 deletions app/src/buildroot/__tests__/selectors.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import * as selectors from '../selectors'
import { mockReachableRobot } from '../../discovery/__fixtures__'
import {
HEALTH_STATUS_NOT_OK,
getViewableRobots,
getRobotApiVersion,
} from '../../discovery/selectors'
getRobotApiVersionByName,
getRobotByName,
} from '../../discovery'

jest.mock('../../discovery/selectors')

describe('buildroot selectors', () => {
beforeEach(() => {
getViewableRobots.mockReturnValue([])
getRobotApiVersionByName.mockReturnValue(null)
getRobotByName.mockReturnValue(null)
})

afterEach(() => {
jest.clearAllMocks()
jest.resetAllMocks()
})

const SPECS = [
Expand Down Expand Up @@ -81,10 +90,10 @@ describe('buildroot selectors', () => {
version: '1.0.0',
},
},
args: [{ name: 'robot-name' }],
args: ['robot-name'],
expected: 'upgrade',
setup: () => {
getRobotApiVersion.mockReturnValueOnce('0.9.9')
getRobotApiVersionByName.mockReturnValue('0.9.9')
},
},
{
Expand All @@ -98,7 +107,7 @@ describe('buildroot selectors', () => {
args: [{ name: 'robot-name' }],
expected: 'downgrade',
setup: () => {
getRobotApiVersion.mockReturnValueOnce('1.0.1')
getRobotApiVersionByName.mockReturnValue('1.0.1')
},
},
{
Expand All @@ -112,7 +121,7 @@ describe('buildroot selectors', () => {
args: [{ name: 'robot-name' }],
expected: 'reinstall',
setup: () => {
getRobotApiVersion.mockReturnValueOnce('1.0.0')
getRobotApiVersionByName.mockReturnValue('1.0.0')
},
},
{
Expand All @@ -126,7 +135,7 @@ describe('buildroot selectors', () => {
args: [{ name: 'robot-name' }],
expected: null,
setup: () => {
getRobotApiVersion.mockReturnValueOnce('1.0.0')
getRobotApiVersionByName.mockReturnValue('1.0.0')
},
},
{
Expand Down Expand Up @@ -159,7 +168,7 @@ describe('buildroot selectors', () => {
},
expected: { name: 'robot-name', host: '10.10.0.0', port: 31950 },
setup: () =>
getViewableRobots.mockReturnValueOnce([
getViewableRobots.mockReturnValue([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come you switched mockReturnValueOnce to mockReturnValue?

Copy link
Contributor Author

@mcous mcous Sep 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mockReturnValue is more accurate to how the selectors would actually behave; a selector (if it chose to) might call some selector it is dependent on multiple times with the same input, and you would expect the same output.

These tests weren't very well written (by me) with respect to mocks, so I've refactored to be more clear

{ name: 'other-robot-name', host: '10.10.0.1', port: 31950 },
{ name: 'robot-name', host: '10.10.0.0', port: 31950 },
{ name: 'another-robot-name', host: '10.10.0.2', port: 31950 },
Expand All @@ -180,7 +189,7 @@ describe('buildroot selectors', () => {
serverHealth: { capabilities: { buildrootUpdate: '/' } },
},
setup: () =>
getViewableRobots.mockReturnValueOnce([
getViewableRobots.mockReturnValue([
{ name: 'other-robot-name', host: '10.10.0.1', port: 31950 },
{
name: 'robot-name',
Expand All @@ -191,12 +200,104 @@ describe('buildroot selectors', () => {
{ name: 'another-robot-name', host: '10.10.0.2', port: 31950 },
]),
},
{
name: 'getBuildrootUpdateDisplayInfo returns not responding if no robot',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: {} },
setup: () => {
getRobotByName.mockReturnValue(null)
},
expected: expect.objectContaining({
autoUpdateDisabledReason: expect.stringMatching(
/update server is not responding/
),
updateFromFileDisabledReason: expect.stringMatching(
/update server is not responding/
),
}),
},
{
name:
'getBuildrootUpdateDisplayInfo returns not responding if robot has unhealthy update server',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: {} },
setup: () => {
getRobotByName.mockReturnValue({
...mockReachableRobot,
serverHealthStatus: HEALTH_STATUS_NOT_OK,
})
},
expected: expect.objectContaining({
autoUpdateDisabledReason: expect.stringMatching(
/update server is not responding/
),
updateFromFileDisabledReason: expect.stringMatching(
/update server is not responding/
),
}),
},
{
name:
'getBuildrootUpdateDisplayInfo returns not allowed if another robot is updating',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: { session: { robotName: 'other-robot-name' } } },
setup: () => {
getRobotByName.mockImplementation((state, name) => {
return { ...mockReachableRobot, name }
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we overwrite the name of the robot here? When I console.log the name that gets passed in to the mock, it's undefined.

Wouldn't we want to always just return the name of mockReachableRobot, which is different than other-robot-name? I.e.

getRobotByName.mockReturnValueOnce(mockReachableRobot)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, test was bad. Good catch!

getViewableRobots.mockReturnValue([
{ name: 'other-robot-name', host: '10.10.0.1', port: 31950 },
])
},
expected: expect.objectContaining({
autoUpdateDisabledReason: expect.stringMatching(
/updating a different robot/
),
updateFromFileDisabledReason: expect.stringMatching(
/updating a different robot/
),
}),
},
{
name:
'getBuildrootUpdateDisplayInfo returns allowed only from file if no auto files',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: {} },
setup: () => {
getRobotByName.mockReturnValue(mockReachableRobot)
},
expected: {
autoUpdateAction: expect.stringMatching(/unavailable/i),
autoUpdateDisabledReason: expect.stringMatching(
/no update files found/i
),
updateFromFileDisabledReason: null,
},
},
{
name:
'getBuildrootUpdateDisplayInfo returns allowed with action if all good',
selector: selectors.getBuildrootUpdateDisplayInfo,
state: { buildroot: { version: '1.0.0' } },
setup: () => {
getRobotByName.mockReturnValue(mockReachableRobot)
getRobotApiVersionByName.mockReturnValue('0.9.9')
},
expected: {
autoUpdateAction: expect.stringMatching(/upgrade/i),
autoUpdateDisabledReason: null,
updateFromFileDisabledReason: null,
},
},
]

SPECS.forEach(spec => {
const { name, selector, state, expected, setup } = spec
const args = spec.args || []
if (typeof setup === 'function') setup()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fun fact: mock setup wasn't working at all in these tests...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃

it(name, () => expect(selector(state, ...args)).toEqual(expected))

it(name, () => {
if (typeof setup === 'function') setup()
expect(selector(state, ...args)).toEqual(expected)
})
})
})
92 changes: 72 additions & 20 deletions app/src/buildroot/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
import semver from 'semver'
import { createSelector } from 'reselect'

import { getViewableRobots, getRobotApiVersion } from '../discovery/selectors'
import {
HEALTH_STATUS_OK,
getViewableRobots,
getRobotApiVersionByName,
getRobotByName,
} from '../discovery'
import * as Constants from './constants'

import type { State } from '../types'
Expand All @@ -14,6 +19,15 @@ import type {
RobotSystemType,
} from './types'

// TODO(mc, 2020-08-02): i18n
const UPDATE_SERVER_UNAVAILABLE =
"Unable to update because your robot's update server is not responding"
const OTHER_ROBOT_UPDATING =
'Unable to update because the app is currently updating a different robot'
const NO_UPDATE_FILES =
'No update files found; please ensure your computer is connected to the internet and check again later'
const UNAVAILABLE = 'Unavailable'

export function getBuildrootUpdateVersion(state: State): string | null {
return state.buildroot.version || null
}
Expand Down Expand Up @@ -87,29 +101,67 @@ export const getBuildrootRobot: State => ViewableRobot | null = createSelector(
}
)

export function getBuildrootUpdateAvailable(
export const getBuildrootUpdateAvailable: (
mcous marked this conversation as resolved.
Show resolved Hide resolved
state: State,
robot: ViewableRobot
): BuildrootUpdateType | null {
const updateVersion = getBuildrootUpdateVersion(state)
const currentVersion = getRobotApiVersion(robot)

const validCurrent: string | null = semver.valid(currentVersion)
const validUpdate: string | null = semver.valid(updateVersion)
let type = null

if (validUpdate) {
if (!validCurrent || semver.gt(validUpdate, validCurrent)) {
type = Constants.UPGRADE
} else if (semver.lt(validUpdate, validCurrent)) {
type = Constants.DOWNGRADE
} else if (semver.eq(validUpdate, validCurrent)) {
type = Constants.REINSTALL
robotName: string
) => BuildrootUpdateType | null = createSelector(
getRobotApiVersionByName,
state => getBuildrootUpdateVersion(state),
(currentVersion, updateVersion) => {
const validCurrent: string | null = semver.valid(currentVersion)
const validUpdate: string | null = semver.valid(updateVersion)
let type = null

if (validUpdate) {
if (!validCurrent || semver.gt(validUpdate, validCurrent)) {
type = Constants.UPGRADE
} else if (semver.lt(validUpdate, validCurrent)) {
type = Constants.DOWNGRADE
} else if (semver.eq(validUpdate, validCurrent)) {
type = Constants.REINSTALL
}
}

return type
}
)

return type
}
export const getBuildrootUpdateDisplayInfo: (
state: State,
robotName: string
) => {|
autoUpdateAction: string,
autoUpdateDisabledReason: string | null,
updateFromFileDisabledReason: string | null,
|} = createSelector(
getRobotByName,
getBuildrootUpdateAvailable,
state => getBuildrootRobot(state),
(robot, autoUpdateType, currentUpdatingRobot) => {
const autoUpdateAction = autoUpdateType ?? UNAVAILABLE
let autoUpdateDisabledReason = null
let updateFromFileDisabledReason = null

if (robot?.serverHealthStatus !== HEALTH_STATUS_OK) {
autoUpdateDisabledReason = UPDATE_SERVER_UNAVAILABLE
updateFromFileDisabledReason = UPDATE_SERVER_UNAVAILABLE
} else if (
currentUpdatingRobot !== null &&
currentUpdatingRobot.name !== robot?.name
) {
autoUpdateDisabledReason = OTHER_ROBOT_UPDATING
updateFromFileDisabledReason = OTHER_ROBOT_UPDATING
} else if (autoUpdateType === null) {
autoUpdateDisabledReason = NO_UPDATE_FILES
}

return {
autoUpdateAction,
autoUpdateDisabledReason,
updateFromFileDisabledReason,
}
}
)

export function getRobotSystemType(
robot: ViewableRobot
Expand Down
5 changes: 2 additions & 3 deletions app/src/components/ConnectPanel/RobotItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,10 @@ export function RobotItemComponent(props: RobotItemProps): React.Node {
const { robot, match } = props
const { name, displayName, status, local: isLocal } = robot
const isUpgradable = useSelector((state: State) => {
return getBuildrootUpdateAvailable(state, robot) === UPGRADE
return getBuildrootUpdateAvailable(state, name) === UPGRADE
})
const isConnectable = status === CONNECTABLE
// NOTE(mc, 2020-03-30): redundant && true to satisfy Flow
const isConnected = Boolean(robot.connected && true)
const isConnected = robot.connected
const isSelected = robot.name === match.params.name
const connectInProgress = useSelector(
(state: State) => RobotSelectors.getConnectRequest(state).inProgress
Expand Down
12 changes: 7 additions & 5 deletions app/src/components/RobotSettings/AdvancedSettingsCard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
LabeledButton,
LabeledToggle,
Icon,
BORDER_SOLID_LIGHT,
} from '@opentrons/components'

import { UploadRobotUpdate } from './UploadRobotUpdate'
Expand Down Expand Up @@ -60,7 +61,7 @@ export function AdvancedSettingsCard(
)
const robotLogsDownloading = useSelector(getRobotLogsDownloading)
const dispatch = useDispatch<Dispatch>()
const disabled = status !== CONNECTABLE
const controlsDisabled = status !== CONNECTABLE
const logsAvailable = health && health.logs

const showLogOptoutModal = settings.some(
Expand All @@ -74,7 +75,7 @@ export function AdvancedSettingsCard(
}, [dispatch, name])

return (
<Card title={TITLE} disabled={disabled}>
<Card title={TITLE}>
<LabeledButton
label="Download Logs"
buttonProps={{
Expand All @@ -83,7 +84,7 @@ export function AdvancedSettingsCard(
) : (
'Download'
),
disabled: disabled || !logsAvailable || robotLogsDownloading,
disabled: controlsDisabled || !logsAvailable || robotLogsDownloading,
onClick: () => dispatch(downloadLogs(robot)),
}}
>
Expand All @@ -92,25 +93,26 @@ export function AdvancedSettingsCard(
<LabeledButton
label="Factory Reset"
buttonProps={{
disabled,
disabled: controlsDisabled,
Component: Link,
to: resetUrl,
children: 'Reset',
}}
>
<p>Restore robot to factory configuration</p>
</LabeledButton>
<UploadRobotUpdate robotName={name} borderBottom={BORDER_SOLID_LIGHT} />
{settings.map(({ id, title, description, value }) => (
<LabeledToggle
key={id}
label={title}
toggledOn={value === true}
disabled={controlsDisabled}
onClick={() => dispatch(updateSetting(name, id, !value))}
>
<p>{description}</p>
</LabeledToggle>
))}
<UploadRobotUpdate robotName={name} />
{showLogOptoutModal && (
<Portal>
<AlertModal
Expand Down
Loading