feat(navigation): navigate to patients profile on related person click #1792
Conversation
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/hospitalrun/hospitalrun-frontend/cyrqb4cw2 |
|
|
||
| const onSave = newRelatedPersonModal.prop('onSave') as any | ||
| onSave(expectedRelatedPerson) | ||
| await act(async () => { |
There was a problem hiding this comment.
What is the reason for this test getting updated?
There was a problem hiding this comment.
I was getting errors and there was an async operation that used the wrappers which I was creating with the setup function so I made setup async and adapted the rest of the file. The test is indeed missing the new functionality, I refactored all the tests for the changes but forgot to add the new one.
But i'm pretty sure this needs to be refactored looking over it. I used the await async example from another test to get the tests working after my update but I think there is a better resolution. I'll take a look at these async actions later and rethink the solution, if anyone has any ideas drop in!
| let history = createMemoryHistory() | ||
| const setup = async (location: string, patient: Patient, user: any) => { | ||
| history = createMemoryHistory() | ||
| history.push(location) | ||
| return mount( | ||
| <Router history={history}> | ||
| <Provider store={mockStore({ patient, user })}> | ||
| <RelatedPersonTab patient={patient} /> | ||
| </Provider> | ||
| </Router>, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Not sure if we need a setup method in this test.
I think that doing the setup in a beforeEach is sufficient. Thoughts?
There was a problem hiding this comment.
If it would be more performant and reusable we make the beforeeach but I think we with the setup wrapper we can call it once on the describe scope instead of each test and in cases where we need a new patient user or route we call the setup function in the individual test. Give me a heads up and I'll finish refactoring this as necessary
There was a problem hiding this comment.
I think a setup may not be necessary because each describe has a different use case.
I’ve found the setup function to be helpful when the same describe needs different setup.
There was a problem hiding this comment.
Reverted the test changes and added the related test!
jackcmeyer
added
🚀enhancement
Fixes #1763
Changes proposed in this pull request: