From 5c04eabfea08f405387562084fc53e97fdaaa0e1 Mon Sep 17 00:00:00 2001 From: koji Date: Thu, 24 Oct 2024 08:41:06 -0400 Subject: [PATCH 01/17] fix(protocol-designer): fix ListItemDescriptor content and description alignment issue (#16588) fix ListItemDescriptor content and description alignment issue --- .../ListItemChildren/ListItemDescriptor.tsx | 13 ++- .../organisms/MaterialsListModal/index.tsx | 80 +++++++++++------- .../src/organisms/SlotInformation/index.tsx | 26 +++--- .../StepForm/StepTools/MagnetTools/index.tsx | 1 - .../ProtocolOverview/InstrumentsInfo.tsx | 83 +++++++++++++++---- .../ProtocolOverview/LiquidDefinitions.tsx | 17 +++- .../ProtocolOverview/ProtocolMetadata.tsx | 49 ++++++++--- .../src/pages/ProtocolOverview/StepsInfo.tsx | 14 ++-- 8 files changed, 194 insertions(+), 89 deletions(-) diff --git a/components/src/atoms/ListItem/ListItemChildren/ListItemDescriptor.tsx b/components/src/atoms/ListItem/ListItemChildren/ListItemDescriptor.tsx index 12b494ea894..dcedecaa9f8 100644 --- a/components/src/atoms/ListItem/ListItemChildren/ListItemDescriptor.tsx +++ b/components/src/atoms/ListItem/ListItemChildren/ListItemDescriptor.tsx @@ -8,15 +8,14 @@ import { SPACING } from '../../../ui-style-constants' interface ListItemDescriptorProps { type: 'default' | 'large' - description: JSX.Element | string - content: JSX.Element | string - isInSlideout?: boolean + description: JSX.Element + content: JSX.Element } export const ListItemDescriptor = ( props: ListItemDescriptorProps ): JSX.Element => { - const { description, content, type, isInSlideout = false } = props + const { description, content, type } = props return ( - - {description} - - {content} + {description} + {content} ) } diff --git a/protocol-designer/src/organisms/MaterialsListModal/index.tsx b/protocol-designer/src/organisms/MaterialsListModal/index.tsx index 6fa69307eef..14324594969 100644 --- a/protocol-designer/src/organisms/MaterialsListModal/index.tsx +++ b/protocol-designer/src/organisms/MaterialsListModal/index.tsx @@ -11,7 +11,6 @@ import { DIRECTION_ROW, Flex, InfoScreen, - JUSTIFY_SPACE_BETWEEN, LiquidIcon, ListItem, ListItemDescriptor, @@ -32,13 +31,14 @@ import { getInitialDeckSetup } from '../../step-forms/selectors' import { getTopPortalEl } from '../../components/portals/TopPortal' import { selectors as labwareIngredSelectors } from '../../labware-ingred/selectors' import { HandleEnter } from '../../atoms/HandleEnter' +import { LINE_CLAMP_TEXT_STYLE } from '../../atoms' import type { AdditionalEquipmentName } from '@opentrons/step-generation' import type { LabwareOnDeck, ModuleOnDeck } from '../../step-forms' import type { OrderedLiquids } from '../../labware-ingred/types' // ToDo (kk:09/04/2024) this should be removed when break-point is set up -const MODAL_MIN_WIDTH = '36.1875rem' +const MODAL_MIN_WIDTH = '37.125rem' export interface FixtureInList { name: AdditionalEquipmentName @@ -82,6 +82,7 @@ export function MaterialsListModal({ title={t('materials_list')} marginLeft="0rem" minWidth={MODAL_MIN_WIDTH} + childrenPadding={SPACING.spacing24} > @@ -95,13 +96,18 @@ export function MaterialsListModal({ - ) : ( - '' - ) + + {fixture.location != null ? ( + + ) : ( + '' + )} + } content={ + + + } content={ + + + + } + content={ + + {lw.def.metadata.displayName} + } - content={lw.def.metadata.displayName} /> ) @@ -246,29 +262,31 @@ export function MaterialsListModal({ } else { return ( - - - - - {liquid.name ?? t('n/a')} - - - - + + + + {liquid.name ?? t('n/a')} + + + } + content={ - - + } + /> ) } diff --git a/protocol-designer/src/organisms/SlotInformation/index.tsx b/protocol-designer/src/organisms/SlotInformation/index.tsx index cd3550ed7d5..a945ebffcd7 100644 --- a/protocol-designer/src/organisms/SlotInformation/index.tsx +++ b/protocol-designer/src/organisms/SlotInformation/index.tsx @@ -1,4 +1,3 @@ -import type * as React from 'react' import { useTranslation } from 'react-i18next' import { useLocation } from 'react-router-dom' import { @@ -12,6 +11,7 @@ import { StyledText, TYPOGRAPHY, } from '@opentrons/components' +import type { FC } from 'react' import { FLEX_ROBOT_TYPE } from '@opentrons/shared-data' import type { RobotType } from '@opentrons/shared-data' @@ -25,7 +25,7 @@ interface SlotInformationProps { fixtures?: string[] } -export const SlotInformation: React.FC = ({ +export const SlotInformation: FC = ({ location, robotType, liquids = [], @@ -50,10 +50,10 @@ export const SlotInformation: React.FC = ({ {liquids.length > 1 ? ( - + {liquids.join(', ')}} description={t('liquid')} /> @@ -115,18 +115,14 @@ function StackInfo({ title, stackInformation }: StackInfoProps): JSX.Element { - {stackInformation} - - ) : ( - t('none') - ) + + {stackInformation ?? t('none')} + } - description={title} + description={{title}} /> ) diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MagnetTools/index.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MagnetTools/index.tsx index 42768144177..2468923d9c2 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MagnetTools/index.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MagnetTools/index.tsx @@ -83,7 +83,6 @@ export function MagnetTools(props: StepFormProps): JSX.Element { + {' '} + + {t('robotType')} + + + } content={ - robotType === FLEX_ROBOT_TYPE - ? t('shared:opentrons_flex') - : t('shared:ot2') + + {robotType === FLEX_ROBOT_TYPE + ? t('shared:opentrons_flex') + : t('shared:ot2')} + } /> + {' '} + + {t('left_pip')} + + + } + content={ + + {pipetteInfo(leftPipette)} + + } /> + {' '} + + {t('right_pip')} + + + } + content={ + + {pipetteInfo(rightPipette)} + + } /> {robotType === FLEX_ROBOT_TYPE ? ( + {' '} + + {t('extension')} + + + } + content={ + + {isGripperAttached ? t('gripper') : t('na')} + + } /> ) : null} diff --git a/protocol-designer/src/pages/ProtocolOverview/LiquidDefinitions.tsx b/protocol-designer/src/pages/ProtocolOverview/LiquidDefinitions.tsx index fc767242929..dc753bee12f 100644 --- a/protocol-designer/src/pages/ProtocolOverview/LiquidDefinitions.tsx +++ b/protocol-designer/src/pages/ProtocolOverview/LiquidDefinitions.tsx @@ -37,11 +37,15 @@ export function LiquidDefinitions({ + @@ -49,7 +53,14 @@ export function LiquidDefinitions({ } - content={liquid.description ?? t('na')} + content={ + + {liquid.description ?? t('na')} + + } /> )) diff --git a/protocol-designer/src/pages/ProtocolOverview/ProtocolMetadata.tsx b/protocol-designer/src/pages/ProtocolOverview/ProtocolMetadata.tsx index 69d8697765b..e24f016e07c 100644 --- a/protocol-designer/src/pages/ProtocolOverview/ProtocolMetadata.tsx +++ b/protocol-designer/src/pages/ProtocolOverview/ProtocolMetadata.tsx @@ -1,14 +1,15 @@ import { useTranslation } from 'react-i18next' import { - Flex, + Btn, + COLORS, DIRECTION_COLUMN, - SPACING, + Flex, JUSTIFY_SPACE_BETWEEN, - TYPOGRAPHY, - StyledText, ListItem, ListItemDescriptor, - Btn, + SPACING, + StyledText, + TYPOGRAPHY, } from '@opentrons/components' import { BUTTON_LINK_STYLE } from '../../atoms' @@ -62,8 +63,21 @@ export function ProtocolMetadata({ + + {t(`${title}`)} + + + } + content={ + + {value ?? t('na')} + + } /> ) @@ -71,10 +85,23 @@ export function ProtocolMetadata({ + + {t('required_app_version')} + + + } + content={ + + {t('app_version', { + version: REQUIRED_APP_VERSION, + })} + + } /> diff --git a/protocol-designer/src/pages/ProtocolOverview/StepsInfo.tsx b/protocol-designer/src/pages/ProtocolOverview/StepsInfo.tsx index 3eacf7beeba..090db7e78d4 100644 --- a/protocol-designer/src/pages/ProtocolOverview/StepsInfo.tsx +++ b/protocol-designer/src/pages/ProtocolOverview/StepsInfo.tsx @@ -35,12 +35,14 @@ export function StepsInfo({ savedStepForms }: StepsInfoProps): JSX.Element { - {t('number_of_steps')} - + + + {t('number_of_steps')} + + } content={ From 2b18f3f218cbe7411865f7bb2c57f2b5536af787 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Thu, 24 Oct 2024 09:14:06 -0400 Subject: [PATCH 02/17] fix(app): avoid reodering in robot dashboard (#16583) Here are some interesting facts: - Array.sort() mutates - Array.reverse() mutates - react query caches data These facts combined to mean that in the ODD robot dashboard, protocols view, and run history, we would almost always rerender multiple times with rapidly-reordering data and move stuff out from under someone trying to interact with us. I'm actually pretty surprised that these would usually end up in the right order instead of exactly reversed. The fix for this is to quit using those methods, and the way to do that is to mark the data read only. We should do this for all data returned by a react-query hook IMO. ## testing - [x] on the odd, you can navigate away from and back to the dashboard (particularly via the protocols tab) and when you come back to the dashboard all the cards will render in order once and not jump around Closes RQA-3422 --- api-client/src/runs/types.ts | 2 +- .../ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts | 2 +- .../Desktop/Devices/__tests__/RecentProtocolRuns.test.tsx | 8 ++++---- app/src/pages/ODD/ProtocolDashboard/index.tsx | 2 +- app/src/pages/ODD/RobotDashboard/index.tsx | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/api-client/src/runs/types.ts b/api-client/src/runs/types.ts index c53c589b231..2c467b4de4f 100644 --- a/api-client/src/runs/types.ts +++ b/api-client/src/runs/types.ts @@ -112,7 +112,7 @@ export interface GetRunsParams { } export interface Runs { - data: RunData[] + data: readonly RunData[] links: RunsLinks } diff --git a/app/src/organisms/ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts b/app/src/organisms/ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts index 597f2129e42..cadfc5cf618 100644 --- a/app/src/organisms/ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts +++ b/app/src/organisms/ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts @@ -9,7 +9,7 @@ export function useHistoricRunDetails( return allHistoricRuns == null ? [] - : allHistoricRuns.data.sort( + : allHistoricRuns.data.toSorted( (a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() ) diff --git a/app/src/organisms/Desktop/Devices/__tests__/RecentProtocolRuns.test.tsx b/app/src/organisms/Desktop/Devices/__tests__/RecentProtocolRuns.test.tsx index 3b258d2c199..daa1fc10251 100644 --- a/app/src/organisms/Desktop/Devices/__tests__/RecentProtocolRuns.test.tsx +++ b/app/src/organisms/Desktop/Devices/__tests__/RecentProtocolRuns.test.tsx @@ -52,9 +52,9 @@ describe('RecentProtocolRuns', () => { }) it('renders table headers if there are runs', () => { vi.mocked(useIsRobotViewable).mockReturnValue(true) - vi.mocked(useNotifyAllRunsQuery).mockReturnValue({ + vi.mocked(useNotifyAllRunsQuery).mockReturnValue(({ data: { - data: [ + data: ([ { createdAt: '2022-05-04T18:24:40.833862+00:00', current: false, @@ -62,9 +62,9 @@ describe('RecentProtocolRuns', () => { protocolId: 'test_protocol_id', status: 'succeeded', }, - ], + ] as any) as Runs, }, - } as UseQueryResult) + } as any) as UseQueryResult) render() screen.getByText('Recent Protocol Runs') screen.getByText('Run') diff --git a/app/src/pages/ODD/ProtocolDashboard/index.tsx b/app/src/pages/ODD/ProtocolDashboard/index.tsx index ba2efa23949..de775795ded 100644 --- a/app/src/pages/ODD/ProtocolDashboard/index.tsx +++ b/app/src/pages/ODD/ProtocolDashboard/index.tsx @@ -98,7 +98,7 @@ export function ProtocolDashboard(): JSX.Element { } const runData = runs.data?.data != null ? runs.data?.data : [] - const allRunsNewestFirst = runData.sort( + const allRunsNewestFirst = runData.toSorted( (a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() ) const sortedProtocols = sortProtocols( diff --git a/app/src/pages/ODD/RobotDashboard/index.tsx b/app/src/pages/ODD/RobotDashboard/index.tsx index b699f6ab569..80ae745b055 100644 --- a/app/src/pages/ODD/RobotDashboard/index.tsx +++ b/app/src/pages/ODD/RobotDashboard/index.tsx @@ -41,7 +41,7 @@ export function RobotDashboard(): JSX.Element { ) const recentRunsOfUniqueProtocols = (allRunsQueryData?.data ?? []) - .reverse() // newest runs first + .toReversed() // newest runs first .reduce((acc, run) => { if ( acc.some(collectedRun => collectedRun.protocolId === run.protocolId) From 80189200081610abd1d507f2b357da54747769df Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Thu, 24 Oct 2024 10:41:05 -0400 Subject: [PATCH 03/17] refactor(api): Allow cache_tip() to overwrite the current tip (#16587) --- api/src/opentrons/hardware_control/api.py | 5 --- .../instruments/ot2/pipette_handler.py | 8 +++++ .../instruments/ot3/pipette_handler.py | 8 +++++ api/src/opentrons/hardware_control/ot3api.py | 14 +++----- .../protocols/instrument_configurer.py | 11 ++++-- .../execution/hardware_stopper.py | 8 +++-- .../protocol_engine/execution/tip_handler.py | 34 +++++++------------ .../execution/test_hardware_stopper.py | 8 ++--- .../execution/test_tip_handler.py | 4 +-- 9 files changed, 55 insertions(+), 45 deletions(-) diff --git a/api/src/opentrons/hardware_control/api.py b/api/src/opentrons/hardware_control/api.py index 909a50a3d8c..ec019ef2f1d 100644 --- a/api/src/opentrons/hardware_control/api.py +++ b/api/src/opentrons/hardware_control/api.py @@ -1189,11 +1189,6 @@ async def tip_pickup_moves( await self.retract(mount, spec.retract_target) - def cache_tip(self, mount: top_types.Mount, tip_length: float) -> None: - instrument = self.get_pipette(mount) - instrument.add_tip(tip_length=tip_length) - instrument.set_current_volume(0) - async def pick_up_tip( self, mount: top_types.Mount, diff --git a/api/src/opentrons/hardware_control/instruments/ot2/pipette_handler.py b/api/src/opentrons/hardware_control/instruments/ot2/pipette_handler.py index 907788d6dda..c1389ea6a5b 100644 --- a/api/src/opentrons/hardware_control/instruments/ot2/pipette_handler.py +++ b/api/src/opentrons/hardware_control/instruments/ot2/pipette_handler.py @@ -430,6 +430,14 @@ def add_tip(self, mount: MountType, tip_length: float) -> None: f"attach tip called while tip already attached to {instr}" ) + def cache_tip(self, mount: MountType, tip_length: float) -> None: + instrument = self.get_pipette(mount) + if instrument.has_tip: + # instrument.add_tip() would raise an AssertionError if we tried to overwrite an existing tip. + instrument.remove_tip() + instrument.add_tip(tip_length=tip_length) + instrument.set_current_volume(0) + def remove_tip(self, mount: MountType) -> None: instr = self._attached_instruments[mount] attached = self.attached_instruments diff --git a/api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py b/api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py index 9f44f7b0ab8..f64078fcbff 100644 --- a/api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py +++ b/api/src/opentrons/hardware_control/instruments/ot3/pipette_handler.py @@ -440,6 +440,14 @@ def add_tip(self, mount: OT3Mount, tip_length: float) -> None: "attach tip called while tip already attached to {instr}" ) + def cache_tip(self, mount: OT3Mount, tip_length: float) -> None: + instrument = self.get_pipette(mount) + if instrument.has_tip: + # instrument.add_tip() would raise an AssertionError if we tried to overwrite an existing tip. + instrument.remove_tip() + instrument.add_tip(tip_length=tip_length) + instrument.set_current_volume(0) + def remove_tip(self, mount: OT3Mount) -> None: instr = self._attached_instruments[mount] attached = self.attached_instruments diff --git a/api/src/opentrons/hardware_control/ot3api.py b/api/src/opentrons/hardware_control/ot3api.py index 856b755565c..f90a0a539dc 100644 --- a/api/src/opentrons/hardware_control/ot3api.py +++ b/api/src/opentrons/hardware_control/ot3api.py @@ -2236,15 +2236,6 @@ async def _tip_motor_action( ) await self.home_gear_motors() - def cache_tip( - self, mount: Union[top_types.Mount, OT3Mount], tip_length: float - ) -> None: - realmount = OT3Mount.from_mount(mount) - instrument = self._pipette_handler.get_pipette(realmount) - - instrument.add_tip(tip_length=tip_length) - instrument.set_current_volume(0) - async def pick_up_tip( self, mount: Union[top_types.Mount, OT3Mount], @@ -2613,6 +2604,11 @@ def add_tip( ) -> None: self._pipette_handler.add_tip(OT3Mount.from_mount(mount), tip_length) + def cache_tip( + self, mount: Union[top_types.Mount, OT3Mount], tip_length: float + ) -> None: + self._pipette_handler.cache_tip(OT3Mount.from_mount(mount), tip_length) + def remove_tip(self, mount: Union[top_types.Mount, OT3Mount]) -> None: self._pipette_handler.remove_tip(OT3Mount.from_mount(mount)) diff --git a/api/src/opentrons/hardware_control/protocols/instrument_configurer.py b/api/src/opentrons/hardware_control/protocols/instrument_configurer.py index c1292620b74..5cd85716e36 100644 --- a/api/src/opentrons/hardware_control/protocols/instrument_configurer.py +++ b/api/src/opentrons/hardware_control/protocols/instrument_configurer.py @@ -142,17 +142,24 @@ def get_instrument_max_height( """ ... - # todo(mm, 2024-10-17): Consider deleting this in favor of cache_tip(), which is - # the same except for `assert`s, if we can do so without breaking anything. + # todo(mm, 2024-10-17): Consider deleting this in favor of cache_tip() + # if we can do so without breaking anything. def add_tip(self, mount: MountArgType, tip_length: float) -> None: """Inform the hardware that a tip is now attached to a pipette. + If a tip is already attached, this no-ops. + This changes the critical point of the pipette to make sure that the end of the tip is what moves around, and allows liquid handling. """ ... def cache_tip(self, mount: MountArgType, tip_length: float) -> None: + """Inform the hardware that a tip is now attached to a pipette. + + This is like `add_tip()`, except that if a tip is already attached, + this replaces it instead of no-opping. + """ ... def remove_tip(self, mount: MountArgType) -> None: diff --git a/api/src/opentrons/protocol_engine/execution/hardware_stopper.py b/api/src/opentrons/protocol_engine/execution/hardware_stopper.py index 28c310acd70..24055f6b03b 100644 --- a/api/src/opentrons/protocol_engine/execution/hardware_stopper.py +++ b/api/src/opentrons/protocol_engine/execution/hardware_stopper.py @@ -78,7 +78,9 @@ async def _drop_tip(self) -> None: try: if self._state_store.labware.get_fixed_trash_id() == FIXED_TRASH_ID: # OT-2 and Flex 2.15 protocols will default to the Fixed Trash Labware - await self._tip_handler.add_tip(pipette_id=pipette_id, tip=tip) + await self._tip_handler.cache_tip( + pipette_id=pipette_id, tip=tip + ) await self._movement_handler.move_to_well( pipette_id=pipette_id, labware_id=FIXED_TRASH_ID, @@ -90,7 +92,9 @@ async def _drop_tip(self) -> None: ) elif self._state_store.config.robot_type == "OT-2 Standard": # API 2.16 and above OT2 protocols use addressable areas - await self._tip_handler.add_tip(pipette_id=pipette_id, tip=tip) + await self._tip_handler.cache_tip( + pipette_id=pipette_id, tip=tip + ) await self._movement_handler.move_to_addressable_area( pipette_id=pipette_id, addressable_area_name="fixedTrash", diff --git a/api/src/opentrons/protocol_engine/execution/tip_handler.py b/api/src/opentrons/protocol_engine/execution/tip_handler.py index 0fe2462ee5e..a963dd9abac 100644 --- a/api/src/opentrons/protocol_engine/execution/tip_handler.py +++ b/api/src/opentrons/protocol_engine/execution/tip_handler.py @@ -83,7 +83,7 @@ async def drop_tip(self, pipette_id: str, home_after: Optional[bool]) -> None: TipAttachedError """ - async def add_tip(self, pipette_id: str, tip: TipGeometry) -> None: + async def cache_tip(self, pipette_id: str, tip: TipGeometry) -> None: """Tell the Hardware API that a tip is attached.""" async def get_tip_presence(self, pipette_id: str) -> TipPresenceStatus: @@ -234,6 +234,11 @@ async def pick_up_tip( labware_definition=self._state_view.labware.get_definition(labware_id), nominal_fallback=nominal_tip_geometry.length, ) + tip_geometry = TipGeometry( + length=actual_tip_length, + diameter=nominal_tip_geometry.diameter, + volume=nominal_tip_geometry.volume, + ) await self._hardware_api.tip_pickup_moves( mount=hw_mount, presses=None, increment=None @@ -241,24 +246,11 @@ async def pick_up_tip( # Allow TipNotAttachedError to propagate. await self.verify_tip_presence(pipette_id, TipPresenceStatus.PRESENT) - self._hardware_api.cache_tip(hw_mount, actual_tip_length) - await self._hardware_api.prepare_for_aspirate(hw_mount) - - self._hardware_api.set_current_tiprack_diameter( - mount=hw_mount, - tiprack_diameter=nominal_tip_geometry.diameter, - ) + await self.cache_tip(pipette_id, tip_geometry) - self._hardware_api.set_working_volume( - mount=hw_mount, - tip_volume=nominal_tip_geometry.volume, - ) + await self._hardware_api.prepare_for_aspirate(hw_mount) - return TipGeometry( - length=actual_tip_length, - diameter=nominal_tip_geometry.diameter, - volume=nominal_tip_geometry.volume, - ) + return tip_geometry async def drop_tip(self, pipette_id: str, home_after: Optional[bool]) -> None: """See documentation on abstract base class.""" @@ -279,11 +271,11 @@ async def drop_tip(self, pipette_id: str, home_after: Optional[bool]) -> None: self._hardware_api.remove_tip(hw_mount) self._hardware_api.set_current_tiprack_diameter(hw_mount, 0) - async def add_tip(self, pipette_id: str, tip: TipGeometry) -> None: + async def cache_tip(self, pipette_id: str, tip: TipGeometry) -> None: """See documentation on abstract base class.""" hw_mount = self._state_view.pipettes.get_mount(pipette_id).to_hw_mount() - self._hardware_api.add_tip(mount=hw_mount, tip_length=tip.length) + self._hardware_api.cache_tip(mount=hw_mount, tip_length=tip.length) self._hardware_api.set_current_tiprack_diameter( mount=hw_mount, @@ -422,12 +414,12 @@ async def drop_tip( expected_has_tip=True, ) - async def add_tip(self, pipette_id: str, tip: TipGeometry) -> None: + async def cache_tip(self, pipette_id: str, tip: TipGeometry) -> None: """Add a tip using a virtual pipette. This should not be called when using virtual pipettes. """ - assert False, "TipHandler.add_tip should not be used with virtual pipettes" + assert False, "TipHandler.cache_tip should not be used with virtual pipettes" async def verify_tip_presence( self, diff --git a/api/tests/opentrons/protocol_engine/execution/test_hardware_stopper.py b/api/tests/opentrons/protocol_engine/execution/test_hardware_stopper.py index 4c3e629d2ed..d6c69d0b170 100644 --- a/api/tests/opentrons/protocol_engine/execution/test_hardware_stopper.py +++ b/api/tests/opentrons/protocol_engine/execution/test_hardware_stopper.py @@ -158,7 +158,7 @@ async def test_hardware_stopping_sequence_no_tip_drop( decoy.verify(await hardware_api.stop(home_after=False), times=1) decoy.verify( - await mock_tip_handler.add_tip( + await mock_tip_handler.cache_tip( pipette_id="pipette-id", tip=TipGeometry(length=1.0, volume=2.0, diameter=3.0), ), @@ -181,7 +181,7 @@ async def test_hardware_stopping_sequence_no_pipette( ) decoy.when( - await mock_tip_handler.add_tip( + await mock_tip_handler.cache_tip( pipette_id="pipette-id", tip=TipGeometry(length=1.0, volume=2.0, diameter=3.0), ), @@ -271,7 +271,7 @@ async def test_hardware_stopping_sequence_with_fixed_trash( await movement.home( axes=[MotorAxis.X, MotorAxis.Y, MotorAxis.LEFT_Z, MotorAxis.RIGHT_Z] ), - await mock_tip_handler.add_tip( + await mock_tip_handler.cache_tip( pipette_id="pipette-id", tip=TipGeometry(length=1.0, volume=2.0, diameter=3.0), ), @@ -320,7 +320,7 @@ async def test_hardware_stopping_sequence_with_OT2_addressable_area( await movement.home( axes=[MotorAxis.X, MotorAxis.Y, MotorAxis.LEFT_Z, MotorAxis.RIGHT_Z] ), - await mock_tip_handler.add_tip( + await mock_tip_handler.cache_tip( pipette_id="pipette-id", tip=TipGeometry(length=1.0, volume=2.0, diameter=3.0), ), diff --git a/api/tests/opentrons/protocol_engine/execution/test_tip_handler.py b/api/tests/opentrons/protocol_engine/execution/test_tip_handler.py index af5c49faf6a..8ddb8840597 100644 --- a/api/tests/opentrons/protocol_engine/execution/test_tip_handler.py +++ b/api/tests/opentrons/protocol_engine/execution/test_tip_handler.py @@ -289,10 +289,10 @@ async def test_add_tip( MountType.LEFT ) - await subject.add_tip(pipette_id="pipette-id", tip=tip) + await subject.cache_tip(pipette_id="pipette-id", tip=tip) decoy.verify( - mock_hardware_api.add_tip(mount=Mount.LEFT, tip_length=50), + mock_hardware_api.cache_tip(mount=Mount.LEFT, tip_length=50), mock_hardware_api.set_current_tiprack_diameter( mount=Mount.LEFT, tiprack_diameter=5, From eb710c036b9576fabee093765f72c40681719976 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Thu, 24 Oct 2024 10:45:17 -0400 Subject: [PATCH 04/17] feat(robot-server): HTTP API for "Ignore error and skip to next step" (#16564) --- api-client/src/runs/types.ts | 9 +++- .../robot_server/runs/action_models.py | 54 ++++++++++++++----- .../runs/error_recovery_mapping.py | 4 ++ .../runs/error_recovery_models.py | 36 ++++++++++--- .../robot_server/runs/router/base_router.py | 1 - .../robot_server/runs/run_controller.py | 12 +++++ 6 files changed, 94 insertions(+), 22 deletions(-) diff --git a/api-client/src/runs/types.ts b/api-client/src/runs/types.ts index 2c467b4de4f..9a34c2b4978 100644 --- a/api-client/src/runs/types.ts +++ b/api-client/src/runs/types.ts @@ -125,12 +125,15 @@ export const RUN_ACTION_TYPE_PAUSE: 'pause' = 'pause' export const RUN_ACTION_TYPE_STOP: 'stop' = 'stop' export const RUN_ACTION_TYPE_RESUME_FROM_RECOVERY: 'resume-from-recovery' = 'resume-from-recovery' +export const RUN_ACTION_TYPE_RESUME_FROM_RECOVERY_ASSUMING_FALSE_POSITIVE: 'resume-from-recovery-assuming-false-positive' = + 'resume-from-recovery-assuming-false-positive' export type RunActionType = | typeof RUN_ACTION_TYPE_PLAY | typeof RUN_ACTION_TYPE_PAUSE | typeof RUN_ACTION_TYPE_STOP | typeof RUN_ACTION_TYPE_RESUME_FROM_RECOVERY + | typeof RUN_ACTION_TYPE_RESUME_FROM_RECOVERY_ASSUMING_FALSE_POSITIVE export interface RunAction { id: string @@ -172,7 +175,11 @@ export type RunError = RunCommandError * Error Policy */ -export type IfMatchType = 'ignoreAndContinue' | 'failRun' | 'waitForRecovery' +export type IfMatchType = + | 'assumeFalsePositiveAndContinue' + | 'ignoreAndContinue' + | 'failRun' + | 'waitForRecovery' export interface ErrorRecoveryPolicy { policyRules: Array<{ diff --git a/robot-server/robot_server/runs/action_models.py b/robot-server/robot_server/runs/action_models.py index ede27d823c6..c640fb33cae 100644 --- a/robot-server/robot_server/runs/action_models.py +++ b/robot-server/robot_server/runs/action_models.py @@ -7,20 +7,53 @@ class RunActionType(str, Enum): - """The type of the run control action. - - * `"play"`: Start or resume a run. - * `"pause"`: Pause a run. - * `"stop"`: Stop (cancel) a run. - * `"resume-from-recovery"`: Resume normal protocol execution after a command failed, - the run was placed in `awaiting-recovery` mode, and manual recovery steps - were taken. + """The type of the run control action, which determines behavior. + + * `"play"`: Start the run, or resume it after it's been paused. + + * `"pause"`: Pause the run. + + * `"stop"`: Stop (cancel) the run. + + * `"resume-from-recovery"`: Resume normal protocol execution after the run was in + error recovery mode. Continue from however the last command left the robot. + + * `"resume-from-recovery-assuming-false-positive"`: Resume normal protocol execution + after the run was in error recovery mode. Act as if the underlying error was a + false positive. + + To see the difference between `"resume-from-recovery"` and + `"resume-from-recovery-assuming-false-positive"`, suppose we've just entered error + recovery mode after a `commandType: "pickUpTip"` command failed with an + `errorType: "tipPhysicallyMissing"` error. That normally leaves the robot thinking + it has no tip attached. If you use `"resume-from-recovery"`, the robot will run + the next protocol command from that state, acting as if there's no tip attached. + (This may cause another error, if the next command needs a tip.) + Whereas if you use `"resume-from-recovery-assuming-false-positive"`, + the robot will try to nullify the error, thereby acting as if it *does* have a tip + attached. + + Generally: + + * If you've tried to recover from the error by sending your own `intent: "fixit"` + commands to `POST /runs/{id}/commands`, use `"resume-from-recovery"`. It's your + responsibility to ensure your `POST`ed commands leave the robot in a good-enough + state to continue with the protocol. + + * Otherwise, use `"resume-from-recovery-assuming-false-positive"`. + + Do not combine `intent: "fixit"` commands with + `"resume-from-recovery-assuming-false-positive"`—the robot's built-in + false-positive recovery may compete with your own. """ PLAY = "play" PAUSE = "pause" STOP = "stop" RESUME_FROM_RECOVERY = "resume-from-recovery" + RESUME_FROM_RECOVERY_ASSUMING_FALSE_POSITIVE = ( + "resume-from-recovery-assuming-false-positive" + ) class RunActionCreate(BaseModel): @@ -41,7 +74,4 @@ class RunAction(ResourceModel): id: str = Field(..., description="A unique identifier to reference the command.") createdAt: datetime = Field(..., description="When the command was created.") - actionType: RunActionType = Field( - ..., - description="Specific type of action, which determines behavior.", - ) + actionType: RunActionType diff --git a/robot-server/robot_server/runs/error_recovery_mapping.py b/robot-server/robot_server/runs/error_recovery_mapping.py index d29ebf4b054..b548394cd8a 100644 --- a/robot-server/robot_server/runs/error_recovery_mapping.py +++ b/robot-server/robot_server/runs/error_recovery_mapping.py @@ -102,6 +102,10 @@ def _map_error_recovery_type(reaction_if_match: ReactionIfMatch) -> ErrorRecover match reaction_if_match: case ReactionIfMatch.IGNORE_AND_CONTINUE: return ErrorRecoveryType.IGNORE_AND_CONTINUE + case ReactionIfMatch.ASSUME_FALSE_POSITIVE_AND_CONTINUE: + # todo(mm, 2024-10-23): Connect to work in + # https://github.com/Opentrons/opentrons/pull/16556. + return ErrorRecoveryType.IGNORE_AND_CONTINUE case ReactionIfMatch.FAIL_RUN: return ErrorRecoveryType.FAIL_RUN case ReactionIfMatch.WAIT_FOR_RECOVERY: diff --git a/robot-server/robot_server/runs/error_recovery_models.py b/robot-server/robot_server/runs/error_recovery_models.py index a2990a007cb..1e2d4ac45aa 100644 --- a/robot-server/robot_server/runs/error_recovery_models.py +++ b/robot-server/robot_server/runs/error_recovery_models.py @@ -24,17 +24,40 @@ class ReactionIfMatch(Enum): - """How to handle a given error. + """How to handle a matching error. - * `"ignoreAndContinue"`: Ignore this error and continue with the next command. * `"failRun"`: Fail the run. - * `"waitForRecovery"`: Enter interactive error recovery mode. + * `"waitForRecovery"`: Enter interactive error recovery mode. You can then + perform error recovery with `POST /runs/{id}/commands` and exit error + recovery mode with `POST /runs/{id}/actions`. + + * `"assumeFalsePositiveAndContinue"`: Continue the run without interruption, acting + as if the error was a false positive. + + This is equivalent to doing `"waitForRecovery"` + and then sending `actionType: "resume-from-recovery-assuming-false-positive"` + to `POST /runs/{id}/actions`, except this requires no ongoing intervention from + the client. + + * `"ignoreAndContinue"`: Continue the run without interruption, accepting whatever + state the error left the robot in. + + This is equivalent to doing `"waitForRecovery"` + and then sending `actionType: "resume-from-recovery"` to `POST /runs/{id}/actions`, + except this requires no ongoing intervention from the client. + + This is probably not useful very often because it's likely to cause downstream + errors—imagine trying an `aspirate` command after a failed `pickUpTip` command. + This is provided for symmetry. """ - IGNORE_AND_CONTINUE = "ignoreAndContinue" FAIL_RUN = "failRun" WAIT_FOR_RECOVERY = "waitForRecovery" + ASSUME_FALSE_POSITIVE_AND_CONTINUE = "assumeFalsePositiveAndContinue" + # todo(mm, 2024-10-22): "ignoreAndContinue" may be a misnomer now: is + # "assumeFalsePositiveAndContinue" not also a way to "ignore"? Consider renaming. + IGNORE_AND_CONTINUE = "ignoreAndContinue" class ErrorMatcher(BaseModel): @@ -69,10 +92,7 @@ class ErrorRecoveryRule(BaseModel): ..., description="The criteria that must be met for this rule to be applied.", ) - ifMatch: ReactionIfMatch = Field( - ..., - description="How to handle errors matched by this rule.", - ) + ifMatch: ReactionIfMatch class ErrorRecoveryPolicy(BaseModel): diff --git a/robot-server/robot_server/runs/router/base_router.py b/robot-server/robot_server/runs/router/base_router.py index 788ca44aa1c..c108fa60a74 100644 --- a/robot-server/robot_server/runs/router/base_router.py +++ b/robot-server/robot_server/runs/router/base_router.py @@ -442,7 +442,6 @@ async def update_run( See `PATCH /errorRecovery/settings`. """ ), - status_code=status.HTTP_201_CREATED, responses={ status.HTTP_200_OK: {"model": SimpleEmptyBody}, status.HTTP_409_CONFLICT: {"model": ErrorBody[RunStopped]}, diff --git a/robot-server/robot_server/runs/run_controller.py b/robot-server/robot_server/runs/run_controller.py index 903bf22f252..1619cd20a08 100644 --- a/robot-server/robot_server/runs/run_controller.py +++ b/robot-server/robot_server/runs/run_controller.py @@ -2,6 +2,7 @@ import logging from datetime import datetime from typing import Optional +from typing_extensions import assert_never from opentrons.protocol_engine import ProtocolEngineError from opentrons_shared_data.errors.exceptions import RoboticsInteractionError @@ -96,6 +97,17 @@ def create_action( elif action_type == RunActionType.RESUME_FROM_RECOVERY: self._run_orchestrator_store.resume_from_recovery() + elif ( + action_type + == RunActionType.RESUME_FROM_RECOVERY_ASSUMING_FALSE_POSITIVE + ): + # todo(mm, 2024-10-23): Connect to work in + # https://github.com/Opentrons/opentrons/pull/16556. + self._run_orchestrator_store.resume_from_recovery() + + else: + assert_never(action_type) + except ProtocolEngineError as e: raise RunActionNotAllowedError(message=e.message, wrapping=[e]) from e From e32c0f361c9919dfe5dd86dbc50ccdd078c45103 Mon Sep 17 00:00:00 2001 From: Shlok Amin Date: Thu, 24 Oct 2024 11:38:33 -0400 Subject: [PATCH 05/17] fix(scripts): make aws deploy scripts work with new sdk (#16586) closes AUTH-979 --- scripts/deploy/lib/copyObject.js | 35 ++++++++++++++++++++---------- scripts/deploy/lib/removeObject.js | 27 ++++++++++++++++------- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/scripts/deploy/lib/copyObject.js b/scripts/deploy/lib/copyObject.js index 1735cb4a55e..63418067b40 100644 --- a/scripts/deploy/lib/copyObject.js +++ b/scripts/deploy/lib/copyObject.js @@ -1,6 +1,7 @@ 'use strict' const mime = require('mime') +const { CopyObjectCommand } = require('@aws-sdk/client-s3') // TODO(mc, 2019-07-16): optimize cache values const getCopyParams = obj => ({ @@ -12,7 +13,7 @@ const getCopyParams = obj => ({ /** * Copy an object to an S3 bucket * - * @param {S3} s3 - AWS.S3 instance + * @param {S3Client} s3 - AWS SDK v3 S3Client instance * @param {S3Object} sourceObj - Object to copy * @param {string} destBucket - Destination bucket * @param {string} [destPath] - Destination bucket folder (root if unspecified) @@ -21,10 +22,10 @@ const getCopyParams = obj => ({ * * @typedef S3Object * @property {string} Bucket - Object bucket - * @property {String} Prefix - Deploy folder in bucket + * @property {string} Prefix - Deploy folder in bucket * @property {string} Key - Full key to object */ -module.exports = function copyObject( +module.exports = async function copyObject( s3, sourceObj, destBucket, @@ -37,18 +38,28 @@ module.exports = function copyObject( const copyParams = getCopyParams(sourceObj) console.log( - `${dryrun ? 'DRYRUN: ' : ''}Copy - Source: ${copySource} - Dest: /${destBucket}/${destKey} - Params: ${JSON.stringify(copyParams)}\n` + `${ + dryrun ? 'DRYRUN: ' : '' + }Copy\nSource: ${copySource}\nDest: /${destBucket}/${destKey}\nParams: ${JSON.stringify( + copyParams + )}\n` ) if (dryrun) return Promise.resolve() - const copyObjectParams = Object.assign( - { Bucket: destBucket, Key: destKey, CopySource: copySource }, - copyParams - ) + const copyObjectParams = { + Bucket: destBucket, + Key: destKey, + CopySource: copySource, + ...copyParams, + } - return s3.copyObject(copyObjectParams).promise() + try { + const command = new CopyObjectCommand(copyObjectParams) + await s3.send(command) + console.log(`Successfully copied to /${destBucket}/${destKey}`) + } catch (err) { + console.error(`Error copying object: ${err.message}`) + throw err + } } diff --git a/scripts/deploy/lib/removeObject.js b/scripts/deploy/lib/removeObject.js index 56bd309a6eb..728d8927496 100644 --- a/scripts/deploy/lib/removeObject.js +++ b/scripts/deploy/lib/removeObject.js @@ -1,9 +1,11 @@ 'use strict' +const { DeleteObjectCommand } = require('@aws-sdk/client-s3'); + /** * Remove an object from S3 * - * @param {AWS.S3} s3 - AWS.S3 instance + * @param {S3Client} s3 - S3Client instance * @param {S3Object} obj - Object to remove * @param {boolean} [dryrun] - Don't actually remove anything * @returns {Promise} Promise that resolves when the removal is complete @@ -13,13 +15,22 @@ * @property {String} Prefix - Deploy folder in bucket * @property {string} Key - Full key to object */ -module.exports = function removeObject(s3, obj, dryrun) { +module.exports = async function removeObject(s3, obj, dryrun) { console.log( - `${dryrun ? 'DRYRUN: ' : ''}Remove - Source: /${obj.Bucket}/${obj.Key}\n` - ) + `${dryrun ? 'DRYRUN: ' : ''}Remove\nSource: /${obj.Bucket}/${obj.Key}\n` + ); + + if (dryrun) return Promise.resolve(); - if (dryrun) return Promise.resolve() + // Construct the deleteObject command with the bucket and key + const deleteParams = { Bucket: obj.Bucket, Key: obj.Key }; - return s3.deleteObject({ Bucket: obj.Bucket, Key: obj.Key }).promise() -} + try { + // Use the send method with DeleteObjectCommand + const result = await s3.send(new DeleteObjectCommand(deleteParams)); + return result; + } catch (error) { + console.error('Error removing object:', error); + throw error; + } +}; From 9d57048fda674858b9684ec3501cb7a7a796d03c Mon Sep 17 00:00:00 2001 From: Ryan Howard Date: Thu, 24 Oct 2024 11:46:32 -0400 Subject: [PATCH 06/17] fix(api): don't use sensor log on ot2 or simulators (#16590) # Overview This makes is so the logging setup only tries to import the sensor log name if it's running on a Flex. ## Test Plan and Hands on Testing ## Changelog ## Review requests ## Risk assessment --- api/src/opentrons/util/logging_config.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/api/src/opentrons/util/logging_config.py b/api/src/opentrons/util/logging_config.py index 42a32501576..0a36468f3bc 100644 --- a/api/src/opentrons/util/logging_config.py +++ b/api/src/opentrons/util/logging_config.py @@ -5,7 +5,11 @@ from opentrons.config import CONFIG, ARCHITECTURE, SystemArchitecture -from opentrons_hardware.sensors import SENSOR_LOG_NAME +if ARCHITECTURE is SystemArchitecture.BUILDROOT: + from opentrons_hardware.sensors import SENSOR_LOG_NAME +else: + # we don't use the sensor log on ot2 or host + SENSOR_LOG_NAME = "unused" def _host_config(level_value: int) -> Dict[str, Any]: From 02236b39b38a048d2231e620289538aae465ac10 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Thu, 24 Oct 2024 11:53:29 -0400 Subject: [PATCH 07/17] chore: fix github workflow and failing tests (#16593) Messed up the app test workflow so it wasn't running and therefore we didn't notice some tests were failing. Fix the workflow, fix the tests. Only changes of note are that vitest doesn't support baseline 2023 copying array reorder functions for some reason so replace them with equivalents. --- .github/workflows/app-test-build-deploy.yaml | 2 +- .../hooks/useHistoricRunDetails.ts | 12 +++++++----- app/src/pages/ODD/RobotDashboard/index.tsx | 3 +-- app/src/redux/config/__tests__/config.test.ts | 2 ++ 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/.github/workflows/app-test-build-deploy.yaml b/.github/workflows/app-test-build-deploy.yaml index d3c03e1f500..f0bfe7d8946 100644 --- a/.github/workflows/app-test-build-deploy.yaml +++ b/.github/workflows/app-test-build-deploy.yaml @@ -148,7 +148,7 @@ jobs: yarn config set cache-folder ${{ github.workspace }}/.yarn-cache make setup-js - name: 'test native(er) packages' - run: make test-js-internal tests="${{}matrix.shell}/src" cov_opts="--coverage=true" + run: make test-js-internal tests="${{matrix.shell}}/src" cov_opts="--coverage=true" - name: 'Upload coverage report' uses: 'codecov/codecov-action@v3' with: diff --git a/app/src/organisms/ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts b/app/src/organisms/ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts index cadfc5cf618..39f6d5e59b8 100644 --- a/app/src/organisms/ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts +++ b/app/src/organisms/ApplyHistoricOffsets/hooks/useHistoricRunDetails.ts @@ -6,11 +6,13 @@ export function useHistoricRunDetails( hostOverride?: HostConfig | null ): RunData[] { const { data: allHistoricRuns } = useNotifyAllRunsQuery({}, {}, hostOverride) - return allHistoricRuns == null ? [] - : allHistoricRuns.data.toSorted( - (a, b) => - new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() - ) + : // TODO(sf): figure out why .toSorted() doesn't work in vitest + allHistoricRuns.data + .map(t => t) + .sort( + (a, b) => + new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime() + ) } diff --git a/app/src/pages/ODD/RobotDashboard/index.tsx b/app/src/pages/ODD/RobotDashboard/index.tsx index 80ae745b055..aa255717388 100644 --- a/app/src/pages/ODD/RobotDashboard/index.tsx +++ b/app/src/pages/ODD/RobotDashboard/index.tsx @@ -41,8 +41,7 @@ export function RobotDashboard(): JSX.Element { ) const recentRunsOfUniqueProtocols = (allRunsQueryData?.data ?? []) - .toReversed() // newest runs first - .reduce((acc, run) => { + .reduceRight((acc, run) => { if ( acc.some(collectedRun => collectedRun.protocolId === run.protocolId) ) { diff --git a/app/src/redux/config/__tests__/config.test.ts b/app/src/redux/config/__tests__/config.test.ts index d99eb95c36e..bf5b4e98004 100644 --- a/app/src/redux/config/__tests__/config.test.ts +++ b/app/src/redux/config/__tests__/config.test.ts @@ -28,6 +28,7 @@ describe('config', () => { expect(Cfg.configInitialized(state.config as any)).toEqual({ type: 'config:INITIALIZED', payload: { config: state.config }, + meta: { shell: true }, }) }) @@ -35,6 +36,7 @@ describe('config', () => { expect(Cfg.configValueUpdated('foo.bar', false)).toEqual({ type: 'config:VALUE_UPDATED', payload: { path: 'foo.bar', value: false }, + meta: { shell: true }, }) }) From 545d0f7dc839c33de3b4210ac829f64926f1dc8e Mon Sep 17 00:00:00 2001 From: Nick Diehl <47604184+ncdiehl11@users.noreply.github.com> Date: Thu, 24 Oct 2024 12:26:35 -0400 Subject: [PATCH 08/17] feat(protocol-designer): update error style and render logic (#16589) This PR fixes some styling bugs for both form-level and timeline errors. Also, I update the logic of the step form 'save' button to always be enabled, and conditionally render form errors and warnings should any exist. To consolidate components that are providing essentially the same function in `ToggleExpandStepFormField` and `CheckboxExpandStepFormField`, I refactor `ToggleExpandStepFormField` to render either a toggle button or a checkbox. Lastly, I update copy for thermocycler step form toggle input field menus. --- .../src/assets/localization/en/form.json | 12 +++--- .../ToggleExpandStepFormField/index.tsx | 34 +++++++++++----- .../src/organisms/Alerts/FormAlerts.tsx | 23 +++++++---- .../src/organisms/Alerts/TimelineAlerts.tsx | 12 +++++- .../StepForm/StepFormToolbox.tsx | 39 +++++++++++-------- .../StepTools/HeaterShakerTools/index.tsx | 1 + .../ThermocyclerTools/ThermocyclerState.tsx | 6 +-- .../pages/Designer/ProtocolSteps/index.tsx | 23 +++++------ .../src/steplist/fieldLevel/errors.ts | 6 +-- 9 files changed, 93 insertions(+), 63 deletions(-) diff --git a/protocol-designer/src/assets/localization/en/form.json b/protocol-designer/src/assets/localization/en/form.json index 995c6ae9d0e..1f3f4bc9e34 100644 --- a/protocol-designer/src/assets/localization/en/form.json +++ b/protocol-designer/src/assets/localization/en/form.json @@ -209,20 +209,20 @@ }, "thermocyclerState": { "block": { - "engage": "Engage block temperature", + "engage": "Block temperature", "label": "Thermocycler block", "temperature": "Block temperature", - "toggleOff": "Deactivated", - "toggleOn": "Active", + "toggleOff": "Deactivate", + "toggleOn": "Activate", "valid_range": "Valid range between 4 and 96ºC" }, "ending_hold": "Ending hold", "lid": { - "engage": "Engage lid temperature", + "engage": "Lid temperature", "label": "Lid", "temperature": "Lid temperature", - "toggleOff": "Deactivated", - "toggleOn": "Active", + "toggleOff": "Deactivate", + "toggleOn": "Activate", "valid_range": "Valid range between 37 and 110ºC" }, "lidPosition": { diff --git a/protocol-designer/src/molecules/ToggleExpandStepFormField/index.tsx b/protocol-designer/src/molecules/ToggleExpandStepFormField/index.tsx index b27b72b7821..05e3883e575 100644 --- a/protocol-designer/src/molecules/ToggleExpandStepFormField/index.tsx +++ b/protocol-designer/src/molecules/ToggleExpandStepFormField/index.tsx @@ -1,6 +1,8 @@ import { ALIGN_CENTER, + Btn, COLORS, + Check, DIRECTION_COLUMN, Flex, JUSTIFY_SPACE_BETWEEN, @@ -23,6 +25,7 @@ interface ToggleExpandStepFormFieldProps extends FieldProps { onLabel?: string offLabel?: string caption?: string + toggleElement?: 'toggle' | 'checkbox' } export function ToggleExpandStepFormField( props: ToggleExpandStepFormFieldProps @@ -37,6 +40,7 @@ export function ToggleExpandStepFormField( toggleUpdateValue, toggleValue, caption, + toggleElement = 'toggle', ...restProps } = props @@ -58,6 +62,7 @@ export function ToggleExpandStepFormField( } } + const label = isSelected ? onLabel : offLabel ?? null return ( {title} - - {isSelected ? onLabel : offLabel ?? null} - - { - onToggleUpdateValue() - }} - label={isSelected ? onLabel : offLabel} - toggledOn={isSelected} - /> + {label != null ? ( + + {isSelected ? onLabel : offLabel ?? null} + + ) : null} + {toggleElement === 'toggle' ? ( + + ) : ( + + + + )} diff --git a/protocol-designer/src/organisms/Alerts/FormAlerts.tsx b/protocol-designer/src/organisms/Alerts/FormAlerts.tsx index 5aa2833ee60..290295305af 100644 --- a/protocol-designer/src/organisms/Alerts/FormAlerts.tsx +++ b/protocol-designer/src/organisms/Alerts/FormAlerts.tsx @@ -32,7 +32,7 @@ interface FormAlertsProps { dirtyFields?: StepFieldName[] } -function FormAlertsComponent(props: FormAlertsProps): JSX.Element { +function FormAlertsComponent(props: FormAlertsProps): JSX.Element | null { const { focusedField, dirtyFields } = props const { t } = useTranslation('alert') const dispatch = useDispatch() @@ -95,7 +95,7 @@ function FormAlertsComponent(props: FormAlertsProps): JSX.Element { } const makeAlert: MakeAlert = (alertType, data, key) => ( - + {data.title} - - {data.description} - + {data.description != null ? ( + + {data.description} + + ) : null} @@ -151,15 +154,19 @@ function FormAlertsComponent(props: FormAlertsProps): JSX.Element { ) } } - return ( - + return [...formErrors, ...formWarnings, ...timelineWarnings].length > 0 ? ( + {formErrors.map((error, key) => makeAlert('error', error, key))} {formWarnings.map((warning, key) => makeAlert('warning', warning, key))} {timelineWarnings.map((warning, key) => makeAlert('warning', warning, key) )} - ) + ) : null } export const FormAlerts = memo(FormAlertsComponent) diff --git a/protocol-designer/src/organisms/Alerts/TimelineAlerts.tsx b/protocol-designer/src/organisms/Alerts/TimelineAlerts.tsx index 4df442b44f7..fbc36ddac76 100644 --- a/protocol-designer/src/organisms/Alerts/TimelineAlerts.tsx +++ b/protocol-designer/src/organisms/Alerts/TimelineAlerts.tsx @@ -11,10 +11,12 @@ import { } from '@opentrons/components' import { getRobotStateTimeline } from '../../file-data/selectors' import { ErrorContents } from './ErrorContents' + +import type { StyleProps } from '@opentrons/components' import type { CommandCreatorError } from '@opentrons/step-generation' import type { MakeAlert } from './types' -function TimelineAlertsComponent(): JSX.Element { +function TimelineAlertsComponent(props: StyleProps): JSX.Element | null { const { t } = useTranslation('alert') const timeline = useSelector(getRobotStateTimeline) @@ -26,6 +28,10 @@ function TimelineAlertsComponent(): JSX.Element { }) ) + if (timelineErrors.length === 0) { + return null + } + const makeAlert: MakeAlert = (alertType, data, key) => ( {timelineErrors.map((error, key) => makeAlert('error', error, key))} + + {timelineErrors.map((error, key) => makeAlert('error', error, key))} + ) } diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx index f101237cb45..9c60605163c 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx @@ -99,6 +99,10 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { ? 1 : 0 ) + const [ + showFormErrorsAndWarnings, + setShowFormErrorsAndWarnings, + ] = useState(false) const [isRename, setIsRename] = useState(false) const icon = stepIconsByType[formData.stepType] @@ -126,18 +130,22 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { const numErrors = timeline.errors?.length ?? 0 const handleSaveClick = (): void => { - handleSave() - makeSnackbar( - getSaveStepSnackbarText({ - numWarnings, - numErrors, - stepTypeDisplayName: i18n.format( - t(`stepType.${formData.stepType}`), - 'capitalize' - ), - t, - }) as string - ) + if (canSave) { + handleSave() + makeSnackbar( + getSaveStepSnackbarText({ + numWarnings, + numErrors, + stepTypeDisplayName: i18n.format( + t(`stepType.${formData.stepType}`), + 'capitalize' + ), + t, + }) as string + ) + } else { + setShowFormErrorsAndWarnings(true) + } } return ( @@ -195,9 +203,6 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { } : handleSaveClick } - disabled={ - isMultiStepToolbox && toolboxStep === 0 ? false : !canSave - } width="100%" > {isMultiStepToolbox && toolboxStep === 0 @@ -215,7 +220,9 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { } > - + {showFormErrorsAndWarnings ? ( + + ) : null} diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/ThermocyclerTools/ThermocyclerState.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/ThermocyclerTools/ThermocyclerState.tsx index 4872610a284..a5f1676f2c8 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/ThermocyclerTools/ThermocyclerState.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/ThermocyclerTools/ThermocyclerState.tsx @@ -77,11 +77,9 @@ export function ThermocyclerState(props: ThermocyclerStateProps): JSX.Element { fieldTitle={i18n.format(t('stepType.temperature'), 'capitalize')} units={t('units.degrees')} isSelected={formData[lidFieldActive] === true} - onLabel={t( - 'form:step_edit_form.field.thermocyclerState.lidPosition.toggleOn' - )} + onLabel={t('form:step_edit_form.field.thermocyclerState.lid.toggleOn')} offLabel={t( - 'form:step_edit_form.field.thermocyclerState.lidPosition.toggleOff' + 'form:step_edit_form.field.thermocyclerState.lid.toggleOff' )} /> - + {tab === 'protocolSteps' ? ( - - - + ) : null} - + {deckView === leftString ? ( ) : ( diff --git a/protocol-designer/src/steplist/fieldLevel/errors.ts b/protocol-designer/src/steplist/fieldLevel/errors.ts index 6ddc0d3dfd2..7405f097643 100644 --- a/protocol-designer/src/steplist/fieldLevel/errors.ts +++ b/protocol-designer/src/steplist/fieldLevel/errors.ts @@ -58,20 +58,20 @@ export const minimumWellCount = (minimum: number): ErrorChecker => ( export const minFieldValue = (minimum: number): ErrorChecker => ( value: unknown ): string | null => - value === null || Number(value) >= minimum + !value || Number(value) >= minimum ? null : `${FIELD_ERRORS.UNDER_RANGE_MINIMUM} ${minimum}` export const maxFieldValue = (maximum: number): ErrorChecker => ( value: unknown ): string | null => - value === null || Number(value) <= maximum + !value || Number(value) <= maximum ? null : `${FIELD_ERRORS.OVER_RANGE_MAXIMUM} ${maximum}` export const temperatureRangeFieldValue = ( minimum: number, maximum: number ): ErrorChecker => (value: unknown): string | null => - value === null || (Number(value) <= maximum && Number(value) >= minimum) + !value || (Number(value) <= maximum && Number(value) >= minimum) ? null : `${FIELD_ERRORS.OUTSIDE_OF_RANGE} ${minimum} and ${maximum} °C` export const realNumber: ErrorChecker = (value: unknown) => From a0086ccc095470b094348561de6044890a6908c4 Mon Sep 17 00:00:00 2001 From: Jethary Alcid <66035149+jerader@users.noreply.github.com> Date: Thu, 24 Oct 2024 12:48:46 -0400 Subject: [PATCH 09/17] feat(protocol-designer): magnetic module change hint wire up (#16498) closes AUTH-797 --- .../localization/en/starting_deck_state.json | 8 +- .../__tests__/BlockingHintModal.test.tsx | 38 ++ .../src/organisms/BlockingHintModal/index.tsx | 86 ++++ .../BlockingHintModal/useBlockingHint.tsx | 40 ++ protocol-designer/src/organisms/index.ts | 1 + .../Designer/DeckSetup/DeckSetupTools.tsx | 411 ++++++++++-------- .../DeckSetup/MagnetModuleChangeContent.tsx | 40 ++ .../__tests__/DeckSetupTools.test.tsx | 3 + .../MagnetModuleChangeContent.test.tsx | 32 ++ 9 files changed, 481 insertions(+), 178 deletions(-) create mode 100644 protocol-designer/src/organisms/BlockingHintModal/__tests__/BlockingHintModal.test.tsx create mode 100644 protocol-designer/src/organisms/BlockingHintModal/index.tsx create mode 100644 protocol-designer/src/organisms/BlockingHintModal/useBlockingHint.tsx create mode 100644 protocol-designer/src/pages/Designer/DeckSetup/MagnetModuleChangeContent.tsx create mode 100644 protocol-designer/src/pages/Designer/DeckSetup/__tests__/MagnetModuleChangeContent.test.tsx diff --git a/protocol-designer/src/assets/localization/en/starting_deck_state.json b/protocol-designer/src/assets/localization/en/starting_deck_state.json index 1e284990c62..5fc398de084 100644 --- a/protocol-designer/src/assets/localization/en/starting_deck_state.json +++ b/protocol-designer/src/assets/localization/en/starting_deck_state.json @@ -8,10 +8,14 @@ "add_liquid": "Add liquid", "add_module": "Add a module", "add_rest": "Add labware and liquids to complete deck setup", + "alter_pause": "You may also need to alter the time you pause while your magnet is engaged.", "aluminumBlock": "Aluminum block", "clear_labware": "Clear labware", "clear_slot": "Clear slot", "clear": "Clear", + "command_click_to_multi_select": "Command + Click for multi-select", + "convert_gen1_to_gen2": "To convert engage heights from GEN1 to GEN2, divide your engage height by 2.", + "convert_gen2_to_gen1": "To convert engage heights from GEN2 to GEN1, multiply your engage height by 2.", "custom": "Custom labware definitions", "customize_slot": "Customize slot", "deck_hardware": "Deck hardware", @@ -25,9 +29,11 @@ "edit_protocol": "Edit protocol", "edit_slot": "Edit slot", "edit": "Edit", + "gen1_gen2_different_units": "Switching between GEN1 and GEN2 Magnetic Modules will clear all non-default engage heights from existing magnet steps in your protocol. GEN1 and GEN2 Magnetic Modules do not use the same units.", "heater_shaker_adjacent_to": "A module is adjacent to this slot. The Heater-Shaker cannot be placed next to a module", "heater_shaker_adjacent": "A Heater-Shaker is adjacent to this slot. Modules cannot be placed next to a Heater-Shaker", "heater_shaker_trash": "The heater-shaker cannot be next to the trash bin", + "here": "here.", "labware": "Labware", "liquids": "Liquids", "no_offdeck_labware": "No off-deck labware added", @@ -37,8 +43,8 @@ "onDeck": "On deck", "one_item": "No more than 1 {{hardware}} allowed on the deck at one time", "only_display_rec": "Only display recommended labware", - "command_click_to_multi_select": "Command + Click for multi-select", "protocol_starting_deck": "Protocol starting deck", + "read_more_gen1_gen2": "Read more about the differences between GEN1 and GEN2 Magnetic Modules", "rename_lab": "Rename labware", "reservoir": "Reservoir", "shift_click_to_select_all": "Shift + Click to select all", diff --git a/protocol-designer/src/organisms/BlockingHintModal/__tests__/BlockingHintModal.test.tsx b/protocol-designer/src/organisms/BlockingHintModal/__tests__/BlockingHintModal.test.tsx new file mode 100644 index 00000000000..b92295ff060 --- /dev/null +++ b/protocol-designer/src/organisms/BlockingHintModal/__tests__/BlockingHintModal.test.tsx @@ -0,0 +1,38 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { fireEvent, screen } from '@testing-library/react' +import { i18n } from '../../../assets/localization' +import { renderWithProviders } from '../../../__testing-utils__' +import { removeHint } from '../../../tutorial/actions' +import { BlockingHintModal } from '..' +import type { ComponentProps } from 'react' + +vi.mock('../../../tutorial/actions') + +const render = (props: ComponentProps) => { + return renderWithProviders(, { + i18nInstance: i18n, + })[0] +} + +describe('BlockingHintModal', () => { + let props: ComponentProps + + beforeEach(() => { + props = { + content:
mock content
, + handleCancel: vi.fn(), + handleContinue: vi.fn(), + hintKey: 'change_magnet_module_model', + } + }) + it('renders the hint with buttons and checkbox', () => { + render(props) + fireEvent.click(screen.getByRole('button', { name: 'Cancel' })) + expect(props.handleCancel).toHaveBeenCalled() + expect(vi.mocked(removeHint)).toHaveBeenCalled() + fireEvent.click(screen.getByRole('button', { name: 'Continue' })) + expect(props.handleContinue).toHaveBeenCalled() + expect(vi.mocked(removeHint)).toHaveBeenCalled() + screen.getByText('mock content') + }) +}) diff --git a/protocol-designer/src/organisms/BlockingHintModal/index.tsx b/protocol-designer/src/organisms/BlockingHintModal/index.tsx new file mode 100644 index 00000000000..be33b06742f --- /dev/null +++ b/protocol-designer/src/organisms/BlockingHintModal/index.tsx @@ -0,0 +1,86 @@ +import { useCallback, useState } from 'react' +import { createPortal } from 'react-dom' +import { useTranslation } from 'react-i18next' +import { useDispatch } from 'react-redux' +import { + ALIGN_CENTER, + COLORS, + Check, + Flex, + JUSTIFY_SPACE_BETWEEN, + Modal, + PrimaryButton, + SPACING, + SecondaryButton, + StyledText, +} from '@opentrons/components' +import { actions } from '../../tutorial' +import { getMainPagePortalEl } from '../../components/portals/MainPageModalPortal' +import type { ReactNode } from 'react' +import type { HintKey } from '../../tutorial' + +export interface HintProps { + hintKey: HintKey + handleCancel: () => void + handleContinue: () => void + content: ReactNode +} + +export function BlockingHintModal(props: HintProps): JSX.Element { + const { content, hintKey, handleCancel, handleContinue } = props + const { t, i18n } = useTranslation(['alert', 'shared']) + const dispatch = useDispatch() + + const [rememberDismissal, setRememberDismissal] = useState(false) + + const toggleRememberDismissal = useCallback(() => { + setRememberDismissal(prevDismissal => !prevDismissal) + }, []) + + const onCancelClick = (): void => { + dispatch(actions.removeHint(hintKey, rememberDismissal)) + handleCancel() + } + + const onContinueClick = (): void => { + dispatch(actions.removeHint(hintKey, rememberDismissal)) + handleContinue() + } + + return createPortal( + + + + + {t('hint.dont_show_again')} + + + + + {t('shared:cancel')} + + + {i18n.format(t('shared:continue'), 'capitalize')} + + + + } + > + {content} + , + getMainPagePortalEl() + ) +} diff --git a/protocol-designer/src/organisms/BlockingHintModal/useBlockingHint.tsx b/protocol-designer/src/organisms/BlockingHintModal/useBlockingHint.tsx new file mode 100644 index 00000000000..19926e2d51b --- /dev/null +++ b/protocol-designer/src/organisms/BlockingHintModal/useBlockingHint.tsx @@ -0,0 +1,40 @@ +import { useSelector } from 'react-redux' +import { getDismissedHints } from '../../tutorial/selectors' +import { BlockingHintModal } from './index' +import type { HintKey } from '../../tutorial' + +export interface HintProps { + /** `enabled` should be a condition that the parent uses to toggle whether the hint should be active or not. + * If the hint is enabled but has been dismissed, it will automatically call `handleContinue` when enabled. + * useBlockingHint expects the parent to disable the hint on cancel/continue */ + enabled: boolean + hintKey: HintKey + content: React.ReactNode + handleCancel: () => void + handleContinue: () => void +} + +export const useBlockingHint = (args: HintProps): JSX.Element | null => { + const { enabled, hintKey, handleCancel, handleContinue, content } = args + const isDismissed = useSelector(getDismissedHints).includes(hintKey) + + if (isDismissed) { + if (enabled) { + handleContinue() + } + return null + } + + if (!enabled) { + return null + } + + return ( + + ) +} diff --git a/protocol-designer/src/organisms/index.ts b/protocol-designer/src/organisms/index.ts index cda71137c57..0fd6e481e3e 100644 --- a/protocol-designer/src/organisms/index.ts +++ b/protocol-designer/src/organisms/index.ts @@ -1,6 +1,7 @@ export * from './Alerts' export * from './AnnouncementModal' export * from './AssignLiquidsModal' +export * from './BlockingHintModal' export * from './DefineLiquidsModal' export * from './EditInstrumentsModal' export * from './EditNickNameModal' diff --git a/protocol-designer/src/pages/Designer/DeckSetup/DeckSetupTools.tsx b/protocol-designer/src/pages/Designer/DeckSetup/DeckSetupTools.tsx index e062fa4784d..6c000ad0428 100644 --- a/protocol-designer/src/pages/Designer/DeckSetup/DeckSetupTools.tsx +++ b/protocol-designer/src/pages/Designer/DeckSetup/DeckSetupTools.tsx @@ -17,6 +17,9 @@ import { FLEX_ROBOT_TYPE, getModuleDisplayName, getModuleType, + MAGNETIC_MODULE_TYPE, + MAGNETIC_MODULE_V1, + MAGNETIC_MODULE_V2, OT2_ROBOT_TYPE, } from '@opentrons/shared-data' @@ -38,12 +41,15 @@ import { selectZoomedIntoSlot, } from '../../../labware-ingred/actions' import { getEnableAbsorbanceReader } from '../../../feature-flags/selectors' +import { useBlockingHint } from '../../../organisms/BlockingHintModal/useBlockingHint' import { selectors } from '../../../labware-ingred/selectors' import { useKitchen } from '../../../organisms/Kitchen/hooks' +import { getDismissedHints } from '../../../tutorial/selectors' import { createContainerAboveModule } from '../../../step-forms/actions/thunks' import { FIXTURES, MOAM_MODELS } from './constants' import { getSlotInformation } from '../utils' import { getModuleModelsBySlot, getDeckErrors } from './utils' +import { MagnetModuleChangeContent } from './MagnetModuleChangeContent' import { LabwareTools } from './LabwareTools' import type { ModuleModel } from '@opentrons/shared-data' @@ -65,6 +71,9 @@ export function DeckSetupTools(props: DeckSetupToolsProps): JSX.Element | null { const { makeSnackbar } = useKitchen() const selectedSlotInfo = useSelector(selectors.getZoomedInSlotInfo) const robotType = useSelector(getRobotType) + const isDismissedModuleHint = useSelector(getDismissedHints).includes( + 'change_magnet_module_model' + ) const dispatch = useDispatch>() const enableAbsorbanceReader = useSelector(getEnableAbsorbanceReader) const deckSetup = useSelector(getDeckSetupForActiveItem) @@ -76,6 +85,9 @@ export function DeckSetupTools(props: DeckSetupToolsProps): JSX.Element | null { selectedNestedLabwareDefUri, } = selectedSlotInfo const { slot, cutout } = selectedSlot + const [changeModuleWarningInfo, displayModuleWarning] = useState( + false + ) const [selectedHardware, setSelectedHardware] = useState< ModuleModel | Fixture | null >(null) @@ -95,6 +107,34 @@ export function DeckSetupTools(props: DeckSetupToolsProps): JSX.Element | null { const [tab, setTab] = useState<'hardware' | 'labware'>( moduleModels?.length === 0 || slot === 'offDeck' ? 'labware' : 'hardware' ) + const hasMagneticModule = Object.values(deckSetup.modules).some( + module => module.type === MAGNETIC_MODULE_TYPE + ) + const moduleOnSlotIsMagneticModuleV1 = + Object.values(deckSetup.modules).find(module => module.slot === slot) + ?.model === MAGNETIC_MODULE_V1 + + const changeModuleWarning = useBlockingHint({ + hintKey: 'change_magnet_module_model', + handleCancel: () => { + displayModuleWarning(false) + }, + handleContinue: () => { + setSelectedHardware( + moduleOnSlotIsMagneticModuleV1 ? MAGNETIC_MODULE_V2 : MAGNETIC_MODULE_V1 + ) + dispatch( + selectModule({ + moduleModel: moduleOnSlotIsMagneticModuleV1 + ? MAGNETIC_MODULE_V2 + : MAGNETIC_MODULE_V1, + }) + ) + displayModuleWarning(false) + }, + content: , + enabled: changeModuleWarningInfo, + }) if (slot == null || (onDeckProps == null && slot !== 'offDeck')) { return null @@ -236,194 +276,211 @@ export function DeckSetupTools(props: DeckSetupToolsProps): JSX.Element | null { dispatch(selectZoomedIntoSlot({ slot: null, cutout: null })) onCloseClick() } - return ( - - - - {t('customize_slot')} + <> + {changeModuleWarning} + + + + {t('customize_slot')} + + + } + closeButton={ + + {t('clear')} - - } - closeButton={ - {t('clear')} - } - onCloseClick={() => { - handleClear() - handleResetToolbox() - }} - onConfirmClick={() => { - handleConfirm() - }} - confirmButtonText={t('done')} - > - - {slot !== 'offDeck' ? : null} - {tab === 'hardware' ? ( - - - - {t('add_module')} - - - {moduleModels?.map(model => { - const modelSomewhereOnDeck = Object.values( - deckSetupModules - ).filter( - module => module.model === model && module.slot !== slot - ) - const typeSomewhereOnDeck = Object.values( - deckSetupModules - ).filter( - module => - module.type === getModuleType(model) && - module.slot !== slot - ) - const moamModels = MOAM_MODELS - - const collisionError = getDeckErrors({ - modules: deckSetupModules, - selectedSlot: slot, - selectedModel: model, - labware: deckSetupLabware, - robotType, - }) - - return ( - { - if (onDeckProps?.setHoveredModule != null) { - onDeckProps.setHoveredModule(null) - } - }} - setHovered={() => { - if (onDeckProps?.setHoveredModule != null) { - onDeckProps.setHoveredFixture(null) - onDeckProps.setHoveredModule(model) - } - }} - largeDesktopBorderRadius - buttonLabel={ - - - - {getModuleDisplayName(model)} - - - } - key={`${model}_${slot}`} - buttonValue={model} - onChange={() => { - if ( - modelSomewhereOnDeck.length === 1 && - !moamModels.includes(model) && - robotType === FLEX_ROBOT_TYPE - ) { - makeSnackbar( - t('one_item', { - hardware: getModuleDisplayName(model), - }) as string - ) - } else if ( - typeSomewhereOnDeck.length > 0 && - robotType === OT2_ROBOT_TYPE - ) { - makeSnackbar( - t('one_item', { - hardware: t( - `shared:${getModuleType(model).toLowerCase()}` - ), - }) as string - ) - } else if (collisionError != null) { - makeSnackbar(t(`${collisionError}`) as string) - } else { - setSelectedHardware(model) - dispatch(selectFixture({ fixture: null })) - dispatch(selectModule({ moduleModel: model })) - dispatch(selectLabware({ labwareDefUri: null })) - dispatch( - selectNestedLabware({ nestedLabwareDefUri: null }) - ) - } - }} - isSelected={model === selectedHardware} - /> - ) - })} - - - {robotType === OT2_ROBOT_TYPE || fixtures.length === 0 ? null : ( + } + onCloseClick={() => { + handleClear() + handleResetToolbox() + }} + onConfirmClick={() => { + handleConfirm() + }} + confirmButtonText={t('done')} + > + + {slot !== 'offDeck' ? ( + + ) : null} + {tab === 'hardware' ? ( + - {t('add_fixture')} + {t('add_module')} - {fixtures.map(fixture => ( - { - if (onDeckProps?.setHoveredFixture != null) { - onDeckProps.setHoveredFixture(null) - } - }} - setHovered={() => { - if (onDeckProps?.setHoveredFixture != null) { - onDeckProps.setHoveredModule(null) - onDeckProps.setHoveredFixture(fixture) - } - }} - largeDesktopBorderRadius - buttonLabel={t(`shared:${fixture}`)} - key={`${fixture}_${slot}`} - buttonValue={fixture} - onChange={() => { - // delete this when multiple trash bins are supported - if (fixture === 'trashBin' && hasTrash) { - makeSnackbar( - t('one_item', { - hardware: t('shared:trashBin'), - }) as string - ) - } else { - setSelectedHardware(fixture) - dispatch(selectModule({ moduleModel: null })) - dispatch(selectFixture({ fixture })) - dispatch(selectLabware({ labwareDefUri: null })) - dispatch( - selectNestedLabware({ nestedLabwareDefUri: null }) - ) + {moduleModels?.map(model => { + const modelSomewhereOnDeck = Object.values( + deckSetupModules + ).filter( + module => module.model === model && module.slot !== slot + ) + const typeSomewhereOnDeck = Object.values( + deckSetupModules + ).filter( + module => + module.type === getModuleType(model) && + module.slot !== slot + ) + const moamModels = MOAM_MODELS + + const collisionError = getDeckErrors({ + modules: deckSetupModules, + selectedSlot: slot, + selectedModel: model, + labware: deckSetupLabware, + robotType, + }) + + return ( + { + if (onDeckProps?.setHoveredModule != null) { + onDeckProps.setHoveredModule(null) + } + }} + setHovered={() => { + if (onDeckProps?.setHoveredModule != null) { + onDeckProps.setHoveredModule(model) + } + }} + largeDesktopBorderRadius + buttonLabel={ + + + + {getModuleDisplayName(model)} + + } - }} - isSelected={fixture === selectedHardware} - /> - ))} + key={`${model}_${slot}`} + buttonValue={model} + onChange={() => { + if ( + modelSomewhereOnDeck.length === 1 && + !moamModels.includes(model) && + robotType === FLEX_ROBOT_TYPE + ) { + makeSnackbar( + t('one_item', { + hardware: getModuleDisplayName(model), + }) as string + ) + } else if ( + typeSomewhereOnDeck.length > 0 && + robotType === OT2_ROBOT_TYPE + ) { + makeSnackbar( + t('one_item', { + hardware: t( + `shared:${getModuleType(model).toLowerCase()}` + ), + }) as string + ) + } else if (collisionError != null) { + makeSnackbar(t(`${collisionError}`) as string) + } else if ( + hasMagneticModule && + (model === 'magneticModuleV1' || + model === 'magneticModuleV2') && + !isDismissedModuleHint + ) { + displayModuleWarning(true) + } else { + setSelectedHardware(model) + dispatch(selectFixture({ fixture: null })) + dispatch(selectModule({ moduleModel: model })) + dispatch(selectLabware({ labwareDefUri: null })) + dispatch( + selectNestedLabware({ nestedLabwareDefUri: null }) + ) + } + }} + isSelected={model === selectedHardware} + /> + ) + })} - )} - - ) : ( - - )} - - + {robotType === OT2_ROBOT_TYPE || fixtures.length === 0 ? null : ( + + + {t('add_fixture')} + + + {fixtures.map(fixture => ( + { + if (onDeckProps?.setHoveredFixture != null) { + onDeckProps.setHoveredFixture(null) + } + }} + setHovered={() => { + if (onDeckProps?.setHoveredFixture != null) { + onDeckProps.setHoveredFixture(fixture) + } + }} + largeDesktopBorderRadius + buttonLabel={t(`shared:${fixture}`)} + key={`${fixture}_${slot}`} + buttonValue={fixture} + onChange={() => { + // delete this when multiple trash bins are supported + if (fixture === 'trashBin' && hasTrash) { + makeSnackbar( + t('one_item', { + hardware: t('shared:trashBin'), + }) as string + ) + } else { + setSelectedHardware(fixture) + dispatch(selectModule({ moduleModel: null })) + dispatch(selectFixture({ fixture })) + dispatch(selectLabware({ labwareDefUri: null })) + dispatch( + selectNestedLabware({ nestedLabwareDefUri: null }) + ) + } + }} + isSelected={fixture === selectedHardware} + /> + ))} + + + )} + + ) : ( + + )} + + + ) } diff --git a/protocol-designer/src/pages/Designer/DeckSetup/MagnetModuleChangeContent.tsx b/protocol-designer/src/pages/Designer/DeckSetup/MagnetModuleChangeContent.tsx new file mode 100644 index 00000000000..0a5e9c18471 --- /dev/null +++ b/protocol-designer/src/pages/Designer/DeckSetup/MagnetModuleChangeContent.tsx @@ -0,0 +1,40 @@ +import { useTranslation } from 'react-i18next' +import { + DIRECTION_COLUMN, + Flex, + Link, + SPACING, + StyledText, +} from '@opentrons/components' + +export function MagnetModuleChangeContent(): JSX.Element { + const { t } = useTranslation('starting_deck_state') + + return ( + + + {t('gen1_gen2_different_units')} + + + {t('convert_gen1_to_gen2')} + + + {t('convert_gen2_to_gen1')} + + + {t('alter_pause')} + + + + {t('read_more_gen1_gen2')}{' '} + + {t('here')} + + + + + ) +} diff --git a/protocol-designer/src/pages/Designer/DeckSetup/__tests__/DeckSetupTools.test.tsx b/protocol-designer/src/pages/Designer/DeckSetup/__tests__/DeckSetupTools.test.tsx index cb85cf12693..ffb4c968acd 100644 --- a/protocol-designer/src/pages/Designer/DeckSetup/__tests__/DeckSetupTools.test.tsx +++ b/protocol-designer/src/pages/Designer/DeckSetup/__tests__/DeckSetupTools.test.tsx @@ -18,6 +18,7 @@ import { deleteDeckFixture, } from '../../../../step-forms/actions/additionalItems' import { selectors } from '../../../../labware-ingred/selectors' +import { getDismissedHints } from '../../../../tutorial/selectors' import { getDeckSetupForActiveItem } from '../../../../top-selectors/labware-locations' import { DeckSetupTools } from '../DeckSetupTools' import { LabwareTools } from '../LabwareTools' @@ -32,6 +33,7 @@ vi.mock('../../../../labware-ingred/actions') vi.mock('../../../../step-forms/actions') vi.mock('../../../../step-forms/actions/additionalItems') vi.mock('../../../../labware-ingred/selectors') +vi.mock('../../../../tutorial/selectors') const render = (props: React.ComponentProps) => { return renderWithProviders(, { i18nInstance: i18n, @@ -66,6 +68,7 @@ describe('DeckSetupTools', () => { additionalEquipmentOnDeck: {}, pipettes: {}, }) + vi.mocked(getDismissedHints).mockReturnValue([]) }) it('should render the relevant modules and fixtures for slot D3 on Flex with tabs', () => { render(props) diff --git a/protocol-designer/src/pages/Designer/DeckSetup/__tests__/MagnetModuleChangeContent.test.tsx b/protocol-designer/src/pages/Designer/DeckSetup/__tests__/MagnetModuleChangeContent.test.tsx new file mode 100644 index 00000000000..1767d0cea18 --- /dev/null +++ b/protocol-designer/src/pages/Designer/DeckSetup/__tests__/MagnetModuleChangeContent.test.tsx @@ -0,0 +1,32 @@ +import { describe, it } from 'vitest' +import { screen } from '@testing-library/react' +import { i18n } from '../../../../assets/localization' +import { renderWithProviders } from '../../../../__testing-utils__' +import { MagnetModuleChangeContent } from '../MagnetModuleChangeContent' + +const render = () => { + return renderWithProviders(, { + i18nInstance: i18n, + })[0] +} + +describe('MagnetModuleChangeContent', () => { + it('renders the text for the modal content', () => { + render() + screen.getByText( + 'Switching between GEN1 and GEN2 Magnetic Modules will clear all non-default engage heights from existing magnet steps in your protocol. GEN1 and GEN2 Magnetic Modules do not use the same units.' + ) + screen.getByText( + 'To convert engage heights from GEN1 to GEN2, divide your engage height by 2.' + ) + screen.getByText( + 'To convert engage heights from GEN2 to GEN1, multiply your engage height by 2.' + ) + screen.getByText( + 'You may also need to alter the time you pause while your magnet is engaged.' + ) + screen.getByText( + 'Read more about the differences between GEN1 and GEN2 Magnetic Modules' + ) + }) +}) From eb837050ccd7622df12a14e51d39a1ec3ac5c286 Mon Sep 17 00:00:00 2001 From: Jethary Alcid <66035149+jerader@users.noreply.github.com> Date: Thu, 24 Oct 2024 13:03:12 -0400 Subject: [PATCH 10/17] feat(protocol-designer, components): revamp form errors and fix logic for rendering (#16576) closes AUTH-972 --- components/src/atoms/InputField/index.tsx | 16 +++++++------ .../src/molecules/DropdownMenu/index.tsx | 8 +++++++ .../molecules/DropdownStepFormField/index.tsx | 4 ++++ .../src/organisms/Alerts/FormAlerts.tsx | 1 + .../PipetteFields/WellSelectionField.tsx | 4 +++- .../StepForm/StepFormToolbox.tsx | 23 +++++++++++++++++-- .../StepForm/StepTools/MixTools/index.tsx | 7 +++++- .../StepTools/MoveLiquidTools/index.tsx | 12 +++++++++- .../StepTools/__tests__/MagnetTools.test.tsx | 1 + .../__tests__/TemperatureTools.test.tsx | 1 + .../Designer/ProtocolSteps/StepForm/index.tsx | 10 ++++---- .../Designer/ProtocolSteps/StepForm/types.ts | 2 ++ .../src/steplist/formLevel/errors.ts | 6 ++--- 13 files changed, 76 insertions(+), 19 deletions(-) diff --git a/components/src/atoms/InputField/index.tsx b/components/src/atoms/InputField/index.tsx index 8933ca5345c..14a4f520377 100644 --- a/components/src/atoms/InputField/index.tsx +++ b/components/src/atoms/InputField/index.tsx @@ -71,6 +71,7 @@ export interface InputFieldProps { leftIcon?: IconName showDeleteIcon?: boolean onDelete?: () => void + hasBackgroundError?: boolean } export const InputField = React.forwardRef( @@ -83,6 +84,7 @@ export const InputField = React.forwardRef( tooltipText, tabIndex = 0, showDeleteIcon = false, + hasBackgroundError = false, ...inputProps } = props const hasError = props.error != null @@ -103,11 +105,13 @@ export const InputField = React.forwardRef( const INPUT_FIELD = css` display: flex; - background-color: ${COLORS.white}; + background-color: ${hasBackgroundError ? COLORS.red30 : COLORS.white}; border-radius: ${BORDERS.borderRadius4}; padding: ${SPACING.spacing8}; - border: 1px ${BORDERS.styleSolid} - ${hasError ? COLORS.red50 : COLORS.grey50}; + border: ${hasBackgroundError + ? 'none' + : `1px ${BORDERS.styleSolid} + ${hasError ? COLORS.red50 : COLORS.grey50}`}; font-size: ${TYPOGRAPHY.fontSizeP}; width: 100%; height: ${size === 'small' ? '2rem' : '2.75rem'}; @@ -321,10 +325,7 @@ export const InputField = React.forwardRef( ) : null} {hasError ? ( - + {props.error} ) : null} @@ -335,6 +336,7 @@ export const InputField = React.forwardRef( ) const StyledInput = styled.input` + background-color: transparent; &::placeholder { color: ${COLORS.grey40}; } diff --git a/components/src/molecules/DropdownMenu/index.tsx b/components/src/molecules/DropdownMenu/index.tsx index 30a02209121..af336737fa5 100644 --- a/components/src/molecules/DropdownMenu/index.tsx +++ b/components/src/molecules/DropdownMenu/index.tsx @@ -63,6 +63,10 @@ export interface DropdownMenuProps { tabIndex?: number /** optional error */ error?: string | null + /** focus handler */ + onFocus?: React.FocusEventHandler + /** blur handler */ + onBlur?: React.FocusEventHandler } // TODO: (smb: 4/15/22) refactor this to use html select for accessibility @@ -79,6 +83,8 @@ export function DropdownMenu(props: DropdownMenuProps): JSX.Element { tooltipText, tabIndex = 0, error, + onFocus, + onBlur, } = props const [targetProps, tooltipProps] = useHoverTooltip() const [showDropdownMenu, setShowDropdownMenu] = React.useState(false) @@ -222,6 +228,8 @@ export function DropdownMenu(props: DropdownMenuProps): JSX.Element { e.preventDefault() toggleSetShowDropdownMenu() }} + onFocus={onFocus} + onBlur={onBlur} css={DROPDOWN_STYLE} tabIndex={tabIndex} > diff --git a/protocol-designer/src/molecules/DropdownStepFormField/index.tsx b/protocol-designer/src/molecules/DropdownStepFormField/index.tsx index a6777a5be00..6cd81c742c4 100644 --- a/protocol-designer/src/molecules/DropdownStepFormField/index.tsx +++ b/protocol-designer/src/molecules/DropdownStepFormField/index.tsx @@ -22,6 +22,8 @@ export function DropdownStepFormField( tooltipContent, addPadding = true, width = '17.5rem', + onFieldFocus, + onFieldBlur, } = props const { t } = useTranslation('tooltip') const availableOptionId = options.find(opt => opt.value === value) @@ -35,6 +37,8 @@ export function DropdownStepFormField( dropdownType="neutral" filterOptions={options} title={title} + onBlur={onFieldBlur} + onFocus={onFieldFocus} currentOption={ availableOptionId ?? { name: 'Choose option', value: '' } } diff --git a/protocol-designer/src/organisms/Alerts/FormAlerts.tsx b/protocol-designer/src/organisms/Alerts/FormAlerts.tsx index 290295305af..d08e25e1ec6 100644 --- a/protocol-designer/src/organisms/Alerts/FormAlerts.tsx +++ b/protocol-designer/src/organisms/Alerts/FormAlerts.tsx @@ -105,6 +105,7 @@ function FormAlertsComponent(props: FormAlertsProps): JSX.Element | null { : undefined } width="100%" + iconMarginLeft={SPACING.spacing4} > diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/PipetteFields/WellSelectionField.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/PipetteFields/WellSelectionField.tsx index 0775e14304e..57125f7b8a1 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/PipetteFields/WellSelectionField.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/PipetteFields/WellSelectionField.tsx @@ -28,6 +28,7 @@ export type WellSelectionFieldProps = FieldProps & { nozzles: string | null pipetteId?: string | null labwareId?: string | null + hasFormError?: boolean } export const WellSelectionField = ( @@ -45,6 +46,7 @@ export const WellSelectionField = ( disabled, errorToShow, tooltipContent, + hasFormError, } = props const { t, i18n } = useTranslation(['form', 'tooltip']) const dispatch = useDispatch() @@ -90,7 +92,6 @@ export const WellSelectionField = ( ? t(`step_edit_form.wellSelectionLabel.columns_${name}`) : t(`step_edit_form.wellSelectionLabel.wells_${name}`) const [targetProps, tooltipProps] = useHoverTooltip() - return ( <> @@ -116,6 +117,7 @@ export const WellSelectionField = ( error={errorToShow} value={primaryWellCount} onClick={handleOpen} + hasBackgroundError={hasFormError} /> {createPortal( diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx index 9c60605163c..79287acc49f 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx @@ -36,10 +36,15 @@ import { TemperatureTools, ThermocyclerTools, } from './StepTools' -import { getSaveStepSnackbarText } from './utils' +import { + getSaveStepSnackbarText, + getVisibleFormErrors, + getVisibleFormWarnings, +} from './utils' import type { StepFieldName } from '../../../../steplist/fieldLevel' import type { FormData, StepType } from '../../../../form-types' import type { FieldPropsByName, FocusHandlers, StepFormProps } from './types' +import { getFormLevelErrorsForUnsavedForm } from '../../../../step-forms/selectors' type StepFormMap = { [K in StepType]?: React.ComponentType | null @@ -91,6 +96,9 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { const timelineWarningsForSelectedStep = useSelector( getTimelineWarningsForSelectedStep ) + const formLevelErrorsForUnsavedForm = useSelector( + getFormLevelErrorsForUnsavedForm + ) const timeline = useSelector(getRobotStateTimeline) const [toolboxStep, setToolboxStep] = useState( // progress to step 2 if thermocycler form is populated @@ -103,6 +111,16 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { showFormErrorsAndWarnings, setShowFormErrorsAndWarnings, ] = useState(false) + const visibleFormWarnings = getVisibleFormWarnings({ + focusedField, + dirtyFields: dirtyFields ?? [], + errors: formWarningsForSelectedStep, + }) + const visibleFormErrors = getVisibleFormErrors({ + focusedField, + dirtyFields: dirtyFields ?? [], + errors: formLevelErrorsForUnsavedForm, + }) const [isRename, setIsRename] = useState(false) const icon = stepIconsByType[formData.stepType] @@ -126,7 +144,7 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { formData.stepType === 'mix' || formData.stepType === 'thermocycler' const numWarnings = - formWarningsForSelectedStep.length + timelineWarningsForSelectedStep.length + visibleFormWarnings.length + timelineWarningsForSelectedStep.length const numErrors = timeline.errors?.length ?? 0 const handleSaveClick = (): void => { @@ -229,6 +247,7 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { propsForFields, focusHandlers, toolboxStep, + visibleFormErrors, }} /> diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MixTools/index.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MixTools/index.tsx index b9781a92118..893a2b4d5f1 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MixTools/index.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MixTools/index.tsx @@ -42,7 +42,7 @@ import { import type { StepFormProps } from '../../types' export function MixTools(props: StepFormProps): JSX.Element { - const { propsForFields, formData, toolboxStep } = props + const { propsForFields, formData, toolboxStep, visibleFormErrors } = props const pipettes = useSelector(getPipetteEntities) const enableReturnTip = useSelector(getEnableReturnTip) const labwares = useSelector(getLabwareEntities) @@ -89,6 +89,11 @@ export function MixTools(props: StepFormProps): JSX.Element { labwareId={formData.labware} pipetteId={formData.pipette} nozzles={String(propsForFields.nozzles.value) ?? null} + hasFormError={ + visibleFormErrors?.some(error => + error.dependentFields.includes('labware') + ) ?? false + } /> diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MoveLiquidTools/index.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MoveLiquidTools/index.tsx index f30e5338b60..551754dc75e 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MoveLiquidTools/index.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MoveLiquidTools/index.tsx @@ -50,7 +50,7 @@ const makeAddFieldNamePrefix = (prefix: string) => ( ): StepFieldName => `${prefix}_${fieldName}` export function MoveLiquidTools(props: StepFormProps): JSX.Element { - const { toolboxStep, propsForFields, formData } = props + const { toolboxStep, propsForFields, formData, visibleFormErrors } = props const { t, i18n } = useTranslation(['protocol_steps', 'form']) const { path } = formData const [tab, setTab] = useState<'aspirate' | 'dispense'>('aspirate') @@ -126,6 +126,11 @@ export function MoveLiquidTools(props: StepFormProps): JSX.Element { labwareId={String(propsForFields.aspirate_labware.value)} pipetteId={formData.pipette} nozzles={String(propsForFields.nozzles.value) ?? null} + hasFormError={ + visibleFormErrors?.some(error => + error.dependentFields.includes('aspirate_labware') + ) ?? false + } /> @@ -136,6 +141,11 @@ export function MoveLiquidTools(props: StepFormProps): JSX.Element { labwareId={String(propsForFields.dispense_labware.value)} pipetteId={formData.pipette} nozzles={String(propsForFields.nozzles.value) ?? null} + hasFormError={ + visibleFormErrors?.some(error => + error.dependentFields.includes('dispense_wells') + ) ?? false + } /> )} diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/MagnetTools.test.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/MagnetTools.test.tsx index 5a901290c37..c6cb5f13383 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/MagnetTools.test.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/MagnetTools.test.tsx @@ -47,6 +47,7 @@ describe('MagnetTools', () => { dirtyFields: [], focusedField: null, }, + visibleFormErrors: [], toolboxStep: 1, propsForFields: { magnetAction: { diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/TemperatureTools.test.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/TemperatureTools.test.tsx index 34b0b0bd501..8edd87af457 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/TemperatureTools.test.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/TemperatureTools.test.tsx @@ -40,6 +40,7 @@ describe('TemperatureTools', () => { dirtyFields: [], focusedField: null, }, + visibleFormErrors: [], toolboxStep: 1, propsForFields: { moduleId: { diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/index.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/index.tsx index 0b7049bd546..ad668a98852 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/index.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/index.tsx @@ -79,9 +79,12 @@ function StepFormManager(props: StepFormManagerProps): JSX.Element | null { if (fieldName === focusedField) { setFocusedField(null) } - if (!dirtyFields.includes(fieldName)) { - setDirtyFields([...dirtyFields, fieldName]) - } + setDirtyFields(prevDirtyFields => { + if (!prevDirtyFields.includes(fieldName)) { + return [...prevDirtyFields, fieldName] + } + return prevDirtyFields + }) } const stepId = formData?.id const handleDelete = (): void => { @@ -144,7 +147,6 @@ function StepFormManager(props: StepFormManagerProps): JSX.Element | null { ) { handleSave = confirmAddPauseUntilHeaterShakerTempStep } - return ( <> {/* TODO: update these modals to match new modal design */} diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/types.ts b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/types.ts index 9d9762594af..c7b063c4731 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/types.ts +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/types.ts @@ -1,4 +1,5 @@ import type { FormData, StepFieldName } from '../../../../form-types' +import type { StepFormErrors } from '../../../../steplist' export interface FocusHandlers { focusedField: StepFieldName | null dirtyFields: StepFieldName[] @@ -24,4 +25,5 @@ export interface StepFormProps { focusHandlers: FocusHandlers propsForFields: FieldPropsByName toolboxStep: number + visibleFormErrors: StepFormErrors } diff --git a/protocol-designer/src/steplist/formLevel/errors.ts b/protocol-designer/src/steplist/formLevel/errors.ts index e5203cdc84e..9bf495b3134 100644 --- a/protocol-designer/src/steplist/formLevel/errors.ts +++ b/protocol-designer/src/steplist/formLevel/errors.ts @@ -50,15 +50,15 @@ export interface FormError { dependentFields: StepFieldName[] } const INCOMPATIBLE_ASPIRATE_LABWARE: FormError = { - title: 'Selected aspirate labware is incompatible with selected pipette', + title: 'Selected aspirate labware is incompatible with pipette', dependentFields: ['aspirate_labware', 'pipette'], } const INCOMPATIBLE_DISPENSE_LABWARE: FormError = { - title: 'Selected dispense labware is incompatible with selected pipette', + title: 'Selected dispense labware is incompatible with pipette', dependentFields: ['dispense_labware', 'pipette'], } const INCOMPATIBLE_LABWARE: FormError = { - title: 'Selected labware is incompatible with selected pipette', + title: 'Selected labware is incompatible with pipette', dependentFields: ['labware', 'pipette'], } const PAUSE_TYPE_REQUIRED: FormError = { From 01713802034aa78f74a465a1cfc9c3d38b7016e4 Mon Sep 17 00:00:00 2001 From: koji Date: Thu, 24 Oct 2024 13:03:52 -0400 Subject: [PATCH 11/17] refactor(protocol-designer): address feedback for RQA-3424 (#16596) * refactor(protocol-designer): address feedback for RQA-3424 --- protocol-designer/src/organisms/SlotInformation/index.tsx | 2 +- .../src/pages/ProtocolOverview/InstrumentsInfo.tsx | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/protocol-designer/src/organisms/SlotInformation/index.tsx b/protocol-designer/src/organisms/SlotInformation/index.tsx index a945ebffcd7..4e7d2ffbfd6 100644 --- a/protocol-designer/src/organisms/SlotInformation/index.tsx +++ b/protocol-designer/src/organisms/SlotInformation/index.tsx @@ -11,8 +11,8 @@ import { StyledText, TYPOGRAPHY, } from '@opentrons/components' -import type { FC } from 'react' import { FLEX_ROBOT_TYPE } from '@opentrons/shared-data' +import type { FC } from 'react' import type { RobotType } from '@opentrons/shared-data' interface SlotInformationProps { diff --git a/protocol-designer/src/pages/ProtocolOverview/InstrumentsInfo.tsx b/protocol-designer/src/pages/ProtocolOverview/InstrumentsInfo.tsx index 9f661bb4137..b6ca5356d96 100644 --- a/protocol-designer/src/pages/ProtocolOverview/InstrumentsInfo.tsx +++ b/protocol-designer/src/pages/ProtocolOverview/InstrumentsInfo.tsx @@ -119,7 +119,6 @@ export function InstrumentsInfo({ type="large" description={ - {' '} - {' '} - {' '} Date: Thu, 24 Oct 2024 13:51:55 -0400 Subject: [PATCH 12/17] fix(protocol-designer): fix long step name, step details, and step summary layout issue (#16547) fixes RQA-3355 # Overview Fixing layout issues in `StepFormToolbox`, `StepContainer`, and `StepSummary` when step names, step details, and step summaries are too long, causing the step icon to shrink, the step name and step summaries content to go off-screen, and the step details text to exceed the window size. Refer to the ticket for details on the current layout issues and the expected design. ## Test Plan and Hands on Testing ## Changelog ## Review requests ## Risk assessment --------- Co-authored-by: shiyaochen --- .../StepForm/StepFormToolbox.tsx | 13 +++-- .../Designer/ProtocolSteps/StepSummary.tsx | 47 ++++++++++--------- .../ProtocolSteps/Timeline/StepContainer.tsx | 16 ++++++- 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx index 79287acc49f..1f5f5d0ac64 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx @@ -2,7 +2,6 @@ import { useState } from 'react' import get from 'lodash/get' import { useTranslation } from 'react-i18next' import { useSelector } from 'react-redux' - import { ALIGN_CENTER, Btn, @@ -24,7 +23,7 @@ import { RenameStepModal } from '../../../../organisms/RenameStepModal' import { getFormWarningsForSelectedStep } from '../../../../dismiss/selectors' import { getTimelineWarningsForSelectedStep } from '../../../../top-selectors/timelineWarnings' import { getRobotStateTimeline } from '../../../../file-data/selectors' -import { BUTTON_LINK_STYLE } from '../../../../atoms' +import { BUTTON_LINK_STYLE, LINE_CLAMP_TEXT_STYLE } from '../../../../atoms' import { CommentTools, HeaterShakerTools, @@ -231,8 +230,14 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { } title={ - - + + {i18n.format(t(formData.stepName), 'capitalize')} diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepSummary.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepSummary.tsx index 18938c3975b..7d76fc02ae2 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepSummary.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepSummary.tsx @@ -5,23 +5,20 @@ import flatten from 'lodash/flatten' import last from 'lodash/last' import { ALIGN_CENTER, - BORDERS, - COLORS, DIRECTION_COLUMN, Flex, - FLEX_MAX_CONTENT, + ListItem, SPACING, StyledText, Tag, } from '@opentrons/components' import { getModuleDisplayName } from '@opentrons/shared-data' - import { getLabwareEntities, getModuleEntities, } from '../../../step-forms/selectors' import { getLabwareNicknamesById } from '../../../ui/labware/selectors' - +import { LINE_CLAMP_TEXT_STYLE } from '../../../atoms' import type { FormData } from '../../../form-types' interface StyledTransProps { @@ -29,11 +26,19 @@ interface StyledTransProps { tagText?: string values?: object } + function StyledTrans(props: StyledTransProps): JSX.Element { const { i18nKey, tagText, values } = props const { t } = useTranslation(['protocol_steps', 'application']) return ( - + {stepSummaryContent != null ? ( - - {stepSummaryContent} - + + {stepSummaryContent} + ) : null} {stepDetails != null && stepDetails !== '' ? ( - - {stepDetails} - + + + + {stepDetails} + + + ) : null} ) : null diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/Timeline/StepContainer.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/Timeline/StepContainer.tsx index 039c09220c2..c869fdf72dd 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/Timeline/StepContainer.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/Timeline/StepContainer.tsx @@ -30,6 +30,7 @@ import { populateForm, } from '../../../../ui/steps/actions/actions' import { getMultiSelectItemIds } from '../../../../ui/steps/selectors' +import { LINE_CLAMP_TEXT_STYLE } from '../../../../atoms' import { StepOverflowMenu } from './StepOverflowMenu' import { capitalizeFirstLetterAfterNumber } from './utils' @@ -217,9 +218,20 @@ export function StepContainer(props: StepContainerProps): JSX.Element { width="100%" > {iconName && ( - + )} - + {capitalizeFirstLetterAfterNumber(title)} From 565865def36986071591a05c2f47b1f084fcadb7 Mon Sep 17 00:00:00 2001 From: TamarZanzouri Date: Thu, 24 Oct 2024 14:00:14 -0400 Subject: [PATCH 13/17] fix(api): home all gripper axis when a stall is detected (#16579) --- .../ErrorRecoveryWizard.tsx | 4 +- .../__tests__/useHomeGripperZAxis.test.ts | 40 +++++++++++++------ .../__tests__/useRecoveryCommands.test.ts | 9 +++-- .../ErrorRecoveryFlows/hooks/index.ts | 2 +- ...eHomeGripperZAxis.ts => useHomeGripper.ts} | 8 ++-- .../hooks/useRecoveryCommands.ts | 22 +++++++--- 6 files changed, 55 insertions(+), 30 deletions(-) rename app/src/organisms/ErrorRecoveryFlows/hooks/{useHomeGripperZAxis.ts => useHomeGripper.ts} (82%) diff --git a/app/src/organisms/ErrorRecoveryFlows/ErrorRecoveryWizard.tsx b/app/src/organisms/ErrorRecoveryFlows/ErrorRecoveryWizard.tsx index 5f38dfabf48..e1dd7c5add2 100644 --- a/app/src/organisms/ErrorRecoveryFlows/ErrorRecoveryWizard.tsx +++ b/app/src/organisms/ErrorRecoveryFlows/ErrorRecoveryWizard.tsx @@ -29,7 +29,7 @@ import { import { RecoveryInProgress } from './RecoveryInProgress' import { getErrorKind } from './utils' import { RECOVERY_MAP } from './constants' -import { useHomeGripperZAxis } from './hooks' +import { useHomeGripper } from './hooks' import type { LabwareDefinition2, RobotType } from '@opentrons/shared-data' import type { RecoveryRoute, RouteStep, RecoveryContentProps } from './types' @@ -90,7 +90,7 @@ export function ErrorRecoveryWizard( routeUpdateActions, }) - useHomeGripperZAxis(props) + useHomeGripper(props) return } diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useHomeGripperZAxis.test.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useHomeGripperZAxis.test.ts index 197dfbfd3e7..32f5d939eb8 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useHomeGripperZAxis.test.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useHomeGripperZAxis.test.ts @@ -1,12 +1,14 @@ import { renderHook, act } from '@testing-library/react' import { describe, it, expect, vi, beforeEach } from 'vitest' -import { useHomeGripperZAxis } from '../useHomeGripperZAxis' +import { useHomeGripper } from '../useHomeGripper' import { RECOVERY_MAP } from '/app/organisms/ErrorRecoveryFlows/constants' -describe('useHomeGripperZAxis', () => { +describe('useHomeGripper', () => { const mockRecoveryCommands = { - homeGripperZAxis: vi.fn().mockResolvedValue(undefined), + updatePositionEstimatorsAndHomeGripper: vi + .fn() + .mockResolvedValue(undefined), } const mockRouteUpdateActions = { @@ -28,7 +30,7 @@ describe('useHomeGripperZAxis', () => { it('should home gripper Z axis when in manual gripper step and door is closed', async () => { renderHook(() => { - useHomeGripperZAxis({ + useHomeGripper({ recoveryCommands: mockRecoveryCommands, routeUpdateActions: mockRouteUpdateActions, recoveryMap: mockRecoveryMap, @@ -43,7 +45,9 @@ describe('useHomeGripperZAxis', () => { expect(mockRouteUpdateActions.handleMotionRouting).toHaveBeenCalledWith( true ) - expect(mockRecoveryCommands.homeGripperZAxis).toHaveBeenCalled() + expect( + mockRecoveryCommands.updatePositionEstimatorsAndHomeGripper + ).toHaveBeenCalled() expect(mockRouteUpdateActions.handleMotionRouting).toHaveBeenCalledWith( false ) @@ -51,7 +55,7 @@ describe('useHomeGripperZAxis', () => { it('should go back to previous step when door is open', () => { renderHook(() => { - useHomeGripperZAxis({ + useHomeGripper({ recoveryCommands: mockRecoveryCommands, routeUpdateActions: mockRouteUpdateActions, recoveryMap: mockRecoveryMap, @@ -60,12 +64,14 @@ describe('useHomeGripperZAxis', () => { }) expect(mockRouteUpdateActions.goBackPrevStep).toHaveBeenCalled() - expect(mockRecoveryCommands.homeGripperZAxis).not.toHaveBeenCalled() + expect( + mockRecoveryCommands.updatePositionEstimatorsAndHomeGripper + ).not.toHaveBeenCalled() }) it('should not home again if already homed once', async () => { const { rerender } = renderHook(() => { - useHomeGripperZAxis({ + useHomeGripper({ recoveryCommands: mockRecoveryCommands, routeUpdateActions: mockRouteUpdateActions, recoveryMap: mockRecoveryMap, @@ -77,17 +83,21 @@ describe('useHomeGripperZAxis', () => { await new Promise(resolve => setTimeout(resolve, 0)) }) - expect(mockRecoveryCommands.homeGripperZAxis).toHaveBeenCalledTimes(1) + expect( + mockRecoveryCommands.updatePositionEstimatorsAndHomeGripper + ).toHaveBeenCalledTimes(1) rerender() - expect(mockRecoveryCommands.homeGripperZAxis).toHaveBeenCalledTimes(1) + expect( + mockRecoveryCommands.updatePositionEstimatorsAndHomeGripper + ).toHaveBeenCalledTimes(1) }) it('should reset hasHomedOnce when step changes to non-manual gripper step and back', async () => { const { rerender } = renderHook( ({ recoveryMap }) => { - useHomeGripperZAxis({ + useHomeGripper({ recoveryCommands: mockRecoveryCommands, routeUpdateActions: mockRouteUpdateActions, recoveryMap, @@ -103,7 +113,9 @@ describe('useHomeGripperZAxis', () => { await new Promise(resolve => setTimeout(resolve, 0)) }) - expect(mockRecoveryCommands.homeGripperZAxis).toHaveBeenCalledTimes(1) + expect( + mockRecoveryCommands.updatePositionEstimatorsAndHomeGripper + ).toHaveBeenCalledTimes(1) rerender({ recoveryMap: { step: 'SOME_OTHER_STEP' } as any }) @@ -117,6 +129,8 @@ describe('useHomeGripperZAxis', () => { await new Promise(resolve => setTimeout(resolve, 0)) }) - expect(mockRecoveryCommands.homeGripperZAxis).toHaveBeenCalledTimes(2) + expect( + mockRecoveryCommands.updatePositionEstimatorsAndHomeGripper + ).toHaveBeenCalledTimes(2) }) }) diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useRecoveryCommands.test.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useRecoveryCommands.test.ts index 11a15edfbfd..a553bdcb4ee 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useRecoveryCommands.test.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useRecoveryCommands.test.ts @@ -14,7 +14,8 @@ import { RELEASE_GRIPPER_JAW, buildPickUpTips, buildIgnorePolicyRules, - HOME_GRIPPER_Z_AXIS, + UPDATE_ESTIMATORS_EXCEPT_PLUNGERS, + HOME_GRIPPER_Z, } from '../useRecoveryCommands' import { RECOVERY_MAP } from '../../constants' @@ -264,15 +265,15 @@ describe('useRecoveryCommands', () => { ) }) - it('should call homeGripperZAxis and resolve the promise', async () => { + it('should call useUpdatePositionEstimators and resolve the promise', async () => { const { result } = renderHook(() => useRecoveryCommands(props)) await act(async () => { - await result.current.homeGripperZAxis() + await result.current.updatePositionEstimatorsAndHomeGripper() }) expect(mockChainRunCommands).toHaveBeenCalledWith( - [HOME_GRIPPER_Z_AXIS], + [UPDATE_ESTIMATORS_EXCEPT_PLUNGERS, HOME_GRIPPER_Z], false ) }) diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/index.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/index.ts index baa685c0dcc..75904a24966 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/index.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/index.ts @@ -5,7 +5,7 @@ export { useRouteUpdateActions } from './useRouteUpdateActions' export { useERUtils } from './useERUtils' export { useRecoveryTakeover } from './useRecoveryTakeover' export { useRetainedFailedCommandBySource } from './useRetainedFailedCommandBySource' -export { useHomeGripperZAxis } from './useHomeGripperZAxis' +export { useHomeGripper } from './useHomeGripper' export type { ERUtilsProps } from './useERUtils' export type { UseRouteUpdateActionsResult } from './useRouteUpdateActions' diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/useHomeGripperZAxis.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/useHomeGripper.ts similarity index 82% rename from app/src/organisms/ErrorRecoveryFlows/hooks/useHomeGripperZAxis.ts rename to app/src/organisms/ErrorRecoveryFlows/hooks/useHomeGripper.ts index 649fb801d44..b165e59ebd4 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/useHomeGripperZAxis.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/useHomeGripper.ts @@ -3,8 +3,8 @@ import { RECOVERY_MAP } from '/app/organisms/ErrorRecoveryFlows/constants' import type { ErrorRecoveryWizardProps } from '/app/organisms/ErrorRecoveryFlows/ErrorRecoveryWizard' -// Home the gripper z-axis implicitly. Because the z-home is not tied to a CTA, it must be handled here. -export function useHomeGripperZAxis({ +// Home the gripper implicitly. Because the home is not tied to a CTA, it must be handled here. +export function useHomeGripper({ recoveryCommands, routeUpdateActions, recoveryMap, @@ -20,7 +20,7 @@ export function useHomeGripperZAxis({ useLayoutEffect(() => { const { handleMotionRouting, goBackPrevStep } = routeUpdateActions - const { homeGripperZAxis } = recoveryCommands + const { updatePositionEstimatorsAndHomeGripper } = recoveryCommands if (!hasHomedOnce) { if (isManualGripperStep) { @@ -28,7 +28,7 @@ export function useHomeGripperZAxis({ void goBackPrevStep() } else { void handleMotionRouting(true) - .then(() => homeGripperZAxis()) + .then(() => updatePositionEstimatorsAndHomeGripper()) .then(() => { setHasHomedOnce(true) }) diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/useRecoveryCommands.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/useRecoveryCommands.ts index f0962b07693..69101d92fe9 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/useRecoveryCommands.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/useRecoveryCommands.ts @@ -63,7 +63,7 @@ export interface UseRecoveryCommandsResult { /* A non-terminal recovery command */ releaseGripperJaws: () => Promise /* A non-terminal recovery command */ - homeGripperZAxis: () => Promise + updatePositionEstimatorsAndHomeGripper: () => Promise /* A non-terminal recovery command */ moveLabwareWithoutPause: () => Promise } @@ -269,8 +269,13 @@ export function useRecoveryCommands({ return chainRunRecoveryCommands([RELEASE_GRIPPER_JAW]) }, [chainRunRecoveryCommands]) - const homeGripperZAxis = useCallback((): Promise => { - return chainRunRecoveryCommands([HOME_GRIPPER_Z_AXIS]) + const updatePositionEstimatorsAndHomeGripper = useCallback((): Promise< + CommandData[] + > => { + return chainRunRecoveryCommands([ + UPDATE_ESTIMATORS_EXCEPT_PLUNGERS, + HOME_GRIPPER_Z, + ]) }, [chainRunRecoveryCommands]) const moveLabwareWithoutPause = useCallback((): Promise => { @@ -291,7 +296,7 @@ export function useRecoveryCommands({ homePipetteZAxes, pickUpTips, releaseGripperJaws, - homeGripperZAxis, + updatePositionEstimatorsAndHomeGripper, moveLabwareWithoutPause, skipFailedCommand, ignoreErrorKindThisRun, @@ -310,10 +315,15 @@ export const RELEASE_GRIPPER_JAW: CreateCommand = { intent: 'fixit', } -export const HOME_GRIPPER_Z_AXIS: CreateCommand = { +// in case the gripper does not know the position after a stall/collision we must update the position. +export const UPDATE_ESTIMATORS_EXCEPT_PLUNGERS: CreateCommand = { + commandType: 'unsafe/updatePositionEstimators', + params: { axes: ['x', 'y', 'extensionZ'] }, +} + +export const HOME_GRIPPER_Z: CreateCommand = { commandType: 'home', params: { axes: ['extensionZ'] }, - intent: 'fixit', } const buildMoveLabwareWithoutPause = ( From dcc8f76d6be83ff2900a695cbdccf6d2d6c66cce Mon Sep 17 00:00:00 2001 From: syao1226 <146495172+syao1226@users.noreply.github.com> Date: Thu, 24 Oct 2024 14:30:12 -0400 Subject: [PATCH 14/17] fix(protocol-designer): preserve uppercase letters of step name (#16584) fixes RQA-3398 # Overview Fixes the issue where renaming a step name with uppercase characters caused everything after the first character to be lowercased. ## Test Plan and Hands on Testing ## Changelog - added a `capitalizeFirstLetter()` to `StepForm/utils` to capitalize onlt the first character while leaving the rest of the string unchanged ## Review requests ## Risk assessment --------- Co-authored-by: shiyaochen --- .../src/organisms/RenameStepModal/index.tsx | 6 +++--- .../ProtocolSteps/StepForm/StepFormToolbox.tsx | 3 ++- .../ProtocolSteps/StepForm/__tests__/utils.test.ts | 14 +++++++++++++- .../pages/Designer/ProtocolSteps/StepForm/utils.ts | 3 +++ 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/protocol-designer/src/organisms/RenameStepModal/index.tsx b/protocol-designer/src/organisms/RenameStepModal/index.tsx index 2b98546751a..9ee1d607fbe 100644 --- a/protocol-designer/src/organisms/RenameStepModal/index.tsx +++ b/protocol-designer/src/organisms/RenameStepModal/index.tsx @@ -17,7 +17,7 @@ import { TYPOGRAPHY, InputField, } from '@opentrons/components' -import { i18n } from '../../assets/localization' +import { capitalizeFirstLetter } from '../../pages/Designer/ProtocolSteps/StepForm/utils' import { getTopPortalEl } from '../../components/portals/TopPortal' import { renameStep } from '../../labware-ingred/actions' import type { FormData } from '../../form-types' @@ -31,7 +31,7 @@ export function RenameStepModal(props: RenameStepModalProps): JSX.Element { const { onClose, formData } = props const dispatch = useDispatch() const { t } = useTranslation(['form', 'shared', 'protocol_steps']) - const initialName = i18n.format(t(formData.stepName), 'capitalize') + const initialName = capitalizeFirstLetter(String(formData.stepName)) const [stepName, setStepName] = useState(initialName) const [stepDetails, setStepDetails] = useState( String(formData.stepDetails) @@ -43,7 +43,7 @@ export function RenameStepModal(props: RenameStepModalProps): JSX.Element { renameStep({ stepId, update: { - stepName: stepName, + stepName: stepName !== '' ? stepName : initialName, stepDetails: stepDetails, }, }) diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx index 1f5f5d0ac64..d18bee31915 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepFormToolbox.tsx @@ -39,6 +39,7 @@ import { getSaveStepSnackbarText, getVisibleFormErrors, getVisibleFormWarnings, + capitalizeFirstLetter, } from './utils' import type { StepFieldName } from '../../../../steplist/fieldLevel' import type { FormData, StepType } from '../../../../form-types' @@ -238,7 +239,7 @@ export function StepFormToolbox(props: StepFormToolboxProps): JSX.Element { word-break: break-all `} > - {i18n.format(t(formData.stepName), 'capitalize')} + {capitalizeFirstLetter(String(formData.stepName))} } diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/__tests__/utils.test.ts b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/__tests__/utils.test.ts index e95aad5d427..3d3e3a47393 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/__tests__/utils.test.ts +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/__tests__/utils.test.ts @@ -3,7 +3,10 @@ import { SOURCE_WELL_BLOWOUT_DESTINATION, DEST_WELL_BLOWOUT_DESTINATION, } from '@opentrons/step-generation' -import { getBlowoutLocationOptionsForForm } from '../utils' +import { + capitalizeFirstLetter, + getBlowoutLocationOptionsForForm, +} from '../utils' describe('getBlowoutLocationOptionsForForm', () => { const destOption = { @@ -57,3 +60,12 @@ describe('getBlowoutLocationOptionsForForm', () => { expect(result).toEqual([]) }) }) + +describe('capitalizeFirstLetter', () => { + it('should capitalize the first letter of a step name and leave the rest unchanged', () => { + const stepName = 'move labware to D3 on top of Magnetic Block' + expect(capitalizeFirstLetter(stepName)).toBe( + 'Move labware to D3 on top of Magnetic Block' + ) + }) +}) diff --git a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/utils.ts b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/utils.ts index 9293b6b6c64..00f4749a71b 100644 --- a/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/utils.ts +++ b/protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/utils.ts @@ -322,3 +322,6 @@ export const getSaveStepSnackbarText = ( return t(`protocol_steps:save_no_errors`, { stepType: stepTypeDisplayName }) } } + +export const capitalizeFirstLetter = (stepName: string): string => + `${stepName.charAt(0).toUpperCase()}${stepName.slice(1)}` From d454914478fd2732e9f348018263e8de2a65c98a Mon Sep 17 00:00:00 2001 From: Shlok Amin Date: Thu, 24 Oct 2024 14:46:32 -0400 Subject: [PATCH 15/17] fix(scripts): set correct aws bucket region when deploying (#16598) --- scripts/deploy/promote-to-production.js | 3 ++- scripts/deploy/promote-to-staging.js | 3 ++- scripts/deploy/rollback.js | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/scripts/deploy/promote-to-production.js b/scripts/deploy/promote-to-production.js index 3d4c84680ac..56470ef305a 100644 --- a/scripts/deploy/promote-to-production.js +++ b/scripts/deploy/promote-to-production.js @@ -65,7 +65,8 @@ async function runPromoteToProduction() { } const s3Client = new S3Client({ apiVersion: '2006-03-01', - region: 'us-east-1', + region: + projectDomain === PROTOCOL_DESIGNER_DOMAIN ? 'us-east-1' : 'us-east-2', credentials: productionCredentials, }) diff --git a/scripts/deploy/promote-to-staging.js b/scripts/deploy/promote-to-staging.js index e8b0d91ea0c..6fdc9f60e40 100644 --- a/scripts/deploy/promote-to-staging.js +++ b/scripts/deploy/promote-to-staging.js @@ -76,7 +76,8 @@ async function runPromoteToStaging() { const s3WithCreds = new S3Client({ apiVersion: '2006-03-01', - region: 'us-east-1', + region: + projectDomain === PROTOCOL_DESIGNER_DOMAIN ? 'us-east-1' : 'us-east-2', credentials: stagingCredentials, }) console.log(`Promoting ${projectDomain} from sandbox to staging\n`) diff --git a/scripts/deploy/rollback.js b/scripts/deploy/rollback.js index 7012fb838d3..cf5e5174199 100644 --- a/scripts/deploy/rollback.js +++ b/scripts/deploy/rollback.js @@ -97,7 +97,8 @@ async function runRollback() { const s3Client = new S3Client({ apiVersion: '2006-03-01', - region: 'us-east-1', + region: + projectDomain === PROTOCOL_DESIGNER_DOMAIN ? 'us-east-1' : 'us-east-2', credentials: rollBackCredentials, }) From d907591b7db6d400e91a4028352b31c5a121c2d8 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Thu, 24 Oct 2024 15:19:53 -0400 Subject: [PATCH 16/17] fix(app): remove some dangerous reverse()s (#16591) There weer a couple command texts that were doing a `.reverse()` of the commands array. This is O(N) which is gross and also might be cached which would cause bugs, so replace it with some uses of findLast() that definitely weren't inspired by wanting to do some fun typing exercises. ## testing - [x] tests pass, really --- .../utils/getFinalLabwareLocation.ts | 50 +++++++++++---- .../utils/getWellRange.ts | 61 +++++++++++++------ 2 files changed, 80 insertions(+), 31 deletions(-) diff --git a/app/src/local-resources/commands/hooks/useCommandTextString/utils/getFinalLabwareLocation.ts b/app/src/local-resources/commands/hooks/useCommandTextString/utils/getFinalLabwareLocation.ts index 80cd4e26a4e..e674794a265 100644 --- a/app/src/local-resources/commands/hooks/useCommandTextString/utils/getFinalLabwareLocation.ts +++ b/app/src/local-resources/commands/hooks/useCommandTextString/utils/getFinalLabwareLocation.ts @@ -1,4 +1,25 @@ -import type { LabwareLocation, RunTimeCommand } from '@opentrons/shared-data' +import type { + LabwareLocation, + RunTimeCommand, + LoadLabwareRunTimeCommand, + MoveLabwareRunTimeCommand, +} from '@opentrons/shared-data' + +const findLastAt = ( + arr: readonly T[], + pred: ((el: T) => boolean) | ((el: T) => el is U) +): [U, number] | [undefined, -1] => { + let arrayLoc = -1 + const lastEl = arr.findLast((el: T, idx: number): el is U => { + arrayLoc = idx + return pred(el) + }) + if (lastEl === undefined) { + return [undefined, -1] + } else { + return [lastEl, arrayLoc] + } +} /** * given a list of commands and a labwareId, calculate the resulting location @@ -11,15 +32,22 @@ export function getFinalLabwareLocation( labwareId: string, commands: RunTimeCommand[] ): LabwareLocation | null { - for (const c of commands.reverse()) { - if (c.commandType === 'loadLabware' && c.result?.labwareId === labwareId) { - return c.params.location - } else if ( - c.commandType === 'moveLabware' && - c.params.labwareId === labwareId - ) { - return c.params.newLocation - } + const [lastMove, lastMoveIndex] = findLastAt( + commands, + (c: RunTimeCommand): c is MoveLabwareRunTimeCommand => + c.commandType === 'moveLabware' && c.params.labwareId === labwareId + ) + + const [lastLoad, lastLoadIndex] = findLastAt( + commands, + (c: RunTimeCommand): c is LoadLabwareRunTimeCommand => + c.commandType === 'loadLabware' && c.result?.labwareId === labwareId + ) + if (lastMoveIndex > lastLoadIndex) { + return lastMove?.params?.newLocation ?? null + } else if (lastLoadIndex > lastMoveIndex) { + return lastLoad?.params?.location ?? null + } else { + return null } - return null } diff --git a/app/src/local-resources/commands/hooks/useCommandTextString/utils/getWellRange.ts b/app/src/local-resources/commands/hooks/useCommandTextString/utils/getWellRange.ts index a0700357413..791ddf2f4c3 100644 --- a/app/src/local-resources/commands/hooks/useCommandTextString/utils/getWellRange.ts +++ b/app/src/local-resources/commands/hooks/useCommandTextString/utils/getWellRange.ts @@ -1,5 +1,42 @@ import { getPipetteNameSpecs } from '@opentrons/shared-data' -import type { PipetteName, RunTimeCommand } from '@opentrons/shared-data' +import type { + PipetteName, + RunTimeCommand, + ConfigureNozzleLayoutRunTimeCommand, +} from '@opentrons/shared-data' + +const usedChannelsFromCommand = ( + command: ConfigureNozzleLayoutRunTimeCommand | undefined, + defaultChannels: number +): number => + command?.params?.configurationParams?.style === 'SINGLE' + ? 1 + : command?.params?.configurationParams?.style === 'COLUMN' + ? 8 + : defaultChannels + +const usedChannelsForPipette = ( + pipetteId: string, + commands: RunTimeCommand[], + defaultChannels: number +): number => + usedChannelsFromCommand( + commands.findLast( + (c: RunTimeCommand): c is ConfigureNozzleLayoutRunTimeCommand => + c.commandType === 'configureNozzleLayout' && + c.params?.pipetteId === pipetteId + ), + defaultChannels + ) + +const usedChannels = ( + pipetteId: string, + commands: RunTimeCommand[], + pipetteChannels: number +): number => + pipetteChannels === 96 + ? usedChannelsForPipette(pipetteId, commands, pipetteChannels) + : pipetteChannels /** * @param pipetteName name of pipette being used @@ -16,26 +53,10 @@ export function getWellRange( const pipetteChannels = pipetteName ? getPipetteNameSpecs(pipetteName)?.channels ?? 1 : 1 - let usedChannels = pipetteChannels - if (pipetteChannels === 96) { - for (const c of commands.reverse()) { - if ( - c.commandType === 'configureNozzleLayout' && - c.params?.pipetteId === pipetteId - ) { - // TODO(sb, 11/9/23): add support for quadrant and row configurations when needed - if (c.params.configurationParams.style === 'SINGLE') { - usedChannels = 1 - } else if (c.params.configurationParams.style === 'COLUMN') { - usedChannels = 8 - } - break - } - } - } - if (usedChannels === 96) { + const channelCount = usedChannels(pipetteId, commands, pipetteChannels) + if (channelCount === 96) { return 'A1 - H12' - } else if (usedChannels === 8) { + } else if (channelCount === 8) { const column = wellName.substr(1) return `A${column} - H${column}` } From df01e7722af56dbb6c1b2d1bf8a4f3edc82399ef Mon Sep 17 00:00:00 2001 From: Jeremy Leon Date: Thu, 24 Oct 2024 16:12:21 -0400 Subject: [PATCH 17/17] fix(api): prevent moving a labware onto itself (#16600) Fixes bug where you could move a labware onto itself, causing a recursion error later in the protocol --- .../protocol_engine/commands/move_labware.py | 4 +++ .../commands/test_move_labware.py | 35 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/api/src/opentrons/protocol_engine/commands/move_labware.py b/api/src/opentrons/protocol_engine/commands/move_labware.py index d5c188d219f..7f52a8e83e4 100644 --- a/api/src/opentrons/protocol_engine/commands/move_labware.py +++ b/api/src/opentrons/protocol_engine/commands/move_labware.py @@ -186,6 +186,10 @@ async def execute(self, params: MoveLabwareParams) -> _ExecuteReturn: # noqa: C top_labware_definition=current_labware_definition, bottom_labware_id=available_new_location.labwareId, ) + if params.labwareId == available_new_location.labwareId: + raise LabwareMovementNotAllowedError( + "Cannot move a labware onto itself." + ) # Allow propagation of ModuleNotLoadedError. new_offset_id = self._equipment.find_applicable_labware_offset_id( diff --git a/api/tests/opentrons/protocol_engine/commands/test_move_labware.py b/api/tests/opentrons/protocol_engine/commands/test_move_labware.py index d1309761d8f..aa02d85349a 100644 --- a/api/tests/opentrons/protocol_engine/commands/test_move_labware.py +++ b/api/tests/opentrons/protocol_engine/commands/test_move_labware.py @@ -803,3 +803,38 @@ async def test_move_labware_raises_when_moving_fixed_trash_labware( match="Cannot move fixed trash labware 'My cool labware'.", ): await subject.execute(data) + + +async def test_labware_raises_when_moved_onto_itself( + decoy: Decoy, + subject: MoveLabwareImplementation, + state_view: StateView, +) -> None: + """It should raise when the OnLabwareLocation has the same labware ID as the labware being moved.""" + data = MoveLabwareParams( + labwareId="the-same-labware-id", + newLocation=OnLabwareLocation(labwareId="a-cool-labware-id"), + strategy=LabwareMovementStrategy.MANUAL_MOVE_WITH_PAUSE, + ) + + decoy.when(state_view.labware.get(labware_id="the-same-labware-id")).then_return( + LoadedLabware( + id="the-same-labware-id", + loadName="load-name", + definitionUri="opentrons-test/load-name/1", + location=DeckSlotLocation(slotName=DeckSlotName.SLOT_4), + offsetId=None, + ) + ) + + decoy.when( + state_view.geometry.ensure_location_not_occupied( + location=OnLabwareLocation(labwareId="a-cool-labware-id"), + ) + ).then_return(OnLabwareLocation(labwareId="the-same-labware-id")) + + with pytest.raises( + errors.LabwareMovementNotAllowedError, + match="Cannot move a labware onto itself.", + ): + await subject.execute(data)