Scrub interested users #564
Conversation
There was a problem hiding this comment.
Along with the specific comments in the most recent commit, can you please add unit tests that exercise the relevant functions for both successful and failed outcomes to ensure that we're handling each outcome appropriately?
Also, is there any way to cleanly isolate the most recent commit and remove the commits related to the deletion queue from this branch? If not, it's not a huge deal, but it muddies the water on comparing changes to have the same things showing up on multiple PRs.
| interested_user.residential_address = False | ||
|
|
||
| if self.update_interested_user(interested_user) == 1: | ||
| return True |
There was a problem hiding this comment.
I believe that this will break the for loop that begins on line 225, meaning that only one record in interested_users will actually get scrubbed. Can you take a more defensive approach, such as creating an errors variable with a value of False, setting it to True if any records fail to update, then return errors?
| email = "%" + email + "%" | ||
| with self._transaction.dict_cursor() as cur: | ||
| cur.execute( | ||
| "SELECT * FROM campaign.interested_users WHERE email LIKE %s", |
There was a problem hiding this comment.
This should check email based on case-insensitive equivalency, not a LIKE match. If we're deleting [email protected] from our database, this code as written would inappropriately scrub [email protected] and [email protected] from the interested_users table.
There was a problem hiding this comment.
Bah, you're right, thanks. I'll fix that.
| @@ -872,6 +872,8 @@ def delete_account(account_id, token_info): | |||
|
|
|||
| acct_repo.scrub(account_id) | |||
|
|
|||
| interested_users_repo.scrub(acct.email) | |||
There was a problem hiding this comment.
The interested_users_repo.scrub() function can return False if one or more of the records fails to scrub, but its return value is ignored here. Ideally, it would raise an exception so that we don't end up with inconsistent state in the database, where the account has been scrubbed but records in interested_users have not.
There was a problem hiding this comment.
@cassidysymons I see, at the moment I see a user can update their email address, but it doesn't look like it also updates their respective address on the interested_users table so there are probably users that have updated their email that wouldn't be on the table.
@cassidysymons I've decided to close this PR and make a new one (with most of the improvements) to make it easier for migration 👍 |
Once the account scrub is done we want to also scrub the campaign.interested_users entry as well per #424. Currently, this uses the account email address and finds the entry in the interested_users table.