chore(slack): actually type issues message builder correctly #75212

cathteng · 2024-07-29T23:32:35Z

and don't break Slack in the process

Re-up of #74876

sentry-io · 2024-07-29T23:32:48Z

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: src/sentry/integrations/slack/message_builder/issues.py

Function	Unhandled Issue
`build`	TypeError: 'NoneType' object cannot be interpreted as an integer sentry.tasks.activity.send_activi... `Event Count:` 19
`build`	Actor.InvalidActor: Cannot find a User with id=2493315 sentry.tasks.digests.deliver_... `Event Count:` 2

_{Did you find this useful? React with a 👍 or 👎}

codecov · 2024-07-30T00:06:29Z

Codecov Report

Attention: Patch coverage is 89.13043% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.22%. Comparing base (3e9875e) to head (9b4549d).
Report is 3 commits behind head on master.

Files	Patch %	Lines
...entry/integrations/slack/message_builder/issues.py	84.00%	1 Missing and 3 partials ⚠️
src/sentry/integrations/message_builder.py	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #75212   +/-   ##
=======================================
  Coverage   78.21%   78.22%           
=======================================
  Files        6775     6775           
  Lines      302013   301997   -16     
  Branches    51963    51956    -7     
=======================================
+ Hits       236217   236226    +9     
+ Misses      59443    59417   -26     
- Partials     6353     6354    +1

Files	Coverage Δ
...y/integrations/slack/message_builder/base/block.py	`94.05% <ø> (ø)`
...sentry/integrations/slack/webhooks/options_load.py	`96.36% <100.00%> (+0.20%)`	⬆️
src/sentry/notifications/utils/participants.py	`91.50% <100.00%> (+0.03%)`	⬆️
src/sentry/utils/committers.py	`91.71% <100.00%> (ø)`
src/sentry/integrations/message_builder.py	`88.00% <85.71%> (-0.80%)`	⬇️
...entry/integrations/slack/message_builder/issues.py	`92.26% <84.00%> (+6.30%)`	⬆️

... and 10 files with indirect coverage changes

src/sentry/integrations/slack/message_builder/base/block.py

GabeVillalobos · 2024-07-30T21:12:28Z

src/sentry/integrations/slack/message_builder/issues.py

    """Format the release tag using the short version and make it a link"""
+    if not event:
+        return ""


Would it be better if we return None here instead of ""? Otherwise the caller will have to perform falsey checks to validate whether this was successful or not.

this will show up as a release tag like the below, i believe None gets rendered as "null" when dumped into json. i'm not sure which is preferable tbh

src/sentry/utils/committers.py

src/sentry/integrations/slack/webhooks/options_load.py

asottile-sentry

schew2381

for good measure

asottile-sentry · 2024-08-01T20:26:53Z

oops looks like this got caught in a merge race:

src/sentry/integrations/msteams/card_builder/issues.py:187: error: Argument 1 to "format_actor_options" has incompatible type "BaseQuerySet[Team, Team]"; expected "Sequence[Team | RpcUser]"  [arg-type]
src/sentry/integrations/slack/message_builder/issues.py:240: error: Argument 1 to "format_actor_options" has incompatible type "BaseQuerySet[Team, Team]"; expected "Sequence[Team | RpcUser]"  [arg-type]

getsentry-bot · 2024-08-01T20:27:20Z

PR reverted: d2f0d29

…75212)" This reverts commit a5885d4. Co-authored-by: asottile-sentry <103459774+asottile-sentry@users.noreply.github.com>

asottile-sentry · 2024-08-01T20:27:25Z

src/sentry/integrations/message_builder.py

@@ -20,17 +20,17 @@


 def format_actor_options(
-    actors: Iterable[Team | RpcUser], use_block_kit: bool = False
+    actors: Sequence[Team | RpcUser], is_slack: bool = False


this particular line -- the Iterable should have stayed -- probably a merge conflict sorry about that!

oopsie, thanks for letting me know

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 29, 2024

vercel bot deployed to Preview July 29, 2024 23:34 View deployment

vercel bot deployed to Preview July 30, 2024 19:00 View deployment

cathteng force-pushed the cathy/slack/type-issues-message-builder branch from 162860a to e34a292 Compare July 30, 2024 19:01

vercel bot deployed to Preview July 30, 2024 19:05 View deployment

GabeVillalobos reviewed Jul 30, 2024

View reviewed changes

cathteng added 13 commits July 30, 2024 15:10

correctly type issue message builder

e929389

fix tests

a98eaf2

fix typing lol

90c8bd5

fix typing again oops

ad3a6b2

remove unused code

8fddf32

test releases tag

4ab00ff

fix typing

d8230f7

just use isinstance

3c3097f

change use_block_kit to is_slack

2cc66fd

return new option group type

bc7f41d

fix what broke slack

ef572f6

fix typing

b0267ff

fix typing

9b4549d

cathteng force-pushed the cathy/slack/type-issues-message-builder branch from e34a292 to 9b4549d Compare July 30, 2024 22:14

vercel bot deployed to Preview July 30, 2024 22:17 View deployment

cathteng marked this pull request as ready for review July 30, 2024 22:51

cathteng requested review from a team as code owners July 30, 2024 22:51

cathteng requested a review from a team July 30, 2024 22:51

asottile-sentry approved these changes Jul 31, 2024

View reviewed changes

cathteng requested a review from GabeVillalobos July 31, 2024 16:13

iamrajjoshi approved these changes Jul 31, 2024

View reviewed changes

GabeVillalobos approved these changes Jul 31, 2024

View reviewed changes

schew2381 approved these changes Jul 31, 2024

View reviewed changes

cathteng merged commit a5885d4 into master Aug 1, 2024
49 of 50 checks passed

cathteng deleted the cathy/slack/type-issues-message-builder branch August 1, 2024 20:06

asottile-sentry added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Aug 1, 2024

getsentry-bot added a commit that referenced this pull request Aug 1, 2024

Revert "chore(slack): actually type issues message builder correctly (#…

d2f0d29

…75212)" This reverts commit a5885d4. Co-authored-by: asottile-sentry <103459774+asottile-sentry@users.noreply.github.com>

asottile-sentry reviewed Aug 1, 2024

View reviewed changes

cathteng mentioned this pull request Aug 1, 2024

fix(slack): type issues message builder correctly attempt #3 #75484

Merged

github-actions bot locked and limited conversation to collaborators Aug 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(slack): actually type issues message builder correctly #75212

chore(slack): actually type issues message builder correctly #75212

cathteng commented Jul 29, 2024

sentry-io bot commented Jul 29, 2024

codecov bot commented Jul 30, 2024 •

edited

Loading

GabeVillalobos Jul 30, 2024

cathteng Jul 30, 2024

asottile-sentry left a comment

schew2381 left a comment

asottile-sentry commented Aug 1, 2024

getsentry-bot commented Aug 1, 2024

asottile-sentry Aug 1, 2024

cathteng Aug 1, 2024

chore(slack): actually type issues message builder correctly #75212

chore(slack): actually type issues message builder correctly #75212

Conversation

cathteng commented Jul 29, 2024

sentry-io bot commented Jul 29, 2024

🔍 Existing Issues For Review

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

GabeVillalobos Jul 30, 2024

Choose a reason for hiding this comment

cathteng Jul 30, 2024

Choose a reason for hiding this comment

asottile-sentry left a comment

Choose a reason for hiding this comment

schew2381 left a comment

Choose a reason for hiding this comment

asottile-sentry commented Aug 1, 2024

getsentry-bot commented Aug 1, 2024

asottile-sentry Aug 1, 2024

Choose a reason for hiding this comment

cathteng Aug 1, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2024 •

edited

Loading