-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
ref(slack): Refactor Link Identity View #72792
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #72792 +/- ##
========================================
Coverage 78.05% 78.05%
========================================
Files 6601 6605 +4
Lines 294319 294515 +196
Branches 50753 50763 +10
========================================
+ Hits 229725 229886 +161
- Misses 58358 58383 +25
- Partials 6236 6246 +10
|
except IntegrityError: | ||
_logger.exception("slack.link.integrity_error") | ||
metrics.incr( | ||
self._METRICS_FAILURE_KEYKEY + ".post.identity.integrity_error", |
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.
might be a typo here
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.
fixed
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.
unblocking as you add more changes, functionally looks solid atm!
params.update({"organization": organization, "integration": integration, "idp": idp}) | ||
return super().dispatch(request, params=params) |
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.
do we need to update params or could we just pass them into super().dispatch() as specific key word arguments? super().dispatch(org=organization)
params = IdentityParams( | ||
organization=params_dict["organization"], | ||
integration=params_dict["integration"], | ||
idp=params_dict["idp"], | ||
slack_id=params_dict["slack_id"], | ||
channel_id=params_dict["channel_id"], | ||
) |
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 do any key error catches here?
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
https://www.notion.so/sentry/Slack-ff5b8ab4fd1e4760ab829438ce97ce15
Addresses
bot-3
task.Here, I refactored the view to have two separate methods for get and post. I added bunch of logging and metrics & improved tests.