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.

Sign up for GitHub

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

Jump to bottom

ref: fix typing for sentry.digests.notifications #72367

Merged
merged 1 commit into from
Aug 13, 2024
Merged

ref: fix typing for sentry.digests.notifications #72367

merged 1 commit into from
Aug 13, 2024

Conversation

asottile-sentry
Copy link
Member

@asottile-sentry asottile-sentry commented Jun 7, 2024 •
edited
Loading

so this ended up being a pretty big change -- even with splitting out most of the smaller digestible parts

the main problem preventing this from being typesafe was the original code stuffed user_count / event_count attributes into Group instances

refactoring that out required breaking the FP structure of digests.notifications (though an argument can be made that it wasn't FP at all since it mutated the inputs (!))

this actually ended up simplifying the digest grouping significantly, and made it a lot easier to follow what was happening

I was able to preserve the tests for the most part as well!

I did have to adjust the digest-text template since it was relying on the attribute stuffing -- I also needed to adjust the debug mail view since the way it was faking organizations was relying on the previous attribute stuffing

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 7, 2024
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch 3 times, most recently from 35e25e1 to cadc564 Compare June 7, 2024 22:42
@codecov Codecov
Copy link

codecov bot commented Jun 7, 2024 •
edited
Loading

Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time.

❌ Failed Test Results:

Completed 21749 tests with 1 failed, 21547 passed and 201 skipped.

View the full list of failed tests

acceptance

  • Class name: tests.acceptance.test_emails.EmailTestCase
    Test name: test_emails
    Flags:
    • backend

    #x1B[1m#x1B[31mtests/acceptance/test_emails.py#x1B[0m:99: in test_emails
        assert redact_notification_uuid(fixture_src) == redact_notification_uuid(text_src)
    #x1B[1m#x1B[31mE   assert 'Notification... happen&");\n' == 'Notification... happen&");\n'#x1B[0m
    #x1B[1m#x1B[31mE     #x1B[0m
    #x1B[1m#x1B[31mE     Skipping 350 identical leading characters in diff, use -v to show#x1B[0m
    #x1B[1m#x1B[31mE     - .../sentry/issues/x/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &           - ^^^^        ^#x1B[0m
    #x1B[1m#x1B[31mE     + .../example/issues/201/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &            ^^^^^^        ^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * N+1 Query (4418 events, 3851 users)#x1B[0m
    #x1B[1m#x1B[31mE     -   http:.../sentry/issues/100/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                   - ^^^^#x1B[0m
    #x1B[1m#x1B[31mE     +   http:.../example/issues/100/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                    ^^^^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * SnailError: molly engaged wahoo feline hedgehog salmon novel boxer polecat (920 events, 5856 users)#x1B[0m
    #x1B[1m#x1B[31mE         http:.../example/issues/3/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * something bad happened (349 events, 6979 users)#x1B[0m
    #x1B[1m#x1B[31mE     -   http:.../sentry/issues/200/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                   - ^^^^#x1B[0m
    #x1B[1m#x1B[31mE     +   http:.../example/issues/200/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                    ^^^^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       Rule #2#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * DecidingKoiError: faithful hookworm top sharp filly merely sought trivially (8193 events, 9186 users)#x1B[0m
    #x1B[1m#x1B[31mE         http:.../example/issues/2/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * N+1 Query (4418 events, 3851 users)#x1B[0m
    #x1B[1m#x1B[31mE     -   http:.../sentry/issues/100/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                   - ^^^^#x1B[0m
    #x1B[1m#x1B[31mE     +   http:.../example/issues/100/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                    ^^^^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * N+1 Query (2057 events, 2439 users)#x1B[0m
    #x1B[1m#x1B[31mE     -   http:.../sentry/issues/101/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                   - ^^^^#x1B[0m
    #x1B[1m#x1B[31mE     +   http:.../example/issues/101/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                    ^^^^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * SnailError: molly engaged wahoo feline hedgehog salmon novel boxer polecat (920 events, 5856 users)#x1B[0m
    #x1B[1m#x1B[31mE         http:.../example/issues/3/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * something bad happened (349 events, 6979 users)#x1B[0m
    #x1B[1m#x1B[31mE     -   http:.../sentry/issues/200/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                   - ^^^^#x1B[0m
    #x1B[1m#x1B[31mE     +   http:.../example/issues/200/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                    ^^^^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       Rule #1#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * DecidingKoiError: faithful hookworm top sharp filly merely sought trivially (8193 events, 9186 users)#x1B[0m
    #x1B[1m#x1B[31mE         http:.../example/issues/2/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * something bad happened (5113 events, 5954 users)#x1B[0m
    #x1B[1m#x1B[31mE     -   http://testserver/orga.../sentry/issues/x/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                   - ^^^^        ^#x1B[0m
    #x1B[1m#x1B[31mE     +   http://testserver/orga.../example/issues/201/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                    ^^^^^^        ^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * N+1 Query (4418 events, 3851 users)#x1B[0m
    #x1B[1m#x1B[31mE     -   http:.../sentry/issues/100/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                   - ^^^^#x1B[0m
    #x1B[1m#x1B[31mE     +   http:.../example/issues/100/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                    ^^^^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * SnailError: molly engaged wahoo feline hedgehog salmon novel boxer polecat (920 events, 5856 users)#x1B[0m
    #x1B[1m#x1B[31mE         http:.../example/issues/3/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       * something bad happened (349 events, 6979 users)#x1B[0m
    #x1B[1m#x1B[31mE     -   http:.../sentry/issues/200/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                   - ^^^^#x1B[0m
    #x1B[1m#x1B[31mE     +   http:.../example/issues/200/&referrer=digest_email#x1B[0m
    #x1B[1m#x1B[31mE     &                                    ^^^^^^#x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       #x1B[0m
    #x1B[1m#x1B[31mE       Unsubscribe: javascript:alert("This is a preview page, what did you expect to happen&");#x1B[0m

@getsantry
Copy link
Contributor

getsantry bot commented Jun 29, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Jun 29, 2024
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from cadc564 to 73ab1b2 Compare July 1, 2024 17:14
@getsantry
Copy link
Contributor

getsantry bot commented Jul 23, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Jul 23, 2024
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from 73ab1b2 to 3250bd3 Compare July 23, 2024 13:30
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from 3250bd3 to d51a50f Compare August 7, 2024 21:56
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from d51a50f to a490c07 Compare August 8, 2024 15:32
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from a490c07 to 8b9a437 Compare August 8, 2024 19:18
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from 8b9a437 to b632a3e Compare August 8, 2024 19:25
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch 2 times, most recently from 3b4108f to b920c32 Compare August 8, 2024 19:36
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from b920c32 to f535e97 Compare August 9, 2024 20:58
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from f535e97 to a4166e5 Compare August 12, 2024 15:18
@asottile-sentry asottile-sentry force-pushed the asottile-fix-typing-digests-notifications branch from a4166e5 to ad5eee0 Compare August 12, 2024 15:28
@asottile-sentry asottile-sentry marked this pull request as ready for review August 12, 2024 15:34
@asottile-sentry asottile-sentry requested review from a team as code owners August 12, 2024 15:34
@asottile-sentry asottile-sentry requested review from a team August 12, 2024 15:35
nhsiehgit
nhsiehgit approved these changes Aug 12, 2024
View reviewed changes
src/sentry/digests/notifications.py
# sans-io implementation details
bound_records = _bind_records(records, groups, rules)
grouped = _group_records(bound_records, groups, rules)
return _sort_digest(grouped, event_counts=event_counts, user_counts=user_counts)
Copy link
Contributor

Choose a reason for hiding this comment

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

👨‍🍳 💋

@asottile-sentry asottile-sentry merged commit c31e19f into master Aug 13, 2024
50 checks passed
@asottile-sentry asottile-sentry deleted the asottile-fix-typing-digests-notifications branch August 13, 2024 13:07
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nhsiehgit nhsiehgit nhsiehgit approved these changes

Assignees
No one assigned
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@asottile-sentry @nhsiehgit