1544/ami overrides + unit form refactor #1706

emilyjablonski · 2021-08-23T16:31:50Z

Pull Request Template

Issue

Addresses #1706

This change addresses the issue in full
This change addresses only certain aspects of the issue
This change is a dependency for another issue
This change has a dependency from another issue

Description

Adds the ability to create ami chart overrides in listings management, and also due to lots of state issues refactors the UnitForm component.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Prototype/POC (not to merge)
This change is a refactor/address technical debt
This change requires a documentation update
This change requires a SQL Script

How Can This Be Tested/Reviewed?

Create a new listing. Set an ami chart. Set an ami percentage from the dropdown (will pulls unique ami percentages from the selected dropdown). The table should populate with the ami chart values. Change one of the values (creating an override). Save the unit. Re-open the unit before saving the listing and ensure the override is still there. Change the percentage and the chart and ensure the override isn't sticky.

Save a listing with a unit with an overrides and view the details page for that unit and ensure the only value saved as an override is the one(s) that was overridden and that the default values aren't duplicated.

Ensure Copy and New works and brings overrides with it. Ensure Save and New and Save and Exit work as well.

Open an existing seeded listing to edit it and ensure the ami values populate.

Checklist:

netlify · 2021-08-23T16:31:56Z

✔️ Deploy Preview for dev-partners-bloom ready!

🔨 Explore the source changes: b5b010e

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-partners-bloom/deploys/614222308dae9600070adac3

😎 Browse the preview: https://deploy-preview-1706--dev-partners-bloom.netlify.app

netlify · 2021-08-23T16:31:56Z

❌ Deploy Preview for dev-storybook-bloom failed.

🔨 Explore the source changes: b5b010e

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/6142223029c0e90007a733cf

netlify · 2021-08-23T16:31:56Z

✔️ Deploy Preview for dev-bloom ready!

🔨 Explore the source changes: b5b010e

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-bloom/deploys/61422230a9533b00075e81c0

😎 Browse the preview: https://deploy-preview-1706--dev-bloom.netlify.app

seanmalbert · 2021-08-27T04:31:43Z

#1575 , which has the backend ami override work, was merged.

netlify · 2021-08-27T20:21:21Z

✔️ Deploy Preview for clever-edison-cd22c1 ready!

🔨 Explore the source changes: e0a65b8

🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/61394c7720f950000795f83c

😎 Browse the preview: https://deploy-preview-1706--clever-edison-cd22c1.netlify.app

seanmalbert

@emilyjablonski ,
This is definitely looking better than where it was and it's much easier to follow the event loop . I have a couple of question/consideration comments on the code.

Other than that, I have a general question of how should we handle a user entering in a percentage that is not in the chart? Let's say that a user selects a chart and then adds 50, which has a match in the chart, but then changes it to 45, which doesn't. On the form now, the fields don't clear out, but if I edit an entry, save it and reopen, I will only see the value I added. I think we should either clear out the fields if they add a percentage that isn't in the chart, or we update the percentage field to be a select based off of the available, unique percentages in the chart.

seanmalbert · 2021-09-13T19:50:57Z

sites/partners/src/listings/PaperListingForm/UnitForm.tsx

+      const thisAmiChart = await amiChartsService.retrieve({
+        amiChartId: defaultChartID ?? amiChartID,
+      })
+      const amiChartData = thisAmiChart.items
+        .filter((item) => item.percentOfAmi === parseInt(defaultAmiPercentage ?? amiPercentage))
+        .sort(function (a, b) {
+          return a.householdSize - b.householdSize
+        })
+      setCurrentAmiChart(amiChartData)
+      amiChartData.forEach((amiValue, index) => {
+        setValue(`maxIncomeHouseholdSize${index + 1}`, amiValue.income.toString())
+      })


Would it make sense to decouple the fetching of the ami chart from setting the ami chart values? I know we want to fetch the chart on chart change, do we need to do that on ami percentage change too? Since this shouldn't be something that's getting changed too much, I'd rather stay on the side of simplicity, so if pulling those two pieces apart complicates the flow, then let's keep as is.

Makes sense, I changed amiPercentage to be a select based on the available percentages in the selected chart and am only fetching new charts on chart change

seanmalbert · 2021-09-13T19:55:00Z

sites/partners/src/listings/PaperListingForm/UnitForm.tsx

+    const existingId = units.filter((unit) => unit.tempId === defaultUnit?.tempId)[0]?.tempId
+    const nextId = units.length + 1
+


Would it simplify things to pass defaultUnit.tempId as the id we expect it to be already? And if that's the case, then could we also pass in a new prop for nextId to use in the case where the user is doing a & new action?

Passing two new props for the IDs and removed units as a prop

seanmalbert · 2021-09-13T19:56:21Z

sites/partners/src/listings/PaperListingForm/UnitForm.tsx

+            <table>
+              <thead>
+                <tr>
+                  <th>{t("listings.householdSize")}</th>
+                  <th>{t("listings.maxAnnualIncome")}</th>
+                </tr>
+              </thead>
+              <tbody>{getAmiChartTableData()}</tbody>
+            </table>


If you set the same classes on these fields here as what MinimalTable/StandardTable set, I think we can probably get it pretty close to what they gives us.

Added styles to make it match

seanmalbert · 2021-09-15T15:18:44Z

sites/partners/src/listings/PaperListingForm/UnitForm.tsx

+        .filter((item: AmiChartItem) => item.householdSize === index + 1)
+        .filter((item: AmiChartItem) => item.percentOfAmi === parseInt(amiPercentage))[0]


I didn't notice this before, but can you make this a single filter?

seanmalbert

@emilyjablonski ,
This looks good and seems to be working well. There's some minor cleanup and then I think you can merge.

seanmalbert · 2021-09-15T15:21:00Z

sites/partners/src/listings/PaperListingForm/index.tsx

@@ -33,6 +33,7 @@ import {
  PaperApplication,
  PaperApplicationCreate,
  ListingReviewOrder,
+  UnitAmiChartOverrideCreate,


I don't think this is used now.

seanmalbert · 2021-09-15T15:21:49Z

sites/partners/src/listings/PaperListingForm/UnitForm.tsx

+    void resetDefaultValues()
+  }, [])
+
+  const fetchAmiChart = async (defaultChartID?: string, defaultAmiPercentage?: string) => {


defaultAmiPercentage isn't used

seanmalbert · 2021-09-15T15:23:13Z

sites/partners/src/listings/PaperListingForm/UnitForm.tsx

+      setValue("priorityType.id", defaultUnit.priorityType?.id)
+      setValue("unitType.id", defaultUnit.unitType?.id)
+    }
+  }, [options])


The linter is complaining about the dependency arrays for this and two others. Does updating them cause issues?

…rrides

emilyjablonski added the wip This PR is not ready for review, do not review it's a “Work In Progress” label Aug 23, 2021

Base automatically changed from dev to master August 25, 2021 17:59

emilyjablonski changed the base branch from master to dev August 25, 2021 18:03

emilyjablonski force-pushed the 1544/ami-overrides branch from add0a1f to 55d956f Compare August 27, 2021 20:03

emilyjablonski force-pushed the 1544/ami-overrides branch 2 times, most recently from af0b1a4 to db193e5 Compare September 10, 2021 15:35

seanmalbert reviewed Sep 13, 2021

View reviewed changes

emilyjablonski marked this pull request as ready for review September 14, 2021 18:27

emilyjablonski added ready for review and removed wip This PR is not ready for review, do not review it's a “Work In Progress” labels Sep 14, 2021

emilyjablonski changed the title ~~1544/ami overrides~~ 1544/ami overrides + unit form refactor Sep 14, 2021

seanmalbert reviewed Sep 15, 2021

View reviewed changes

seanmalbert approved these changes Sep 15, 2021

View reviewed changes

emilyjablonski added 12 commits September 15, 2021 10:37

1544/ami overrides

a3328d7

input values not changing, not working

a060077

different approach always have fields in the dom

d2bd4af

defaults stay, but override on submit

6a3bd30

useMemo

01be71e

useMemo2

c49675d

latest logs

3c55779

cannot copy and new

46bd83c

wip refactor

5c815d4

remove minimal table

e4bc95e

close, apart from save and new

26ce25c

copy and new works!

7ad85b0

emilyjablonski and others added 18 commits September 15, 2021 10:37

cleanup

36c8b52

rebase issues

0eb1a09

cleanup 2

1d6dfb0

use effect which sets selects also removes overrides

80ef852

move fetch to on change

fbfb125

rent type working

95203cf

load ami chart on detail

08b985f

fix rent type

621126e

save overrides

fb22a77

load saved overrides

0f3b055

display overrides in partners

01c05ba

clean up logs

743e3c6

pr feedback

3017bbb

load values when opening existing listing

8abac97

changelog

139f1a5

fix linting

003593a

cleanup

2e24133

Fix code style issues with Prettier

37fd381

emilyjablonski force-pushed the 1544/ami-overrides branch from 47edb36 to 37fd381 Compare September 15, 2021 16:38

lock file discrepancies

b5b010e

emilyjablonski merged commit d594b12 into dev Sep 15, 2021

emilyjablonski deleted the 1544/ami-overrides branch September 15, 2021 17:53

kathyccheng mentioned this pull request Sep 16, 2021

Listings Mgmt: AMI Overrides #1544

Closed

seanmalbert pushed a commit to CityOfDetroit/bloom that referenced this pull request Jun 23, 2022

Merge pull request bloom-housing#1706 from bloom-housing/1544/ami-ove…

e78001e

…rrides

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1544/ami overrides + unit form refactor #1706

1544/ami overrides + unit form refactor #1706

emilyjablonski commented Aug 23, 2021 •

edited

Loading

netlify bot commented Aug 23, 2021 •

edited

Loading

netlify bot commented Aug 23, 2021 •

edited

Loading

netlify bot commented Aug 23, 2021 •

edited

Loading

seanmalbert commented Aug 27, 2021

netlify bot commented Aug 27, 2021 •

edited

Loading

seanmalbert left a comment

seanmalbert Sep 13, 2021

emilyjablonski Sep 14, 2021

seanmalbert Sep 13, 2021

emilyjablonski Sep 14, 2021

seanmalbert Sep 13, 2021

emilyjablonski Sep 14, 2021

seanmalbert Sep 15, 2021

seanmalbert left a comment

seanmalbert Sep 15, 2021

seanmalbert Sep 15, 2021

seanmalbert Sep 15, 2021

		const existingId = units.filter((unit) => unit.tempId === defaultUnit?.tempId)[0]?.tempId
		const nextId = units.length + 1

		.filter((item: AmiChartItem) => item.householdSize === index + 1)
		.filter((item: AmiChartItem) => item.percentOfAmi === parseInt(amiPercentage))[0]

1544/ami overrides + unit form refactor #1706

1544/ami overrides + unit form refactor #1706

Conversation

emilyjablonski commented Aug 23, 2021 • edited Loading

Pull Request Template

Issue

Description

Type of change

How Can This Be Tested/Reviewed?

Checklist:

netlify bot commented Aug 23, 2021 • edited Loading

netlify bot commented Aug 23, 2021 • edited Loading

netlify bot commented Aug 23, 2021 • edited Loading

seanmalbert commented Aug 27, 2021

netlify bot commented Aug 27, 2021 • edited Loading

seanmalbert 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

seanmalbert 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

emilyjablonski commented Aug 23, 2021 •

edited

Loading

netlify bot commented Aug 23, 2021 •

edited

Loading

netlify bot commented Aug 23, 2021 •

edited

Loading

netlify bot commented Aug 23, 2021 •

edited

Loading

netlify bot commented Aug 27, 2021 •

edited

Loading