Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(app): gather all protocol and calibration data #8182

Merged
merged 26 commits into from
Aug 23, 2021

Conversation

smb2268
Copy link
Contributor

@smb2268 smb2268 commented Jul 29, 2021

fix #8097

Overview

This PR includes all of the selectors needed to gather data on required labware from a json protocol and check that against the labware that is currently on the robot.

Changelog

  1. Create selector to synthesize needed pipette and tiprack info from protocol
  2. Create selector to check if necessary labware is on the robot
  3. Create selector to synthesize matching labware and calibration info
  4. Create selector to determine whether robot calibration step is complete
  5. Display this data in plain text in the robot calibration step in the app
  6. TODO: Fix TS errors and add testing

Review requests

Am I missing any possible reasons that the calibration step is not complete? Is this all of the data we need to consider to display all scenarios?

Risk assessment

Mid - behind feature flag

@smb2268 smb2268 added the app Affects the `app` project label Jul 29, 2021
@smb2268 smb2268 self-assigned this Jul 29, 2021
Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is looking good. These are some gnarly bits of the client that you're untangling and you've tackled it well.

I made a few house keeping suggestions. I'll do another pass over the code, and test it out, once you've got some tests written too.

app/src/organisms/ProtocolSetup/DeckCalibration.tsx Outdated Show resolved Hide resolved
app/src/organisms/ProtocolSetup/DeckCalibration.tsx Outdated Show resolved Hide resolved
app/src/organisms/ProtocolSetup/DeckCalibration.tsx Outdated Show resolved Hide resolved
app/src/organisms/ProtocolSetup/DeckCalibration.tsx Outdated Show resolved Hide resolved
app/src/organisms/ProtocolSetup/DeckCalibration.tsx Outdated Show resolved Hide resolved
app/src/organisms/ProtocolSetup/RobotCalibrationStep.tsx Outdated Show resolved Hide resolved
app/src/organisms/ProtocolSetup/RobotCalibrationStep.tsx Outdated Show resolved Hide resolved
app/src/organisms/ProtocolSetup/RunSetupCard.tsx Outdated Show resolved Hide resolved
if (deckCalStatus !== 'OK') {
return {
complete: false,
reason: 'calibrate deck',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to happen in this PR, but it would be good to capture these reasons in a typed const, that way we can also use them later on as a sort of enum key to the actual tool tip message.

app/src/redux/protocol/selectors.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #8182 (509dad3) into edge (2b9e77c) will decrease coverage by 0.01%.
The diff coverage is 68.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #8182      +/-   ##
==========================================
- Coverage   74.77%   74.76%   -0.02%     
==========================================
  Files        1668     1670       +2     
  Lines       44373    44482     +109     
  Branches     4440     4469      +29     
==========================================
+ Hits        33181    33256      +75     
- Misses      10438    10471      +33     
- Partials      754      755       +1     
Flag Coverage Δ
app 70.62% <68.80%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
...sms/ProtocolSetup/RunSetupCard/CollapsibleStep.tsx 100.00% <ø> (ø)
...unSetupCard/LabwareSetup/ExtraAttentionWarning.tsx 92.30% <ø> (ø)
...p/RunSetupCard/LabwareSetup/LabwareInfoOverlay.tsx 100.00% <ø> (ø)
...up/RunSetupCard/LabwareSetup/LabwareSetupModal.tsx 100.00% <ø> (ø)
...p/RunSetupCard/LabwareSetup/SecureLabwareModal.tsx 16.66% <ø> (ø)
.../ProtocolSetup/RunSetupCard/LabwareSetup/index.tsx 83.33% <ø> (ø)
...p/RunSetupCard/LabwareSetup/utils/getModuleName.ts 100.00% <ø> (ø)
...p/utils/getModuleTypesThatRequireExtraAttention.ts 100.00% <ø> (ø)
...tocolSetup/RunSetupCard/ModuleSetup/ModuleInfo.tsx 16.66% <ø> (ø)
.../RunSetupCard/ModuleSetup/MultipleModulesModal.tsx 50.00% <ø> (ø)
... and 9 more

@smb2268 smb2268 force-pushed the app_robot-calibration-setup-8097 branch 5 times, most recently from bb0f71b to 76ad9d3 Compare August 12, 2021 00:36
@smb2268 smb2268 force-pushed the app_robot-calibration-setup-8097 branch from f3fb6fa to a7da085 Compare August 17, 2021 03:57
@smb2268 smb2268 marked this pull request as ready for review August 17, 2021 15:38
@smb2268 smb2268 requested a review from a team as a code owner August 17, 2021 15:39
Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to merge! 🧇

Copy link
Member

@shlokamin shlokamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work Sarah!

Left a few comments. I don't want to hold up this PR, but I think we should come back and add some missing test coverage because some of these selectors you wrote are pretty important and we'll rely on them heavily.

Also, I see you moved RobotCalibrationStep into RunSetupCard. When I did labware setup I kept LabwareSetup as it's own directory as a sibling to RunSetupCard, and I see @jerader did the same for ModuleSetup.

It doesn't really matter to me which structure we choose, but we should be consistent in where we place each of these steps (since they're siblings), and be consistent in the naming (postfix with Step or not).

@smb2268
Copy link
Contributor Author

smb2268 commented Aug 19, 2021

Also, I see you moved RobotCalibrationStep into RunSetupCard. When I did labware setup I kept LabwareSetup as it's own directory as a sibling to RunSetupCard, and I see @jerader did the same for ModuleSetup.

It doesn't really matter to me which structure we choose, but we should be consistent in where we place each of these steps (since they're siblings), and be consistent in the naming (postfix with Step or not).

@shlokamin I don't have a strong opinion on how these should be organized. I'll drop the postfix Step since I'm last to merge. For file organization I was going off of @b-cooper's suggestion in this comment, which I do think makes sense since all three of these are technically children of RunSetupCard, but since that's more of a structural component without a lot of logic nI don't mind if the children are outside of the folder. #8182 (comment)

@smb2268 smb2268 force-pushed the app_robot-calibration-setup-8097 branch from 7f42c07 to ac12926 Compare August 20, 2021 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Robot Cal Accordion Step: Display Cal Values
3 participants