PSP-8094, PSP-8095 Warn user that property is part of an existing acquisition/research file

[asanchezr]

[github-actions]

✅ No secrets were detected in the code.

[github-actions]

✅ No secrets were detected in the code.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 32.71605% with 109 lines in your changes are missing coverage. Please review.

Project coverage is 73.51%. Comparing base (c845829) to head (1200507).

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #3931      +/-   ##
==========================================
- Coverage   75.01%   73.51%   -1.50%     
==========================================
  Files        1440      956     -484     
  Lines       39608    22197   -17411     
  Branches     8152     6966    -1186     
==========================================
- Hits        29713    16319   -13394     
+ Misses       9598     5878    -3720     
+ Partials      297        0     -297     

Flag	Coverage Δ
unittests	73.51% <32.71%> (-1.50%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...eatures/mapSideBar/acquisition/AcquisitionView.tsx	49.05% <ø> (ø)
.../mapSideBar/acquisition/add/AddAcquisitionForm.tsx	58.53% <100.00%> (+0.51%)	⬆️
...eatures/mapSideBar/disposition/DispositionView.tsx	90.47% <ø> (ø)
...atures/mapSideBar/research/add/AddResearchForm.tsx	100.00% <100.00%> (ø)
...es/mapSideBar/disposition/DispositionContainer.tsx	91.52% <50.00%> (-0.72%)	⬇️
...ntend/src/features/mapSideBar/router/MapRouter.tsx	46.77% <0.00%> (ø)
.../src/features/mapSideBar/research/ResearchView.tsx	73.68% <66.66%> (-12.04%)	⬇️
...es/mapSideBar/acquisition/AcquisitionContainer.tsx	74.21% <25.00%> (-3.29%)	⬇️
...res/mapSideBar/research/add/ResearchProperties.tsx	31.03% <15.38%> (-13.41%)	⬇️
...eBar/shared/update/properties/UpdateProperties.tsx	65.16% <14.28%> (-8.86%)	⬇️
... and 4 more

... and 484 files with indirect coverage changes

[asanchezr]

I refactored the ResearchContainer to have the view injected via dependency injection (like other file containers in the code base). This makes testing easier and improves code maintainability.

[devinleighsmith]

I have to admit I'm a bit surprised by this implementation. Is there any reason why this was written instead of the standard, api-driven user override pattern we've been using up until now? Perhaps I'm just missing something that precludes the use of that workflow.

[devinleighsmith]

Also, I think the
formProperty.apiId
may be incorrect. Since this is an "Add" workflow, the form should not have an apiId yet - that would only be available in update workflows. I think this is why in my testing I was able to save two new research files back-to-back with the same property.

[asanchezr]

let's chat over Teams. Easier than typing here but my interpretation (and we asked this) is that they want the check done "before" saving the file (so we can not do an api-driven user override)

[asanchezr]

Also, I think the formProperty.apiId may be incorrect. Since this is an "Add" workflow, the form should not have an apiId yet - that would only be available in update workflows. I think this is why in my testing I was able to save two new research files back-to-back with the same property.

The formProperty.apiId will indeed be populated if you create a file from an existing property on the map (which has apiId set) and that also belongs to another ACQ file. I can show you the flow in the way I tested - maybe I missed something

[devinleighsmith]

same issue here.

[devinleighsmith]

Oh, I thought we agreed to do this on save, and that was why we went with "User Override Required" as the header. This is unfortunate, as I think this may have just been an issue with the requirements, and now the implementation is significantly more complex.

[asanchezr]

Oh, I thought we agreed to do this on save, and that was why we went with "User Override Required" as the header. This is unfortunate, as I think this may have just been an issue with the requirements, and now the implementation is significantly more complex.

I recall that we devs pushed for doing on Save and BAs went to business and said "no we want on add property before save" that is why I believe it was pointed at 5. To accommodate the fact that we would be going away from stablished pattern

[github-actions]

✅ No secrets were detected in the code.

[devinleighsmith]

Oh, I thought we agreed to do this on save, and that was why we went with "User Override Required" as the header. This is unfortunate, as I think this may have just been an issue with the requirements, and now the implementation is significantly more complex.

I recall that we devs pushed for doing on Save and BAs went to business and said "no we want on add property before save" that is why I believe it was pointed at 5. To accommodate the fact that we would be going away from stablished pattern

regardless of this, when adding a new research file that has the same property as a pre-existing research file, no warning displays either on property add or on save.