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(protocol-designer,-shared-data): introduce push out field in PD #17835

Merged
merged 3 commits into from
Mar 20, 2025

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Mar 20, 2025

Overview

This PR introduces UI and form data for push out as a checkbox expand form field in moveLiquid -> dispense. Previously, push out volumes were not configurable by the user, and were added implicitly. In order to migrate older protocols, we need to grab their default pushouts from their pipette specs. The utility getDefaultPushOutVolume takes into account transfer volume in order to access the correct liquids properties (default or low volume).

Closes AUTH-911

Test Plan and Hands on Testing

new

  • create new transfer step
  • navigate to dispense options and verify that push out field is present (we will pre-populate with the correct liquid class push out volume in a followup PR)
  • verify error handling for saving without entering a pushout/saving with pushout out of specified range
  • verify correct pushout tooltip text

migrated

  • import a protocol with at least one transfer step
  • open the step, and navigate to dispense options
  • verify that pushout field is correctly populated (inspect shared-data/pipette/definitions/2/liquid -> its definition to find its defaultPushOutVolume

Changelog

  • add pushout checkbox expand formfield
  • wire up maskers and errors
  • create utilities for getting pipette default push out volumes and max push out volumes (per-pipette, per-tip)
  • add migration to pull default push out volumes from pipette definitions (previously implicit and non-configurable)

Review requests

see test plan. Please reach out if you have any questions on the logic of getting default or max push out volumes

Risk assessment

low

This PR introduces UI and form data for push out as a checkbox expand form field in moveLiquid -> dispense. Previously, push out volumes were not configurable by the user, and were added implicitly. In order to migrate older protocols, we need to grab their default pushouts from their pipette specs. The utility `getDefaultPushOutVolume` takes into account transfer volume in order to access the correct liquids properties (default or low volume).

Closes AUTH-911
Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 37.82051% with 97 lines in your changes missing coverage. Please review.

Project coverage is 63.57%. Comparing base (7e44ff5) to head (45ee15c).
Report is 3 commits behind head on edge.

Files with missing lines Patch % Lines
...ols/MoveLiquidTools/SecondStepsMoveLiquidTools.tsx 0.00% 35 Missing ⚠️
protocol-designer/src/load-file/migration/8_5_0.ts 0.00% 22 Missing ⚠️
protocol-designer/src/utils/index.ts 41.66% 21 Missing ⚠️
protocol-designer/src/steplist/formLevel/errors.ts 45.71% 19 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #17835       +/-   ##
===========================================
+ Coverage   23.41%   63.57%   +40.16%     
===========================================
  Files        2891     2891               
  Lines      222559   222952      +393     
  Branches    19013    19248      +235     
===========================================
+ Hits        52112   141744    +89632     
+ Misses     170436    81018    -89418     
- Partials       11      190      +179     
Flag Coverage Δ
protocol-designer 18.89% <37.82%> (+0.03%) ⬆️
step-generation 4.38% <0.00%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
...src/step-forms/test/createPresavedStepForm.test.ts 100.00% <100.00%> (ø)
protocol-designer/src/steplist/fieldLevel/index.ts 74.24% <100.00%> (+0.24%) ⬆️
...r/src/steplist/formLevel/getDefaultsForStepType.ts 93.92% <100.00%> (+0.05%) ⬆️
protocol-designer/src/steplist/formLevel/index.ts 87.90% <100.00%> (+0.19%) ⬆️
...list/formLevel/test/getDefaultsForStepType.test.ts 100.00% <100.00%> (ø)
...tocol-designer/src/ui/steps/test/selectors.test.ts 100.00% <100.00%> (ø)
protocol-designer/src/steplist/formLevel/errors.ts 47.11% <45.71%> (-0.05%) ⬇️
protocol-designer/src/utils/index.ts 42.04% <41.66%> (-0.35%) ⬇️
protocol-designer/src/load-file/migration/8_5_0.ts 0.95% <0.00%> (-0.08%) ⬇️
...ols/MoveLiquidTools/SecondStepsMoveLiquidTools.tsx 0.70% <0.00%> (-0.07%) ⬇️

... and 1671 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ncdiehl11 ncdiehl11 self-assigned this Mar 20, 2025
@ncdiehl11 ncdiehl11 requested review from koji, jerader and syao1226 March 20, 2025 20:11
@ncdiehl11 ncdiehl11 marked this pull request as ready for review March 20, 2025 20:11
@ncdiehl11 ncdiehl11 requested review from a team as code owners March 20, 2025 20:11
@ncdiehl11 ncdiehl11 removed request for a team March 20, 2025 20:11
@jerader
Copy link
Collaborator

jerader commented Mar 20, 2025

wait so by default, when you create a new form, pushOut is null? so it would be using the default push out if the user never clicks on the checkbox? Wouldn't when they import their protocol back into PD, the checkbox would be selected?

@ncdiehl11
Copy link
Collaborator Author

ncdiehl11 commented Mar 20, 2025

wait so by default, when you create a new form, pushOut is null? so it would be using the default push out if the user never clicks on the checkbox? Wouldn't when they import their protocol back into PD, the checkbox would be selected?

This won't be the case in practice. The only reason that field is null (empty) is because getting the liquid class push out volume for a given transfer is outside of the scope of this PR. A closed checkbox will evaluate to 0 pushout volume, which would respected when re-uploading.

@jerader
Copy link
Collaborator

jerader commented Mar 20, 2025

wait so by default, when you create a new form, pushOut is null? so it would be using the default push out if the user never clicks on the checkbox? Wouldn't when they import their protocol back into PD, the checkbox would be selected?

This won't be the case in practice. The only reason that field is null (empty) is because getting the liquid class push out volume for a given transfer is outside of the scope of this PR.

Got it, thanks!

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

No nits 😄 code is very organized and field and errors work as expected. thanks for writing the test cases for the utils!

@ncdiehl11
Copy link
Collaborator Author

No nits 😄 code is very organized and field and errors work as expected. thanks for writing the test cases for the utils!

Thank you for the prompt review! Excited to get this wired up finally

@ncdiehl11 ncdiehl11 merged commit f89fd02 into edge Mar 20, 2025
42 checks passed
@ncdiehl11 ncdiehl11 deleted the feat_pd-pushout branch March 20, 2025 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants