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

Add properties pieces into listings #1328

Closed
kathyccheng opened this issue Jun 7, 2021 · 4 comments
Closed

Add properties pieces into listings #1328

kathyccheng opened this issue Jun 7, 2021 · 4 comments
Assignees

Comments

@kathyccheng
Copy link
Collaborator

kathyccheng commented Jun 7, 2021

Capturing issues for back end work to move property fields to listings

Blocks #1315, #1317, #1319

@kathyccheng
Copy link
Collaborator Author

cc @seanmalbert

@pbn4
Copy link
Contributor

pbn4 commented Jun 8, 2021

@seanmalbert I have hit som problems when approaching migrating property to listing:

  1. CRUD for Property is a separate endpoint /properties
  2. When creating a new listing one specifies only Id of the related property

Assuming we want to create an adapter layer in v1 over a new model, how do I handle a case where a v1 client wants to create a property using POST /properties, since it has been moved into the listing I would need to take that data and put it into the v2 listing object, but there is no listing yet, at least it's not available in the incoming POST /properties payload.

What I proposed here would make this problem go away #1269 (comment)

(MP): develop an adapter layer /v2 which will be compatible with the new API model but old underyling DB model

In this specific case we keep the POST /properties as is and create a v2 endpoint POST /listings which accepts property field encapsulated within listing, but under the hood it creates a row in properties table (so it's a change compatible and visible in GET /properties). Same thing in read direction: v2 fetches property from separate table but forms the response with property encapsulated in listing.

With that approach we do not have to do anything immediately. We can migrate partners portal, and public apps to v2 in any time we want, but we have to keep the old schema under the hood.

The requirement for moving onto the new schema is: every client has migrated to v2. Also moving schema to v2 requires writing a migration script.

@seanmalbert Let me know what do you think or if I'm missing something.

@seanmalbert
Copy link
Collaborator

@pbn4 ,

I think what you're proposing makes sense and is probably the quickest and cleanest pathway. I do have some new thoughts though, that I'd like to dump out and get your feedback on first:

In this case, don't we have the advantage in that we're the only ones who use the property endpoints? Is it the case that partners and public only ever get properties via the listing service list and findOne endpoints, and aside from that the listings-importer uses the property service to create a property before adding it to the listing?

If that's not true, then you can stop reading here. If that is true, then we could get away with removing the property service/endpoints altogether (we would keep the entity for now). This would require an update to listings-importer (and looks like it'd make it more simple) and the largest change would be to the listing service.

In the listing service, we could remove the LeftJoinAndSelects related to property

.leftJoinAndSelect("listings.property", "property")
.leftJoinAndSelect("property.buildingAddress", "buildingAddress")
.leftJoinAndSelect("property.units", "units")

This would also require making the property/listing relationship not required. So if listing.property_id is not null then query the Property entity with the buildingAddress and units joins and add it to the returned listing. If listing.property_id is null, then we know property is already on the listing and we can query for buildingAddress and units with the listing id.

What I don't like about what I'm thinking is it's less clean in that it relies on a conditional and requires more separate queries, which may affect performance. What I like about it is that it makes less for us to maintain and the bulk of this new logic for the listing service could be handled in one function and added in to list and findOne.

@pbn4
Copy link
Contributor

pbn4 commented Jun 15, 2021

@dominikx96 If could take a look at changes in partners DetailProperty, Property is now encapsulated in Listing and there is no Property ID, so I have flattened locales file too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants