PSP-7622 | Added Subdivision view and backend retrieval of property operations #3816
Conversation
|
✅ No secrets were detected in the code. |
1 similar comment
|
✅ No secrets were detected in the code. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #3816 +/- ##
==========================================
- Coverage 75.42% 75.26% -0.17%
==========================================
Files 1415 1424 +9
Lines 38470 38628 +158
Branches 7908 7928 +20
==========================================
+ Hits 29015 29072 +57
- Misses 9161 9262 +101
Partials 294 294
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| #region Constructors | ||
|
|
||
| /// <summary> | ||
| /// Creates a new instance of a NoteRepository, and initializes it with the specified arguments. |
There was a problem hiding this comment.
nit: "Create an instance of <PropertyOperationRepository> ..."
| public int Count() | ||
| { | ||
| return this.Context.PimsNotes.Count(); | ||
| } |
There was a problem hiding this comment.
Hmm, this doesn't seem right. Either delete it of return the count for Context.PimsPropertyOperations
| "no-prototype-builtins": "off", | ||
| "no-debugger": "off" |
There was a problem hiding this comment.
nit: I'm all for improving our linting rules but this PR seemed focused on backend changes. Fine for now, but we should try to keep changes relevant to their stories
| @@ -19,6 +21,18 @@ const storeState = { | |||
|
|
|||
| // mock keycloak auth library | |||
| jest.mock('@react-keycloak/web'); | |||
| jest.mock('@/hooks/pims-api/useApiPropertyOperation.ts'); | |||
There was a problem hiding this comment.
I'm not sure if this works but we don't put ".ts" in jest.mock() calls. I searched our codebase and we typically do:
// no .ts extension here
jest.mock('@/hooks/pims-api/useApiPropertyOperation');
...
| })); | ||
|
|
||
|
|
||
| jest.mock('@/hooks/pims-api/useApiProperties.ts'); |
There was a problem hiding this comment.
same comment here regarding .ts extension in mock calls
| import { booleanToYesNoUnknownString, stringToBoolean } from '@/utils/formUtils'; | ||
| import { getPrettyLatLng } from '@/utils/mapPropertyUtils'; | ||
|
|
||
| import { IPropertyDetailsForm, readOnlyMultiSelectStyle } from './PropertyDetailsTabView.helpers'; | ||
| import { SubsivisionContainer } from './propertyOperation/SubdivisionContainer'; |
There was a problem hiding this comment.
nit: SubsivisionContainer -> SubdivisionContainer
| export const SubsivisionContainer: React.FunctionComponent<ISubdivisionContainer> = ({ | ||
| propertyId, | ||
| }) => { | ||
| const View = SubdivisionView; |
There was a problem hiding this comment.
quick question - we usually pass in the "View" as parameter to the container so we can de-couple and test them independently. This doesn't seem the case here as we are "hardcoding" the view by using the imported view directly in the container.
Is there any special reasoning for this departure of our standards? If so, a comment would go a long way to explain to other devs looking at this code
There was a problem hiding this comment.
That's right I will change it once the tests are implementented
| const retrievedSources = await Promise.all(sourcePromises); | ||
| const retrievedDestinations = await Promise.all(destinationPromises); |
There was a problem hiding this comment.
nit: something to thing about/ tech-debt: this will (potentially) make a lot of requests to the GetProperty api endpoint. We should consider a "batch" get endpoint where we pass an array of Ids and get back the matching properties
… into features/psp-7622
|
✅ No secrets were detected in the code. |
|
✅ No secrets were detected in the code. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #3816 +/- ##
==========================================
+ Coverage 75.42% 76.99% +1.56%
==========================================
Files 1415 483 -932
Lines 38470 17228 -21242
Branches 7908 1166 -6742
==========================================
- Hits 29015 13264 -15751
+ Misses 9161 3670 -5491
Partials 294 294
Flags with carried forward coverage won't be shown. Click here to find out more.
|
No description provided.