PIMS-1220, PIMS-1221, PIMS-1222 #88

devinleighsmith · 2020-03-27T04:25:08Z

Automap Evaluations
Move logic from controller to DAL

devinleighsmith · 2020-03-27T04:29:13Z

backend/dal/Services/Concrete/ParcelService.cs

-            entity.UpdatedOn = DateTime.UtcNow;
-
-            this.Context.Parcels.Update(entity);
+            //Add/Update a parcel and all child collections


this entire 60ish block could be replaced by:
context.Update(rootEntity);
context.SaveChanges();
which handles "Upserting" the parent and all child entities, supporting mixing and matching adding new entities and updating existing entities. The problem is that "Upserting" does not support deletes. My original thought was that I could messily handle the deletes by loading the existing parcel and then disposing it after so I could use Context.Update (can only attach one unique entity in a context at a time). However, the concept of detaching entities with their children is not supported as of this time. dotnet/efcore#18406

Only solution at this time is to not handle any kinds of delete operations in this update function.

I think not supporting delete here makes the most sense presently.

devinleighsmith · 2020-03-27T04:30:39Z

backend/api/Controllers/ParcelController.cs

@@ -67,6 +67,7 @@ public IActionResult GetParcels()
        /// <returns></returns>
        [HttpPost]
        [HasPermission(Permissions.PropertyView)]
+        [Route("/api/parcels/filter")]


Can't have two posts with the same route. @Fosol let me know if this is a breaking change.

Should be fine, these aren't used yet.

I understand that there can be no two POST on the same route. But with this change it is now confusing to the API consumer how to filter parcels. There's now two (different) routes to do it:

GET /api/parcels?field1=value1&field2=value2

POST /api/parcels/filter/ with a body of { field1: value1, ...}

I know the POST is there for convenience but I rather remove the [HttpPost] attribute and let it be a GET only operation. Note that I'm not saying we discard the whole GetParcels(filter) function, just not exposing it via HTTP

There's a lot of debate out there on what is "right", I only ask we remain consistent, ideas?

https://stackoverflow.com/questions/19637459/rest-api-using-post-instead-of-get

https://stackoverflow.com/questions/46585/when-do-you-use-post-and-when-do-you-use-get

https://www.moesif.com/blog/technical/api-design/REST-API-Design-Filtering-Sorting-and-Pagination/

I think this is outside the scope of this review since I think you had the same issue with the GET/POST before I was forced to rename it. Feel free to discuss that here but I'm not planning on changing anything at this time.

Fosol

A couple minor changes.

Fosol · 2020-03-27T11:41:16Z

backend/api/Controllers/ParcelController.cs

@@ -67,6 +67,7 @@ public IActionResult GetParcels()
        /// <returns></returns>
        [HttpPost]
        [HasPermission(Permissions.PropertyView)]
+        [Route("/api/parcels/filter")]


Should be fine, these aren't used yet.

Fosol · 2020-03-27T11:46:15Z

backend/dal/PIMSContext.cs

+            // get entries that are being Added or Updated
+            var modifiedEntries = ChangeTracker.Entries()
+                    .Where(x => (x.State == EntityState.Added || x.State == EntityState.Modified));
+            var userId = _httpContextAccessor.HttpContext.User.FindFirst(ClaimTypes.NameIdentifier).Value;


Do you have access to the extension method User.GetUserId()?

oh, right. let me see.

Fosol · 2020-03-27T11:47:31Z

backend/dal/PIMSContext.cs

+                        entity.CreatedById = new Guid(userId);
+                        entity.CreatedOn = DateTime.UtcNow;
+                    }
+                    entity.UpdatedById = new Guid(userId);


Don't do this if it's a EntityState.Delete as it will most likely confuse EF with related properties.

sure, updating.

Fosol · 2020-03-27T11:48:41Z

backend/dal/Services/Concrete/ParcelService.cs

+                .Include(p => p.Evaluations)
+                .Include(p => p.Buildings).ThenInclude(b => b.Evaluations)
+                .Include(p => p.Buildings).ThenInclude(b => b.Address)
+                .SingleOrDefault(u => u.Id == parcel.Id) ?? throw new KeyNotFoundException();


Probably should change to FirstOrDefault(...)

Hmm, I don't know, I kind of like the documented behaviour of this when we are searching for a unique key:

Returns the only element of a sequence, or a default value if the sequence is empty; this method throws an exception if there is more than one element in the sequence.

seems appropriate since there should never be more than one match.

At the end of the day it doesn't change much here. As the LINQ statement becomes an SQL statement and if it returns more than one it would throw an error. However since you're making a request for the primary key there never will be more than one or none returned. So using Single technically is doing more work than is required.

Fosol · 2020-03-27T11:50:35Z

backend/dal/Services/Concrete/ParcelService.cs

-            entity.UpdatedOn = DateTime.UtcNow;
-
-            this.Context.Parcels.Update(entity);
+            //Add/Update a parcel and all child collections


I think not supporting delete here makes the most sense presently.

asanchezr · 2020-03-27T17:56:07Z

Couple of comments re: process.

The title of this PR is PIMS-976 but I can't seem to find that issue in JIRA. A more descriptive title would help me understand what I should be reviewing
The PR "source" branch is named PIMS-1220 which doesn't match the issue number of the PR title so I'm a bit lost what this PR is about. PIMS-1220 is about single parcel GET and I see a lot of changes related to creating, updating and deleting parcels...

devinleighsmith · 2020-03-27T19:19:45Z

@asanchezr Agreed. I messed up the commit message on this one, I'm not in any rush so I'll correct it.

Also I ended up doing all three API stories at the same time. Theoretically I'd rather have submitted a PR for each one separately. However, realistically I updated all three at the same time as

I was using the API outputs to test the inputs, so as I updated one API I often found/resolved issues in the others.
There were some foundational changes required by all three APIs.

I'm also really annoyed that Github doesn't allow me to respond directly to your comment above. Why Github, Why?

Automap Evaluations Move logic from controller to DAL Rewrite DAL logic to be less verbose, support direct write of automapper entities.

* PIMS-1220, PIMS-1221, PIMS-1222 Automap Evaluations Move logic from controller to DAL Rewrite DAL logic to be less verbose, support direct write of automapper entities. * code review comments Co-authored-by: Smith <[email protected]>

devinleighsmith requested review from asanchezr, Fosol and jdtoombs as code owners March 27, 2020 04:25

devinleighsmith commented Mar 27, 2020

View reviewed changes

Fosol suggested changes Mar 27, 2020

View reviewed changes

asanchezr closed this Mar 27, 2020

asanchezr reopened this Mar 27, 2020

Fosol previously approved these changes Mar 27, 2020

View reviewed changes

Smith added 2 commits March 27, 2020 12:21

PIMS-1220, PIMS-1221, PIMS-1222

bbef4c7

Automap Evaluations Move logic from controller to DAL Rewrite DAL logic to be less verbose, support direct write of automapper entities.

code review comments

aa462fb

devinleighsmith dismissed Fosol’s stale review via aa462fb March 27, 2020 19:24

devinleighsmith force-pushed the PIMS-1220 branch from b030204 to aa462fb Compare March 27, 2020 19:24

Fosol approved these changes Mar 27, 2020

View reviewed changes

jdtoombs approved these changes Mar 27, 2020

View reviewed changes

devinleighsmith changed the title ~~PIMS-976~~ PIMS-1220, PIMS-1221, PIMS-1222 Mar 27, 2020

devinleighsmith merged commit dc3b655 into bcgov:dev Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIMS-1220, PIMS-1221, PIMS-1222 #88

PIMS-1220, PIMS-1221, PIMS-1222 #88

devinleighsmith commented Mar 27, 2020

devinleighsmith Mar 27, 2020 •

edited

Loading

Fosol Mar 27, 2020

devinleighsmith Mar 27, 2020

Fosol Mar 27, 2020

asanchezr Mar 27, 2020

asanchezr Mar 27, 2020

devinleighsmith Mar 27, 2020

Fosol left a comment

Fosol Mar 27, 2020

Fosol Mar 27, 2020

devinleighsmith Mar 27, 2020

Fosol Mar 27, 2020

devinleighsmith Mar 27, 2020

Fosol Mar 27, 2020

devinleighsmith Mar 27, 2020

Fosol Mar 27, 2020

Fosol Mar 27, 2020

asanchezr commented Mar 27, 2020

devinleighsmith commented Mar 27, 2020

PIMS-1220, PIMS-1221, PIMS-1222 #88

PIMS-1220, PIMS-1221, PIMS-1222 #88

Conversation

devinleighsmith commented Mar 27, 2020

devinleighsmith Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fosol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asanchezr commented Mar 27, 2020

devinleighsmith commented Mar 27, 2020

devinleighsmith Mar 27, 2020 •

edited

Loading