Skip to content
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

fix(slack): don't raise RuntimeError if unable to fetch property in slack request #67798

Merged
merged 3 commits into from
May 15, 2024

Conversation

cathteng
Copy link
Member

@cathteng cathteng commented Mar 27, 2024

Resolves #67679

We are currently raising a RuntimeError if the channel_id, team_id, or user_id does not exist in an incoming Slack request. We use team_id and user_id here:

try:
team_id = self.team_id
user_id = self.user_id
except Exception:
pass
context = integration_service.get_integration_identity_context(
integration_provider="slack",
integration_external_id=team_id,
identity_external_id=user_id,
identity_provider_external_id=team_id,
)

And these values can be None, so we don't need to be raising a RuntimeError:

def get_integration_identity_context(
self,
*,
integration_provider: str | None = None,
integration_external_id: str | None = None,
identity_external_id: str | None = None,
identity_provider_external_id: str | None = None,
) -> RpcIntegrationIdentityContext:

NOTE: Not yet sure if it's okay for channel_id to be None. We can remove that part if need be.

Replacing get_field_id which raises the error with _get_field_id_option which can return None should fix this.

@cathteng cathteng requested a review from a team March 27, 2024 17:19
@cathteng cathteng requested a review from a team as a code owner March 27, 2024 17:19
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 27, 2024
@cathteng cathteng marked this pull request as draft March 27, 2024 17:24
Copy link
Member

@ykamo001 ykamo001 left a comment

Choose a reason for hiding this comment

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

Let's make sure this doesn't break our async process where we try to get the team/channel id when the user only passes in team/channel name

@@ -115,20 +115,20 @@ def integration(self) -> RpcIntegration:
return self._integration

@property
def channel_id(self) -> str:
def channel_id(self) -> str | None:
return get_field_id(self.data, "channel")
Copy link
Member

Choose a reason for hiding this comment

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

could we use _get_field_id_option here as well, or no? Wonder why it's different

Copy link
Member Author

Choose a reason for hiding this comment

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

we always expect a channel. i forgot to undo the type change

@getsantry
Copy link
Contributor

getsantry bot commented Apr 23, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Apr 23, 2024
@kimxogus
Copy link

I'm waiting for this

@cathteng cathteng removed the Stale label Apr 23, 2024
@getsantry getsantry bot added the Stale label May 15, 2024
@getsantry
Copy link
Contributor

getsantry bot commented May 15, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@kimxogus
Copy link

Please merge this PR 🙏

@cathteng cathteng merged commit 62d8fcb into master May 15, 2024
54 checks passed
@cathteng cathteng deleted the cathy/slack/get_field_id_option branch May 15, 2024 16:30
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2024
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 Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuntimeError in slack action
4 participants