Skip to content

fix(integrations)-org-in-url-notify-disable #54234

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

Merged
merged 6 commits into from
Aug 7, 2023
Merged

Conversation

chloeho7
Copy link
Contributor

@chloeho7 chloeho7 commented Aug 4, 2023

Fix for #53533
Replace URL for notify when integrations are disabled with: /settings/sentry/developer-settings/just-a-test-eb81a3/
Currently: /settings/developer-settings/projects/just%20a%20test/
Part of Notify on Disabled Integration project

@chloeho7 chloeho7 requested a review from a team as a code owner August 4, 2023 21:53
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 4, 2023
integration.slug if hasattr(integration, "slug") else integration.provider,
)

integration_name = (
Copy link
Contributor

Choose a reason for hiding this comment

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

@chloeho7 can we expose methods on the SentryApp and RpcIntegration to return the slug/name so this function doesn't have to have this kind of logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, now using .get_provider() from RpcIntegration instead of hasattr()

Copy link
Member

Choose a reason for hiding this comment

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

Damn typing really doesn't like this, it'll have to be rewritten a bit.

f"/settings/{self.organization.slug}/integrations/{self.integration.provider}"
)
in msg.body
)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test to make sure the URL is correct for sentry apps? You can add it in #53522 after this is merged and you rebase

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #54234 (3cda2e9) into master (a1c6ee4) will increase coverage by 0.01%.
Report is 38 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 3cda2e9 differs from pull request most recent head 8643f15. Consider uploading reports for the commit 8643f15 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #54234      +/-   ##
==========================================
+ Coverage   79.63%   79.64%   +0.01%     
==========================================
  Files        4982     4985       +3     
  Lines      211115   211381     +266     
  Branches    35964    36026      +62     
==========================================
+ Hits       168111   168357     +246     
- Misses      37837    37848      +11     
- Partials     5167     5176       +9     
Files Changed Coverage Δ
src/sentry/integrations/notify_disable.py 72.72% <100.00%> (ø)

... and 34 files with indirect coverage changes

@chloeho7 chloeho7 requested a review from ceorourke August 7, 2023 16:37
@chloeho7 chloeho7 requested a review from ceorourke August 7, 2023 19:02
@chloeho7 chloeho7 enabled auto-merge (squash) August 7, 2023 19:05
@chloeho7 chloeho7 requested review from scefali and removed request for scefali August 7, 2023 19:53
@chloeho7 chloeho7 merged commit a0cdd08 into master Aug 7, 2023
@chloeho7 chloeho7 deleted the chloe/notify-on-disable branch August 7, 2023 20:23
@chloeho7 chloeho7 restored the chloe/notify-on-disable branch August 7, 2023 23:12
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants