-
Notifications
You must be signed in to change notification settings - Fork 415
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
Handle soft deleted integrations #2217
Handle soft deleted integrations #2217
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
api/integrations/slack/views.py
Outdated
@@ -81,8 +81,8 @@ def slack_oauth_callback(self, request, environment_api_key): | |||
code, self._get_slack_callback_url(environment_api_key) | |||
) | |||
|
|||
SlackConfiguration.objects.update_or_create( | |||
project=env.project, defaults={"api_token": bot_token} | |||
SlackConfiguration.objects.all_with_deleted().update_or_create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create a test case that covers this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fought with this for a long time to write a test that makes sense but, without refactoring all this code into a service layer, there is no easy (not hacky) way to test it. In doing this though, I realised that there is currently no way to delete a SlackConfiguration object anyway so, the way I see, the pragmatic solutions are:
- Keep the change I made just in case it happens in the future, but accept there is no test case to cover it
- Just revert this change so we are at least alerted via Sentry if it does happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer 2 because it should not happen under normal circumstances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, ok, I've done that.
Changes
Refactors all integration models to upsert upsert if there is a soft deleted integration that already exists.
How did you test this code?
Added unit tests.