Linking tags and log records table

[connor-bechthold]

GitHub Issue link

Linking Tags and Log Records Table

Implementation description

• Tags to log records to a many-to-many relationship, so a junction table named log_record_tag was used to link the existing log_records table and new tags table
• As a result of this, the create, read, update, and delete log records route services had to be updated to correctly fill in the new tables appropriately

Steps to test

Setup

• Run flask db upgrade on the branch to ensure your local environment includes the new migrations
• You first need to populate the tags DB. Since we don't have API's for that yet, run docker exec -it SHOW-backend /bin/bash -c "flask db upgrade" and insert the following tags:
  • INSERT into tags (name, status) VALUES ('tagA', 'Active');
  • INSERT into tags (name, status) VALUES ('tagB, 'Active');
  • INSERT into tags (name, status) VALUES ('tagC', 'Active');

Create log record functionality

• Test if creating a log record with an invalid tag return an error. POST localhost:8080/log_records with:
  { "attn_to": 1, "building": "Building A", "datetime": "2023-03-25 08:34:56-04:00", "employee_id": 1, "flagged": false, "note": "This is a note", "resident_first_name": "Saf", "resident_last_name": "Waan", "tags": ["wrong"] }
• In the DB container select * from log_record_tag; and select * from log_records; to ensure nothing was made
• Use the same body and make 3 valid log records. Use "tags": ["tagA"], "tags": ["tagA", "tagB"], and "tags": ["tagA", "tagB", "tagC"]. Once again, check the two tables in the previous step after every insert to ensure everything is being updated properly

Get log records functionality

• Hit the GET log records route with the following (you can test more if you really really really wanna):
  • http://localhost:8080/log_records?filters={"tags":["tagA"]} should give you all three logs
  • http://localhost:8080/log_records?filters={"tags":["tagB"]} should give you two logs
  • http://localhost:8080/log_records?filters={"tags":["tagC"]} should give you one log
  • http://localhost:8080/log_records?filters={"tags":["safwaan"]} should give you no logs
  • http://localhost:8080/log_records?filters={"tags":["tagB", "tagC"]} should give you two logs

Update log records functionality

• Hit the PUT log records route (which is http://localhost:8080/log_records/{id} where id is the id of your log record you want to update). For the body just send along the same one you used in the create section and change the tags as follows (after every request, select * from log_record_tag; to make sure things were updated properly ):
  • Setting tags to [] should result in the log_record_tag table to be empty for that log ID
  • Setting tags to ["safwaan"] should toss an error, and the table should remain unchanged
  • Any combo of tagA, tagB, and tagC should be valid

Delete log records functionality

• Hitting the DELETE log records route (which is http://localhost:8080/log_records/{id} where id is the id of your log record you want to update). Once this is successful, the corresponding entries in the log_record_tag table should also be deleted

What should reviewers focus on?

• One thing I noticed is in the relation between log records, we aren't using the ORM properly (should be using this kind of syntax in the models: log_records = db.relationship("LogRecords", secondary="log_record_tag", back_populates="tags"). We should probably look into updating this eventually as it makes managing both tables a lot less painful
• The SQL query we're using when getting log records is becoming hella large, and it's probably super slow. With an ORM, there shouldn't be a need to even use raw SQL in the first place. We should look into changing this to only use SQLAlchemy queries (from the brief investigation I did, this should be possible)

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 updated other Docs as needed

[Safewaan]

Senior dev moment 😤 coming up with junction tables actually popped off MASSIVELY

There is some changes I suggested and some things to fix #skill-issue #junior-developer-moment #get-good #lbtm

[Safewaan]

for consistency, I mentioned we should either do <table_name>_id or just id. I'm leaning towards switching everything to id. What are your thoughts?

The only reason for this switch is because the resident table has id and resident_id which is a little confusing at first.

[Safewaan]

thinking this filename should be tags instead of tag (all of our models use plural instead of singular except user, which should also be updated)

[Safewaan]

should also be applied to the log_record_tags file

[Safewaan]

Realizing this now but name should probably be unique. There should be some validation (both in the database as a table constraint and the routes layer to validate that the name is unique).

[Safewaan]

Also, maybe status isn't the best approach... it's creating unused data in the db which isn't a big deal in the grand scheme of things but its not ideal...

what if we remove the status column entirely, and do a delete similar to how log_records are handled? First, delete all the occurences of tag_id in the new table, log_record_tag, then delete the tag from its table? This would make sure we don't have unused data in the db and makes it easier on the user (we would've needed to show unused tags to the user as well)

[Safewaan]

I think the one complaint about this approach was that any log record with a specific tag would lose that tag, but we can throw a warning to the admin and indicate to them that every log with the selected tag will lose that tag

[Safewaan]

this new approach would need to be handled on the API side so it doesn't need to be addressed here but we should have something concrete for #61

[Safewaan]

when filtering, I'm noticing log records appear when they shouldn't. In this example, this is the query param: filters={"tags":["tagA", "TagB"]}

Here is the response:

{
    "log_records": [
        {
            "attn_to": 1,
            "attn_to_first_name": "Safwaan",
            "attn_to_last_name": "Chowdhury",
            "building": "Building D",
            "datetime": "2023-06-13 15:15:40.849777-04:00",
            "employee_first_name": "Safwaan",
            "employee_id": 1,
            "employee_last_name": "Chowdhury",
            "flagged": false,
            "log_id": 8,
            "note": "This is a note",
            "resident_first_name": "Saf",
            "resident_last_name": "Waan",
            "tags": [
                "tagA"
            ]
        },
        {
            "attn_to": 1,
            "attn_to_first_name": "Safwaan",
            "attn_to_last_name": "Chowdhury",
            "building": "Building D",
            "datetime": "2023-06-13 15:04:12.437682-04:00",
            "employee_first_name": "Safwaan",
            "employee_id": 1,
            "employee_last_name": "Chowdhury",
            "flagged": false,
            "log_id": 7,
            "note": "This is a note",
            "resident_first_name": "Saf",
            "resident_last_name": "Waan",
            "tags": [
                "tagA",
                "tagB"
            ]
        }
    ],
    "num_results": 2
}

The first log record shouldn't appear, yes it does have tag A, but it doesn't have tag B. We should only show log records that have both tag A and tag B, and other tags if any. For example, suppose another log record existed with A, B, C- that log record should appear if A and B are filtered for.

Sorry if I explained it poorly the first time 😖

[connor-bechthold]

Future comment - investigate use of JSON_AGG for grouping tag row aggregation

[github-actions]

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

https://blueprintsupportivehousing--pr84-connor-linking-tags-ggf6u95y.web.app

_{(expires Sat, 11 Nov 2023 00:18:27 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09}