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): fix module calibration while h/s is shaking #13507

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

koji
Copy link
Contributor

@koji koji commented Sep 8, 2023

Overview

When calibrate heater-shaker, the app will send stop shaking command.
check a module type (h/s), currentSpeed > 0 and latch is open/close.
fix RQA-1518

Test Plan

Changelog

Review requests

Risk assessment

low

koji added 2 commits September 8, 2023 17:41
When calibrate heater-shaker, the app will send stop shaking command.

fix RQA-1518
@koji koji requested a review from a team September 8, 2023 21:54
@koji koji marked this pull request as ready for review September 8, 2023 21:54
@koji koji requested a review from a team as a code owner September 8, 2023 21:54
@koji koji requested review from jerader and removed request for a team September 8, 2023 21:54
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #13507 (e97c47c) into chore_release-7.0.0 (96fb52d) will increase coverage by 0.03%.
Report is 7 commits behind head on chore_release-7.0.0.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.0   #13507      +/-   ##
=======================================================
+ Coverage                71.33%   71.37%   +0.03%     
=======================================================
  Files                     2418     1586     -832     
  Lines                    67908    52728   -15180     
  Branches                  7876     3435    -4441     
=======================================================
- Hits                     48443    37635   -10808     
+ Misses                   17623    14564    -3059     
+ Partials                  1842      529    -1313     
Flag Coverage Δ
app 43.95% <ø> (-24.92%) ⬇️

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

see 832 files with indirect coverage changes

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.

looks good so far, but maybe we can add some additional logic to only omit the command when necessary.

@@ -225,6 +231,22 @@ export const ModuleCard = (props: ModuleCardProps): JSX.Element | null => {
}

const handleCalibrateClick = (): void => {
if (module.moduleType === HEATERSHAKER_MODULE_TYPE) {
Copy link
Collaborator

@jerader jerader Sep 11, 2023

Choose a reason for hiding this comment

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

I would also check if the currentSpeed status is a number > 0, so we would only emit the command if the module is shaking. Additionally, maybe we can emit a "close latch" command as well, only if the latch is opened?


const handleCalibrate = (): void => {
if (module.attachedModuleMatch?.moduleType === HEATERSHAKER_MODULE_TYPE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same for this!


const requiredAttachOrCalibratePipette =
formattedPipetteOffsetCalibrations.length === 0 ||
(formattedPipetteOffsetCalibrations[0].lastCalibrated == null &&
formattedPipetteOffsetCalibrations[1].lastCalibrated == null)

const handleCalibration = (): void => {
if (attachedModule.moduleType === HEATERSHAKER_MODULE_TYPE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this!

const { getByRole } = render(props)

getByRole('button', { name: 'Calibrate' }).click()
expect(props.handleCalibrateClick).toHaveBeenCalled()
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe you can further add on to this test to see if the useCreateLiveCommand gets called with the correct command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing createLiveCommand is kind of complicated so I will skip this time but created a ticket under RAUT.
I will follow it later.

@@ -225,7 +234,30 @@ export const ModuleCard = (props: ModuleCardProps): JSX.Element | null => {
}

const handleCalibrateClick = (): void => {
setshowCalModal(true)
if (!isLatchClosed) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to switch the order of these, right? If the latch is open, the module can't be shaking. So it's better to check if it is shaking then check if the latch is open, i think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually, i guess either way works since you won't run into a situation where you need to run both the commands in a specific order 🤔

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 think stop shaking and open a latch would be natural.
I will change the order.

@koji koji requested a review from jerader September 11, 2023 18:35
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.

lgtm! tested it in my dev env and saw the correct commands in network tab at the correct times

@koji koji merged commit 4da8e69 into chore_release-7.0.0 Sep 12, 2023
@koji koji deleted the fix_module-calibration-hs branch September 12, 2023 00:09
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