-
Notifications
You must be signed in to change notification settings - Fork 0
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
Create buildings table #125
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still need to review the API changes but putting up review for the pre-population of building data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥
There are 2 changes you need to make to the GET
service for log_records, but other than that everything looks correct :D
Before merging, I would also just double-check that all changes services are working correctly.
Co-authored-by: Safwaan Chowdhury <57375371+Safewaan@users.noreply.github.com>
…supportive-housing into helen/create-building-table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yas queen helen slay lgtm asf girlypop
Visit the preview URL for this PR (updated for commit 0e9f68c): https://blueprintsupportivehousing--pr125-helen-create-buildin-yw22mu1h.web.app (expires Sun, 12 Nov 2023 19:30:45 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JK SO MANY THINGS ARE BROKEN... I'm working on it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk why i just approved my own pr lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the amount of comments 😅 helen was defo plotting so thanks for cleaning this up!!
with regards to your comments
- you're right, we'll need a GET endpoint for getting all the buildings from the DB, but that can be done in a separate ticket
- leaving as displaying the ID in the residents table is fine for now, but all we need to do is also get the building name when we're fetching a resident (similar to what we do when we're getting log records). however this can also be done in a separate ticket, and is apart of the reason why i want you to set up those
relationship
columns in the models
…ueprint/supportive-housing into helen/create-building-table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops running the linter blew the number of files changed but this is GOOD TO GO 🧑🍳
Helen's PR
GitHub Issue link
Create Buildings Table
Implementation description
Steps to test
What should reviewers focus on?
Kelly's Edits
building_id
instead ofbuilding
building_id
instead ofbuilding
building_id
building_id
Steps to test
bash ./seeding/create-buildings.sh
What should reviewers focus on?
Checklist