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

PSP-8194 Add Complete Support for MultiPolygons #4061

Merged
merged 20 commits into from
May 31, 2024

Conversation

asanchezr
Copy link
Collaborator

Tested by temporarily displaying the boundaries in the PSP database as a map layer. I was able to see multi-polygon boundaries being added to the database and displayed correctly on the map

demo.mp4

@asanchezr asanchezr added enhancement New feature or request 5.3 labels May 30, 2024
@asanchezr asanchezr self-assigned this May 30, 2024
Copy link
Contributor

✅ No secrets were detected in the code.

Copy link

codecov bot commented May 30, 2024

Codecov Report

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

Project coverage is 76.79%. Comparing base (84d4ddc) to head (ffcd7c9).
Report is 49 commits behind head on dev.

Current head ffcd7c9 differs from pull request most recent head b7a91cb

Please upload reports for the commit b7a91cb to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##              dev    #4061       +/-   ##
===========================================
- Coverage   85.37%   76.79%    -8.59%     
===========================================
  Files        1609      490     -1119     
  Lines      121154    17912   -103242     
  Branches     9375     1229     -8146     
===========================================
- Hits       103436    13755    -89681     
+ Misses      17414     3857    -13557     
+ Partials      304      300        -4     
Flag Coverage Δ
unittests 76.79% <18.98%> (-8.59%) ⬇️

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

Files Coverage Δ
...rce/backend/api/Services/DispositionFileService.cs 81.86% <100.00%> (-0.43%) ⬇️
.../apimodels/Models/Concepts/Property/GeometryMap.cs 100.00% <100.00%> (ø)
...rce/backend/dal/Repositories/PropertyRepository.cs 85.34% <100.00%> (ø)
...rce/backend/api/Services/AcquisitionFileService.cs 74.45% <0.00%> (+1.47%) ⬆️
source/backend/api/Services/ResearchFileService.cs 73.56% <66.66%> (+11.47%) ⬆️
source/backend/api/Services/LeaseService.cs 45.26% <33.33%> (+6.43%) ⬆️
...backend/api/Services/CoordinateTransformService.cs 61.53% <0.00%> (-22.68%) ⬇️
...oordinateTransformService.TransformationFilter .cs 0.00% <0.00%> (ø)
source/backend/api/Services/PropertyService.cs 42.33% <22.85%> (-1.00%) ⬇️

... and 1119 files with indirect coverage changes

var newCoords = property.Boundary.Coordinates.Select(coord => _coordinateService.TransformCoordinates(boundaryGeom.SRID, SpatialReference.BCALBERS, coord));
var gf = NetTopologySuite.NtsGeometryServices.Instance.CreateGeometryFactory(SpatialReference.BCALBERS);
property.Boundary = gf.CreatePolygon(newCoords.ToArray());
_coordinateService.TransformGeometry(boundaryGeom.SRID, SpatialReference.BCALBERS, boundaryGeom);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to do this here, or should we just call the new UpdateLocation method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The UpdateLocation calls the property repository to- well - update the property in the DB. This method is called when we are adding a brand new one as part of a larger parent entity (say an acquisition file that gets a brand new property added).

Do you have a suggestion on how to reuse the UpdateLocation to not call the DB?

@devinleighsmith
Copy link
Collaborator

Given the scope of these changes, I think we will be better served by pushing this to 5.4, what do you think?

@asanchezr
Copy link
Collaborator Author

Given the scope of these changes, I think we will be better served by pushing this to 5.4, what do you think?

I was actually thinking the same, I can change the PR to target the 5.4 branch if you agree. I'd rather see this tested with lots of time ahead for fixes etc

Copy link
Collaborator

@devinleighsmith devinleighsmith left a comment

Choose a reason for hiding this comment

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

approved, given that tests are coming in a future PR.

@devinleighsmith
Copy link
Collaborator

yes, lets push it and the story to 5.4, that will give us some increased confidence, given that a lot of our existing integration tests don't test multipolygons.

@asanchezr asanchezr changed the title PSP-8194 Add Complete Support for MultiPolygons PSP-8194 Add Complete Support for MultiPolygons - DO NOT MERGE May 30, 2024
@devinleighsmith devinleighsmith added 5.4 and removed 5.3 labels May 30, 2024
Copy link
Contributor

✅ No secrets were detected in the code.

Copy link
Contributor

✅ No secrets were detected in the code.

@asanchezr asanchezr changed the base branch from dev to 5.4 May 31, 2024 19:36
Copy link
Contributor

✅ No secrets were detected in the code.

@asanchezr asanchezr changed the title PSP-8194 Add Complete Support for MultiPolygons - DO NOT MERGE PSP-8194 Add Complete Support for MultiPolygons May 31, 2024
@asanchezr asanchezr merged commit e077ff1 into bcgov:5.4 May 31, 2024
7 checks passed
@asanchezr asanchezr deleted the psp-8194-multi-polygons branch June 5, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.4 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants