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

remove personalisation from db #783

Merged
merged 17 commits into from
Mar 8, 2024
Merged

remove personalisation from db #783

merged 17 commits into from
Mar 8, 2024

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented Feb 1, 2024

Description

We previously removed the recipient's phone number from the notifications table as part of trying to avoid our limitation of not having more than 10000 rows of data in notifications that contains PII. However, the phone number is present still in that table in another form -- encrypting inside the personalisation column, which contains everything in the csv row for that notification.

So the goal of this PR is to stop storing this personalisation info in the notifications table, and instead when we need personalisation info, we will fetch it from the ExpiringDict (our little cache) and if it is not present there, then we will fetch it from S3.

At this time we have not actually removed the phone_number and personalisation columns from the notifications table, we just no longer write to them. To remove these columns breaks many tests and requires many code changes and at the time this PR was made, there was another PR in progress (the "enums" PR) which was making similar, but conflicting changes everywhere.

Security Considerations

While not a security consideration per se, the goal of this PR is to move PII out of the database and rely on fetching it from S3.

@terrazoon terrazoon linked an issue Feb 5, 2024 that may be closed by this pull request
@ccostino
Copy link
Contributor

ccostino commented Mar 4, 2024

@terrazoon, could you please add a full description in here so that we can prep this PR for peer review? You can use the pull request template if you need a reference of what to include.

Once that information is in there, we can make sure it gets a peer review. 🙁 Thanks!

@terrazoon terrazoon requested a review from a team March 4, 2024 16:40
Copy link
Member

@A-Shumway42 A-Shumway42 left a comment

Choose a reason for hiding this comment

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

It looks great to me. I don't have major questions but if anyone takes a look and catches something I'm all for understanding.

Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Thank you, @terrazoon!

tests/app/user/test_rest_verify.py Show resolved Hide resolved
@ccostino ccostino merged commit c84d01b into main Mar 8, 2024
5 checks passed
@ccostino ccostino deleted the notify-api-749 branch March 8, 2024 20:37
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.

stop writing personalization to the db
3 participants