code review comments

bcgov · Mar 27, 2020 · b030204 · b030204
1 parent 29f9da1
commit b030204
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 85 deletions.
diff --git a/backend/dal/PIMSContext.cs b/backend/dal/PIMSContext.cs
@@ -5,6 +5,7 @@
 using Microsoft.EntityFrameworkCore;
 using Pims.Dal.Configuration;
 using Pims.Dal.Entities;
+using Pims.Dal.Helpers.Extensions;
 using Pims.Dal.Helpers.Migrations;
 
 namespace Pims.Dal
@@ -114,7 +115,7 @@ public override int SaveChanges()
             // 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;
+            var userId = _httpContextAccessor.HttpContext.User.GetUserId();
             if (userId != null)
             {
                 foreach (var entry in modifiedEntries)
@@ -123,11 +124,14 @@ public override int SaveChanges()
 
                     if (entry.State == EntityState.Added)
                     {
-                        entity.CreatedById = new Guid(userId);
+                        entity.CreatedById = userId;
                         entity.CreatedOn = DateTime.UtcNow;
                     }
-                    entity.UpdatedById = new Guid(userId);
-                    entity.UpdatedOn = DateTime.UtcNow;
+                    else if (entry.State != EntityState.Deleted)
+                    { 
+                        entity.UpdatedById = userId;
+                        entity.UpdatedOn = DateTime.UtcNow;
+                    }
                 }
             }
 

diff --git a/backend/dal/Services/Concrete/ParcelService.cs b/backend/dal/Services/Concrete/ParcelService.cs
@@ -207,6 +207,7 @@ public Parcel Update(Parcel parcel)
                 .Include(p => p.Evaluations)
                 .Include(p => p.Buildings).ThenInclude(b => b.Evaluations)
                 .Include(p => p.Buildings).ThenInclude(b => b.Address)
+                .AsNoTracking()
                 .SingleOrDefault(u => u.Id == parcel.Id) ?? throw new KeyNotFoundException();
 
             parcel.ThrowIfNotAllowedToEdit(nameof(parcel), this.User, "property-edit");
@@ -218,87 +219,7 @@ public Parcel Update(Parcel parcel)
             if (existingParcel.AgencyId != parcel.AgencyId) throw new NotAuthorizedException("Parcel cannot be transferred to the specified agency.");
 
             this.Context.Parcels.ThrowIfNotUnique(parcel);
-
-            //Add/Update a parcel and all child collections
-            if (existingParcel == null)
-            {
-                this.Context.Add(parcel);
-            }
-            else
-            {
-                this.Context.Entry(existingParcel).CurrentValues.SetValues(parcel);
-                this.Context.Entry(existingParcel.Address).CurrentValues.SetValues(parcel.Address);
-                foreach (var building in parcel.Buildings)
-                {
-                    var existingBuilding = existingParcel.Buildings
-                        .FirstOrDefault(b => b.Id == building.Id);
-                    this.Context.Entry(existingBuilding.Address).CurrentValues.SetValues(building.Address);
-                    if (existingBuilding == null)
-                    {
-                        existingParcel.Buildings.Add(building);
-                    }
-                    else
-                    {
-                        this.Context.Entry(existingBuilding).CurrentValues.SetValues(building);
-
-                        foreach (var buildingEvaluation in building.Evaluations)
-                        {
-                            var existingBuildingEvaluation = existingBuilding.Evaluations
-                                .FirstOrDefault(e => e.BuildingId == buildingEvaluation.BuildingId && e.FiscalYear == buildingEvaluation.FiscalYear);
-
-                            if (existingBuildingEvaluation == null)
-                            {
-                                existingBuilding.Evaluations.Add(buildingEvaluation);
-                            }
-                            else
-                            {
-                                this.Context.Entry(existingBuildingEvaluation).CurrentValues.SetValues(buildingEvaluation);
-                            }
-                        }
-                    }
-                }
-                foreach (var parcelEvaluation in parcel.Evaluations)
-                {
-                    var existingEvaluation = existingParcel.Evaluations
-                        .FirstOrDefault(e => e.ParcelId == parcelEvaluation.ParcelId && e.FiscalYear == parcelEvaluation.FiscalYear);
-
-                    if (existingEvaluation == null)
-                    {
-                        existingParcel.Evaluations.Add(parcelEvaluation);
-                    }
-                    else
-                    {
-                        this.Context.Entry(existingEvaluation).CurrentValues.SetValues(parcelEvaluation);
-                    }
-                }
-            }
-
-            //Delete any missing records in child collections.
-            foreach (var building in existingParcel.Buildings)
-            {
-                var matchingBuilding = parcel.Buildings.FirstOrDefault(b => b.Id == building.Id);
-                if (matchingBuilding == null)
-                {
-                    this.Context.Buildings.Remove(building);
-                    continue;
-                }
-                foreach (var buildingEvaluation in building.Evaluations)
-                {
-                    if (!matchingBuilding.Evaluations.Any(e => (e.BuildingId == buildingEvaluation.BuildingId && e.FiscalYear == buildingEvaluation.FiscalYear)))
-                    {
-                        this.Context.BuildingEvaluations.Remove(buildingEvaluation);
-                    }
-                }
-            }
-            foreach (var parcelEvaluation in existingParcel.Evaluations)
-            {
-                if (!parcel.Evaluations.Any(e => (e.ParcelId == parcelEvaluation.ParcelId && e.FiscalYear == parcelEvaluation.FiscalYear)))
-                {
-                    this.Context.ParcelEvaluations.Remove(parcelEvaluation);
-                }
-            }
-
-            this.Context.SaveChanges();
+            this.Context.Update(parcel);
             this.Context.CommitTransaction();
             return parcel;
         }