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 modal to pause after set temp #5182

Merged
merged 12 commits into from
Mar 16, 2020

Conversation

IanLondon
Copy link
Contributor

overview

Closes #5117

changelog

  • Add modal
  • Refactor add/select step selectors for mocking and add tests

review requests

Code

  • This modal is controlled by the step-form/ButtonRow component state. I think that's the best place to put it b/c there it should be extensible to different steps if we want "auto-add a subsequent step" to be a regular thing. Seems OK?
  • Tests OK? Thunks calling thunks requires me to mock more than I'm used to

Behavior

  • Saving a Temperature step with "set temp" should trigger the modal. Modal should match design in PD: Add prompt modal after saving Temperature step #5117. Temperature number should be correct in the modal.
  • Clicking "I will build a pause later" should save the current form and do nothing else. The Temperature step should still be selected
  • Clicking "Pause protocol now" should create a new Pause step which will be selected in the sidebar but the form should be closed (as if you just clicked "save" on that form). The Pause step should "pause until temperature reached" with the same temperature
  • Making a Temperature step with "deactivate" should not trigger the new modal
  • Try to break it by deleting the module at different points. It should be robust and not whitescreen anywhere

@IanLondon IanLondon added ready for review protocol designer Affects the `protocol-designer` project labels Mar 9, 2020
@codecov
Copy link

codecov bot commented Mar 9, 2020

Codecov Report

Merging #5182 into edge will increase coverage by 0.02%.
The diff coverage is 37.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #5182      +/-   ##
==========================================
+ Coverage   60.29%   60.31%   +0.02%     
==========================================
  Files        1025     1027       +2     
  Lines       29062    29452     +390     
==========================================
+ Hits        17522    17765     +243     
- Misses      11540    11687     +147
Impacted Files Coverage Δ
protocol-designer/src/step-forms/reducers/index.js 41.75% <ø> (ø) ⬆️
...ocol-designer/src/components/StepCreationButton.js 0% <0%> (ø) ⬆️
...ner/src/components/StepEditForm/ButtonRow/index.js 0% <0%> (ø) ⬆️
...omponents/modals/AutoAddPauseUntilTempStepModal.js 0% <0%> (ø)
...ol-designer/src/ui/steps/actions/thunks/addStep.js 100% <100%> (ø)
protocol-designer/src/labware-ingred/selectors.js 52.5% <100%> (ø) ⬆️
...rotocol-designer/src/step-forms/selectors/index.js 24.02% <33.33%> (+0.18%) ⬆️
...ocol-designer/src/ui/steps/actions/thunks/index.js 41.81% <41.81%> (ø)
...bot-server/robot_server/service/models/settings.py 73.68% <0%> (-26.32%) ⬇️
...t-server/robot_server/service/models/networking.py 64.8% <0%> (-25.21%) ⬇️
... and 23 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 b15d360...ee7eec6. Read the comment docs.

import type { StepType, StepIdType, FormData } from '../../../../form-types'
import type { GetState, ThunkDispatch } from '../../../../types'

export const addAndSelectStepWithHints = (payload: { stepType: StepType }) => (
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 refactored this to be distinct from the more generic addStep b/c I figured we don't want generally want hints showing up to steps that are programatically added (eg this new automatically-added "pause until temp")

@@ -0,0 +1,195 @@
// @flow
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file used to be thunks.js

const payload = { stepType }
addAndSelectStepWithHints(payload)(dispatch, getState)

expect(addStepMock.mock.calls).toEqual([[payload]])
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 feel it's more ergonomic to use someMock.mock.calls and toEqual instead of doing expect(someMock).toHaveBeenCalledTimes(2); expect(someMock).toHaveBeenNthCalledWith(1, 'stuff'); expect(someMock).toHaveBeenNthCalledWith(2, 'otherStuff') -- any objections for this matcher in cases like these? It gives you a clear diff; the main con is that you have to remember the outer [] representing the array of calls

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.

For the action creator tests using redux-thunk, you could use the mock store so you don't have to mock out the thunks. There is an example in the [https://redux.js.org/recipes/writing-tests/#async-action-creators](redux docs).

])
})

describe('should dispatch ADD_STEP, and also ADD_HINT "module_without_labware" for module steps if module lacks labware', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the describe block text sounds like it should be reversed with the testName text. It will read better and follow conventions.

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.

🌡 ⏯

  • Saving a Temperature step with "set temp" should trigger the modal. Modal should match design in PD: Add prompt modal after saving Temperature step #5117. Temperature number should be correct in the modal.
  • Clicking "I will build a pause later" should save the current form and do nothing else. The Temperature step should still be selected
  • Clicking "Pause protocol now" should create a new Pause step which will be selected in the sidebar but the form should be closed (as if you just clicked "save" on that form). The Pause step should "pause until temperature reached" with the same temperature
  • Making a Temperature step with "deactivate" should not trigger the new modal
  • Really tried to break it. No white screens and accurate error messages when reordering or deleting steps/modules!

@IanLondon
Copy link
Contributor Author

IanLondon commented Mar 10, 2020

@jen-fong

For the action creator tests using redux-thunk, you could use the mock store so you don't have to mock out the thunks. There is an example in the redux docs.

This could be a good approach. I tried it out just now with the addAndSelectStepWithHints test and it didn't work out there. Since addAndSelectStepWithHints dispatches addStep which in turn dispatches selectStep (which uses a handful of selectors), we would need to mock uuid for addStep, and also mock the new selectors that selectStep uses. It would also end up re-testing things that are already tested eg addStep.test.js should cover addStep so we don't need the same tests for addAndSelectStepWithHints -- I think??

I'm now going to try using the mock store like in the example to add tests for selectStep. So then we should have full coverage for this cascade

  • addAndSelectStepWithHints should just be dispatching addStep which is mocked
  • addStep should dispatch ADD_STEP with a uuid and also dispatch selectStep, both are mocked
  • selectStep should dispatch SELECT_STEP then compute the step contents and dispatch them in a POPULATE_STEP

@IanLondon IanLondon requested a review from jen-fong March 13, 2020 21:20
@IanLondon IanLondon merged commit dbae680 into edge Mar 16, 2020
@IanLondon IanLondon deleted the pd_prompt-after-save-temp-5117 branch March 16, 2020 14:27
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: Add prompt modal after saving Temperature step
3 participants