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): Verify attached/protocol pipettes #3458

Merged
merged 1 commit into from
May 14, 2019

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented May 14, 2019

overview

This PR served as both a Bug Report and a Bug Fix. Support had reported wrong/no pipette attached messages in File Info and Calibrate Pipettes pages even after the user had gone to instrument settings and attached the correct/missing pipettes. The app was comparing models vs name. This PR gets both actual and session pipette names from shared data to avoid pipette version mismatch.

changelog

  • fix(app): Verify attached/protocol pipettes

review requests

Can be tested on sanniti-bot with api/tests/opentrons/data/bradford_assay.py.

  • detach one of the pipettes before loading protocol

  • upload protocol and observe missing/wrong pipette message in File Info and Calibrate Pipettes pages

  • attach the pipette

  • Missing pipette warning no longer exists in File Info Page

  • Missing pipette warning no longer exists in Calibrate Pipettes Page

@Kadee80 Kadee80 added bug app Affects the `app` project ready for review fix PR fixes a bug labels May 14, 2019
@Kadee80 Kadee80 requested a review from mcous May 14, 2019 16:28
@Kadee80 Kadee80 self-assigned this May 14, 2019
@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #3458 into edge will decrease coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3458      +/-   ##
==========================================
- Coverage   52.23%   52.14%   -0.09%     
==========================================
  Files         794      794              
  Lines       23155    23219      +64     
==========================================
+ Hits        12095    12108      +13     
- Misses      11060    11111      +51
Impacted Files Coverage Δ
...pp/src/components/FileInfo/ProtocolPipettesCard.js 0% <0%> (ø) ⬆️
app/src/components/calibrate-pipettes/Pipettes.js 0% <0%> (ø) ⬆️
...brary/src/components/website-navigation/NavLink.js 60% <0%> (-6.67%) ⬇️
.../src/components/website-navigation/SubdomainNav.js 100% <0%> (ø) ⬆️
app/src/components/ChangePipette/index.js 0% <0%> (ø) ⬆️
...p/src/components/ChangePipette/PipetteSelection.js 0% <0%> (ø) ⬆️
app/src/components/ChangePipette/Instructions.js 0% <0%> (ø) ⬆️
...rary/src/components/website-navigation/nav-data.js 100% <0%> (ø) ⬆️
app/src/config/index.js 33.33% <0%> (+8.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15bd097...04f425c. Read the comment docs.

sfoster1 added a commit that referenced this pull request May 14, 2019
When we introduced our pipetteModelSpecs/pipetteNameSpecs split for our pipette
data, we settled on
pipette 'name' being the _kind_ of pipette - 'p300_single', 'p10_multi', etc
pipette 'model' being the specific model version of pipette -
'p300_single_v1.5', 'p+20_single_v2.0', etc

However, we didn't fully realize this throughout our stack. The name attribute
of both the robot singleton pipette objects and the hardware control pipette
objects (and everything that depended on them) still used 'name' to mean
'model'. This causes confusion for devs and also bugs like
#3458

This PR fixes that by
- Making the 'name' attributes on both the old and new pipettes return the name
- Making new 'model' attributes on old and new pipettes that return the model
- Piping everything around internally

There's also changes to the tests to adapt their internal mocks for the above.

Though the above pr does fix the bug it mentions, this also provides another way
for clients to avoid confusing the simulated _v1 pipettes and the real versioned
pipettes: check that the pipette _names_ match between the /pipettes endpoint
and the protocol rpc output.
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

📽

@Kadee80 Kadee80 merged commit 20988b8 into edge May 14, 2019
@Kadee80 Kadee80 deleted the app_fix-wrong-pipette-attached-bug branch May 14, 2019 19:08
sfoster1 added a commit that referenced this pull request May 17, 2019
When we introduced our pipetteModelSpecs/pipetteNameSpecs split for our pipette
data, we settled on
pipette 'name' being the _kind_ of pipette - 'p300_single', 'p10_multi', etc
pipette 'model' being the specific model version of pipette -
'p300_single_v1.5', 'p+20_single_v2.0', etc

However, we didn't fully realize this throughout our stack. The name attribute
of both the robot singleton pipette objects and the hardware control pipette
objects (and everything that depended on them) still used 'name' to mean
'model'. This causes confusion for devs and also bugs like
#3458

This PR fixes that by
- Making the 'name' attributes on both the old and new pipettes return the name
- Making new 'model' attributes on old and new pipettes that return the model
- Piping everything around internally

There's also changes to the tests to adapt their internal mocks for the above.

Though the above pr does fix the bug it mentions, this also provides another way
for clients to avoid confusing the simulated _v1 pipettes and the real versioned
pipettes: check that the pipette _names_ match between the /pipettes endpoint
and the protocol rpc output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project bug fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants