Bloom Review - General Q&A

[seanmalbert]

👋 Welcome!

This discussion is reserved for questions that have come up during your review of Bloom's codebase. Questions like, what is this field used for, why is the data structured this way and how do I access this service are all welcomed.

[willrlin]

Here are some initial questions from the team:

• What do you use tags on your master branch for? Releases?
• What differentiates a production build from a dev build? Some environment variables are different? Anything else? (Is there deployment documentation?)
• When should we pull upstream changes into our fork? Is anything on master ready to pull?

This question probably deserves a bigger discussion - the recommended approach for forking Bloom is to copy sites/public and sites/partners to individual localities, such as sites/public-ala, so how does one then consume upstream changes? Would we need to copy over individual changes or rebase commits? We will likely need to keep our copy of backend/core in order to make separate breaking changes there.

[seanmalbert]

What do you use tags on your master branch for?

Yes, we use them for releases, at least the ones starting from May 21st going forward.

What differentiates a production build from a dev build?

Environment variables and where they're defined. That's about it right now. We recently had discussions of splitting out different configs for dev/prod environments (for webpack for example).

In general we need to update/create more documentation, so I appreciate these questions as they should help guide and prioritize that for us. Here's the current deployment documentation we have:
https://github.com/bloom-housing/bloom/blob/master/docs/DeployAppsNetlify.md
https://github.com/bloom-housing/bloom/blob/master/docs/DeployServicesHeroku.md

When should we pull upstream changes into our fork?

We have recently introduced a dev branch into our workflow, so that all of our development/prs go into this branch. On a biweekly basis, we then merge dev into master, upon which master is frozen for a week (ideally), which we call "staging". Once it's staged for a week we then publish master to production. So for the safest and cleanest pull, I recommend doing it on those days. We are trying to establish and follow a two week release cycle. So the next time to pull would be next Wednesday and then every two weeks following that.

When your team does contribute upstream, it should be to the dev branch.

To your last question, we can setup time to discuss it. I think that the documentation is outdated and the recommended approach is to fork Bloom and keep sites/public and sites/partners and work off of those and not create new apps like sites/public-ala. I believe that was a road the team tried out with Alameda and abandoned due to difficulties keeping it up-to-date with upstream.

Detroit, like Alameda, would be deployed using that fork, and if Detroit's backend requires breaking changes to core then the public apps can point to a server running your version. Ideally we don't let that go on for too long so the two don't diverge greatly.

For an even larger discussion, in the long term, I would like to move towards the jurisdictions being completely based off of configuration and not have to deploy the separate sites based from their forks. I don't think it's too far off from accommodating that, but I'm not sure it's something we want to focus on within the next six months.

[willrlin]

What services are needed to deploy Bloom other than the 3 NodeJS environments, Redis, and PostgreSQL database? Here's what we've discovered, but wondering if there are any we missed:

• Email server provider: for sending account creation/forgot password emails.
• Mapping provider: for displaying area map on the listing detail view.
• Image hosting services: for uploading and displaying listing images.

[seanmalbert]

You covered it. For some of the services listed, particularly with redis, email and assets, I think it makes sense to abstract those so anyone wanting to implement Bloom wouldn't be tied to any particular service.

[willrlin]

In our deployment of this site, neither of our front-ends are able to login using the separately deployed backend (see CityOfDetroit#100). We think it's probably related to certificates, but I didn't see an obvious fix for that. Did you run into anything like this before?

[seanmalbert]

I don't think we have run into this issue, since the frontends and backend both get served over https in production. As I know it, you'll get that issue when a page using https fetches content that is served from http. It looks like you may have solved the issue, can you confirm? It's strange that you saw a cors error, since it should already be enabled in the backend app.

[willrlin]

Yep, for any future readers - I had mistakenly put http in the environment variable during build time for our Docker container, which caused React to generate those pages so even though I had https in the BACKEND_API_BASE during runtime, it would fail with this error.

[avaleske]

What is the roadmap and timeline for filters on the /listings endpoint? I didn't see any open issues for filtering when I searched a few weeks ago, but I noticed you recently added a framework for filters in #1359.

Will there be more filters related changes coming? I don't want to duplicate effort if so.

Relatedly, when will these changes be merged to master? I'd like to build on them, but I don't want to wait two weeks for the next release.

Thank you!

@willrlin and @ pfeeny5 for visibility
(Feel free to move to another discussion if you'd prefer roadmap questions be elsewhere.)

[seanmalbert]

Hey @avaleske ,

Do you have opinions about adding a map from filter name to the type of where clause used?

I like the idea of a map, since it makes it very clear what filter is getting added and the current logic makes it more difficult to account for special cases. What do you think about having the filter maps be defined in their respective modules and not within addFilter? So the listing service would pass in its own map to use. One thought I had was that if a key was something like property.neighborhood, that it would get split so property would be the schema in this case, but to effectively do that, we'd likely create a map anyway to add the property, so a map like what you're proposing makes sense to me.

we were curious about your thoughts on changing the querystring format itself

We're very open to changing the query string format itself. Perhaps using a library like https://www.npmjs.com/package/query-string would make it easier to define and stringify client side and then parse server side. It doesn't support nesting, however, so we'd still have to come up with a way to define the comparison type on a flat level. Maybe something like [param]-[comparison]=[value]. Changing the comparison keys to something like $eq and $neq might make it easier to use as well. What are your thoughts on that?

I'm not sure your requirements for actually displaying a query string in the URL (maybe useful for sharing searches), but another thought I had was that we could change the GET requests to POST where we want to filter, so we could pass in a JSON body with the filters. This way we could pass filters like:

[
{
  "depositMin": { ">":  500 }
},
{
  "depositMin": { "<": 900 }
}
]

This would also open the door to more easily allow for setting more complex filters with AND and/or OR, but that isn't a current use case for us.

[avaleske]

@seanmalbert this is really helpful, thanks! Would you want us to make it more generic before we contribute what we have upstream?

I'm running into an issue writing a test for listings.service.ts, if you have a minute to help troubleshoot:
I'm trying to instantiate a query params object with a filter property here, matching the same format that I see in the controller in the debugger, but I'm getting type issues.

All throughout the listings code, the filter variable is declared as an array of ListingFilterParams, with type ListingFilterParams[], including in the signature for addFilter, but then that same variable is treated as a plain ListingFilterParams object here in addFilter.

The confusing thing to me is that in the debugger, it never looks like an array of ListingFilterParams, it just looks like a ListingFilterParams object. If I add a breakpoint in the controller here doesn't even have a length property even though it's an array type.

Is there some type weirdness happening when the querystring is read into a variable by Nest? Does it just force it into whatever type we say, even if it doesn't actually match that type? And if it's actually just an object that typescript is treating as an array, does the comment about treating it like an array to keep the keys in order actually apply? If it's an object underneath anyway, it doesn't seem like it would?

[avaleske]

I ended up changing the type of the filter property to ListingFilterParams from ListingFilterParams[], since that's what it was underneath.

That work is in CityOfDetroit#180 if you're curious.

[seanmalbert]

Thanks @avaleske ,
Thank you for your work on this! The reason for the Array type ListingFilterParams[] was because of weirdness with nest/swagger and testing it via backend localhost:3100/docs/#/listings/list, you can see it at https://bloom-backend-dev.herokuapp.com/docs/#/listings/list too. Though I'm not too concerned about that having a nice interface to work with there over having types and tests that make sense.

Where on your roadmap do you think 210 and 202 are?

[avaleske]

CityOfDetroit#202 is in our next sprint. I'm not sure about CityOfDetroit#210, since imo it'd be nice but it's not as crucial as other things on our roadmap.

[willrlin]

There's a field under the property named "unitsAvailable", and then under each unit type / rent there's a "totalAvailable". Are these the units that are actually available for rent? We're trying to map what data we will eventually have onto the fields that exist right now. We will end up displaying all the properties with a few that have availability for unit types (1 BR, 2 BR) at specific rents.

[seanmalbert]

Yes, totalAvailable represents the units that have a status of "available". I think there's some debate on the value "status" on a unit, since so far I think it's always set to "available". It also looks like the only current use case of "totalAvailable" is to determine whether to use "unit" vs "units" in display. So I think this is an area where we have a lot of flexibility.

As a separate question, are you or will you be displaying properties independently from listings? We are trending towards that not being the case, and on master we have updates to map property data directly onto the listing in the listing DTO and frontend updates to accommodate that. I don't want us to go further down that road with changing the property model itself, if this will hinder your efforts.

[abbiefarr]

Would you be open to changing the unit status type to an enum? Context is here. I'm happy to submit a PR upstream if it's useful to you.

Re: your question about properties and listings, I don't believe we're planning on displaying properties independently from listings. I think we've been running under the assumption that there was currently a 1:1 mapping, and that you might be rethinking the relationship between the entities in the near future.

[seanmalbert]

@abbiefarr ,
Adding an enum for unit status is a great idea, and a PR to upstream dev would be much appreciated. Thank you!

Re: properties and listings, 👍

[willrlin]

How does the client.ts file in backend/core get generated? We're running into a lot of merge conflicts and some failing tests after trying to merge 1.0.4-stage into our repo. I see the yarn generate:client for generating backend-swagger.ts, but nothing for client.ts.

[willrlin]

Great, thanks! Some of the test errors we're seeing are coming from a regenerated backend-swagger.ts. Do you have a way to automatically generate it and keep it up to date? I don't think we can just pull yours because we've made some interface changes. Strangely, all tests pass locally, but some fail on Travis CI: https://app.travis-ci.com/github/CityOfDetroit/bloom/builds/232307346.

[seanmalbert]

We currently don't have a way to auto generate backend-swagger.ts, but it would be useful to have. The nest CLI is a wrapper for webpack, so we should be able to define our own configuration for hot module reload and control the script that's called once the build is done or use a separate watcher like nodemon, chokidar or pm2.

I'd like to discuss the changes you're making to the interfaces to see if we can align and keep the projects better in sync. I'll reach out to you directly to set something up.

[seanmalbert]

Also, @willrlin , we're seeing similar issues with backend-swagger with newer PRs that have it updated, so we're going to have to fix that soon. Automating the generation should be a separate task, does your team have capacity to tackle that?

[seanmalbert]

We have an issue for the backend-swagger fixes @ #1503, with a PR @ #1504, and I created an issue to handle auto generating the client @ #1506.

[willrlin]

Thanks, we'll take a look. I think this may be something we can pick up depending on how big the scope is.

[anders-schneider]

I'm curious about Exygy's roadmap for listing photos. I've scanned #1177 and #1348, and I was excited to see that this seems like something you all are actively working on! Two quick questions:

• Are there any plans to allow partners to upload more than one image per listing? (We've had conversations on our side about supporting e.g. a photo carousel)
  • In this comment it sounded like the intention might be to create an "image_id foreign key pointing to assets array", but it looks instead like the image field points to a single Asset?
• Do you all have plans/dates for when you expect to launch this feature?

[seanmalbert]

Hey @anders-schneider ,
Yes, that was an oversight that we didn't create that one to many relationship. We have talked about getting that updated within the next couple of months (sooner if we get ahead of our "beta" epic) since it wasn't a requirement for our MVP. I created #1530 . I believe this would have implications on our frontend apps, but if you need this sooner and can submit a PR for the backend changes upstream, we can resolve the frontend issues separately and merge in tandem.

[anders-schneider]

Thanks @seanmalbert ! We are talking to Detroit about how high to prioritize support for multiple photos per listing; if we get a strong signal, we'd be happy to send a PR for #1530 .

A small follow-up: in the current absence of a way to upload photos through the partner portal, what does the process look like for adding/modifying a listing's photo?

[seanmalbert]

We haven't had the opportunity to add/update a new listing since we got those updates in, but prior to this it's been a manual process of uploading the image to s3 and including the image url in the import. Now, we can use what we have for the listing management form to add/edit an image for the listing. See

I don't know if your team has seen the latest updates we have for listing's management in partners, so let me know if you want to schedule a demo. We have some new components in ui-components to handle this. Including a Dropzone component, which accepts an uploader prop, which is a callback that receives a file. We also have a CloudinaryUpload component, which we use in conjunction with that. I'm not sure your plans on asset hosting, but you should be able to use Dropzone with your own uploader.

[willrlin]

Do you have a spec (or a sample) for the JSON that the HOUSING_COUNSELOR_SERVICE_URL is supposed to return? Ours is still pointing at sfgov, which doesn't have anything, thus that cypress test fails.

[seanmalbert]

Hey @willrlin , looking at what's in /public/pages/housing-counselors.tsx, it looks to me that a lot of this may be defunct, since https://housing.acgov.org/ doesn't seem to be using it properly either. It should return data that has the shape of HousingCounselor[] as defined in backend/core/types/src/HousingCounselor:

name: string
languages: string[]
address?: string
citystate?: string
phone?: string
website?: string

That being said, both housing-counselors.tsx and the tests in integration/pages/HousingCounselors.ts both look like they account for not requiring a proper return from HOUSING_COUNSELOR_SERVICE_URL, but it does look like the props aren't getting properly set for that component. In looking into that further I found that there is likely an issue with the getInitialProps in housing-counselors and how NextJS is dealing with that. I created a bug (#1604 ) to address this.

[elenm]

Hi Sean,
I see that the links in the progress bar don't currently redirect to the corresponding page. Is there an intent to use them as links in the future or do we want the progress bar to be a static visual representation of the progress so far?
Would you be open to us updating them to be list items instead of links to avoid accessibility problems (issue 263, issue 265)?

Thanks for your time!

[seanmalbert]

Hi @elenm , this is a good question for @slowbot and @kathyccheng . As far as I know, they're just indicators of progress, so I think updating them to list items makes sense.

[slowbot]

@seanmalbert @elenm The progress bar items were originally intended to be links. Since there are no immediate plans to make them functional, the fastest thing would probably be to convert them to list items and create a ticket in the backlog to explore converting them to links in the future.

[willrlin]

We're looking into adding RTL support since Detroit needs Arabic translations. We found a few different plugins, but would like to contribute this upstream since it likely involves changes to ui-components. Did you already have plans in the works for RTL and if so, which plugin were you all thinking of using? We tried to add in tailwindcss-rtl, but I don't think I configured the ui-components/tailwind.config.js correctly, since it wasn't recognizing RTL Tailwind attributes like text-start. Any tips/thoughts?

[seanmalbert]

Hey @willrlin , we don't have any plans for RTL, and that plugin looks as good as any. I think the issues you're seeing likely have to do with how the app is using tailwind. We have an issue to upgrade tailwind to 2.0, which will change how we configure tailwind. As part of this, we can make sure that tailwind plugins like tailwindcss-rtl function as expected.

[willrlin]

We're running into an issue (described in CityOfDetroit#429) where it doesn't seem like React is re-rendering the modal pop-up when something changes in the form that's embedded inside (I've also tried doing other actions inside the modal and those have the same effect). Have you ever encountered anything like this / know how we could go about investigating?

[seanmalbert]

Hey @willrlin ,
I didn't boot it up, but I looked at the code and it looks like the form validation might not be firing. If you do the following, it might help:

1. Add trigger to the destructured values for useForm, so const { handleSubmit, register, errors, trigger } = useForm()
2. At the top of the onSubmit, add await trigger() and set that to a variable if you need to do any logic with it there (I think it returns a boolean)
3. If you log errors, you should see an object in the console with the field name as one of the keys.

I hope that helps, but if it doesn't, I'm down to talk about it at our next meeting and step through the issue if needed.

Edit: and I should say that on the site-public URL you shared with me, it all seems to be working fine except for the validation piece, which is why my response focuses on that.

[willrlin]

Turns out the issue was much more simple - in the Field component I was checking errors?.zipCodeField, which was the id of the Field. I needed to change the error to check the name of the field, which was ListingFilterKeys.zipcode ("zipcode") instead of "zipCodeField". I didn't need to check await trigger() in the submit; the validation seems to occur before the onSubmit runs.

[willrlin]

We noticed that the seed data doesn't seem to be populating the ami_chart_item table, only the items column of the ami_chart table. Does that mean that the ami_chart_item table is not used? Or that it gets populated in a real deployment but not in the seed data?

[pbn4]

@willrlin Currently AmiChartItem is a jsonb column in ami_charts table:

@Entity()
export class AmiChart {
...
  @Column("jsonb")
  @Expose()
  @IsDefined({ groups: [ValidationsGroupsEnum.default] })
  @ValidateNested({ groups: [ValidationsGroupsEnum.default], each: true })
  @Type(() => AmiChartItem)
  items: AmiChartItem[]
...
}

so ami_chart_items table is not in use and it's deletion was probably missed by us at some point.

[willrlin]

Great, thanks! Just wanted to be sure that we weren't missing anything.

[willrlin]

I noticed in the recent build (as of October 1st I think) that there's a button to invite a user as a partner or administrator. Do you also plan to have a UI for upgrading an existing user from a public side account to a partner or admin? I haven't tested this scenario, but I'm wondering if the email is already in use on the public side if the invitation/confirmation would succeed?