feature: show multiple rent types

[emilyjablonski]

Pull Request Template

Issue

No issue exists for this change (I thought one existed! Oops)

Description

The units for Coliseum introduced a new complexity in that within an individual unit type there were different rent types (a static number vs % income). We didn't have a way to show anything but one row for a unit type before this change. Below you'll find first a table I made with an overview of the rent by unit type for Coliseum, and then a screenshot of the summary tables with these changes. I essentially copy-pastad the summary-creator into its function and am iterating over the units differently. I also had to make changes to the occupancy table as it was showing duplicate rows.

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)
• This change requires a documentation update
• This change requires a SQL Script

How Can This Be Tested/Reviewed?

To test this locally you'll need to import the Coliseum listing. You can find it on the alameda_0.3 branch in housingbayarea (here).

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have reviewed the changes in a desktop view
• I have reviewed the changes in a mobile view
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
  ** It's unclear to me how to test the backend changes in unit-transformations but the occupancy changes are tested
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have assigned reviewers

[seanmalbert]

Hey @emilyjablonski , for the most part the changes look good, though I do have concerns about the assumptions that the existing code and this had/have. It relies on units being grouped by type and since there isn't any logic (at least that I saw) to group them, it relies on them being inserted that way. I'm not against getting this in so people can review it, but I think we should refactor before it getting released to better handle grouping units together.

On the listing detail, Unit Features appears to have duplicates, which may be a result of how units are or are not getting grouped together.

I think what would make it easier is to have strict guidelines laid out of when units can be grouped together and then use that to create a map that could be easier to work with like:

unitMap: {
   [some composite key that takes into account unit type and other values]: Units[]
}

So, while I have requested changes here, if we're ok with merging for review sake, then we can come back to this with a refactor sometime before listing management gets released at the latest.

[emilyjablonski]

I also agree it's not the best long term solution to assume the groupings, and was surprised to see that there. I'll lean on your advice as to whether or not to merge as-is to move Coliseum along, or to refactor now.

[emilyjablonski]

Refactoring in progress :)

[emilyjablonski]

@seanmalbert Made the changes requested 👍

[netlify]

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

🔨 Explore the source changes: 83c8fff

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

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

[seanmalbert]

@emilyjablonski , nice work and we're almost there. A consequence of having rent type as part of the map key, which is necessary elsewhere, is that now the UnitTables component doesn't display units properly in the case of the Coliseum listing, because UnitTables, which is called by ListingView, takes unitsSummarized.byUnitType which now can have multiple entries for the same bedroom count. For example, instead of having one unitSummary for a one bedroom with a sq.ft. range of 486-491 and 4 units, we have two unit summaries for one bedrooms. One for 486 and one for 491 and because it filters the units in UnitTables by unitType, they both show four units. So it looks like there's a total of 8 one bedroom units 4 for each of the square footages.

There are many ways to address this. The two main pathways I see are one, to set another key in transformUnits to have units the way UnitTables expects them and pass that in, so you don't have to make any changes to UnitTables, and two, to make updates to UnitTables to handle the issue. Adding another key is likely the quickest and the least likely to break listings with units that don't fall into the same category as Coliseum.

Let me know if you need clarification on the changes.

[emilyjablonski]

Good catch, I saw issues with occupancy which were caused by the exact same thing and fixed em, but didn't see the unit table ones :( I've pushed a refactor. There are now two unit-based summaries, one by just unit type (the old way), and one by both unit type and rent. So now I can revert the changes I made to occupancy and use the now-still-available "old" summaries there and in the unit table where you saw the issue, so those should display as they did before this PR. Elsewhere in the AMI chart and the unit overview I am using the new table that includes separate rows by rent. Let me know what you think and thank you for the thorough reviews!

[seanmalbert]

Thank you, @emilyjablonski , the updates look good to me!

[seanmalbert]

@emilyjablonski ,

Sorry, one more thing, can you please add an entry to the changelog before merging?