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

feat: Add notification_uuid to digest URLs #55951

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

jangjodi
Copy link
Member

@jangjodi jangjodi commented Sep 8, 2023

Add notification_uuid to digest links

closes #54415

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 8, 2023
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #55951 (e9d3156) into master (cf29f5e) will decrease coverage by 0.01%.
Report is 5 commits behind head on master.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master   #55951      +/-   ##
==========================================
- Coverage   80.00%   80.00%   -0.01%     
==========================================
  Files        5066     5066              
  Lines      217936   217949      +13     
  Branches    36881    36884       +3     
==========================================
+ Hits       174356   174365       +9     
- Misses      38233    38235       +2     
- Partials     5347     5349       +2     
Files Changed Coverage
src/sentry/notifications/notifications/digest.py ø
src/sentry/tasks/digests.py 78.57%
src/sentry/digests/notifications.py 100.00%
src/sentry/mail/adapter.py 100.00%
src/sentry/web/frontend/debug/mail.py 100.00%

@jangjodi jangjodi marked this pull request as ready for review September 11, 2023 15:00
@jangjodi jangjodi requested a review from a team as a code owner September 11, 2023 15:00
# Conflicts:
#	tests/sentry/notifications/notifications/test_digests.py
# Conflicts:
#	tests/sentry/notifications/notifications/test_digests.py
@scttcper scttcper merged commit 2a46053 into master Sep 13, 2023
52 of 53 checks passed
@scttcper scttcper deleted the jodi/notification_uuid_digests branch September 13, 2023 20:09
@scttcper scttcper added the Trigger: Revert add to a merged PR to revert it (skips CI) label Sep 13, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: 6477a19

getsentry-bot added a commit that referenced this pull request Sep 13, 2023
This reverts commit 2a46053.

Co-authored-by: scttcper <1400464+scttcper@users.noreply.github.com>
scttcper added a commit that referenced this pull request Sep 14, 2023
@@ -42,7 +43,7 @@ def schedule_digests():
queue="digests.delivery",
silo_mode=SiloMode.REGION,
)
def deliver_digest(key, schedule_timestamp=None):
def deliver_digest(key, schedule_timestamp=None, notification_uuid: Optional[str] = None):
Copy link
Member

Choose a reason for hiding this comment

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

You may want an interim PR to use kwargs and then swap to a explicit variable.

Copy link
Member

Choose a reason for hiding this comment

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

I'm told that this may be a 3 way rollout:
(1) add new optional parameter
(2) start sending new parameter
(3) remove optional-ness

@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add notification_uuid to Digests
4 participants