chore(slack): reorganize unfurl SLOs #102500
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #102500 +/- ##
===========================================
+ Coverage 80.89% 80.91% +0.01%
===========================================
Files 8847 8850 +3
Lines 390072 390290 +218
Branches 24812 24812
===========================================
+ Hits 315559 315788 +229
+ Misses 74148 74137 -11
Partials 365 365 |
cathteng
changed the title
| ois = integration_service.get_organization_integrations( | ||
| integration_id=slack_request.integration.id, limit=1 | ||
| ) | ||
| organization_id = ois[0].organization_id if len(ois) > 0 else None | ||
| organization_context = ( | ||
| organization_service.get_organization_by_id( | ||
| id=organization_id, | ||
| user_id=None, | ||
| include_projects=False, | ||
| include_teams=False, | ||
| ) | ||
| if organization_id | ||
| else None | ||
| ) | ||
| organization = organization_context.organization if organization_context else None |
There was a problem hiding this comment.
Is this organization ID actually useful for us? We're only pulling the first org ID, which may or may not even belong to this region.
There was a problem hiding this comment.
The only thing it's used for is to determine if somebody is pasting a discover link and asking them to link their identity if necessary
sentry/src/sentry/integrations/slack/webhooks/event.py
Lines 214 to 221 in 0f5e0de
I'm not sure of a good replacement for this logic that doesn't use organization
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| # Link can't be unfurled | ||
| if link_type is None or args is None: | ||
| lifecycle.record_halt("Unfurlable link", extra={"url": url}) |
There was a problem hiding this comment.
Bug: Cannot unfurl: misleading halt message under None conditions
The halt message "Unfurlable link" is misleading. This message is recorded when link_type is None or args is None, which means the link CANNOT be unfurled. The message should say something like "Cannot unfurl link" or "Non-unfurlable link" to accurately reflect the condition being checked.
GabeVillalobos
left a comment
GabeVillalobos
left a comment
There was a problem hiding this comment.
Changes seem fine, but we probably want some more refactors in the future to handle more of these error cases
| ) | ||
| url = item["url"] | ||
| except Exception: | ||
| lifecycle.record_failure("Failed to parse link", extra={**logger_params}) |
There was a problem hiding this comment.
This try/except is sort of weird. Realistically, this should only ever be a key error, and we can probably handle it a bit more gracefully with a None check 🤔
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
How unfurl works:
link_sharedwebhook, which can contain multiple linkschat.unfurlendpoint to display the payload for the message