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

chore(slack): move some floating functions into SlackService #73695

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Jul 2, 2024

Move _get_attachments, _notify_recipient, and _send_message_to_slack_channel under SlackService, which were previously floating functions. We should centralize Slack logic where possible.

Also fixes typing for SlackBlock and SlackAttachment (which are literally the same thing), we should be using dict since we're mutating the result in get_attachments (typing is enforced in its home).

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

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

Project coverage is 78.12%. Comparing base (6b1b5cd) to head (31c972e).
Report is 37 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #73695       +/-   ##
===========================================
+ Coverage   56.99%   78.12%   +21.13%     
===========================================
  Files        6642     6651        +9     
  Lines      297290   297594      +304     
  Branches    51155    51204       +49     
===========================================
+ Hits       169446   232506    +63060     
+ Misses     123101    58777    -64324     
- Partials     4743     6311     +1568     
Files Coverage Δ
...try/integrations/slack/message_builder/__init__.py 100.00% <100.00%> (ø)
...y/integrations/slack/message_builder/base/block.py 94.05% <100.00%> (+53.28%) ⬆️
src/sentry/integrations/slack/notifications.py 97.29% <100.00%> (+53.39%) ⬆️
...try/notifications/additional_attachment_manager.py 80.64% <100.00%> (+16.12%) ⬆️
...rc/sentry/tasks/integrations/slack/post_message.py 87.50% <60.00%> (+25.96%) ⬆️
src/sentry/integrations/slack/service.py 93.52% <95.31%> (+58.34%) ⬆️

... and 1993 files with indirect coverage changes

@cathteng cathteng force-pushed the cathy/slack/move-floating-functions branch from 9836883 to 31c972e Compare July 3, 2024 16:26
@cathteng cathteng marked this pull request as ready for review July 3, 2024 17:28
@cathteng cathteng requested review from a team as code owners July 3, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant