fix(app): fix Browse file system button issues (#11113)

This PR will fix Browse file system button bugs (make disabled work as expected and use
useIsRobotBusy correctly)
fix #11105

Author: koji

app/src/assets/localization/en/device_settings.json

@@ -94,7 +94,7 @@
   "jupyter_notebook_link": "Learn more about using Jupyter notebook with the OT-2",
   "update_robot_software": "Update robot software manually with a local file (.zip)",
   "update_robot_software_description": "Bypass the Opentrons App auto-update process and update the robot software manually.",
-  "update_robot_software_browse_button": "Browse file system",
+  "browse_file_system": "Browse file system",
   "update_robot_software_link": "Launch Opentrons software update page",
   "update_robot_software_troubleshooting": "Troubleshooting",
   "update_robot_software_download_logs": "Download logs",

app/src/organisms/Devices/RobotSettings/AdvancedTab/DisplayRobotName.tsx

@@ -14,7 +14,6 @@ import {
 import { StyledText } from '../../../../atoms/text'
 import { TertiaryButton } from '../../../../atoms/buttons'
 import { useIsRobotBusy } from '../../hooks'
-
 interface DisplayRobotNameProps {
   robotName: string
   updateIsExpanded: (

app/src/organisms/Devices/RobotSettings/AdvancedTab/UpdateRobotSoftware.tsx

@@ -10,15 +10,14 @@ import {
   SPACING,
   SPACING_AUTO,
   Box,
-  Tooltip,
   useHoverTooltip,
   TYPOGRAPHY,
 } from '@opentrons/components'
 
 import { StyledText } from '../../../../atoms/text'
 import { ExternalLink } from '../../../../atoms/Link/ExternalLink'
 import { TertiaryButton } from '../../../../atoms/buttons'
-import { useIsRobotBusy } from '../../hooks'
+import { Tooltip } from '../../../../atoms/Tooltip'
 import {
   getBuildrootUpdateDisplayInfo,
   startBuildrootUpdate,
@@ -34,14 +33,14 @@ const HIDDEN_CSS = css`
 
 interface UpdateRobotSoftwareProps {
   robotName: string
-  updateIsRobotBusy: (isRobotBusy: boolean) => void
   onUpdateStart: () => void
+  isRobotBusy: boolean
 }
 
 export function UpdateRobotSoftware({
   robotName,
-  updateIsRobotBusy,
   onUpdateStart,
+  isRobotBusy,
 }: UpdateRobotSoftwareProps): JSX.Element {
   const { t } = useTranslation('device_settings')
   const dispatch = useDispatch<Dispatch>()
@@ -50,20 +49,20 @@ export function UpdateRobotSoftware({
   })
   const updateDisabled = updateFromFileDisabledReason !== null
   const [updateButtonProps, updateButtonTooltipProps] = useHoverTooltip()
-  const isBusy = useIsRobotBusy()
+  const inputRef = React.useRef<HTMLInputElement>(null)
 
   const handleChange: React.ChangeEventHandler<HTMLInputElement> = event => {
-    if (isBusy) {
-      updateIsRobotBusy(true)
-    } else {
-      const { files } = event.target
-      if (files?.length === 1 && !updateDisabled) {
-        dispatch(startBuildrootUpdate(robotName, files[0].path))
-        onUpdateStart()
-      }
+    const { files } = event.target
+    if (files?.length === 1 && !updateDisabled) {
+      dispatch(startBuildrootUpdate(robotName, files[0].path))
+      onUpdateStart()
     }
   }
 
+  const handleClick: React.MouseEventHandler<HTMLButtonElement> = () => {
+    inputRef.current?.click()
+  }
+
   return (
     <Flex
       alignItems={ALIGN_CENTER}
@@ -84,21 +83,23 @@ export function UpdateRobotSoftware({
         </ExternalLink>
       </Box>
       <TertiaryButton
-        as="label"
         marginLeft={SPACING_AUTO}
         id="AdvancedSettings_softwareUpdateButton"
         {...updateButtonProps}
+        disabled={updateDisabled || isRobotBusy}
+        onClick={handleClick}
       >
-        {t('update_robot_software_browse_button')}
+        {t('browse_file_system')}
         <input
+          ref={inputRef}
           type="file"
           onChange={handleChange}
           disabled={updateDisabled}
           css={HIDDEN_CSS}
         />
       </TertiaryButton>
       {updateFromFileDisabledReason != null && (
-        <Tooltip {...updateButtonTooltipProps}>
+        <Tooltip tooltipProps={updateButtonTooltipProps}>
           {updateFromFileDisabledReason}
         </Tooltip>
       )}

app/src/organisms/Devices/RobotSettings/AdvancedTab/__tests__/UpdateRobotSoftware.test.tsx

@@ -1,12 +1,10 @@
 import * as React from 'react'
 import { MemoryRouter } from 'react-router-dom'
-import { fireEvent } from '@testing-library/react'
 
 import { renderWithProviders } from '@opentrons/components'
 
 import { i18n } from '../../../../../i18n'
 import { getBuildrootUpdateDisplayInfo } from '../../../../../redux/buildroot'
-import { useIsRobotBusy } from '../../../hooks'
 
 import { UpdateRobotSoftware } from '../UpdateRobotSoftware'
 
@@ -15,13 +13,9 @@ jest.mock('../../../../../redux/discovery')
 jest.mock('../../../../../redux/buildroot/selectors')
 jest.mock('../../../hooks')
 
-const mockUpdateRobotStatus = jest.fn()
 const mockGetBuildrootUpdateDisplayInfo = getBuildrootUpdateDisplayInfo as jest.MockedFunction<
   typeof getBuildrootUpdateDisplayInfo
 >
-const mockUseIsRobotBusy = useIsRobotBusy as jest.MockedFunction<
-  typeof useIsRobotBusy
->
 
 const mockOnUpdateStart = jest.fn()
 
@@ -30,7 +24,7 @@ const render = () => {
     <MemoryRouter>
       <UpdateRobotSoftware
         robotName="otie"
-        updateIsRobotBusy={mockUpdateRobotStatus}
+        isRobotBusy={false}
         onUpdateStart={mockOnUpdateStart}
       />
     </MemoryRouter>,
@@ -41,24 +35,23 @@ const render = () => {
 describe('RobotSettings UpdateRobotSoftware', () => {
   beforeEach(() => {
     mockGetBuildrootUpdateDisplayInfo.mockReturnValue({
-      autoUpdateAction: 'reinstall',
+      autoUpdateAction: 'update',
       autoUpdateDisabledReason: null,
       updateFromFileDisabledReason: null,
     })
-    mockUseIsRobotBusy.mockReturnValue(false)
   })
 
   afterEach(() => {
     jest.resetAllMocks()
   })
 
   it('should render title, description and toggle button', () => {
-    const [{ getByText, getByLabelText }] = render()
+    const [{ getByText }] = render()
     getByText('Update robot software manually with a local file (.zip)')
     getByText(
       'Bypass the Opentrons App auto-update process and update the robot software manually.'
     )
-    getByLabelText('Browse file system')
+    getByText('Browse file system')
     getByText('Launch Opentrons software update page')
   })
 
@@ -69,11 +62,14 @@ describe('RobotSettings UpdateRobotSoftware', () => {
     expect(link.closest('a')).toHaveAttribute('href', targetLink)
   })
 
-  it('should call update robot status if a robot is busy', () => {
-    mockUseIsRobotBusy.mockReturnValue(true)
-    const [{ getByLabelText }] = render()
-    const button = getByLabelText('Browse file system')
-    fireEvent.change(button)
-    expect(mockUpdateRobotStatus).toHaveBeenCalled()
+  it('should be disabled if updateFromFileDisabledReason is not null', () => {
+    mockGetBuildrootUpdateDisplayInfo.mockReturnValue({
+      autoUpdateAction: 'update',
+      autoUpdateDisabledReason: null,
+      updateFromFileDisabledReason: 'mock reason',
+    })
+    const [{ getByText }] = render()
+    const button = getByText('Browse file system')
+    expect(button).toBeDisabled()
   })
 })

app/src/organisms/Devices/RobotSettings/RobotSettingsAdvanced.tsx

@@ -6,7 +6,7 @@ import { Box, SPACING, IconProps } from '@opentrons/components'
 
 import { Divider } from '../../../atoms/structure'
 import { Toast } from '../../../atoms/Toast'
-import { useRobot } from '../hooks'
+import { useIsRobotBusy, useRobot } from '../hooks'
 import { DisplayRobotName } from './AdvancedTab/DisplayRobotName'
 import { RobotInformation } from './AdvancedTab/RobotInformation'
 import { RobotServerVersion } from './AdvancedTab/RobotServerVersion'
@@ -25,15 +25,15 @@ import { RenameRobotSlideout } from './AdvancedTab/AdvancedTabSlideouts/RenameRo
 import { FactoryResetSlideout } from './AdvancedTab/AdvancedTabSlideouts/FactoryResetSlideout'
 import { FactoryResetModal } from './AdvancedTab/AdvancedTabSlideouts/FactoryResetModal'
 import { UpdateBuildroot } from './UpdateBuildroot'
+import { UNREACHABLE } from '../../../redux/discovery'
+import { Portal } from '../../../App/portal'
 
 import type { State, Dispatch } from '../../../redux/types'
 import type {
   RobotSettings,
   RobotSettingsField,
 } from '../../../redux/robot-settings/types'
 import type { ResetConfigRequest } from '../../../redux/robot-admin/types'
-import { UNREACHABLE } from '../../../redux/discovery'
-import { Portal } from '../../../App/portal'
 
 interface RobotSettingsAdvancedProps {
   robotName: string
@@ -65,6 +65,8 @@ export function RobotSettingsAdvanced({
     false
   )
 
+  const isRobotBusy = useIsRobotBusy({ poll: true })
+
   const toastIcon: IconProps = { name: 'ot-spinner', spin: true }
   const robot = useRobot(robotName)
   const ipAddress = robot?.ip != null ? robot.ip : ''
@@ -103,6 +105,7 @@ export function RobotSettingsAdvanced({
   const updateDownloadLogsStatus = (isDownloading: boolean): void =>
     setShowDownloadToast(isDownloading)
 
+  // TODO: kj 07/1 this function and all props will be removed by another PR
   const updateIsRobotBusy = (isRobotBusy: boolean): void => {
     updateRobotStatus(isRobotBusy)
   }
@@ -113,6 +116,10 @@ export function RobotSettingsAdvanced({
     dispatch(fetchSettings(robotName))
   }, [dispatch, robotName])
 
+  React.useEffect(() => {
+    updateRobotStatus(isRobotBusy)
+  }, [isRobotBusy, updateRobotStatus])
+
   return (
     <>
       {showSoftwareUpdateModal &&
@@ -185,7 +192,7 @@ export function RobotSettingsAdvanced({
         <Divider marginY={SPACING.spacing5} />
         <UpdateRobotSoftware
           robotName={robotName}
-          updateIsRobotBusy={updateIsRobotBusy}
+          isRobotBusy={isRobotBusy}
           onUpdateStart={() => setShowSoftwareUpdateModal(true)}
         />
         <Divider marginY={SPACING.spacing4} />

app/src/organisms/Devices/__tests__/RecentProtocolRuns.test.tsx

@@ -57,7 +57,7 @@ describe('RecentProtocolRuns', () => {
     mockUseIsRobotViewable.mockReturnValue(true)
     mockUseAllRunsQuery.mockReturnValue({
       data: {},
-    } as UseQueryResult<Runs>)
+    } as UseQueryResult<Runs, Error>)
     const [{ getByText }] = render()
 
     getByText('No protocol runs yet!')
@@ -76,7 +76,7 @@ describe('RecentProtocolRuns', () => {
           },
         ],
       },
-    } as UseQueryResult<Runs>)
+    } as UseQueryResult<Runs, Error>)
     const [{ getByText }] = render()
     getByText(`otie's Protocol Runs`)
     getByText('Run')

app/src/organisms/Devices/hooks/__tests__/useIsRobotBusy.test.ts

@@ -1,45 +1,59 @@
 import { UseQueryResult } from 'react-query'
-import { useAllSessionsQuery } from '@opentrons/react-api-client'
+import {
+  useAllSessionsQuery,
+  useAllRunsQuery,
+} from '@opentrons/react-api-client'
 
-import { useCurrentRunId } from '../../../ProtocolUpload/hooks'
 import { useIsRobotBusy } from '../useIsRobotBusy'
 
-import type { Sessions } from '@opentrons/api-client'
+import type { Sessions, Runs } from '@opentrons/api-client'
 
 jest.mock('@opentrons/react-api-client')
 jest.mock('../../../ProtocolUpload/hooks')
 
-const mockUseCurrentRunId = useCurrentRunId as jest.MockedFunction<
-  typeof useCurrentRunId
->
 const mockUseAllSessionsQuery = useAllSessionsQuery as jest.MockedFunction<
   typeof useAllSessionsQuery
 >
+const mockUseAllRunsQuery = useAllRunsQuery as jest.MockedFunction<
+  typeof useAllRunsQuery
+>
 
 describe('useIsRobotBusy', () => {
   beforeEach(() => {
-    mockUseCurrentRunId.mockReturnValue('123')
     mockUseAllSessionsQuery.mockReturnValue({
       data: {},
     } as UseQueryResult<Sessions, Error>)
+    mockUseAllRunsQuery.mockReturnValue({
+      data: {
+        links: {
+          current: {},
+        },
+      },
+    } as UseQueryResult<Runs, Error>)
   })
 
   afterEach(() => {
     jest.resetAllMocks()
   })
 
   it('returns true when current runId is not null', () => {
-    const result = useIsRobotBusy()
+    const result = useIsRobotBusy({ poll: false })
     expect(result).toBe(true)
   })
 
   it('returns true when sessions are not empty', () => {
-    const result = useIsRobotBusy()
+    const result = useIsRobotBusy({ poll: false })
     expect(result).toBe(true)
   })
 
   it('returns false when current runId is null and sessions are empty', () => {
-    mockUseCurrentRunId.mockReturnValue(null)
+    mockUseAllRunsQuery.mockReturnValue({
+      data: {
+        links: {
+          current: null,
+        },
+      },
+    } as any)
     mockUseAllSessionsQuery.mockReturnValue(({
       data: [
         {
@@ -55,4 +69,16 @@ describe('useIsRobotBusy', () => {
     const result = useIsRobotBusy()
     expect(result).toBe(false)
   })
+
+  // TODO: kj 07/13/2022 This test is temporary pending but should be solved by another PR.
+  // it('should poll the run and sessions if poll option is true', async () => {
+  //   const result = useIsRobotBusy({ poll: true })
+  //   expect(result).toBe(true)
+
+  //   act(() => {
+  //     jest.advanceTimersByTime(30000)
+  //   })
+  //   expect(mockUseAllRunsQuery).toHaveBeenCalled()
+  //   expect(mockUseAllSessionsQuery).toHaveBeenCalled()
+  // })
 })

app/src/organisms/Devices/hooks/useIsRobotBusy.ts

@@ -1,10 +1,21 @@
-import { useAllSessionsQuery } from '@opentrons/react-api-client'
+import {
+  useAllSessionsQuery,
+  useAllRunsQuery,
+} from '@opentrons/react-api-client'
 
-import { useCurrentRunId } from '../../ProtocolUpload/hooks'
+const ROBOT_STATUS_POLL_MS = 30000
 
-export function useIsRobotBusy(): boolean {
-  const robotHasCurrentRun = useCurrentRunId() !== null
-  const allSessionsQueryResponse = useAllSessionsQuery()
+interface UseIsRobotBusyOptions {
+  poll: boolean
+}
+export function useIsRobotBusy(
+  options: UseIsRobotBusyOptions = { poll: false }
+): boolean {
+  const { poll } = options
+  const queryOptions = poll ? { refetchInterval: ROBOT_STATUS_POLL_MS } : {}
+  const robotHasCurrentRun =
+    useAllRunsQuery(queryOptions)?.data?.links?.current != null
+  const allSessionsQueryResponse = useAllSessionsQuery(queryOptions)
   return (
     robotHasCurrentRun ||
     (allSessionsQueryResponse?.data?.data != null &&