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

Log record many residents: Backend #192

Merged
merged 48 commits into from
Nov 23, 2023
Merged

Conversation

braydonwang
Copy link
Contributor

Notion task link

Task Name

Implementation description

  • Database: log_record_residents association table between log record & resident tables
  • Backend: modify create, read, update, delete routes for logs to accommodate multiple residents per log

Steps to test

  1. Migrate database: bash ./scripts/flask-db-upgrade.sh
  2. Create log records through postman with no residents, one resident, or multiple residents. The corresponding rows should show up in the junction table.
  3. Also test editing log records, deleting log records, and filtering log records by multiple residents

What should reviewers focus on?

  • Verify SQL query for filtering and database migration

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR
  • If I have made API changes, I have updated the REST API Docs
  • IF I have made changes to the db/models, I have updated the Data Models Page
  • I have documented this PR in the Production Release Page
  • I have updated other Docs as needed

carolynzhang18 and others added 27 commits October 18, 2023 20:14
Copy link

github-actions bot commented Nov 5, 2023

Visit the preview URL for this PR (updated for commit 7432af9):

https://blueprintsupportivehousing--pr192-log-record-many-resi-zx87p792.web.app

(expires Thu, 30 Nov 2023 00:44:53 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09

Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great work!! Couple few comments to address (and also just reverting back to using the resident ID primary key), and this should be good to go

FROM log_records logs\n \
LEFT JOIN users attn_tos ON logs.attn_to = attn_tos.id\n \
JOIN users employees ON logs.employee_id = employees.id \n \
LEFT JOIN\n \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Since this join statement is decently large and also used in the count function it can be put into a helper function similar to what join_tag_attributes does - can be named something like join_resident_attributes

JOIN users employees ON logs.employee_id = employees.id"

JOIN users employees ON logs.employee_id = employees.id\n\
LEFT JOIN\n \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you can just call the helper function here, similar to join_tag_attributes

log_record = LogRecords.query.filter_by(log_id=log_id).first()
if log_record:
log_record.residents = []
if "residents" in updated_log_record:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

residents should always be in a log record so this can be removed

Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you resolve the merge conflicts? Just want to make sure that goes smoothly before testing everything again. Had a couple more small nits/q's to address but otherwise this should be good to go after!!

@@ -158,6 +173,14 @@ def filter_log_records(self, filters=None):
sql = sql + "\nAND " + options[filter](filters.get(filter))
return sql

def join_resident_attributes(self):
return "LEFT JOIN\n \
(SELECT logs.log_id, string_to_array(string_agg(CAST(residents.id AS VARCHAR(10)), ','), ',') AS resident_ids, string_to_array(string_agg(CONCAT(residents.initial, residents.room_num), ','), ',') AS residents FROM log_records logs\n \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using string_to_array(string_agg()), you should be able to use ARRAY_AGG() instead to match the same functionality

Also I'm curious, is there a reason why you casted residents.id to a VARCHAR?

self.construct_tags(new_log_record, tag_names)

db.session.add(new_log_record)
db.session.commit()
return log_record
return {**log_record, "residents": residents}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why log_record isn't just returned?

@@ -246,6 +270,7 @@ def delete_log_record(self, log_id):
raise Exception(
"Log record with id {log_id} not found".format(log_id=log_id)
)
log_record_to_delete.residents = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is log_record_to_delete.residents getting set to an empty list before getting deleted? Also, do the rows on the LogRecordResidents also need to get deleted?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you set this to an empty list an call commit(), the ORM automatically deletes all associated rows in LogRecordResidents

Copy link
Collaborator

@connor-bechthold connor-bechthold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM this is the last major DB change that we'll need so having this done is great FANTASTIC WORK WITH THIS 😈 🔥

Copy link
Contributor

@phamkelly17 phamkelly17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@braydonwang braydonwang merged commit 90a181e into main Nov 23, 2023
4 checks passed
@braydonwang braydonwang deleted the log-record-many-residents branch November 23, 2023 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants