From 2aa8d2c6bd455d960f2d9e1346d3b1fc022d25ce Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Wed, 23 Oct 2019 14:47:16 -0400 Subject: [PATCH 1/2] feat(app): add robot restart alert for FF changes that require restart --- app/src/analytics/selectors.js | 5 +- .../RobotSettings/AdvancedSettingsCard.js | 48 +++- app/src/epic.js | 2 + app/src/pages/Calibrate/Labware.js | 5 +- app/src/reducer.js | 4 + app/src/robot-api/resources/settings.js | 43 +--- app/src/robot-api/resources/types.js | 15 -- app/src/robot-api/types.js | 4 +- .../robot-settings/__tests__/actions.test.js | 59 +++++ .../robot-settings/__tests__/epics.test.js | 78 ++++++ .../robot-settings/__tests__/reducer.test.js | 228 ++++++++++++++++++ .../__tests__/selectors.test.js | 55 +++++ app/src/robot-settings/actions.js | 39 +++ app/src/robot-settings/constants.js | 12 + app/src/robot-settings/epic.js | 19 ++ app/src/robot-settings/index.js | 5 + app/src/robot-settings/reducer.js | 61 +++++ app/src/robot-settings/selectors.js | 19 ++ app/src/robot-settings/types.js | 42 ++++ app/src/types.js | 7 + 20 files changed, 678 insertions(+), 72 deletions(-) create mode 100644 app/src/robot-settings/__tests__/actions.test.js create mode 100644 app/src/robot-settings/__tests__/epics.test.js create mode 100644 app/src/robot-settings/__tests__/reducer.test.js create mode 100644 app/src/robot-settings/__tests__/selectors.test.js create mode 100644 app/src/robot-settings/actions.js create mode 100644 app/src/robot-settings/constants.js create mode 100644 app/src/robot-settings/epic.js create mode 100644 app/src/robot-settings/index.js create mode 100644 app/src/robot-settings/reducer.js create mode 100644 app/src/robot-settings/selectors.js create mode 100644 app/src/robot-settings/types.js diff --git a/app/src/analytics/selectors.js b/app/src/analytics/selectors.js index f1ac64ddc78..b554875808d 100644 --- a/app/src/analytics/selectors.js +++ b/app/src/analytics/selectors.js @@ -24,7 +24,8 @@ import { getRobotSystemType, } from '../shell' -import { getRobotSettingsState, getPipettesState } from '../robot-api' +import { getRobotSettings } from '../robot-settings' +import { getPipettesState } from '../robot-api' import hash from './hash' @@ -77,7 +78,7 @@ export function getRobotAnalyticsData(state: State): RobotAnalyticsData | null { if (robot) { const pipettes = getPipettesState(state, robot.name) - const settings = getRobotSettingsState(state, robot.name) + const settings = getRobotSettings(state, robot.name) return settings.reduce( (result, setting) => ({ diff --git a/app/src/components/RobotSettings/AdvancedSettingsCard.js b/app/src/components/RobotSettings/AdvancedSettingsCard.js index 02726f637a0..b10f5a43cf1 100644 --- a/app/src/components/RobotSettings/AdvancedSettingsCard.js +++ b/app/src/components/RobotSettings/AdvancedSettingsCard.js @@ -6,11 +6,14 @@ import { Link } from 'react-router-dom' import { fetchSettings, - setSettings, - getRobotSettingsState, -} from '../../robot-api' + updateSetting, + clearRestartRequired, + getRobotSettings, + getRobotRestartRequired, +} from '../../robot-settings' import { CONNECTABLE } from '../../discovery' +import { restartRobot } from '../../robot-admin' import { downloadLogs } from '../../shell/robot-logs/actions' import { getRobotLogsDownloading } from '../../shell/robot-logs/selectors' import { Portal } from '../portal' @@ -23,8 +26,9 @@ import { } from '@opentrons/components' import type { State, Dispatch } from '../../types' -import type { ViewableRobot } from '../../discovery' -import type { RobotSettings } from '../../robot-api' +import type { ViewableRobot } from '../../discovery/types' +import type { RobotSettings } from '../../robot-settings/types' + import UploadRobotUpdate from './UploadRobotUpdate' type Props = {| @@ -49,11 +53,22 @@ const ROBOT_LOGS_OPTOUT_MESSAGE = ( ) +const RESTART_REQUIRED_HEADING = 'Robot Restart Required' +const RESTART_REQUIRED_MESSAGE = ( +

+ You must restart your robot for this setting change to take effect. Would + you like to restart now? +

+) + export default function AdvancedSettingsCard(props: Props) { const { robot, resetUrl } = props const { name, health, status } = robot const settings = useSelector(state => - getRobotSettingsState(state, name) + getRobotSettings(state, name) + ) + const restartRequired = useSelector(state => + getRobotRestartRequired(state, name) ) const robotLogsDownloading = useSelector(getRobotLogsDownloading) const dispatch = useDispatch() @@ -64,7 +79,10 @@ export default function AdvancedSettingsCard(props: Props) { s => s.id === ROBOT_LOGS_OPTOUT_ID && s.value === null ) const setLogOptout = (value: boolean) => - dispatch(setSettings(robot, { id: ROBOT_LOGS_OPTOUT_ID, value })) + dispatch(updateSetting(robot, ROBOT_LOGS_OPTOUT_ID, value)) + + const clearRestartAlert = () => dispatch(clearRestartRequired(name)) + const restart = () => dispatch(restartRobot(robot)) React.useEffect(() => { dispatch(fetchSettings(robot)) @@ -102,7 +120,7 @@ export default function AdvancedSettingsCard(props: Props) { key={id} label={title} toggledOn={value === true} - onClick={() => dispatch(setSettings(robot, { id, value: !value }))} + onClick={() => dispatch(updateSetting(robot, id, !value))} >

{description}

@@ -122,6 +140,20 @@ export default function AdvancedSettingsCard(props: Props) { )} + {restartRequired && ( + + clearRestartAlert() }, + { children: 'Restart', onClick: restart }, + ]} + > + {RESTART_REQUIRED_MESSAGE} + + + )} ) } diff --git a/app/src/epic.js b/app/src/epic.js index c11d3141fc0..1887b4a0cd2 100644 --- a/app/src/epic.js +++ b/app/src/epic.js @@ -6,6 +6,7 @@ import { analyticsEpic } from './analytics' import { discoveryEpic } from './discovery/epic' import { robotApiEpic } from './robot-api' import { robotAdminEpic } from './robot-admin/epic' +import { robotSettingsEpic } from './robot-settings/epic' import { shellEpic } from './shell' export default combineEpics( @@ -13,5 +14,6 @@ export default combineEpics( discoveryEpic, robotApiEpic, robotAdminEpic, + robotSettingsEpic, shellEpic ) diff --git a/app/src/pages/Calibrate/Labware.js b/app/src/pages/Calibrate/Labware.js index 258c4a771ca..62af0827ffd 100644 --- a/app/src/pages/Calibrate/Labware.js +++ b/app/src/pages/Calibrate/Labware.js @@ -7,7 +7,7 @@ import { push } from 'connected-react-router' import { selectors as robotSelectors } from '../../robot' import { getConnectedRobot } from '../../discovery' -import { getRobotSettingsState, type Module } from '../../robot-api' +import { getRobotSettings } from '../../robot-settings' import { getUnpreparedModules } from '../../robot-api/resources/modules' import Page from '../../components/Page' @@ -22,6 +22,7 @@ import type { ContextRouter } from 'react-router-dom' import type { State, Dispatch } from '../../types' import type { Labware } from '../../robot' import type { Robot } from '../../discovery' +import type { Module } from '../../robot-api' type OP = ContextRouter @@ -106,7 +107,7 @@ function mapStateToProps(state: State, ownProps: OP): SP { const hasModulesLeftToReview = modules.length > 0 && !robotSelectors.getModulesReviewed(state) const robot = getConnectedRobot(state) - const settings = robot && getRobotSettingsState(state, robot.name) + const settings = robot && getRobotSettings(state, robot.name) // TODO(mc, 2018-07-23): make diagram component a container const calToBottomFlag = diff --git a/app/src/reducer.js b/app/src/reducer.js index 8d5685a5b63..7e3a3e537d9 100644 --- a/app/src/reducer.js +++ b/app/src/reducer.js @@ -17,6 +17,9 @@ import { robotApiReducer } from './robot-api' // robot administration state import { robotAdminReducer } from './robot-admin/reducer' +// robot settings state +import { robotSettingsReducer } from './robot-settings/reducer' + // app shell state import { shellReducer } from './shell' @@ -42,6 +45,7 @@ const rootReducer: Reducer = combineReducers<_, Action>({ api: apiReducer, robotApi: robotApiReducer, robotAdmin: robotAdminReducer, + robotSettings: robotSettingsReducer, config: configReducer, discovery: discoveryReducer, labware: customLabwareReducer, diff --git a/app/src/robot-api/resources/settings.js b/app/src/robot-api/resources/settings.js index 93660a0ec89..7e5eabf6ceb 100644 --- a/app/src/robot-api/resources/settings.js +++ b/app/src/robot-api/resources/settings.js @@ -10,7 +10,6 @@ import { createBaseRobotApiEpic, passRobotApiResponseAction, GET, - POST, PATCH, } from '../utils' @@ -20,47 +19,26 @@ import type { State as AppState, Action, ActionLike, Epic } from '../../types' import type { RobotHost, RobotApiAction, RobotApiRequestState } from '../types' import type { SettingsState as State, - RobotSettings, PipetteSettings, - RobotSettingsFieldUpdate, PipetteSettingsUpdate, } from './types' -const INITIAL_STATE: State = { robot: [], pipettesById: {} } +const INITIAL_STATE: State = { pipettesById: {} } -export const SETTINGS_PATH = '/settings' export const PIPETTE_SETTINGS_PATH = '/settings/pipettes' const makePipetteSettingsPath = (id: string) => `${PIPETTE_SETTINGS_PATH}/${id}` -export const FETCH_SETTINGS: 'robotApi:FETCH_SETTINGS' = - 'robotApi:FETCH_SETTINGS' - -export const SET_SETTINGS: 'robotApi:SET_SETTINGS' = 'robotApi:SET_SETTINGS' - export const FETCH_PIPETTE_SETTINGS: 'robotApi:FETCH_PIPETTE_SETTINGS' = 'robotApi:FETCH_PIPETTE_SETTINGS' export const SET_PIPETTE_SETTINGS: 'robotApi:SET_PIPETTE_SETTINGS' = 'robotApi:SET_PIPETTE_SETTINGS' -export const fetchSettings = (host: RobotHost): RobotApiAction => ({ - type: FETCH_SETTINGS, - payload: { host, method: GET, path: SETTINGS_PATH }, -}) - export const fetchPipetteSettings = (host: RobotHost): RobotApiAction => ({ type: FETCH_PIPETTE_SETTINGS, payload: { host, method: GET, path: PIPETTE_SETTINGS_PATH }, }) -export const setSettings = ( - host: RobotHost, - body: RobotSettingsFieldUpdate -): RobotApiAction => ({ - type: SET_SETTINGS, - payload: { host, body, method: POST, path: SETTINGS_PATH }, -}) - export const setPipetteSettings = ( host: RobotHost, id: string, @@ -72,8 +50,6 @@ export const setPipetteSettings = ( meta: { id }, }) -const fetchSettingsEpic = createBaseRobotApiEpic(FETCH_SETTINGS) -const setSettingsEpic = createBaseRobotApiEpic(SET_SETTINGS) const fetchPipetteSettingsEpic = createBaseRobotApiEpic(FETCH_PIPETTE_SETTINGS) const setPipetteSettingsEpic = createBaseRobotApiEpic(SET_PIPETTE_SETTINGS) @@ -88,8 +64,6 @@ const fetchPipettesForSettingsEpic: Epic = action$ => ) export const settingsEpic = combineEpics( - fetchSettingsEpic, - setSettingsEpic, fetchPipetteSettingsEpic, setPipetteSettingsEpic, fetchPipettesForSettingsEpic @@ -105,12 +79,6 @@ export function settingsReducer( const { payload, meta } = resAction const { method, path, body } = payload - // grabs responses from GET /settings and POST /settings - // settings in body check is a guard against an old version of GET /settings - if (path === SETTINGS_PATH && 'settings' in body) { - return { ...state, robot: body.settings } - } - // grabs responses from GET /settings/pipettes if (path === PIPETTE_SETTINGS_PATH) { return { ...state, pipettesById: body } @@ -135,15 +103,6 @@ export function settingsReducer( return state } -export function getRobotSettingsState( - state: AppState, - robotName: string -): RobotSettings { - const robotState = getRobotApiState(state, robotName) - - return robotState?.resources.settings.robot || [] -} - export function getPipetteSettingsState( state: AppState, robotName: string, diff --git a/app/src/robot-api/resources/types.js b/app/src/robot-api/resources/types.js index d7d6aa91b47..1399c4b9313 100644 --- a/app/src/robot-api/resources/types.js +++ b/app/src/robot-api/resources/types.js @@ -97,29 +97,14 @@ export type MotorAxis = 'a' | 'b' | 'c' | 'x' | 'y' | 'z' // settings export type SettingsState = {| - robot: RobotSettings, pipettesById: {| [id: string]: PipetteSettings |}, |} -export type RobotSettings = Array - export type PipetteSettings = {| info: {| name: ?string, model: ?string |}, fields: PipetteSettingsFieldsMap, |} -export type RobotSettingsField = {| - id: string, - title: string, - description: string, - value: boolean | null, -|} - -export type RobotSettingsFieldUpdate = {| - id: $PropertyType, - value: $PropertyType, -|} - export type PipetteSettingsFieldsMap = {| [fieldId: string]: PipetteSettingsField, quirks?: PipetteQuirksField, diff --git a/app/src/robot-api/types.js b/app/src/robot-api/types.js index d8594c9bbae..26f4927578e 100644 --- a/app/src/robot-api/types.js +++ b/app/src/robot-api/types.js @@ -10,7 +10,7 @@ export * from './resources/types' export type Method = 'GET' | 'POST' | 'PATCH' | 'DELETE' -export type RequestMeta = { [string]: mixed } +export type RequestMeta = $Shape<{| [string]: mixed |}> // api call + response types @@ -53,9 +53,7 @@ export type RobotApiAction = meta: {| id: string |}, |} | {| type: 'robotApi:FETCH_PIPETTES', payload: RobotApiRequest |} - | {| type: 'robotApi:FETCH_SETTINGS', payload: RobotApiRequest |} | {| type: 'robotApi:FETCH_PIPETTE_SETTINGS', payload: RobotApiRequest |} - | {| type: 'robotApi:SET_SETTINGS', payload: RobotApiRequest |} | {| type: 'robotApi:SET_PIPETTE_SETTINGS', payload: RobotApiRequest, diff --git a/app/src/robot-settings/__tests__/actions.test.js b/app/src/robot-settings/__tests__/actions.test.js new file mode 100644 index 00000000000..e49ff17e0a0 --- /dev/null +++ b/app/src/robot-settings/__tests__/actions.test.js @@ -0,0 +1,59 @@ +// @flow + +import * as Actions from '../actions' + +import type { RobotSettingsAction } from '../types' + +type ActionSpec = {| + name: string, + creator: (...Array) => mixed, + args: Array, + expected: RobotSettingsAction, +|} + +describe('robot settings actions', () => { + const SPECS: Array = [ + { + name: 'robotSettings:FETCH_SETTINGS', + creator: Actions.fetchSettings, + args: [{ name: 'robotName', ip: 'localhost', port: 31950 }], + expected: { + type: 'robotSettings:FETCH_SETTINGS', + payload: { + host: { name: 'robotName', ip: 'localhost', port: 31950 }, + method: 'GET', + path: '/settings', + }, + }, + }, + { + name: 'robotSettings:UPDATE_SETTING', + creator: Actions.updateSetting, + args: [{ name: 'robotName', ip: 'localhost', port: 31950 }, 'foo', true], + expected: { + type: 'robotSettings:UPDATE_SETTING', + meta: { settingId: 'foo' }, + payload: { + host: { name: 'robotName', ip: 'localhost', port: 31950 }, + method: 'POST', + path: '/settings', + body: { id: 'foo', value: true }, + }, + }, + }, + { + name: 'robotSettings:CLEAR_RESTART_REQUIRED', + creator: Actions.clearRestartRequired, + args: ['robotName'], + expected: { + type: 'robotSettings:CLEAR_RESTART_REQUIRED', + payload: { robotName: 'robotName' }, + }, + }, + ] + + SPECS.forEach(spec => { + const { name, creator, args, expected } = spec + test(name, () => expect(creator(...args)).toEqual(expected)) + }) +}) diff --git a/app/src/robot-settings/__tests__/epics.test.js b/app/src/robot-settings/__tests__/epics.test.js new file mode 100644 index 00000000000..6c15d40d8a0 --- /dev/null +++ b/app/src/robot-settings/__tests__/epics.test.js @@ -0,0 +1,78 @@ +// @flow +import { TestScheduler } from 'rxjs/testing' + +import * as ApiUtils from '../../robot-api/utils' +import * as Actions from '../actions' +import { robotSettingsEpic } from '../epic' + +import type { RobotApiRequest, RequestMeta } from '../../robot-api/types' + +jest.mock('../../robot-api/utils') + +const mockMakeApiRequest: JestMockFn<[RobotApiRequest, RequestMeta], mixed> = + ApiUtils.makeRobotApiRequest + +const mockRobot = { name: 'robot', ip: '127.0.0.1', port: 31950 } + +const setupMockMakeApiRequest = cold => { + mockMakeApiRequest.mockImplementation((req, meta) => + cold('-a', { a: { req, meta } }) + ) +} + +describe('robotSettingsEpic', () => { + let testScheduler + + beforeEach(() => { + testScheduler = new TestScheduler((actual, expected) => { + expect(actual).toEqual(expected) + }) + }) + + afterEach(() => { + jest.resetAllMocks() + }) + + test('makes GET /settings request on FETCH_SETTINGS', () => { + const action = Actions.fetchSettings(mockRobot) + + testScheduler.run(({ hot, cold, expectObservable }) => { + setupMockMakeApiRequest(cold) + + const action$ = hot('-a', { a: action }) + const state$: any = null + const output$ = robotSettingsEpic(action$, state$) + + expectObservable(output$).toBe('--a', { + a: { + req: { host: mockRobot, method: 'GET', path: '/settings' }, + meta: {}, + }, + }) + }) + }) + + test('makes POST /settings request on UPDATE_SETTING', () => { + const action = Actions.updateSetting(mockRobot, 'settingId', true) + + testScheduler.run(({ hot, cold, expectObservable }) => { + setupMockMakeApiRequest(cold) + + const action$ = hot('-a', { a: action }) + const state$: any = null + const output$ = robotSettingsEpic(action$, state$) + + expectObservable(output$).toBe('--a', { + a: { + req: { + host: mockRobot, + method: 'POST', + path: '/settings', + body: { id: 'settingId', value: true }, + }, + meta: { settingId: 'settingId' }, + }, + }) + }) + }) +}) diff --git a/app/src/robot-settings/__tests__/reducer.test.js b/app/src/robot-settings/__tests__/reducer.test.js new file mode 100644 index 00000000000..d32e3b19b56 --- /dev/null +++ b/app/src/robot-settings/__tests__/reducer.test.js @@ -0,0 +1,228 @@ +// @flow + +import { robotSettingsReducer } from '../reducer' + +import type { Action, ActionLike } from '../../types' +import type { RobotSettingsState } from '../types' + +type ReducerSpec = {| + name: string, + action: Action | ActionLike, + state: RobotSettingsState, + expected: RobotSettingsState, +|} + +describe('robotSettingsReducer', () => { + const SPECS: Array = [ + { + name: 'handles initial robotApi:RESPONSE for GET /settings', + action: { + type: 'robotApi:RESPONSE__GET__/settings', + meta: {}, + payload: { + host: { name: 'robotName' }, + method: 'GET', + path: '/settings', + body: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, + ], + }, + }, + }, + state: {}, + expected: { + robotName: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, + ], + restartRequired: false, + }, + }, + }, + { + name: 'handles subsequent robotApi:RESPONSE for GET /settings', + action: { + type: 'robotApi:RESPONSE__GET__/settings', + meta: {}, + payload: { + host: { name: 'robotName' }, + method: 'GET', + path: '/settings', + body: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + ], + }, + }, + }, + state: { + robotName: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, + ], + restartRequired: true, + }, + }, + expected: { + robotName: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + ], + restartRequired: true, + }, + }, + }, + { + name: 'handles robotApi:RESPONSE for POST /settings', + action: { + type: 'robotApi:RESPONSE__POST__/settings', + meta: {}, + payload: { + host: { name: 'robotName' }, + method: 'POST', + path: '/settings', + body: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, + ], + }, + }, + }, + state: { + robotName: { + settings: [], + restartRequired: false, + }, + }, + expected: { + robotName: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, + ], + restartRequired: false, + }, + }, + }, + { + name: 'handles robotApi:RESPONSE for POST /settings', + action: { + type: 'robotApi:RESPONSE__POST__/settings', + meta: { settingId: 'bar' }, + payload: { + host: { name: 'robotName' }, + method: 'POST', + path: '/settings', + body: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + { id: 'bar', title: 'Bar', description: 'bazqux', value: true }, + ], + }, + }, + }, + state: { + robotName: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, + ], + restartRequired: false, + }, + }, + expected: { + robotName: { + settings: [ + { id: 'foo', title: 'Foo', description: 'foobar', value: true }, + { id: 'bar', title: 'Bar', description: 'bazqux', value: true }, + ], + restartRequired: false, + }, + }, + }, + { + name: + 'handles robotApi:RESPONSE for POST /settings where restart is required', + action: { + type: 'robotApi:RESPONSE__POST__/settings', + meta: { settingId: 'baz' }, + payload: { + host: { name: 'robotName' }, + method: 'POST', + path: '/settings', + body: { + settings: [ + { + id: 'baz', + title: 'Baz', + description: 'bazqux', + value: true, + restart_required: true, + }, + ], + }, + }, + }, + state: { + robotName: { + settings: [ + { + id: 'baz', + title: 'Baz', + description: 'bazqux', + value: false, + restart_required: true, + }, + ], + restartRequired: false, + }, + }, + expected: { + robotName: { + settings: [ + { + id: 'baz', + title: 'Baz', + description: 'bazqux', + value: true, + restart_required: true, + }, + ], + restartRequired: true, + }, + }, + }, + { + name: 'handles CLEAR_RESTART_REQUIRED', + action: { + type: 'robotSettings:CLEAR_RESTART_REQUIRED', + payload: { robotName: 'robotName' }, + }, + state: { robotName: { settings: [], restartRequired: true } }, + expected: { robotName: { settings: [], restartRequired: false } }, + }, + { + name: 'handles robotAdmin:RESTART', + action: { + type: 'robotAdmin:RESTART', + meta: { robot: true }, + payload: { host: { name: 'robotName', ip: 'localhost', port: 31950 } }, + }, + state: { robotName: { settings: [], restartRequired: true } }, + expected: { robotName: { settings: [], restartRequired: false } }, + }, + ] + + SPECS.forEach(spec => { + const { name, action, state, expected } = spec + + test(name, () => { + expect(robotSettingsReducer(state, action)).toEqual(expected) + }) + }) +}) diff --git a/app/src/robot-settings/__tests__/selectors.test.js b/app/src/robot-settings/__tests__/selectors.test.js new file mode 100644 index 00000000000..63a5bb42d06 --- /dev/null +++ b/app/src/robot-settings/__tests__/selectors.test.js @@ -0,0 +1,55 @@ +// @flow +import * as Selectors from '../selectors' +import type { State } from '../../types' + +type SelectorSpec = {| + name: string, + selector: ($Shape, ...Array) => mixed, + state: $Shape, + args?: Array, + expected: mixed, +|} + +describe('robot settings selectors', () => { + const SPECS: Array = [ + { + name: 'getRobotSettings', + selector: Selectors.getRobotSettings, + state: { + robotSettings: { + robotName: { + restartRequired: false, + settings: [ + { id: 'foo', title: 'Foo', description: 'Foo', value: true }, + ], + }, + }, + }, + args: ['robotName'], + expected: [{ id: 'foo', title: 'Foo', description: 'Foo', value: true }], + }, + { + name: 'getRobotRestartRequired', + selector: Selectors.getRobotRestartRequired, + state: { + robotSettings: { + robotName: { + restartRequired: true, + settings: [], + }, + }, + }, + args: ['robotName'], + expected: true, + }, + ] + + SPECS.forEach(spec => { + const { name, selector, state, args = [], expected } = spec + + test(name, () => { + const result = selector(state, ...args) + expect(result).toEqual(expected) + }) + }) +}) diff --git a/app/src/robot-settings/actions.js b/app/src/robot-settings/actions.js new file mode 100644 index 00000000000..0338a6dfe2a --- /dev/null +++ b/app/src/robot-settings/actions.js @@ -0,0 +1,39 @@ +// @flow +import { GET, POST } from '../robot-api/utils' + +import { + FETCH_SETTINGS, + UPDATE_SETTING, + CLEAR_RESTART_REQUIRED, + SETTINGS_PATH, +} from './constants' + +import type { RobotHost } from '../robot-api/types' +import type { RobotSettingsAction } from './types' + +export const fetchSettings = (host: RobotHost): RobotSettingsAction => ({ + type: FETCH_SETTINGS, + payload: { host, method: GET, path: SETTINGS_PATH }, +}) + +export const updateSetting = ( + host: RobotHost, + settingId: string, + value: boolean | null +): RobotSettingsAction => ({ + type: UPDATE_SETTING, + meta: { settingId }, + payload: { + host, + method: POST, + path: SETTINGS_PATH, + body: { id: settingId, value }, + }, +}) + +export const clearRestartRequired = ( + robotName: string +): RobotSettingsAction => ({ + type: CLEAR_RESTART_REQUIRED, + payload: { robotName }, +}) diff --git a/app/src/robot-settings/constants.js b/app/src/robot-settings/constants.js new file mode 100644 index 00000000000..c6366b2d288 --- /dev/null +++ b/app/src/robot-settings/constants.js @@ -0,0 +1,12 @@ +// @flow + +export const SETTINGS_PATH: '/settings' = '/settings' + +export const FETCH_SETTINGS: 'robotSettings:FETCH_SETTINGS' = + 'robotSettings:FETCH_SETTINGS' + +export const UPDATE_SETTING: 'robotSettings:UPDATE_SETTING' = + 'robotSettings:UPDATE_SETTING' + +export const CLEAR_RESTART_REQUIRED: 'robotSettings:CLEAR_RESTART_REQUIRED' = + 'robotSettings:CLEAR_RESTART_REQUIRED' diff --git a/app/src/robot-settings/epic.js b/app/src/robot-settings/epic.js new file mode 100644 index 00000000000..a10630023f2 --- /dev/null +++ b/app/src/robot-settings/epic.js @@ -0,0 +1,19 @@ +// @flow +import { ofType } from 'redux-observable' +import { switchMap } from 'rxjs/operators' +import { makeRobotApiRequest } from '../robot-api/utils' +import { FETCH_SETTINGS, UPDATE_SETTING } from './constants' + +import type { Epic } from '../types' +import type { RequestMeta } from '../robot-api/types' +import type { RobotSettingsApiAction } from './types' + +export const robotSettingsEpic: Epic = action$ => { + return action$.pipe( + ofType(FETCH_SETTINGS, UPDATE_SETTING), + switchMap(a => { + const meta: RequestMeta = a.meta || {} + return makeRobotApiRequest(a.payload, meta) + }) + ) +} diff --git a/app/src/robot-settings/index.js b/app/src/robot-settings/index.js new file mode 100644 index 00000000000..552091d705d --- /dev/null +++ b/app/src/robot-settings/index.js @@ -0,0 +1,5 @@ +// @flow +// robot settings actions, constants, selectors re-exports for convenience +export * from './actions' +export * from './constants' +export * from './selectors' diff --git a/app/src/robot-settings/reducer.js b/app/src/robot-settings/reducer.js new file mode 100644 index 00000000000..133cda5ac1c --- /dev/null +++ b/app/src/robot-settings/reducer.js @@ -0,0 +1,61 @@ +// @flow +import { passRobotApiResponseAction, POST } from '../robot-api/utils' +import { RESTART } from '../robot-admin' +import { CLEAR_RESTART_REQUIRED, SETTINGS_PATH } from './constants' + +import type { Action, ActionLike } from '../types' +import type { RobotSettings, RobotSettingsState } from './types' + +export const INITIAL_STATE: RobotSettingsState = {} + +export function robotSettingsReducer( + state: RobotSettingsState = INITIAL_STATE, + action: Action | ActionLike +): RobotSettingsState { + const resAction = passRobotApiResponseAction(action) + + if (resAction) { + const { payload, meta } = resAction + const { host, method, path, body } = payload + const { name: robotName } = host + + // grabs responses from GET /settings and POST /settings + // settings in body check is a guard against an old version of GET /settings + if (path === SETTINGS_PATH && 'settings' in body) { + const robotState = state[robotName] + const settings: RobotSettings = body.settings + // restart is required if the setting we just updated (meta.settingId) + // is a restart_required setting, or if restart was already required + const restartRequired = + Boolean(robotState?.restartRequired) || + (method === POST && + settings.some( + s => s.id === meta.settingId && s.restart_required === true + )) + + return { ...state, [robotName]: { settings, restartRequired } } + } + } + + switch (action.type) { + case CLEAR_RESTART_REQUIRED: { + const { robotName } = action.payload + + return { + ...state, + [robotName]: { ...state[robotName], restartRequired: false }, + } + } + + case RESTART: { + const { name: robotName } = action.payload.host + + return { + ...state, + [robotName]: { ...state[robotName], restartRequired: false }, + } + } + } + + return state +} diff --git a/app/src/robot-settings/selectors.js b/app/src/robot-settings/selectors.js new file mode 100644 index 00000000000..8a828bc549b --- /dev/null +++ b/app/src/robot-settings/selectors.js @@ -0,0 +1,19 @@ +// @flow +import type { State } from '../types' +import type { RobotSettings } from './types' + +const robotState = (state: State, name: string) => state.robotSettings[name] + +export function getRobotSettings( + state: State, + robotName: string +): RobotSettings { + return robotState(state, robotName)?.settings || [] +} + +export function getRobotRestartRequired( + state: State, + robotName: string +): boolean { + return robotState(state, robotName)?.restartRequired || false +} diff --git a/app/src/robot-settings/types.js b/app/src/robot-settings/types.js new file mode 100644 index 00000000000..b86388d078e --- /dev/null +++ b/app/src/robot-settings/types.js @@ -0,0 +1,42 @@ +// @flow + +import type { RobotApiRequest } from '../robot-api/types' + +export type RobotSettingsField = {| + id: string, + title: string, + description: string, + value: boolean | null, + restart_required?: boolean, +|} + +export type RobotSettings = Array + +export type PerRobotRobotSettingsState = {| + settings: RobotSettings, + restartRequired: boolean, +|} + +export type RobotSettingsState = $Shape<{| + [robotName: string]: void | PerRobotRobotSettingsState, +|}> + +export type RobotSettingsFieldUpdate = {| + id: $PropertyType, + value: $PropertyType, +|} + +export type RobotSettingsApiAction = + | {| type: 'robotSettings:FETCH_SETTINGS', payload: RobotApiRequest |} + | {| + type: 'robotSettings:UPDATE_SETTING', + payload: RobotApiRequest, + meta: {| settingId: string |}, + |} + +export type RobotSettingsAction = + | RobotSettingsApiAction + | {| + type: 'robotSettings:CLEAR_RESTART_REQUIRED', + payload: {| robotName: string |}, + |} diff --git a/app/src/types.js b/app/src/types.js index eca42b2682e..0e9ad61acae 100644 --- a/app/src/types.js +++ b/app/src/types.js @@ -18,11 +18,17 @@ import type { CustomLabwareAction, } from './custom-labware/types' +import type { + RobotSettingsState, + RobotSettingsAction, +} from './robot-settings/types' + export type State = $ReadOnly<{| robot: RobotState, api: HttpApiState, robotApi: RobotApiState, robotAdmin: RobotAdminState, + robotSettings: RobotSettingsState, config: Config, discovery: DiscoveryState, labware: CustomLabwareState, @@ -36,6 +42,7 @@ export type Action = | HttpApiAction | RobotApiAction | RobotAdminAction + | RobotSettingsAction | ShellAction | ConfigAction | RouterAction From 358010330d4bc041fae3ff6f0e9d756e18f43825 Mon Sep 17 00:00:00 2001 From: Mike Cousins Date: Thu, 24 Oct 2019 16:28:46 -0400 Subject: [PATCH 2/2] fixup: Defer to API for restart required state --- .../RobotSettings/AdvancedSettingsCard.js | 31 -------- .../RobotSettings/RestartRequiredBanner.js | 47 ++++++++++++ app/src/components/RobotSettings/styles.css | 9 +++ app/src/pages/Robots/RobotSettings.js | 11 ++- app/src/robot-admin/__tests__/epic.test.js | 31 +++++++- app/src/robot-admin/actions.js | 2 +- app/src/robot-admin/epic.js | 24 ++++-- .../robot-settings/__tests__/actions.test.js | 24 ++---- .../robot-settings/__tests__/epics.test.js | 2 +- .../robot-settings/__tests__/reducer.test.js | 73 +++---------------- .../__tests__/selectors.test.js | 29 ++++++-- app/src/robot-settings/actions.js | 15 +--- app/src/robot-settings/constants.js | 4 +- app/src/robot-settings/epic.js | 8 +- app/src/robot-settings/reducer.js | 46 +++--------- app/src/robot-settings/selectors.js | 9 ++- app/src/robot-settings/types.js | 22 ++---- 17 files changed, 180 insertions(+), 207 deletions(-) create mode 100644 app/src/components/RobotSettings/RestartRequiredBanner.js diff --git a/app/src/components/RobotSettings/AdvancedSettingsCard.js b/app/src/components/RobotSettings/AdvancedSettingsCard.js index b10f5a43cf1..f6b2d935d9b 100644 --- a/app/src/components/RobotSettings/AdvancedSettingsCard.js +++ b/app/src/components/RobotSettings/AdvancedSettingsCard.js @@ -7,13 +7,10 @@ import { Link } from 'react-router-dom' import { fetchSettings, updateSetting, - clearRestartRequired, getRobotSettings, - getRobotRestartRequired, } from '../../robot-settings' import { CONNECTABLE } from '../../discovery' -import { restartRobot } from '../../robot-admin' import { downloadLogs } from '../../shell/robot-logs/actions' import { getRobotLogsDownloading } from '../../shell/robot-logs/selectors' import { Portal } from '../portal' @@ -53,23 +50,12 @@ const ROBOT_LOGS_OPTOUT_MESSAGE = ( ) -const RESTART_REQUIRED_HEADING = 'Robot Restart Required' -const RESTART_REQUIRED_MESSAGE = ( -

- You must restart your robot for this setting change to take effect. Would - you like to restart now? -

-) - export default function AdvancedSettingsCard(props: Props) { const { robot, resetUrl } = props const { name, health, status } = robot const settings = useSelector(state => getRobotSettings(state, name) ) - const restartRequired = useSelector(state => - getRobotRestartRequired(state, name) - ) const robotLogsDownloading = useSelector(getRobotLogsDownloading) const dispatch = useDispatch() const disabled = status !== CONNECTABLE @@ -81,9 +67,6 @@ export default function AdvancedSettingsCard(props: Props) { const setLogOptout = (value: boolean) => dispatch(updateSetting(robot, ROBOT_LOGS_OPTOUT_ID, value)) - const clearRestartAlert = () => dispatch(clearRestartRequired(name)) - const restart = () => dispatch(restartRobot(robot)) - React.useEffect(() => { dispatch(fetchSettings(robot)) }, [dispatch, robot]) @@ -140,20 +123,6 @@ export default function AdvancedSettingsCard(props: Props) { )} - {restartRequired && ( - - clearRestartAlert() }, - { children: 'Restart', onClick: restart }, - ]} - > - {RESTART_REQUIRED_MESSAGE} - - - )} ) } diff --git a/app/src/components/RobotSettings/RestartRequiredBanner.js b/app/src/components/RobotSettings/RestartRequiredBanner.js new file mode 100644 index 00000000000..ad84f4f2e25 --- /dev/null +++ b/app/src/components/RobotSettings/RestartRequiredBanner.js @@ -0,0 +1,47 @@ +// @flow +import * as React from 'react' +import { useDispatch } from 'react-redux' +import { AlertItem, OutlineButton } from '@opentrons/components' + +import { restartRobot } from '../../robot-admin' +import styles from './styles.css' + +import type { Dispatch } from '../../types' +import type { RobotHost } from '../../robot-api/types' + +export type RestartRequiredBannerProps = {| + robot: RobotHost, +|} + +// TODO(mc, 2019-10-24): i18n +const TITLE = 'Robot restart required' +const MESSAGE = + 'You must restart your robot for your settings changes to take effect' +const RESTART_NOW = 'Restart Now' + +function RestartRequiredBanner(props: RestartRequiredBannerProps) { + const { robot } = props + const [dismissed, setDismissed] = React.useState(false) + const dispatch = useDispatch() + const restart = React.useCallback(() => dispatch(restartRobot(robot)), [ + dispatch, + robot, + ]) + + if (dismissed) return null + + return ( + setDismissed(true)} + title={TITLE} + > +
+

{MESSAGE}

+ {RESTART_NOW} +
+ + ) +} + +export default RestartRequiredBanner diff --git a/app/src/components/RobotSettings/styles.css b/app/src/components/RobotSettings/styles.css index 86d659c5fb1..25b477807a6 100644 --- a/app/src/components/RobotSettings/styles.css +++ b/app/src/components/RobotSettings/styles.css @@ -85,3 +85,12 @@ position: fixed; clip: rect(1px 1px 1px 1px); } + +.restart_banner_message { + display: flex; + + & > p { + max-width: 24rem; + margin-right: auto; + } +} diff --git a/app/src/pages/Robots/RobotSettings.js b/app/src/pages/Robots/RobotSettings.js index a6f1b950806..e43baf78f7d 100644 --- a/app/src/pages/Robots/RobotSettings.js +++ b/app/src/pages/Robots/RobotSettings.js @@ -18,6 +18,7 @@ import { import { makeGetRobotHome, clearHomeResponse } from '../../http-api-client' import { getRobotRestarting } from '../../robot-admin' +import { getRobotRestartRequired } from '../../robot-settings' import { SpinnerModalPage } from '@opentrons/components' import { ErrorModal } from '../../components/modals' @@ -29,6 +30,7 @@ import UpdateBuildroot from '../../components/RobotSettings/UpdateBuildroot' import CalibrateDeck from '../../components/CalibrateDeck' import ConnectBanner from '../../components/RobotSettings/ConnectBanner' import ReachableRobotBanner from '../../components/RobotSettings/ReachableRobotBanner' +import RestartRequiredBanner from '../../components/RobotSettings/RestartRequiredBanner' import ResetRobotModal from '../../components/RobotSettings/ResetRobotModal' import type { ContextRouter } from 'react-router-dom' @@ -48,6 +50,7 @@ type SP = {| homeInProgress: ?boolean, homeError: ?Error, updateInProgress: boolean, + restartRequired: boolean, restarting: boolean, |} @@ -82,6 +85,7 @@ function RobotSettingsPage(props: Props) { closeConnectAlert, showUpdateModal, updateInProgress, + restartRequired, restarting, match: { path, url }, } = props @@ -103,6 +107,9 @@ function RobotSettingsPage(props: Props) { {robot.status === CONNECTABLE && ( )} + {restartRequired && !restarting && ( + + )} SP { const buildrootUpdateType = getBuildrootUpdateAvailable(state, robot) const updateInProgress = getBuildrootUpdateInProgress(state, robot) const currentBrRobot = getBuildrootRobot(state) - const restarting = getRobotRestarting(state, robot.name) const showUpdateModal = updateInProgress || @@ -205,7 +211,8 @@ function makeMapStateToProps(): (state: State, ownProps: OP) => SP { return { updateInProgress, - restarting, + restarting: getRobotRestarting(state, robot.name), + restartRequired: getRobotRestartRequired(state, robot.name), showUpdateModal: !!showUpdateModal, homeInProgress: homeRequest && homeRequest.inProgress, homeError: homeRequest && homeRequest.error, diff --git a/app/src/robot-admin/__tests__/epic.test.js b/app/src/robot-admin/__tests__/epic.test.js index b4a414a743c..01d403cd061 100644 --- a/app/src/robot-admin/__tests__/epic.test.js +++ b/app/src/robot-admin/__tests__/epic.test.js @@ -2,6 +2,7 @@ import { TestScheduler } from 'rxjs/testing' import * as ApiUtils from '../../robot-api/utils' +import * as SettingsSelectors from '../../robot-settings/selectors' import * as DiscoveryActions from '../../discovery/actions' import * as Actions from '../actions' import { robotAdminEpic } from '../epic' @@ -14,6 +15,7 @@ import type { } from '../../robot-api/types' jest.mock('../../robot-api/utils') +jest.mock('../../robot-settings/selectors') const mockMakeApiRequest: JestMockFn<[RobotApiRequest, RequestMeta], mixed> = ApiUtils.makeRobotApiRequest @@ -23,7 +25,11 @@ const mockPassRobotApiResponseAction: JestMockFn< RobotApiResponseAction | null > = ApiUtils.passRobotApiResponseAction +const mockGetRestartPath: JestMockFn, string | null> = + SettingsSelectors.getRobotRestartPath + const mockRobot = { name: 'robot', ip: '127.0.0.1', port: 31950 } +const mockState = { mock: true } const setupMockMakeApiRequest = cold => { mockMakeApiRequest.mockImplementation((req, meta) => @@ -49,9 +55,10 @@ describe('robotAdminEpic', () => { testScheduler.run(({ hot, cold, expectObservable }) => { setupMockMakeApiRequest(cold) + mockGetRestartPath.mockReturnValue(null) const action$ = hot('-a', { a: action }) - const state$: any = null + const state$ = hot('a-', { a: mockState }) const output$ = robotAdminEpic(action$, state$) expectObservable(output$).toBe('--a', { @@ -63,6 +70,26 @@ describe('robotAdminEpic', () => { }) }) + test('makes a POST to the settings restart path on RESTART is applicable', () => { + const action = Actions.restartRobot(mockRobot) + + testScheduler.run(({ hot, cold, expectObservable }) => { + setupMockMakeApiRequest(cold) + mockGetRestartPath.mockReturnValue('/restart') + + const action$ = hot('-a', { a: action }) + const state$ = hot('a-', { a: mockState }) + const output$ = robotAdminEpic(action$, state$) + + expectObservable(output$).toBe('--a', { + a: { + req: { host: mockRobot, method: 'POST', path: '/restart' }, + meta: {}, + }, + }) + }) + }) + test('starts discovery on restart request success', () => { testScheduler.run(({ hot, expectObservable }) => { const serverSuccessAction = { @@ -81,7 +108,7 @@ describe('robotAdminEpic', () => { mockPassRobotApiResponseAction.mockReturnValue(serverSuccessAction) const action$ = hot('-a', { a: serverSuccessAction }) - const state$: any = null + const state$ = hot('a-', { a: mockState }) const output$ = robotAdminEpic(action$, state$) expectObservable(output$).toBe('-a', { diff --git a/app/src/robot-admin/actions.js b/app/src/robot-admin/actions.js index 250878eee0a..f02a53fa580 100644 --- a/app/src/robot-admin/actions.js +++ b/app/src/robot-admin/actions.js @@ -7,6 +7,6 @@ import type { RobotAdminAction } from './types' export const restartRobot = (host: RobotHost): RobotAdminAction => ({ type: RESTART, - payload: { host, method: POST, path: RESTART_PATH }, + payload: { host, path: RESTART_PATH, method: POST }, meta: { robot: true }, }) diff --git a/app/src/robot-admin/epic.js b/app/src/robot-admin/epic.js index ae3679b0d1a..ea72c83802f 100644 --- a/app/src/robot-admin/epic.js +++ b/app/src/robot-admin/epic.js @@ -1,27 +1,35 @@ // @flow import { of } from 'rxjs' import { ofType, combineEpics } from 'redux-observable' -import { filter, switchMap } from 'rxjs/operators' +import { filter, switchMap, withLatestFrom } from 'rxjs/operators' import { makeRobotApiRequest, passRobotApiResponseAction, } from '../robot-api/utils' -import { startDiscovery } from '../discovery/actions' +import { startDiscovery } from '../discovery' +import { getRobotRestartPath } from '../robot-settings' import { RESTART, RESTART_PATH } from './constants' -import type { Epic, LooseEpic } from '../types' -import type { RequestMeta, RobotApiResponseAction } from '../robot-api/types' +import type { State, Epic, LooseEpic } from '../types' +import type { RobotApiResponseAction } from '../robot-api/types' import type { RobotAdminAction } from './types' export const RESTART_DISCOVERY_TIMEOUT_MS = 60000 -const robotAdminApiEpic: Epic = action$ => { +const robotAdminApiEpic: Epic = (action$, state$) => { return action$.pipe( ofType(RESTART), - switchMap(a => { - const meta: RequestMeta = {} - return makeRobotApiRequest(a.payload, meta) + withLatestFrom(state$), + switchMap<[RobotAdminAction, State], _, _>(([action, state]) => { + const { name: robotName } = action.payload.host + const restartPath = getRobotRestartPath(state, robotName) + const payload = + restartPath !== null + ? { ...action.payload, path: restartPath } + : action.payload + + return makeRobotApiRequest(payload, {}) }) ) } diff --git a/app/src/robot-settings/__tests__/actions.test.js b/app/src/robot-settings/__tests__/actions.test.js index e49ff17e0a0..3e3b58b8c46 100644 --- a/app/src/robot-settings/__tests__/actions.test.js +++ b/app/src/robot-settings/__tests__/actions.test.js @@ -11,45 +11,33 @@ type ActionSpec = {| expected: RobotSettingsAction, |} +const mockRobot = { name: 'robotName', ip: 'localhost', port: 31950 } + describe('robot settings actions', () => { const SPECS: Array = [ { name: 'robotSettings:FETCH_SETTINGS', creator: Actions.fetchSettings, - args: [{ name: 'robotName', ip: 'localhost', port: 31950 }], + args: [mockRobot], expected: { type: 'robotSettings:FETCH_SETTINGS', - payload: { - host: { name: 'robotName', ip: 'localhost', port: 31950 }, - method: 'GET', - path: '/settings', - }, + payload: { host: mockRobot, method: 'GET', path: '/settings' }, }, }, { name: 'robotSettings:UPDATE_SETTING', creator: Actions.updateSetting, - args: [{ name: 'robotName', ip: 'localhost', port: 31950 }, 'foo', true], + args: [mockRobot, 'foo', true], expected: { type: 'robotSettings:UPDATE_SETTING', - meta: { settingId: 'foo' }, payload: { - host: { name: 'robotName', ip: 'localhost', port: 31950 }, + host: mockRobot, method: 'POST', path: '/settings', body: { id: 'foo', value: true }, }, }, }, - { - name: 'robotSettings:CLEAR_RESTART_REQUIRED', - creator: Actions.clearRestartRequired, - args: ['robotName'], - expected: { - type: 'robotSettings:CLEAR_RESTART_REQUIRED', - payload: { robotName: 'robotName' }, - }, - }, ] SPECS.forEach(spec => { diff --git a/app/src/robot-settings/__tests__/epics.test.js b/app/src/robot-settings/__tests__/epics.test.js index 6c15d40d8a0..05e4cd3bce9 100644 --- a/app/src/robot-settings/__tests__/epics.test.js +++ b/app/src/robot-settings/__tests__/epics.test.js @@ -70,7 +70,7 @@ describe('robotSettingsEpic', () => { path: '/settings', body: { id: 'settingId', value: true }, }, - meta: { settingId: 'settingId' }, + meta: {}, }, }) }) diff --git a/app/src/robot-settings/__tests__/reducer.test.js b/app/src/robot-settings/__tests__/reducer.test.js index d32e3b19b56..e8874ab9e14 100644 --- a/app/src/robot-settings/__tests__/reducer.test.js +++ b/app/src/robot-settings/__tests__/reducer.test.js @@ -38,12 +38,12 @@ describe('robotSettingsReducer', () => { { id: 'foo', title: 'Foo', description: 'foobar', value: true }, { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, ], - restartRequired: false, + restartPath: null, }, }, }, { - name: 'handles subsequent robotApi:RESPONSE for GET /settings', + name: 'handles robotApi:RESPONSE for GET /settings with restart required', action: { type: 'robotApi:RESPONSE__GET__/settings', meta: {}, @@ -55,6 +55,7 @@ describe('robotSettingsReducer', () => { settings: [ { id: 'foo', title: 'Foo', description: 'foobar', value: true }, ], + links: { restart: '/server/restart' }, }, }, }, @@ -64,7 +65,7 @@ describe('robotSettingsReducer', () => { { id: 'foo', title: 'Foo', description: 'foobar', value: true }, { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, ], - restartRequired: true, + restartPath: null, }, }, expected: { @@ -72,7 +73,7 @@ describe('robotSettingsReducer', () => { settings: [ { id: 'foo', title: 'Foo', description: 'foobar', value: true }, ], - restartRequired: true, + restartPath: '/server/restart', }, }, }, @@ -96,7 +97,7 @@ describe('robotSettingsReducer', () => { state: { robotName: { settings: [], - restartRequired: false, + restartPath: null, }, }, expected: { @@ -105,43 +106,7 @@ describe('robotSettingsReducer', () => { { id: 'foo', title: 'Foo', description: 'foobar', value: true }, { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, ], - restartRequired: false, - }, - }, - }, - { - name: 'handles robotApi:RESPONSE for POST /settings', - action: { - type: 'robotApi:RESPONSE__POST__/settings', - meta: { settingId: 'bar' }, - payload: { - host: { name: 'robotName' }, - method: 'POST', - path: '/settings', - body: { - settings: [ - { id: 'foo', title: 'Foo', description: 'foobar', value: true }, - { id: 'bar', title: 'Bar', description: 'bazqux', value: true }, - ], - }, - }, - }, - state: { - robotName: { - settings: [ - { id: 'foo', title: 'Foo', description: 'foobar', value: true }, - { id: 'bar', title: 'Bar', description: 'bazqux', value: false }, - ], - restartRequired: false, - }, - }, - expected: { - robotName: { - settings: [ - { id: 'foo', title: 'Foo', description: 'foobar', value: true }, - { id: 'bar', title: 'Bar', description: 'bazqux', value: true }, - ], - restartRequired: false, + restartPath: null, }, }, }, @@ -165,6 +130,7 @@ describe('robotSettingsReducer', () => { restart_required: true, }, ], + links: { restart: '/server/restart' }, }, }, }, @@ -179,7 +145,7 @@ describe('robotSettingsReducer', () => { restart_required: true, }, ], - restartRequired: false, + restartPath: null, }, }, expected: { @@ -193,29 +159,10 @@ describe('robotSettingsReducer', () => { restart_required: true, }, ], - restartRequired: true, + restartPath: '/server/restart', }, }, }, - { - name: 'handles CLEAR_RESTART_REQUIRED', - action: { - type: 'robotSettings:CLEAR_RESTART_REQUIRED', - payload: { robotName: 'robotName' }, - }, - state: { robotName: { settings: [], restartRequired: true } }, - expected: { robotName: { settings: [], restartRequired: false } }, - }, - { - name: 'handles robotAdmin:RESTART', - action: { - type: 'robotAdmin:RESTART', - meta: { robot: true }, - payload: { host: { name: 'robotName', ip: 'localhost', port: 31950 } }, - }, - state: { robotName: { settings: [], restartRequired: true } }, - expected: { robotName: { settings: [], restartRequired: false } }, - }, ] SPECS.forEach(spec => { diff --git a/app/src/robot-settings/__tests__/selectors.test.js b/app/src/robot-settings/__tests__/selectors.test.js index 63a5bb42d06..a48bf76bc87 100644 --- a/app/src/robot-settings/__tests__/selectors.test.js +++ b/app/src/robot-settings/__tests__/selectors.test.js @@ -18,7 +18,7 @@ describe('robot settings selectors', () => { state: { robotSettings: { robotName: { - restartRequired: false, + restartPath: null, settings: [ { id: 'foo', title: 'Foo', description: 'Foo', value: true }, ], @@ -29,19 +29,32 @@ describe('robot settings selectors', () => { expected: [{ id: 'foo', title: 'Foo', description: 'Foo', value: true }], }, { - name: 'getRobotRestartRequired', + name: 'getRobotRestartPath', + selector: Selectors.getRobotRestartPath, + state: { + robotSettings: { robotName: { restartPath: '/restart', settings: [] } }, + }, + args: ['robotName'], + expected: '/restart', + }, + { + name: 'getRobotRestartRequired when required', selector: Selectors.getRobotRestartRequired, state: { - robotSettings: { - robotName: { - restartRequired: true, - settings: [], - }, - }, + robotSettings: { robotName: { restartPath: '/restart', settings: [] } }, }, args: ['robotName'], expected: true, }, + { + name: 'getRobotRestartRequired when not required', + selector: Selectors.getRobotRestartRequired, + state: { + robotSettings: { robotName: { restartPath: null, settings: [] } }, + }, + args: ['robotName'], + expected: false, + }, ] SPECS.forEach(spec => { diff --git a/app/src/robot-settings/actions.js b/app/src/robot-settings/actions.js index 0338a6dfe2a..07e37311f6f 100644 --- a/app/src/robot-settings/actions.js +++ b/app/src/robot-settings/actions.js @@ -1,12 +1,7 @@ // @flow import { GET, POST } from '../robot-api/utils' -import { - FETCH_SETTINGS, - UPDATE_SETTING, - CLEAR_RESTART_REQUIRED, - SETTINGS_PATH, -} from './constants' +import { FETCH_SETTINGS, UPDATE_SETTING, SETTINGS_PATH } from './constants' import type { RobotHost } from '../robot-api/types' import type { RobotSettingsAction } from './types' @@ -22,7 +17,6 @@ export const updateSetting = ( value: boolean | null ): RobotSettingsAction => ({ type: UPDATE_SETTING, - meta: { settingId }, payload: { host, method: POST, @@ -30,10 +24,3 @@ export const updateSetting = ( body: { id: settingId, value }, }, }) - -export const clearRestartRequired = ( - robotName: string -): RobotSettingsAction => ({ - type: CLEAR_RESTART_REQUIRED, - payload: { robotName }, -}) diff --git a/app/src/robot-settings/constants.js b/app/src/robot-settings/constants.js index c6366b2d288..aa396b0b6c7 100644 --- a/app/src/robot-settings/constants.js +++ b/app/src/robot-settings/constants.js @@ -8,5 +8,5 @@ export const FETCH_SETTINGS: 'robotSettings:FETCH_SETTINGS' = export const UPDATE_SETTING: 'robotSettings:UPDATE_SETTING' = 'robotSettings:UPDATE_SETTING' -export const CLEAR_RESTART_REQUIRED: 'robotSettings:CLEAR_RESTART_REQUIRED' = - 'robotSettings:CLEAR_RESTART_REQUIRED' +export const APPLY_WITH_RESTART: 'robotSettings:APPLY_WITH_RESTART' = + 'robotSettings:APPLY_WITH_RESTART' diff --git a/app/src/robot-settings/epic.js b/app/src/robot-settings/epic.js index a10630023f2..eb0995ac23b 100644 --- a/app/src/robot-settings/epic.js +++ b/app/src/robot-settings/epic.js @@ -5,15 +5,13 @@ import { makeRobotApiRequest } from '../robot-api/utils' import { FETCH_SETTINGS, UPDATE_SETTING } from './constants' import type { Epic } from '../types' -import type { RequestMeta } from '../robot-api/types' -import type { RobotSettingsApiAction } from './types' +import type { RobotSettingsAction } from './types' export const robotSettingsEpic: Epic = action$ => { return action$.pipe( ofType(FETCH_SETTINGS, UPDATE_SETTING), - switchMap(a => { - const meta: RequestMeta = a.meta || {} - return makeRobotApiRequest(a.payload, meta) + switchMap(a => { + return makeRobotApiRequest(a.payload, {}) }) ) } diff --git a/app/src/robot-settings/reducer.js b/app/src/robot-settings/reducer.js index 133cda5ac1c..a78b04fa678 100644 --- a/app/src/robot-settings/reducer.js +++ b/app/src/robot-settings/reducer.js @@ -1,10 +1,9 @@ // @flow -import { passRobotApiResponseAction, POST } from '../robot-api/utils' -import { RESTART } from '../robot-admin' -import { CLEAR_RESTART_REQUIRED, SETTINGS_PATH } from './constants' +import { passRobotApiResponseAction } from '../robot-api/utils' +import { SETTINGS_PATH } from './constants' import type { Action, ActionLike } from '../types' -import type { RobotSettings, RobotSettingsState } from './types' +import type { RobotSettingsResponse, RobotSettingsState } from './types' export const INITIAL_STATE: RobotSettingsState = {} @@ -15,45 +14,18 @@ export function robotSettingsReducer( const resAction = passRobotApiResponseAction(action) if (resAction) { - const { payload, meta } = resAction - const { host, method, path, body } = payload + const { payload } = resAction + const { host, path, body } = payload const { name: robotName } = host // grabs responses from GET /settings and POST /settings // settings in body check is a guard against an old version of GET /settings if (path === SETTINGS_PATH && 'settings' in body) { - const robotState = state[robotName] - const settings: RobotSettings = body.settings - // restart is required if the setting we just updated (meta.settingId) - // is a restart_required setting, or if restart was already required - const restartRequired = - Boolean(robotState?.restartRequired) || - (method === POST && - settings.some( - s => s.id === meta.settingId && s.restart_required === true - )) + const { settings, links } = (body: RobotSettingsResponse) + // restart is required if `links` comes back with a `restart` field + const restartPath = links?.restart || null - return { ...state, [robotName]: { settings, restartRequired } } - } - } - - switch (action.type) { - case CLEAR_RESTART_REQUIRED: { - const { robotName } = action.payload - - return { - ...state, - [robotName]: { ...state[robotName], restartRequired: false }, - } - } - - case RESTART: { - const { name: robotName } = action.payload.host - - return { - ...state, - [robotName]: { ...state[robotName], restartRequired: false }, - } + return { ...state, [robotName]: { settings, restartPath } } } } diff --git a/app/src/robot-settings/selectors.js b/app/src/robot-settings/selectors.js index 8a828bc549b..8ada86f754a 100644 --- a/app/src/robot-settings/selectors.js +++ b/app/src/robot-settings/selectors.js @@ -11,9 +11,16 @@ export function getRobotSettings( return robotState(state, robotName)?.settings || [] } +export function getRobotRestartPath( + state: State, + robotName: string +): string | null { + return robotState(state, robotName)?.restartPath || null +} + export function getRobotRestartRequired( state: State, robotName: string ): boolean { - return robotState(state, robotName)?.restartRequired || false + return getRobotRestartPath(state, robotName) !== null } diff --git a/app/src/robot-settings/types.js b/app/src/robot-settings/types.js index b86388d078e..fae771a737b 100644 --- a/app/src/robot-settings/types.js +++ b/app/src/robot-settings/types.js @@ -12,9 +12,14 @@ export type RobotSettingsField = {| export type RobotSettings = Array +export type RobotSettingsResponse = {| + settings: RobotSettings, + links?: {| restart?: string |}, +|} + export type PerRobotRobotSettingsState = {| settings: RobotSettings, - restartRequired: boolean, + restartPath: string | null, |} export type RobotSettingsState = $Shape<{| @@ -26,17 +31,6 @@ export type RobotSettingsFieldUpdate = {| value: $PropertyType, |} -export type RobotSettingsApiAction = - | {| type: 'robotSettings:FETCH_SETTINGS', payload: RobotApiRequest |} - | {| - type: 'robotSettings:UPDATE_SETTING', - payload: RobotApiRequest, - meta: {| settingId: string |}, - |} - export type RobotSettingsAction = - | RobotSettingsApiAction - | {| - type: 'robotSettings:CLEAR_RESTART_REQUIRED', - payload: {| robotName: string |}, - |} + | {| type: 'robotSettings:FETCH_SETTINGS', payload: RobotApiRequest |} + | {| type: 'robotSettings:UPDATE_SETTING', payload: RobotApiRequest |}