fix: unit tables multiple rows for same rent type, sorting issues #1306

emilyjablonski · 2021-06-01T23:34:15Z

Pull Request Template

Issue

Addresses HBA#247 #1297

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

This PR tackles two issues. The first is that when I made the change to show multiple rows for different rent types (% income vs static number), it also ended up showing multiple rows for the same type if the values were different. For example, Coliseum appeared as below with the bug. There should be two rows for 2 BR, not three. The different static rent numbers should appear as a range.

With this fix, it now appears as below.

The second issue this solves is unit summaries coming through unsorted (see the linked issues).

I also just tried my best to clean up the fxns, they were confusing.

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?

Import a listing with rent ranges, like Coliseum. Check out the unit summaries table, the occupancy ranges, and the floor ranges.

Checklist:

netlify · 2021-06-01T23:39:43Z

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

🔨 Explore the source changes: 70685bc

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

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

jaredcwhite

Just saw one typo. Otherwise, changes make sense. I'm wondering if we can further break this file out into more discrete functions/constants? LOC is creeping ever higher…

jaredcwhite · 2021-06-03T19:53:05Z

backend/core/src/shared/units-transformations.ts

-    {
-      min: parseFloat(summary.minIncomeRange?.min),
-      max: parseFloat(summary.minIncomeRange?.max),
+const getCurrenyString = (initialValue: string) => {


⬆️ typo

fixed, thanks!

emilyjablonski · 2021-06-03T20:06:30Z

Curious what you would suggest breaking out? (Also does LOC stand for lines of code?)

jaredcwhite · 2021-06-03T21:40:47Z

Yeah, Lines of Code. So basically anything that isn't directly performing the data transformations specific to unit sumamries/hmi/etc. I would recommend breaking out (and this isn't anything specific to your work, looks like the file's been touched by a bunch of people and there might be some stuff lurking underneath I originally worked on).

Suggestions:

We can remove AnyDict and just use TS's Record
usd could be extracted for use elsewhere. also minMax, minMaxCurrency, getRoundedNumber, etc.
bmrHeaders, UnitTypeSort, etc. feel like constants that could be all defined in one centralized location
UnitMap move to a types folder?

emilyjablonski · 2021-06-03T21:42:02Z

That all makes a ton of sense! Thank you for taking a look and giving me context :)

jaredcwhite · 2021-06-03T21:42:41Z

Sure, np :)

seanmalbert

Hey @emilyjablonski , this looks good. I generally agree with @jaredcwhite 's comments, though I'm not sure how much of it we want to do here. For UnitTypeSort for example, I created a separate issue to convert unitType to an enum and have UnitTypeSort be replaced with that.

I have a question on the summaries sort too.

seanmalbert · 2021-06-07T21:40:08Z

backend/core/src/shared/units-transformations.ts

  return summary
 }

 type UnitMap = {
  [key: string]: Unit[]
 }

+const UnitTypeSort = ["studio", "oneBdrm", "twoBdrm", "threeBdrm"]


This should probably be moved to an enum that unitType also uses on the unit entity. I don't think we need to do this right now with this task though, so I created this issue #1335

seanmalbert · 2021-06-07T21:49:36Z

backend/core/src/shared/units-transformations.ts

-  return summaries.filter((item) => Object.keys(item).length > 0)
+  return summaries.sort((a, b) => {
+    return (
+      UnitTypeSort.indexOf(a.unitType[0]) - UnitTypeSort.indexOf(b.unitType[0]) ||


Isn't the value of unitType a string here, like "threeBdrm"? If that's the case, then a.unitType[0] would be "t" in this example.

It's the index of the string in the array, so the value is a number - double checking this now to see if I goofed!

Should be all set now 👍 I think the reason this typo didn't create any visible issues is because the income is also tied pretty deeply to unit size, and we don't have any units right now where unit would be a tie breaker for income

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

emilyjablonski changed the title ~~fix/unit tables multiple rows for same rent type, sorting issues~~ fix: unit tables multiple rows for same rent type, sorting issues Jun 1, 2021

emilyjablonski linked an issue Jun 1, 2021 that may be closed by this pull request

Units out of order for Coliseum #1297

Closed

emilyjablonski force-pushed the fix/unit-tables-multiple-rows-for-same-rent-type branch from ae1d6d7 to 591461a Compare June 1, 2021 23:55

emilyjablonski requested review from software-project, seanmalbert and jaredcwhite June 2, 2021 22:04

emilyjablonski marked this pull request as ready for review June 2, 2021 22:04

emilyjablonski force-pushed the fix/unit-tables-multiple-rows-for-same-rent-type branch from c8632fc to 350b524 Compare June 2, 2021 22:05

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 Jun 2, 2021

jaredcwhite reviewed Jun 3, 2021

View reviewed changes

seanmalbert mentioned this pull request Jun 7, 2021

Create an Enum for unitType for units module #1335

Closed

seanmalbert reviewed Jun 7, 2021

View reviewed changes

seanmalbert approved these changes Jun 8, 2021

View reviewed changes

emilyjablonski force-pushed the fix/unit-tables-multiple-rows-for-same-rent-type branch from e5ef479 to 2793aa8 Compare June 8, 2021 01:31

emilyjablonski added 9 commits June 8, 2021 14:34

in progress

315b5af

successful ranges

8c505d9

sort summaries

9b8f4ef

fix sorting bug

cb9ef22

changelog

9aee5f3

fix sorting bug pt 2

bb1cf26

rebase issues

db9b938

pr comments

0b62eae

remove indexed typo

70685bc

emilyjablonski force-pushed the fix/unit-tables-multiple-rows-for-same-rent-type branch from 2793aa8 to 70685bc Compare June 8, 2021 20:34

emilyjablonski merged commit 9be5342 into master Jun 8, 2021

emilyjablonski deleted the fix/unit-tables-multiple-rows-for-same-rent-type branch June 8, 2021 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: unit tables multiple rows for same rent type, sorting issues #1306

fix: unit tables multiple rows for same rent type, sorting issues #1306

emilyjablonski commented Jun 1, 2021 •

edited

Loading

netlify bot commented Jun 1, 2021 •

edited

Loading

jaredcwhite left a comment

jaredcwhite Jun 3, 2021

emilyjablonski Jun 3, 2021

emilyjablonski commented Jun 3, 2021

jaredcwhite commented Jun 3, 2021

emilyjablonski commented Jun 3, 2021

jaredcwhite commented Jun 3, 2021

seanmalbert left a comment

seanmalbert Jun 7, 2021

seanmalbert Jun 7, 2021

emilyjablonski Jun 7, 2021 •

edited

Loading

emilyjablonski Jun 7, 2021

fix: unit tables multiple rows for same rent type, sorting issues #1306

fix: unit tables multiple rows for same rent type, sorting issues #1306

Conversation

emilyjablonski commented Jun 1, 2021 • edited Loading

Pull Request Template

Issue

Description

Type of change

How Can This Be Tested/Reviewed?

Checklist:

netlify bot commented Jun 1, 2021 • edited Loading

jaredcwhite left a comment

Choose a reason for hiding this comment

jaredcwhite Jun 3, 2021

Choose a reason for hiding this comment

emilyjablonski Jun 3, 2021

Choose a reason for hiding this comment

emilyjablonski commented Jun 3, 2021

jaredcwhite commented Jun 3, 2021

emilyjablonski commented Jun 3, 2021

jaredcwhite commented Jun 3, 2021

seanmalbert left a comment

Choose a reason for hiding this comment

seanmalbert Jun 7, 2021

Choose a reason for hiding this comment

seanmalbert Jun 7, 2021

Choose a reason for hiding this comment

emilyjablonski Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

emilyjablonski Jun 7, 2021

Choose a reason for hiding this comment

emilyjablonski commented Jun 1, 2021 •

edited

Loading

netlify bot commented Jun 1, 2021 •

edited

Loading

emilyjablonski Jun 7, 2021 •

edited

Loading