1154 email upon privacy request receipt #1303
Conversation
| db: Session, policy: Policy, email: Optional[str] | ||
| ) -> None: | ||
| """Helper function to send request receipt email to the user""" | ||
| if not email: |
There was a problem hiding this comment.
If this function will error if no email is provided, should the function be optional?
There was a problem hiding this comment.
I'd prefer to keep email optional here so that this helper function can encapsulate handling err cases for params needed to send an email. Is that what you meant?
| ) | ||
| except EmailDispatchException as e: | ||
| # catch early since this failure isn't fatal to privacy request, unlike the subject id verification email | ||
| logger.error(e) |
There was a problem hiding this comment.
One of the EmailDispatchExceptions can log the exception error plain, is that a concern here?
There was a problem hiding this comment.
to be safe, I can mask for potential pii here
|
Over to you again @pattisdr, appreciate the review ! |
There was a problem hiding this comment.
This works well @eastandwestwind, just requesting a small change to where you're masking Pii so as much useful information gets surfaced as possible. Your EmailDispatchExceptions are in general very good without revealing Pii so it would be good to surface as much of that as we could to the user.
| @@ -77,7 +76,7 @@ def test_connection(self) -> Optional[ConnectionTestStatus]: | |||
| ], | |||
| ) | |||
| except EmailDispatchException as exc: | |||
| logger.info("Email connector test failed with exception %s", Pii(exc)) | |||
| logger.info("Email connector test failed with exception %s", exc) | |||
There was a problem hiding this comment.
I've updated this, too, to let the email dispatch service handle the PII masking for these exceptions
Purpose
Allows sending email upon privacy request receipt
Changes
FIDESOPS__NOTIFICATIONS__SEND_REQUEST_RECEIPT_NOTIFICATIONChecklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1154