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

fix(app): module calibration should only use calibrated pipette #13956

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

smb2268
Copy link
Contributor

@smb2268 smb2268 commented Nov 8, 2023

Overview

We should not allow users to calibrate modules unless an attached pipette is calibrated. We should ensure a pipette has calibration data before selecting that as the pipette to use for module calibration.

Test Plan

  1. From both run setup and the module card, ensure the calibrate module CTA is disabled if no attached pipette is calibrated
  2. Check that if two pipettes are attached but only the right pipette is calibrated, the right pipette is be used for module calibration.

Changelog

  1. Check for pipette calibration data before selecting a pipette to use for module calibration
  2. Disable module card overflow menu calibrate button if no pipette has calibration data

Review requests

Run through the test plan

Risk assessment

Low

@smb2268 smb2268 self-assigned this Nov 8, 2023
@smb2268 smb2268 requested a review from a team as a code owner November 8, 2023 22:56
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #13956 (41db259) into edge (bcb4d9b) will decrease coverage by 0.01%.
The diff coverage is 20.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13956      +/-   ##
==========================================
- Coverage   70.71%   70.71%   -0.01%     
==========================================
  Files        2504     2504              
  Lines       70674    70676       +2     
  Branches     8666     8667       +1     
==========================================
  Hits        49976    49976              
- Misses      18590    18592       +2     
  Partials     2108     2108              
Flag Coverage Δ
app 68.06% <20.00%> (-0.01%) ⬇️

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

Files Coverage Δ
...pp/src/organisms/Devices/InstrumentsAndModules.tsx 64.15% <100.00%> (ø)
app/src/organisms/ModuleWizardFlows/index.tsx 2.15% <0.00%> (-0.05%) ⬇️

Comment on lines +200 to +204
if (
currentStep == null ||
attachedPipette?.data.calibratedOffset?.last_modified == null
)
return null
Copy link
Contributor

@koji koji Nov 8, 2023

Choose a reason for hiding this comment

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

I think the changes make sense, but these lines could be higher like right after currentStep(L76) 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't really move it much higher because you can't have an early return statement before all of the hooks are called and there are quite a few in the first two hundred lines

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, that totally makes sense. thanks!

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

LGTM

@smb2268 smb2268 merged commit d84b5bc into edge Nov 9, 2023
23 of 24 checks passed
@smb2268 smb2268 deleted the app_fix-module-cal-pipette branch November 9, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants