Notifications: small fixes found after reviewer #10996
Conversation
Use single `{}` since we are using `.format()` to format these strings.
Avoid internal error and log the exception so we can figure it out how to solve it. This happens when the `Notification` does not have _all_ the required `format_values`.
| except KeyError: | ||
| # There was a key missing | ||
| log.exception("There was a missing key when formating a header's Message.") | ||
| return self.header.format_map(defaultdict(str, **self.format_values)) |
There was a problem hiding this comment.
Since we have control over the notification, I think this should fail hard, our tests should catch these instead of silently failing.
There was a problem hiding this comment.
We can't catch this by tests since this rendering happens at runtime. This could happen because a Notification was created with invalid format values, or because we have change the message over time and started required more format values.
There was a problem hiding this comment.
The idea here would be to test for each notification we generate on each part of the code base. If we generate a notification when the user does something, we should have a test for it.
There was a problem hiding this comment.
That's fine, we can test the current state, but it could fail still in the future:
- we have a notification with
body="Go to {url} and put {code}."andformat_values={"url": "https://..", "code": 1234} - in the future, we change the message to
body="Go to {url} and put {code}. Otherwise, contact support at {email}
At this point, all the old notifications with only url and code as format values will start to fail because they are missing the email key. This check here is to protect ourselves against this issue.
There was a problem hiding this comment.
We will find some edge cases as we go that we can improve and write test cases for them and protections like this one against them.
We don't support nested dictionaries in `format_values` or random objects. Only `str` and `int`. That should be enough for now. Skip all the values that are not `str` or `int` from the format values to render the messages.
|
@stsewd I'd like to get an approve here so we can deploy this tomorrow. Can you take a final look at it? 🙏🏼 |
| return { | ||
| key: escape(value) | ||
| for key, value in format_values.items() | ||
| if isinstance(value, (str, int)) |
There was a problem hiding this comment.
I think this is fine without this check, there are more built-in types that can be included (like bool).
| if isinstance(value, (str, int)) |
We should have more tests instead.
There was a problem hiding this comment.
Yeah, we can expand to other types in the future. I want to avoid running escape(dict) and returning 500. For now, str and int are the ones we are using.
| except KeyError: | ||
| # There was a key missing | ||
| log.exception("There was a missing key when formating a header's Message.") | ||
| return self.header.format_map(defaultdict(str, **self.format_values)) |
There was a problem hiding this comment.
The idea here would be to test for each notification we generate on each part of the code base. If we generate a notification when the user does something, we should have a test for it.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Related to #10922 (comment).