PSP-7002 : H0443: Show Acq team (organization) on document #3533
Conversation
|
✅ No secrets were detected in the code. |
Codecov Report
@@ Coverage Diff @@
## dev #3533 +/- ##
==========================================
+ Coverage 68.96% 73.79% +4.82%
==========================================
Files 1370 899 -471
Lines 33519 18499 -15020
Branches 6218 5210 -1008
==========================================
- Hits 23117 13651 -9466
+ Misses 10152 4848 -5304
+ Partials 250 0 -250
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| const organizationPersonMock = getMockPerson({ id: 3, firstName: 'JONH', surname: 'Doe' }); | ||
| getPersonConceptFn.mockResolvedValue(Promise.resolve({ data: organizationPersonMock })); | ||
|
|
||
| const organizationMock = getMockOrganization(); | ||
| getOrganizationConceptFn.mockResolvedValue(Promise.resolve({ data: organizationMock })); |
There was a problem hiding this comment.
nit: no need to use Promise.resolve(...) when calling mockResolvedValue - jest already returns a promise.
Please change to
getPersonConceptFn.mockResolvedValue({ data: organizationPersonMock });There was a problem hiding this comment.
I get an error with that suggestion.
| h0443Data.property_coordinator = personResponse.data | ||
| ? new Api_GeneratePerson(personResponse.data) | ||
| : null; |
There was a problem hiding this comment.
nit: according to github/codecov there are no unit tests covering this new functionality. Please consider adding additional tests for that.
| await act(async () => { | ||
| await generate(0); | ||
| expect(generateFn).toHaveBeenCalled(); | ||
| expect(getPersonConceptFn).toHaveBeenLastCalledWith(3); | ||
| expect(getOrganizationConceptFn).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
I know this is following an existing pattern in this test file but please consider changing this to:
await act(async () => generate(0));
// ...
expect(generateFn).toHaveBeenCalled();
expect(getPersonConceptFn).toHaveBeenLastCalledWith(3);
expect(getOrganizationConceptFn).toHaveBeenCalledTimes(1);The expect statements should be called out of the act callback
| @@ -51,7 +44,7 @@ export const useGenerateH0443 = () => { | |||
| file.acquisitionFileOwners?.filter((x): x is Api_AcquisitionFileOwner => !!x) || []; | |||
| const contactOwner = owners.find(x => x.isPrimaryContact === true); | |||
|
|
|||
| const h0443Data: H0443Data = { | |||
| let h0443Data: H0443Data = { | |||
There was a problem hiding this comment.
nit: I think there is no need to change this from const to let since you are not re-assigning the whole H0443Data object, you are just assigning some sub-properties of an existing object so const can still apply here I think
|
✅ No secrets were detected in the code. |
| propertyCoordinator?.organizationId, | ||
| ); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
@eddherrera for this one, it seems like we don't set a property coordinator for an org unless there is a primary contact. Is that true?
There was a problem hiding this comment.
Correct: It was specified as an acceptance criteria.
If for an applicable case primary contact is not provided leave relevant fields blank
There was a problem hiding this comment.
Yes, that seemed odd to me as well when reviewing this PR because it is a different pattern of what we usually do, but JIRA was very clear about what they want...
Perhaps adding a comment to avoid future confusion?
|
✅ No secrets were detected in the code. |
|
✅ No secrets were detected in the code. |
1 similar comment
|
✅ No secrets were detected in the code. |
devinleighsmith
force-pushed
the
psp-7002
branch
from
4f57911 to
2dbea5e
Compare
|
✅ No secrets were detected in the code. |
|
✅ No secrets were detected in the code. |
|
✅ No secrets were detected in the code. |
1 similar comment
|
✅ No secrets were detected in the code. |
|
✅ No secrets were detected in the code. |
1 similar comment
|
✅ No secrets were detected in the code. |
No description provided.