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): add PD Pause form migration for 3.0.x -> 4.0.0 #5373

Merged
merged 2 commits into from
Apr 6, 2020

Conversation

IanLondon
Copy link
Contributor

overview

Closes #5371

Previously in 3.0.x (eg last release) pauseForAmountOfTime had values true or false.

This PR changes the pauseForAmountOfTime field to pauseAction, and its values are the 3 we have in edge PD rn: untilResume, untilTime, and untilTemperature.

changelog

2 commits:

  1. rename key pauseForAmountOfTime to pauseAction and subsitute some hard-coded strings for constants.js imports
  2. Add migration fn for 3.0.x to 4.0.0

review requests

  • In new protocols, Pause still works in all 3 of its forms
  • Using an old PD protocol (eg fixture from `protocol-designer/src/load-file/tests/fixtures), Pause steps should migrate correctly when protocol is imported

NOTE! I want to write a unit test for 4_0_0.js, but I just moved the fixtures all around in #5369 so I want to add unit tests in after that is merged, as a follow-up. Will make ticket.

risk assessment

PD only

const PAUSE_ACTION_MAP = {
true: 'untilTime',
false: 'untilResume',
}
Copy link
Member

Choose a reason for hiding this comment

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

Previously if pauseForAmountOfTime was true, I assume that always also meant that there would be at least one of pauseSecond, pauseMinute, or ``pauseHour`, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be, unless somehow the user was allowed to save an invalid form in some previous version of PD. But I figured this mapping was more direct/small/simple

@codecov
Copy link

codecov bot commented Apr 6, 2020

Codecov Report

Merging #5373 into edge will decrease coverage by 0.01%.
The diff coverage is 21.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #5373      +/-   ##
==========================================
- Coverage   63.71%   63.69%   -0.02%     
==========================================
  Files        1087     1088       +1     
  Lines       30853    30865      +12     
==========================================
+ Hits        19658    19661       +3     
- Misses      11195    11204       +9     
Impacted Files Coverage Δ
...ner/src/components/StepEditForm/forms/PauseForm.js 0.00% <ø> (ø)
protocol-designer/src/constants.js 97.29% <ø> (ø)
protocol-designer/src/form-types.js 33.33% <ø> (ø)
protocol-designer/src/load-file/migration/index.js 50.00% <ø> (ø)
protocol-designer/src/steplist/fieldLevel/index.js 50.00% <ø> (ø)
protocol-designer/src/steplist/formLevel/errors.js 17.39% <0.00%> (ø)
...r/src/steplist/formLevel/getDefaultsForStepType.js 50.00% <ø> (ø)
...vel/handleFormChange/dependentFieldsUpdatePause.js 25.00% <0.00%> (ø)
...otocol-designer/src/steplist/formLevel/warnings.js 18.42% <0.00%> (ø)
...ocol-designer/src/ui/steps/actions/thunks/index.js 41.81% <ø> (ø)
... and 3 more

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 a34cd99...e7a4267. Read the comment docs.

@Kadee80 Kadee80 added this to the SPDDRS Sprint 6 milestone Apr 6, 2020
Copy link
Member

@shlokamin shlokamin left a comment

Choose a reason for hiding this comment

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

  • In new protocols, Pause still works in all 3 of its forms
  • Using an old PD protocol (eg fixture from `protocol-designer/src/load-file/tests/fixtures), Pause steps should migrate correctly when protocol is imported

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.

🔬

  • Pause still works
  • Older protocol pauses migrate
  • Adding temperature module to imported fixture protocol allows pause until temp

@IanLondon IanLondon merged commit e3ce552 into edge Apr 6, 2020
@IanLondon IanLondon deleted the pd_fix-pause-migration-5371 branch April 6, 2020 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol designer Affects the `protocol-designer` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PD: migrate Pause forms 3.0.x -> 4.0.0
3 participants