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

Cascading Delete Tags #201

Merged
merged 5 commits into from
Nov 30, 2023
Merged

Cascading Delete Tags #201

merged 5 commits into from
Nov 30, 2023

Conversation

danielk1345
Copy link
Contributor

Notion task link

Task Name

Implementation description

  • Implemented the cascading delete so that it deletes all log record references with the associated tag and replaces that status field in the tags database

Steps to test

  1. Migrate your database and upgrade
  2. Create sample tags in your tags database. Ex: INSERT into tags (name, status) VALUES ('tagA', 'Active');
  3. Then, in the browser, create log records that have tags associated with them. Example body:
    { "employee_id": 7, "resident_id": 2, "datetime": "2023-11-12T01:56:01.891Z", "flagged": false, "note": "lalala", "tags": ["tagA", "tagB", "tagC"], "building_id": 1 }
  4. Then, try deleting a tag that is associated with the log record. Verify in your log_record_tag junction table that all entries with that tag_id have now been deleted.
  5. Verify that the status column no longer exists in your database

What should reviewers focus on?

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

Copy link

github-actions bot commented Nov 16, 2023

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

https://blueprintsupportivehousing--pr201-daniel-cascading-del-crxwypyh.web.app

(expires Thu, 07 Dec 2023 01:09:17 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: f6bcdba7452bf82a6ec1a299c37d1bdff7870d09

@braydonwang braydonwang changed the title initial commit Cascading Delete Tags Nov 23, 2023
Copy link
Contributor

@braydonwang braydonwang left a comment

Choose a reason for hiding this comment

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

Other than the two comments I left, everything else looks good! I tested it through Postman and everything works as expected. I'll approve this PR for now since the problems I found aren't too big

backend/app/models/tags.py Outdated Show resolved Hide resolved
backend/migrations/versions/117790caec65_.py Show resolved Hide resolved
@phamkelly17
Copy link
Contributor

Screen Shot 2023-11-22 at 9 06 58 PM

not sure if somethings wrong on my end, but i'm not able to delete tags since theyre referenced in the log_record_tag table

@phamkelly17
Copy link
Contributor

in this file supportive-housing/backend/app/models/log_record_tags.py, would you need something like this:

    log_record_tag_id = db.Column(db.Integer, primary_key=True, nullable=False)
    log_record_id = db.Column(
        db.Integer, db.ForeignKey("log_records.log_id"), nullable=False
    )
    tag_id = db.Column(db.Integer, db.ForeignKey("tags.tag_id"), nullable=False, cascade="all, delete")

idk at all but just a thought

@connor-bechthold
Copy link
Collaborator

in this file supportive-housing/backend/app/models/log_record_tags.py, would you need something like this:

    log_record_tag_id = db.Column(db.Integer, primary_key=True, nullable=False)
    log_record_id = db.Column(
        db.Integer, db.ForeignKey("log_records.log_id"), nullable=False
    )
    tag_id = db.Column(db.Integer, db.ForeignKey("tags.tag_id"), nullable=False, cascade="all, delete")

idk at all but just a thought

so if you try and use the DELETE route (/tags/{id}) this works fine, however you're right that we don't really have cascading set up properly here. we should still be able to go and delete a tag manually and not have any foreign key constraint error. kelly is right about where the change is however I think we tried this before and it didn't work. i looked a little bit more and tested this out and it didn't complain with this (after creating a migration and upgrading)

tag_id = db.Column(db.Integer, db.ForeignKey("tags.tag_id", ondelete='CASCADE'), nullable=False)

all this does is trigger a cascading delete whenever that foreign key is deleted as well

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.

awesome LGTM!!!

@danielk1345 danielk1345 merged commit 435b9dc into main Nov 30, 2023
2 checks passed
@danielk1345 danielk1345 deleted the daniel/cascading-delete branch November 30, 2023 01:19
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