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

[PIMS-227] Fix: Buildings disassociating from land #1698

Merged
merged 4 commits into from
Sep 15, 2023

Conversation

dbarkowsky
Copy link
Collaborator

@dbarkowsky dbarkowsky commented Sep 14, 2023

🎯 Summary

PIMS-227:

Issue:
When updating buildings from the View Inventory page, it was causing some fields, including associated land parcels, to be cleared or reset to default values.
This was because that information is never loaded on the page and was therefore sent back to the API without it, which interpreted it as intentionally removed.

Changes:

  • The route for updating financials was restored to only update the financials.
  • Added an assignment to update the ClassificationId and removed the attempted update to Classification. ClassificationId is the foreign key and should be updated instead.
  • Restored the edit button.

Testing instructions:

  1. Identify a building with associated land.
  2. Go to the View Inventory page and edit that building, then save your edits.
  3. Ensure the building still has associated land and has saved the new classification.

🔰 Checklist

  • I have read and agree with the following checklist and am following the guidelines in our Code of Conduct document.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation where required.
  • I have tested my changes to the best of my ability.
  • My changes generate no new warnings.

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #1698 (3314016) into dev (502faf0) will increase coverage by 0.17%.
Report is 17 commits behind head on dev.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #1698      +/-   ##
==========================================
+ Coverage   64.52%   64.70%   +0.17%     
==========================================
  Files         419      419              
  Lines       13397    13397              
  Branches     1278     1278              
==========================================
+ Hits         8645     8668      +23     
+ Misses       4326     4303      -23     
  Partials      426      426              
Flag Coverage Δ
unittests 64.70% <100.00%> (+0.17%) ⬆️

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

Files Changed Coverage Δ
...i/Areas/Property/Controllers/BuildingController.cs 100.00% <100.00%> (+69.69%) ⬆️
backend/dal/Services/Concrete/BuildingService.cs 34.83% <100.00%> (ø)

@codeclimate
Copy link

codeclimate bot commented Sep 14, 2023

Code Climate has analyzed commit 3314016 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 53.8%.

View more on Code Climate.

@@ -399,6 +399,7 @@ public Building UpdateFinancials(Building building)
{
this.Context.SetOriginalRowVersion(existingBuilding);
this.Context.UpdateBuildingFinancials(existingBuilding, building.Evaluations, building.Fiscals);
existingBuilding.ClassificationId = building.ClassificationId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the simplicity of the fix! Nice work :)

Copy link
Collaborator

@LawrenceLau2020 LawrenceLau2020 left a comment

Choose a reason for hiding this comment

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

Tested the saving of parcel and building financials from the Property List View, and all seems good.

@dbarkowsky
Copy link
Collaborator Author

dbarkowsky commented Sep 15, 2023

Don't merge yet. Working on some tests.
Coverage threshold passed!

@dbarkowsky dbarkowsky merged commit ac62d18 into dev Sep 15, 2023
5 checks passed
@dbarkowsky dbarkowsky deleted the PIMS-227-parcels-disassociating branch September 15, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants