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): finish logic for liquid class getter #18035

Open
wants to merge 3 commits into
base: edge
Choose a base branch
from

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Apr 10, 2025

Overview

This PR finishes the logic for retrieving liquid class values and unpacking them into a moveLiquid or mix step form. updateFieldsForLiquidClass directly updates the fields with values gotten from getLiquidClassesValues. This util also handles 'none' as the selected liquid class according to the form logic agreed upon by the liquid class project stakeholders. Also, I wire up the field updater in StepFormToolbox and in the advanced settings pages of both the moveLiquid and mix step forms.

Closes AUTH-1655

Test Plan and Hands on Testing

I am working on a full set of tests for these utilities. In the meantime, the best way to test this is to create or edit a mix or transfer form. Change various fields that affect liquid class settings on pages 1 and 2 including:

  • pipette
  • tiprack
  • volume
  • path
  • liquid class

Upon clicking "continue" on page 2 and updating values when prompted in the modal, you should be brought to a filled out advanced settings page. (NOTE: if you select "Don't use a liquid class," not all advanced settings will be filled out by design— reference "Conclusion for new form auto-fill" column of bottom table in this doc.)

Once on advanced settings, change some settings on aspirate or dispense tab. At the bottom of the page, click "reset aspirate/dispense settings" and confirm the modal. Verify that the values for only the selected tab were reset.

Changelog

Review requests

Risk assessment

low-medium

This PR finishes the logic for retrieving liquid class values and unpacking them into a moveLiquid or mix step form. `updateFieldsForLiquidClass` directly updates the fields with values gotten from `getLiquidClassesValues`. This util also handles 'none' as the selected liquid class according to the form logic agreed upon by the liquid class project stakeholders. Also, I wire up the field updater in StepFormToolbox and in the advanced settings pages of both the moveLiquid and mix step forms.

Closes AUTH-1655
@ncdiehl11 ncdiehl11 self-assigned this Apr 10, 2025
Copy link

codecov bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 8.34725% with 549 lines in your changes missing coverage. Please review.

Project coverage is 62.88%. Comparing base (16b1ff5) to head (bf410a9).
Report is 8 commits behind head on edge.

Files with missing lines Patch % Lines
...r/src/steplist/formLevel/handleFormChange/utils.ts 8.08% 500 Missing ⚠️
...ols/MoveLiquidTools/SecondStepsMoveLiquidTools.tsx 0.00% 20 Missing ⚠️
...StepForm/StepTools/MixTools/SecondStepMixTools.tsx 0.00% 15 Missing ⚠️
...esigner/ProtocolSteps/StepForm/StepFormToolbox.tsx 0.00% 12 Missing ⚠️
...er/src/components/organisms/ResetSettingsModal.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #18035       +/-   ##
===========================================
+ Coverage   24.40%   62.88%   +38.47%     
===========================================
  Files        2973     2990       +17     
  Lines      229681   232859     +3178     
  Branches    19299    20459     +1160     
===========================================
+ Hits        56060   146423    +90363     
+ Misses     173609    86259    -87350     
- Partials       12      177      +165     
Flag Coverage Δ
app 47.63% <7.34%> (+45.34%) ⬆️
protocol-designer 19.25% <8.34%> (+0.46%) ⬆️
step-generation 4.39% <0.00%> (-0.02%) ⬇️

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

Files with missing lines Coverage Δ
...m/StepTools/MoveLiquidTools/ResetSettingsField.tsx 100.00% <100.00%> (ø)
...er/src/components/organisms/ResetSettingsModal.tsx 3.50% <0.00%> (ø)
...esigner/ProtocolSteps/StepForm/StepFormToolbox.tsx 5.51% <0.00%> (ø)
...StepForm/StepTools/MixTools/SecondStepMixTools.tsx 0.85% <0.00%> (-0.06%) ⬇️
...ols/MoveLiquidTools/SecondStepsMoveLiquidTools.tsx 0.61% <0.00%> (-0.02%) ⬇️
...r/src/steplist/formLevel/handleFormChange/utils.ts 19.27% <8.08%> (-12.38%) ⬇️

... and 1696 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 marked this pull request as ready for review April 11, 2025 16:51
@ncdiehl11 ncdiehl11 requested review from a team as code owners April 11, 2025 16:51
@ncdiehl11 ncdiehl11 requested review from jerader and koji and removed request for a team April 11, 2025 16:51
prefix: string
): Record<string, any> => {
if (mix == null) {
return {}
Copy link
Contributor

@koji koji Apr 11, 2025

Choose a reason for hiding this comment

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

from the return type, this might be null or remove null from the return type since it isn't used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the mix object can be null if returned from multiDispense, so that's why the mix argument here is allowed to be null

: {}),
// keep current tab form data if only updating one tab
...(liquidHandlingAction === 'aspirate'
? getCurrentFormFields(rawForm, Object.keys(dispenseFields))
Copy link
Contributor

@koji koji Apr 11, 2025

Choose a reason for hiding this comment

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

could you tell me why dispenseFields is passed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason for this is because we want to keep the current dispense fields value if the liquid handling action is explicitly aspirate (meaning, the user clicked "Reset aspirate settings" on the advanced settings toolbox aspirate tab

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