-
Notifications
You must be signed in to change notification settings - Fork 291
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
Simplify SlackEventApiEndpointView
methods
#2384
Conversation
@@ -79,8 +82,6 @@ | |||
|
|||
logger = logging.getLogger(__name__) | |||
|
|||
SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID = "SELECT_ORGANIZATION_AND_ROUTE" |
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 couldn't find any usages, so removing it.
SlackEventApiEndpointView._get_organization_from_payload
SlackEventApiEndpointView
methods
|
||
try: | ||
slack_team_identity = SlackTeamIdentity.objects.get(slack_id=slack_team_id) | ||
except SlackTeamIdentity.DoesNotExist as e: | ||
logger.warning("Team identity not detected, that's dangerous!" + str(e)) |
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.
Already logged 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.
LGTM 👍
Up to you, but it might be useful adding a few small unit tests for _get_organization_from_payload
and _get_slack_team_identity_from_payload
to ensure they handle all the possible payload scenarios as you'd expect.
@joeyorlando Thank you for the review! I'm currently working on #2336 and it will introduce a few changes to the Slack view, so I'm planning to add tests on most of this logic in subsequent PRs. |
What this PR does
Simplifies
_get_organization_from_payload
and_get_slack_team_identity_from_payload
methods onSlackEventApiEndpointView
, so it's (hopefully) easier to grasp.Checklist
pr:no public docs
PR label added if not required)CHANGELOG.md
updated (orpr:no changelog
PR label added if not required)