feat(app): usb connection and moam modal functionality in module setup #8257

jerader · 2021-08-25T20:54:45Z

Overview

Closes ticket #8233

Changelog

add usb connection and connection status to module setup
add functionality to Proceed to Labware Setup CTA
add correct functionality to Multiple of a module (MoaM) modal to only show up if MoaM are in the uploaded protocol
address Emily's comment mentioned on PR feat(app): add module setup step to protocol setup #8224

Review requests

Addresses everything in ticket Module Setup Accordion Step: link usb connection connection status #8233 including Emily’s comment and the remaining acceptance criteria in Accordion step 2: Sometimes displayed module setup #7664
For usbPort info, if user has a robot where the server version is below 4.3, rather than displaying what port or hub number the module is connected by, it will say USB Port Connected. Note: the Opentrons-dev bot does not have usbPort info so you should see USB Port Connected.

Risk assessment

low, below FF

codecov · 2021-08-25T20:54:54Z

Codecov Report

Merging #8257 (e8334fb) into edge (9d4274c) will increase coverage by 0.02%.
The diff coverage is 83.01%.

@@            Coverage Diff             @@
##             edge    #8257      +/-   ##
==========================================
+ Coverage   74.84%   74.86%   +0.02%     
==========================================
  Files        1680     1680              
  Lines       44642    44687      +45     
  Branches     4494     4515      +21     
==========================================
+ Hits        33414    33457      +43     
+ Misses      10465    10459       -6     
- Partials      763      771       +8

Flag	Coverage Δ
app	`71.11% <83.01%> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...s/ProtocolSetup/RunSetupCard/ModuleSetup/index.tsx	`76.19% <77.14%> (-0.74%)`	⬇️
...src/organisms/ProtocolSetup/RunSetupCard/index.tsx	`90.90% <85.71%> (-4.33%)`	⬇️
...tocolSetup/RunSetupCard/ModuleSetup/ModuleInfo.tsx	`100.00% <100.00%> (+83.33%)`	⬆️
...src/pages/Calibrate/CalibratePanel/PipetteList.tsx	`83.87% <0.00%> (-0.75%)`	⬇️
.../RunSetupCard/ModuleSetup/MultipleModulesModal.tsx	`100.00% <0.00%> (+50.00%)`	⬆️

shlokamin

coming along!

i think your ModuleInfo tests need a bit of rework. i'd start by thinking about what exactly ModuleInfo's job is, and then craft your test cases from there. i.e. it('should display a warning icon when modules are not attached')

shlokamin · 2021-08-26T18:43:58Z

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

+ "module_setup_step_description": "Plug in and power up the required module via the OT-2 USB Port. Place the module as shown in the deck map.",
+ "modules_setup_step_description": "Plug in and power up the required modules via the OT-2 USB Ports. Place the modules as shown in the deck map.",


do we need both of these?

if the extra is needed to pluralize, i18n can handle that for us i think: https://www.i18next.com/translation-function/plurals

This is something that Emily wanted to implement. She mentioned it in a comment on my last PR #8224

shlokamin · 2021-08-26T18:44:54Z

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

+ "no_usb_port_yet": "No USB Port Yet",
+ "usb_port_connected": "USB Port",
+ "hub_connected": "via hub",
+ "usb_port_connected_old": "USB Port Connected"


usb_port_connected_old makes this seem like we don't need it anymore. what is its purpose?

If your robot server version is below 4.3, the usb and hub ports don't have a string (I double checked with my robot when it was version 4.0). So usb_port_connected_old is to support that.

Maybe making this title more descriptive, something like usb_port_connected_old_server_version could make this more clear

shlokamin · 2021-08-26T18:45:15Z

app/src/organisms/ProtocolSetup/RunSetupCard/LabwareSetup/index.tsx

+ <g>
+ <LabwareInfoOverlay
+ x={x}
+ y={y}
+ definition={labwareDef}
+ />
+ </g>


is there a reason you added a g tag?

whoops. idk how this got added back. removing it now.

shlokamin · 2021-08-26T18:45:34Z

app/src/organisms/ProtocolSetup/RunSetupCard/LabwareSetup/index.tsx

- backgroundColor={C_BLUE}
 {...linkProps}
 {...targetProps}
+ backgroundColor={C_BLUE}


how come you switched this ordering?

some weirdness probably occurred when i rebased this to my last PR. i'll change it back 😬

shlokamin · 2021-08-26T18:47:16Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/ModuleInfo.tsx

-export const ModuleInfo = (props: ModuleInfoProps): JSX.Element => {
- const { x, y, orientation, moduleModel } = props
+export function ModuleInfo(props: ModuleInfoProps): JSX.Element {
+ const { x, y, orientation, moduleModel, usbPort, hubPort, isAttached } = props


how come you changed this from a function expression to a function declaration? we normally stick to function expressions when we can

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/function
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function

shlokamin · 2021-08-26T18:54:57Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/__tests__/ModuleInfo.test.tsx

+import { ModuleInfo } from '../ModuleInfo'
+import { when } from 'jest-when'
+
+jest.mock('../ModuleInfo')


this test should be testing ModuleInfo right? we don't actually want to mock it, we only want to mock the dependencies of ModuleInfo that we need to

shlokamin · 2021-08-26T18:57:24Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/__tests__/ModuleInfo.test.tsx

+ }
+ })
+
+ it('should render no modules connected', () => {


this test isn't actually making any assertion, so it will always pass. what should module info be actually be doing?

the test name says it should not render any modules, but i dont think the ModuleInfo component has any concept of "modules", it just takes in info about a particular module and then renders some text and an icon depending on that info it loooks like

shlokamin · 2021-08-26T18:57:56Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/__tests__/ModuleInfo.test.tsx

+ }
+ })
+
+ it('should render modules connected on a robot server 4.3 or higher', () => {


same deal here, this test isn't making any assertions

shlokamin · 2021-08-26T19:00:38Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/index.tsx

+ <ModuleViz
+ x={x}
+ y={y}
+ orientation={orientation}
+ moduleType={getModuleType(moduleModel)}
+ />
+ <ModuleInfo
+ x={x}
+ y={y}
+ moduleModel={moduleModel}
+ orientation={orientation}
+ isAttached={attached}
+ usbPort={null}
+ hubPort={null}
+ />
+ </React.Fragment>
+ ) : (
+ <React.Fragment
+ key={`LabwareSetup_Module_${moduleModel}_${x}${y}`}
+ >
+ <ModuleViz
+ x={x}
+ y={y}
+ orientation={orientation}
+ moduleType={getModuleType(moduleModel)}
+ />
+ <ModuleInfo
+ x={x}
+ y={y}
+ moduleModel={moduleModel}
+ orientation={orientation}
+ isAttached={attached}
+ usbPort={null}
+ hubPort={null}
+ />
+ </React.Fragment>


same comment earlier about conditionally rendering Icon twice, you can render these components once and change what props you pass in depending on whether modulesByPort is empty or not

shlokamin · 2021-08-26T19:01:14Z

app/src/organisms/ProtocolSetup/RunSetupCard/index.tsx

+ const ROBOT_CALIBRATION_STEP_KEY = 'robot_calibration_step' as const
+ const LABWARE_SETUP_KEY = 'labware_setup_step' as const
+
+ let MODULE_SETUP_KEY = 'module_setup_step'


how come this isn't a const too?

8233

emilywools · 2021-08-30T17:30:01Z

hey Jethary! looking good! Here are some things I noticed:

can you add a space between the end of the sentence and the link in the MoaM modal:?

2. the green successfully connected icon should have a check mark in it instead of a ! 3. The two temp modules are overlapping each other a little bit. Will this be resolved when we have the SVGs?

jerader · 2021-08-30T18:39:25Z

@emilywools - this will be fixed when we have the SVGs 😄

The two temp modules are overlapping each other a little bit. Will this be resolved when we have the SVGs?

emilywools

sounds good! looks good to me

smb2268

Looking really good overall, Jethary! I l just left a few comments that can help DRY things up.

smb2268 · 2021-09-01T16:54:10Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/index.tsx

- expandLabwareSetupStep: () => void
-}
+import type { State, Dispatch } from '../../../../redux/types'
+import type { AttachedModule } from '../../../../redux/modules/types'


Pasting a comment Shlok left on my PR since it's relevant here and he explained it well! #8182 (comment):

nit: we unfortunately have a pattern of imports we follow that is not enforced by a linter
basically we usually do

Builtin Node.js modules (import fs from 'fs')

External npm modules (import * as React from 'react')

Opentrons modules (import { THERMOCYCLER } from '@opentrons/shared-data')

Parent relative modules (import { SomeParentComponent } from '../../parent')

Sibling relative modules (import { SomeSiblingComponent } from './sibling')

type imports (import type {Whatever} from './types')

smb2268 · 2021-09-01T17:02:43Z

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

+ "no_usb_port_yet": "No USB Port Yet",
+ "usb_port_connected": "USB Port",
+ "hub_connected": "via hub",
+ "usb_port_connected_old": "USB Port Connected"


Maybe making this title more descriptive, something like usb_port_connected_old_server_version could make this more clear

smb2268 · 2021-09-01T17:06:23Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/index.tsx

+ hubPort={null}
+ />
+ </React.Fragment>
+ ) : (


There aren't any differences between what you're returning here if attached is true vs false. If the variation between them is handled with different prop inputs than you can remove this ternary!

omg that ternary is for no reason, thank you for noticing this!

smb2268 · 2021-09-01T17:10:56Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/index.tsx

+ moduleModel={moduleModel}
+ orientation={orientation}
+ isAttached={attached}
+ usbPort={port}


For this ternary, it looks like the only difference between having attached be true or false is what you're passing for usbPort. To make this DRY you can remove the larger ternary and do this instead:

<React.Fragment key={`LabwareSetup_Module_${moduleModel}_${x}${y}`} > <ModuleViz x={x} y={y} orientation={orientation} moduleType={getModuleType(moduleModel)} /> <ModuleInfo x={x} y={y} moduleModel={moduleModel} orientation={orientation} isAttached={attached} usbPort={attached === true ? port : null} hubPort={attached === true ? port : null} /> </React.Fragment>

smb2268 · 2021-09-01T17:13:26Z

app/src/organisms/ProtocolSetup/RunSetupCard/ModuleSetup/index.tsx

+ orientation={orientation}
+ isAttached={attached}
+ usbPort={port}
+ hubPort={port}


I'm not familiar with usbPort vs hubPort - we're passing in the string port for both, are they always the same thing?

On the robot, there are 2 or 4 usb ports (depending on if you have the OT-2 or the OT-2-R) If your robot has 2 usb ports which is the more common robot of the 2 and if you need to connect all 3 modules, you'll need to connect a hub that has more usb ports attached to it. port is the same in my code because your module is either connected via usb or hub and const port = module.usbPort.hub || module.usbPort.port. Hope this clears things up!

Gotcha yes that makes sense - thanks for explaining!!

smb2268

lgtm! 🚀

Shlok is OOO for a few weeks and I got other dev reviews

b-cooper

🚢

jerader added the app Affects the `app` project label Aug 25, 2021

jerader marked this pull request as ready for review August 26, 2021 13:51

jerader requested a review from a team as a code owner August 26, 2021 13:51

jerader requested review from b-cooper, shlokamin, smb2268 and emilywools and removed request for a team August 26, 2021 13:51

shlokamin previously requested changes Aug 26, 2021

View reviewed changes

jerader added 20 commits August 30, 2021 09:16

create module setup component and test

c6b2b14

create multiple modules modal and edit styles

eaf9892

create module info and edited MoaM modal

fd5e406

address pr comments

1e2f00d

fix test and deck view box

dc80ed7

fix styling and svg tag

090ce8d

fix margin error

6441bdb

add poll the robot modules ever 5 seconds

296ba27

add some usb info

25c156a

fix connected and not connected

472558b

add button styling

b9b783d

begin to add usb detection

88e4e9b

add frontend port and module connection

aa0a4ff

modify css file

8875e36

fix module variable

522e112

remove styles file and edit usb and hub

c5e984a

add port and mode features

866799b

style fix on labware setup file

1d9bfcb

fix hub and port connection

fb91ff7

add isAttached and fix usb and hub

5daa14c

jerader added 14 commits August 30, 2021 09:16

fix for both port and no port info compatibility

b8496f5

add labware setup step btn function

9e35078

add moam modal functionality

2942f2f

reorg

377d8b3

modify tests and fix labware setup button fxn

e04e11d

modify tests

eec117d

feat(app): usb connection and moam modal functionality in module setup

d0b08a8

8233

clean up code

d21e5cc

fix prettier formatting

111e4af

add test and fix robotname functionality

9263ca3

remove unnecessary imports

a26d83c

address comments and rework moduleInfo test

a02cd01

add expect corresponding matcher call to moduleInfo test

133bde6

fix import

2c3581f

jerader force-pushed the app_module-usb-connection-step branch from 7fc52de to 2c3581f Compare August 30, 2021 13:38

emilywools approved these changes Aug 30, 2021

View reviewed changes

jerader added 2 commits August 30, 2021 16:49

address Emilys comment

a52334c

correct prettier formatting

dc64a67

jerader requested a review from shlokamin August 31, 2021 13:02

smb2268 requested changes Sep 1, 2021

View reviewed changes

address comments

52696fe

smb2268 approved these changes Sep 1, 2021

View reviewed changes

jerader added 2 commits September 3, 2021 10:59

utilize i18n plural capabilities

7109e2f

fix prettier formatting

e8334fb

b-cooper approved these changes Sep 3, 2021

View reviewed changes

jerader merged commit da516da into edge Sep 3, 2021

jerader deleted the app_module-usb-connection-step branch September 3, 2021 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app): usb connection and moam modal functionality in module setup #8257

feat(app): usb connection and moam modal functionality in module setup #8257

jerader commented Aug 25, 2021

codecov bot commented Aug 25, 2021 •

edited

Loading

shlokamin left a comment

shlokamin Aug 26, 2021

shlokamin Aug 26, 2021

jerader Aug 26, 2021

shlokamin Aug 26, 2021

jerader Aug 26, 2021

smb2268 Sep 1, 2021

shlokamin Aug 26, 2021

jerader Aug 26, 2021

shlokamin Aug 26, 2021

jerader Aug 26, 2021

shlokamin Aug 26, 2021

shlokamin Aug 26, 2021

shlokamin Aug 26, 2021

shlokamin Aug 26, 2021

shlokamin Aug 26, 2021

shlokamin Aug 26, 2021

emilywools commented Aug 30, 2021

jerader commented Aug 30, 2021

emilywools left a comment

smb2268 left a comment

smb2268 Sep 1, 2021

smb2268 Sep 1, 2021

smb2268 Sep 1, 2021

jerader Sep 1, 2021

smb2268 Sep 1, 2021

smb2268 Sep 1, 2021 •

edited

Loading

jerader Sep 1, 2021

smb2268 Sep 1, 2021

smb2268 left a comment

b-cooper left a comment

		"module_setup_step_description": "Plug in and power up the required module via the OT-2 USB Port. Place the module as shown in the deck map.",
		"modules_setup_step_description": "Plug in and power up the required modules via the OT-2 USB Ports. Place the modules as shown in the deck map.",

feat(app): usb connection and moam modal functionality in module setup #8257

feat(app): usb connection and moam modal functionality in module setup #8257

Conversation

jerader commented Aug 25, 2021

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Aug 25, 2021 • edited Loading

Codecov Report

shlokamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilywools commented Aug 30, 2021

jerader commented Aug 30, 2021

emilywools left a comment

Choose a reason for hiding this comment

smb2268 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smb2268 Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smb2268 left a comment

Choose a reason for hiding this comment

b-cooper left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 25, 2021 •

edited

Loading

smb2268 Sep 1, 2021 •

edited

Loading