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(protocol-designer): fix bug with selecting magnet > disengage step #5257

Merged
merged 1 commit into from
Mar 23, 2020

Conversation

IanLondon
Copy link
Contributor

overview

Closes #5247

The problem was that existing magnet steps were being treated the same as just-newly-created magnet steps. The desired behavior for just-newly-created steps

review requests

  • Create a disengage step, save it, click off it to another step, select it again. It should still be a "disengage" in the form. Saving the re-opened form shouldn't make changes to the form (you can look at Redux state)

  • You can create a magnet step, click off without saving, reselect, fill it out, and save it successfully

  • Autofilling last entered engage height number if the previous step was disengage should still work w/o regression

  • Ideas for tests? I thought about refactoring the if (newStepType === 'magnet') {...} else if (formData?.stepType === 'magnet') {...} into a function that could be tested on its own, but I'm not very motivated to put work into the pristine-never-saved form stuff since we're going to strip it out :/

risk assessment

PD only, shouldn't affect PRs in flight

@IanLondon IanLondon added ready for review fix PR fixes a bug protocol designer Affects the `protocol-designer` project labels Mar 20, 2020
@codecov
Copy link

codecov bot commented Mar 20, 2020

Codecov Report

Merging #5257 into edge will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #5257      +/-   ##
==========================================
- Coverage   61.44%   61.43%   -0.01%     
==========================================
  Files        1043     1043              
  Lines       29623    29626       +3     
==========================================
+ Hits        18201    18202       +1     
- Misses      11422    11424       +2     
Impacted Files Coverage Δ
protocol-designer/src/ui/steps/actions/actions.js 79.06% <50.00%> (-3.44%) ⬇️

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 d28d38f...e6967f0. Read the comment docs.

Copy link
Contributor

@jen-fong jen-fong left a comment

Choose a reason for hiding this comment

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

  • Create a disengage step, save it, click off it to another step, select it again. It should still be a "disengage" in the form. Saving the re-opened form shouldn't make changes to the form (you can look at Redux state)
  • Autofilling last entered engage height number if the previous step was disengage should still work w/o regression
  • You can create a magnet step, click off without saving, reselect, fill it out, and save it successfully
    ^ When I create a disengage step without saving, clicking away to another step, then reselecting the disengage step, the disengage radio button is no longer defaulted

@IanLondon
Copy link
Contributor Author

IanLondon commented Mar 23, 2020

When I create a disengage step without saving, clicking away to another step, then reselecting the disengage step, the disengage radio button is no longer defaulted

@jen-fong yup that's the best we can get to right now I think, for pristine-never-saved forms we don't have them re-default, it should be similar to pipette field's behavior in mix/transfer steps

the thing I'm intending to test is that moduleId is populated for pristine-never-saved (if you fill it out and cannot save, it failed to populate moduleId)

Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

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

🔍 ⬆️

  • Disengage steps/step forms retain disengage selection
  • You can create a magnet step, click off without saving, reselect, fill it out, and save it successfully
  • Autofilling last entered engage height number if the previous step was disengage still works w/o regression

@IanLondon IanLondon merged commit 42e6a65 into edge Mar 23, 2020
@IanLondon IanLondon deleted the pd_fix-magnet-step-bug-5247 branch March 23, 2020 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix PR fixes a bug protocol designer Affects the `protocol-designer` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: PD regression previously saved magnet steps always revert to engage
3 participants