fix(app): disable module commands when protocol paused (#5209)

The module command cards displayed in the protocol run page have historically allowed users to send module commands (like setting temperature) when the protocol was active but paused. That used to work essentially as a side effect: modules didn't respect pausing, so you could tell them to do things while the protocol was paused. Now that modules respect pausing, sending module commands when the protocol is paused results in the modules not doing anything. The science and UX intent of controlling modules from the protocol run page is to implement pre-protocol preparation (like preheat) or post-protocol behavior (like keeping some incubation going), not to control the modules when paused - and in fact, when more advanced module control behaviors land in protocol designer or in the python api, controlling modules when the protocol is paused could break protocol execution. This PR changes the module command cards to only work when the protocol is not paused or running. When the module cards are disabled, the action button is now displayed but inaccessible, with an explanatory tooltip on hover. Co-authored-by: Mike Cousins <[email protected]>
Opentrons · Mar 18, 2020 · b313c95 · b313c95
1 parent 19dce65
commit b313c95
Show file tree

Hide file tree

Showing 16 changed files with 227 additions and 284 deletions.
diff --git a/app/src/components/InstrumentSettings/AttachedModulesCard.js b/app/src/components/InstrumentSettings/AttachedModulesCard.js
@@ -4,7 +4,11 @@ import * as React from 'react'
 import { useDispatch, useSelector } from 'react-redux'
 
 import { Card, useInterval } from '@opentrons/components'
-import { fetchModules, getAttachedModules } from '../../modules'
+import {
+  fetchModules,
+  getAttachedModules,
+  getModuleControlsDisabled,
+} from '../../modules'
 import { getConnectedRobotName } from '../../robot/selectors'
 import { ModulesCardContents } from './ModulesCardContents'
 
@@ -22,7 +26,9 @@ export function AttachedModulesCard(props: Props) {
   const modules = useSelector((state: State) =>
     getAttachedModules(state, robotName)
   )
-  const canControl = connectedRobotName === robotName
+  const controlDisabledReason = useSelector((state: State) =>
+    getModuleControlsDisabled(state, robotName)
+  )
 
   // if robot is connected, the modules epic will poll /modules automatically,
   // but we need to poll ourselves if we're viewing this robot without
@@ -35,7 +41,10 @@ export function AttachedModulesCard(props: Props) {
 
   return (
     <Card title={TITLE}>
-      <ModulesCardContents modules={modules} canControl={canControl} />
+      <ModulesCardContents
+        modules={modules}
+        controlDisabledReason={controlDisabledReason}
+      />
     </Card>
   )
 }
diff --git a/app/src/components/InstrumentSettings/ModulesCardContents.js b/app/src/components/InstrumentSettings/ModulesCardContents.js
@@ -7,17 +7,21 @@ import type { AttachedModule } from '../../modules/types'
 
 type Props = {|
   modules: Array<AttachedModule>,
-  canControl: boolean,
+  controlDisabledReason: string | null,
 |}
 
 export function ModulesCardContents(props: Props) {
-  const { modules, canControl } = props
+  const { modules, controlDisabledReason } = props
   if (modules.length === 0) return <NoModulesMessage />
 
   return (
     <>
       {modules.map(mod => (
-        <ModuleItem key={mod.serial} module={mod} canControl={canControl} />
+        <ModuleItem
+          key={mod.serial}
+          module={mod}
+          controlDisabledReason={controlDisabledReason}
+        />
       ))}
     </>
   )

diff --git a/app/src/components/ModuleControls/TemperatureControl.js b/app/src/components/ModuleControls/TemperatureControl.js
@@ -18,21 +18,20 @@ import type {
 import { THERMOCYCLER_MODULE_TYPE } from '../../modules'
 import { getModuleDisplayName } from '@opentrons/shared-data'
 
-const CONNECT_FOR_CONTROL = 'Connect to robot to control modules'
 type Props = {|
   module: ThermocyclerModule | TemperatureModule,
   sendModuleCommand: (
     moduleId: string,
     command: ModuleCommand,
     args?: Array<mixed>
   ) => mixed,
-  disabled?: boolean,
+  disabledReason?: string | null,
 |}
 
 export const TemperatureControl = ({
   module,
   sendModuleCommand,
-  disabled,
+  disabledReason,
 }: Props) => {
   const [primaryTempValue, setPrimaryTempValue] = useState(null)
   const [secondaryTempValue, setSecondaryTempValue] = useState(null)
@@ -125,12 +124,12 @@ export const TemperatureControl = ({
           </AlertModal>
         </Portal>
       )}
-      <HoverTooltip tooltipComponent={disabled ? CONNECT_FOR_CONTROL : null}>
+      <HoverTooltip tooltipComponent={disabledReason}>
         {hoverTooltipHandlers => (
           <div {...hoverTooltipHandlers}>
             <OutlineButton
               onClick={handleClick}
-              disabled={disabled}
+              disabled={disabledReason != null}
               className={styles.temp_control_button}
             >
               {hasTarget === true ? 'Deactivate' : 'Set Temp'}

diff --git a/app/src/components/ModuleControls/index.js b/app/src/components/ModuleControls/index.js
@@ -10,11 +10,11 @@ import type { TemperatureModule, ThermocyclerModule } from '../../modules/types'
 
 type Props = {|
   module: TemperatureModule | ThermocyclerModule,
-  canControl: boolean,
+  controlDisabledReason: string | null,
 |}
 
 export function ModuleControls(props: Props) {
-  const { module: mod, canControl } = props
+  const { module: mod, controlDisabledReason } = props
   const sendModuleCommand = useSendModuleCommand()
 
   return (
@@ -43,7 +43,7 @@ export function ModuleControls(props: Props) {
       <div className={styles.control_wrapper}>
         <TemperatureControl
           module={mod}
-          disabled={!canControl}
+          disabledReason={controlDisabledReason}
           sendModuleCommand={sendModuleCommand}
         />
       </div>

diff --git a/app/src/components/ModuleItem/ModuleUpdate.js b/app/src/components/ModuleItem/ModuleUpdate.js
@@ -24,7 +24,6 @@ import type { RequestState } from '../../robot-api/types'
 import styles from './styles.css'
 
 const FW_IS_UP_TO_DATE = 'Module Firmware is up to date'
-const CONNECT_TO_UPDATE = 'Connect to Robot to update Module'
 const OK_TEXT = 'Ok'
 
 const UPDATE = 'update'
@@ -36,19 +35,20 @@ const FAILED_UPDATE_BODY =
 
 type Props = {|
   hasAvailableUpdate: boolean,
-  canControl: boolean,
+  controlDisabledReason: string | null,
   moduleId: string,
 |}
 
 export function ModuleUpdate(props: Props) {
-  const { hasAvailableUpdate, moduleId, canControl } = props
+  const { hasAvailableUpdate, moduleId, controlDisabledReason } = props
   const dispatch = useDispatch<Dispatch>()
   const robotName = useSelector(getConnectedRobotName)
   const [
     dispatchApiRequest,
     requestIds,
   ] = useDispatchApiRequest<UpdateModuleAction>()
 
+  const canControl = controlDisabledReason === null
   const handleClick = () => {
     canControl &&
       robotName &&
@@ -62,8 +62,11 @@ export function ModuleUpdate(props: Props) {
 
   const buttonText = hasAvailableUpdate ? UPDATE : UPDATE_TO_DATE
   let tooltipText = null
-  if (!canControl) tooltipText = CONNECT_TO_UPDATE
-  if (!hasAvailableUpdate) tooltipText = FW_IS_UP_TO_DATE
+  if (!hasAvailableUpdate) {
+    tooltipText = FW_IS_UP_TO_DATE
+  } else if (controlDisabledReason !== null) {
+    tooltipText = controlDisabledReason
+  }
 
   const handleCloseErrorModal = () => {
     dispatch(dismissRequest(latestRequestId))

diff --git a/app/src/components/ModuleItem/__tests__/ModuleUpdate.test.js b/app/src/components/ModuleItem/__tests__/ModuleUpdate.test.js
@@ -1,7 +1,7 @@
 // @flow
 import * as React from 'react'
 import { Provider } from 'react-redux'
-import { shallow, mount } from 'enzyme'
+import { mount } from 'enzyme'
 import configureMockStore from 'redux-mock-store'
 
 import { Button } from '@opentrons/components'
@@ -11,6 +11,7 @@ const mockStore = configureMockStore([])
 
 describe('ModuleUpdate', () => {
   let store
+
   beforeEach(() => {
     store = mockStore({
       mockState: true,
@@ -25,84 +26,88 @@ describe('ModuleUpdate', () => {
     jest.resetAllMocks()
   })
 
-  it('component renders', () => {
-    const treeAvailableCanControl = shallow(
-      <Provider store={store}>
-        <ModuleUpdate
-          canControl={true}
-          moduleId="FAKEMODULE1234"
-          hasAvailableUpdate={true}
-        />
-      </Provider>
-    )
-    const treeAvailableNoControl = shallow(
-      <Provider store={store}>
-        <ModuleUpdate
-          canControl={false}
-          moduleId="FAKEMODULE1234"
-          hasAvailableUpdate={true}
-        />
-      </Provider>
-    )
-    const treeNotAvailableCanControl = shallow(
-      <Provider store={store}>
-        <ModuleUpdate
-          canControl={true}
-          moduleId="FAKEMODULE1234"
-          hasAvailableUpdate={false}
-        />
-      </Provider>
-    )
-    const treeNotAvailableNoControl = shallow(
-      <Provider store={store}>
-        <ModuleUpdate
-          canControl={false}
-          moduleId="FAKEMODULE1234"
-          hasAvailableUpdate={false}
-        />
-      </Provider>
-    )
-    expect(treeAvailableCanControl).toMatchSnapshot()
-    expect(treeNotAvailableCanControl).toMatchSnapshot()
-    expect(treeAvailableNoControl).toMatchSnapshot()
-    expect(treeNotAvailableNoControl).toMatchSnapshot()
-  })
+  // TODO(mc, 2020-03-16): these shallow snapshots don't test anything
+  // remove commented out tests when actual tests have been written
+  // it('component renders', () => {
+  //   const treeAvailableCanControl = shallow(
+  //     <Provider store={store}>
+  //       <ModuleUpdate
+  //         controlDisabledReason={null}
+  //         moduleId="FAKEMODULE1234"
+  //         hasAvailableUpdate={true}
+  //       />
+  //     </Provider>,
+  //   )
+  //   const treeAvailableNoControl = shallow(
+  //     <Provider store={store}>
+  //       <ModuleUpdate
+  //         controlDisabledReason={"Can't touch this"}
+  //         moduleId="FAKEMODULE1234"
+  //         hasAvailableUpdate={true}
+  //       />
+  //     </Provider>
+  //   )
+  //   const treeNotAvailableCanControl = shallow(
+  //     <Provider store={store}>
+  //       <ModuleUpdate
+  //         controlDisabledReason={null}
+  //         moduleId="FAKEMODULE1234"
+  //         hasAvailableUpdate={false}
+  //       />
+  //     </Provider>
+  //   )
+  //   const treeNotAvailableNoControl = shallow(
+  //     <Provider store={store}>
+  //       <ModuleUpdate
+  //         controlDisabledReason={"Can't touch this"}
+  //         moduleId="FAKEMODULE1234"
+  //         hasAvailableUpdate={false}
+  //       />
+  //     </Provider>
+  //   )
+  //   expect(treeAvailableCanControl).toMatchSnapshot()
+  //   expect(treeNotAvailableCanControl).toMatchSnapshot()
+  //   expect(treeAvailableNoControl).toMatchSnapshot()
+  //   expect(treeNotAvailableNoControl).toMatchSnapshot()
+  // })
 
   it('displays a Warning for invalid files', () => {
     const SPECS = [
       {
-        canControl: true,
+        controlDisabledReason: null,
         hasAvailableUpdate: true,
         expectDisabled: false,
       },
       {
-        canControl: true,
+        controlDisabledReason: null,
         hasAvailableUpdate: false,
         expectDisabled: true,
       },
       {
-        canControl: false,
+        controlDisabledReason: "Can't touch this",
         hasAvailableUpdate: true,
         expectDisabled: true,
       },
       {
-        canControl: false,
+        controlDisabledReason: "Can't touch this",
         hasAvailableUpdate: false,
         expectDisabled: true,
       },
     ]
 
-    SPECS.forEach(({ canControl, hasAvailableUpdate, expectDisabled }) => {
-      const wrapper = mount(
-        <Provider store={store}>
-          <ModuleUpdate
-            moduleId="FAKEMODULE1234"
-            canControl={canControl}
-            hasAvailableUpdate={hasAvailableUpdate}
-          />
-        </Provider>
-      )
-      expect(wrapper.find(Button).prop('disabled')).toEqual(expectDisabled)
-    })
+    SPECS.forEach(
+      ({ controlDisabledReason, hasAvailableUpdate, expectDisabled }) => {
+        const wrapper = mount(
+          <Provider store={store}>
+            <ModuleUpdate
+              moduleId="FAKEMODULE1234"
+              controlDisabledReason={controlDisabledReason}
+              hasAvailableUpdate={hasAvailableUpdate}
+            />
+          </Provider>
+        )
+        expect(wrapper.find(Button).prop('disabled')).toEqual(expectDisabled)
+      }
+    )
   })
 })