feat(app): Show connect modules modal when session modules detected #1897

Kadee80 · 2018-07-19T19:08:24Z

overview

This PR closes #1738 and checks of a few boxes in #1737

changelog

feat(app): Show connect modules modal when session modules detected
refactor(app): refactor app specific modal

review requests

Modal prompt will only show if modules flag is true: make dev OT_APP_MODULES=1

Modal Prompt will only show if state.robot.session.modulesBySlot contains SessionModule object(s)

To simulate modules required by the protocol:
- Uncomment lines 344-351 in app/src/robot/api-client/client.js
To simulate modules actually attached:
- Comment out line 113 in app/src/pages/Calibrate/Labware.js
- Replace with const actualModules = [{name: 'magdeck'}] or {name: 'tempdeck'} or both

Expected behavior:

If modules are enabled and protocol requires module(s), module review modal shows
If actual modules do not match protocol modules, show a warning
- Clicking the "Check for Modules" button sends a GET /modules
If actual modules are a superset of protocol modules, show a success
- Clicking "Continue to Labware" takes you to the labware review modal

Todo in upcoming PR: show the deckmap in the module review modal

closes #1737

codecov · 2018-07-19T19:21:05Z

Codecov Report

Merging #1897 into edge will decrease coverage by 0.1%.
The diff coverage is 15.55%.

@@            Coverage Diff             @@
##             edge    #1897      +/-   ##
==========================================
- Coverage   33.22%   33.12%   -0.11%     
==========================================
  Files         401      403       +2     
  Lines        6562     6588      +26     
==========================================
+ Hits         2180     2182       +2     
- Misses       4382     4406      +24

Impacted Files	Coverage Δ
app/src/http-api-client/modules.js	`100% <ø> (ø)`	⬆️
app/src/components/modals/Modal.js	`0% <ø> (ø)`
...pp/src/components/CalibrateLabware/ConfirmModal.js	`0% <ø> (ø)`	⬆️
app/src/robot/reducer/session.js	`100% <ø> (ø)`	⬆️
app/src/components/ConnectModulesModal/Prompt.js	`0% <0%> (ø)`
app/src/components/ReviewDeckModal/index.js	`0% <0%> (ø)`	⬆️
app/src/pages/Calibrate/Pipettes.js	`0% <0%> (ø)`	⬆️
app/src/pages/Calibrate/Labware.js	`0% <0%> (ø)`	⬆️
app/src/components/modals/index.js	`0% <0%> (ø)`	⬆️
...mponents/InstrumentSettings/AttachedModulesCard.js	`0% <0%> (ø)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cf1b4d...2b7eb3e. Read the comment docs.

mcous

Going to push fixups directly to this branch

mcous · 2018-07-19T19:40:53Z

app/src/robot/types.js

 export type StateModule = {
  // resource ID
  _id: number,
  // slot module is installed in
  slot: Slot,
  // name identifier of the module
-  type: 'magdeck' | 'tempdeck',
+  name: ModuleType


mcous · 2018-07-19T19:43:01Z

app/src/pages/Calibrate/Labware.js

+}
+
+// TODO (ka 2018-7-19): type this more specifically
+function getNames (array: Array<any> | []): Array<string> {


Would this work?

function getNames (array: ?Array<{name: string}>): Array<string> {

mcous · 2018-07-19T19:43:32Z

app/src/pages/Calibrate/Labware.js

+// TODO (ka 2018-7-19): type this more specifically
+function getNames (array: Array<any> | []): Array<string> {
+  const names = array
+    ? array.map((m) => { return m.name }).sort()


Prefer m => m.name to m => { return m.name } and also is the sort necessary?

sort isnt neccesary, its left over from another half baked idea

mcous · 2018-07-19T19:44:15Z

app/src/pages/Calibrate/Labware.js

+  return names
+}
+
+function compareNames (a: Array<StateModule>, b: Array<?Module> | null) {


Should the type for b be ?Array<Module>?

mcous · 2018-07-19T19:44:45Z

app/src/robot/types.js

@@ -132,13 +132,15 @@ export type Labware = StateLabware & {

 export type LabwareType = 'tiprack' | 'labware'

+export type ModuleType = 'magdeck' | 'tempdeck'
+
 export type StateModule = {


In hindsight, StateModule is a bad name. We should probably rename it to SessionModule

renaming now

mcous · 2018-07-19T19:48:36Z

app/src/components/InstrumentSettings/ModulesCardContents.js

@@ -4,11 +4,11 @@ import type {Module} from '../../http-api-client'
 import ModuleItem, {NoModulesMessage} from '../ModuleItem'

 type Props = {
-  modules: Array<Module>,
+  modules: Array<?Module>,


This should be ?Array<Module>. If the array exists, it is definitely comprised of Modules, but the array may not exist.

(The type as written says the array definitely exists and is comprised of either Modules or null/undefineds)

mcous · 2018-07-19T19:50:31Z

app/src/http-api-client/modules.js

@@ -21,7 +21,7 @@ export type Module = {
 }

 type FetchModulesResponse = {
-  modules: Array<Module>,
+  modules: Array<?Module>,


This change is incorrect and should be reverted. The API will never respond with [..., null, ...]

mcous · 2018-07-19T19:50:54Z

app/src/pages/Calibrate/Labware.js

  modules: Array<StateModule>,
-  actualModules: ?Array<Module>,
+  actualModules: Array<?Module>,


?Array<Module>

mcous

🐇

mcous · 2018-07-20T14:02:12Z

Note that when we merge this: it does not close #1737 and the commit message should be amended accordingly. It does appear to close #1738

IanLondon

works! couldn't break it

IanLondon · 2018-07-20T17:09:20Z

app/src/components/InstrumentSettings/AttachedModulesCard.js

-    displayName: 'Magnetic Bead Module'
-  }
-]
+// TODO(mc, 2018-07-19): remove this testing code when API returns modules


If this is gonna be toggled in development for much longer, maybe it should be under a feature flag instead of commented out?

Thats a good idea! We can do that in an upcoming PR. This ones already too big

feat(app): Show connect modules modal when session modules detected

8070f6c

closes #1737

Kadee80 added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project labels Jul 19, 2018

Kadee80 requested a review from mcous July 19, 2018 19:08

condition button text

878e6e6

mcous suggested changes Jul 19, 2018

View reviewed changes

mcous added 2 commits July 19, 2018 17:40

fixup: Correct typedefs

5835225

fixup: Fix lint

382cf76

mcous approved these changes Jul 20, 2018

View reviewed changes

mcous added the ready for review label Jul 20, 2018

mcous requested review from b-cooper and IanLondon July 20, 2018 14:00

fixup: remove duplicate modal file

2b7eb3e

mcous mentioned this pull request Jul 20, 2018

feat(app): Show modules on review and calibration deckmaps #1898

Merged

2 tasks

IanLondon approved these changes Jul 20, 2018

View reviewed changes

Kadee80 merged commit 8306130 into edge Jul 20, 2018

IanLondon mentioned this pull request Aug 2, 2018

chore(release): 3.3.0-beta.1 #2001

Merged

mcous deleted the app_review-modules-logic branch August 6, 2018 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app): Show connect modules modal when session modules detected #1897

feat(app): Show connect modules modal when session modules detected #1897

Kadee80 commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 19, 2018 •

edited

Loading

mcous left a comment

mcous Jul 19, 2018

mcous Jul 19, 2018

mcous Jul 19, 2018

Kadee80 Jul 19, 2018

mcous Jul 19, 2018

mcous Jul 19, 2018

Kadee80 Jul 19, 2018

mcous Jul 19, 2018

mcous Jul 19, 2018

mcous Jul 19, 2018

mcous left a comment

mcous commented Jul 20, 2018 •

edited

Loading

IanLondon left a comment

IanLondon Jul 20, 2018

Kadee80 Jul 20, 2018

feat(app): Show connect modules modal when session modules detected #1897

feat(app): Show connect modules modal when session modules detected #1897

Conversation

Kadee80 commented Jul 19, 2018 • edited Loading

overview

changelog

review requests

codecov bot commented Jul 19, 2018 • edited Loading

Codecov Report

mcous 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

mcous left a comment

Choose a reason for hiding this comment

mcous commented Jul 20, 2018 • edited Loading

IanLondon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kadee80 commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 19, 2018 •

edited

Loading

mcous commented Jul 20, 2018 •

edited

Loading