-
Notifications
You must be signed in to change notification settings - Fork 397
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 auth model throwing an error #10991
fix: slack auth model throwing an error #10991
Conversation
✅ Deploy Preview for peaceful-bassi-cbf284 canceled.
|
a2f807c
to
eb95852
Compare
@IrakliJani do you want to review this external PR that fixes the same problem? |
@ZeRego I'll have a look, yes |
eb95852
to
e443663
Compare
if (!installation) { | ||
throw new Error('Could not find slack installation'); | ||
} |
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.
the above model call getInstallationFromOrganizationUuid
was throwing before, so im preserving the behavior 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.
Looks good to me 👍
if (!installation) { | ||
throw new Error('Could not find slack installation'); | ||
} |
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.
same here. this should throw.
if (!installation) { | ||
throw new Error('Could not find slack installation'); | ||
} |
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.
same 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.
Changes acknowledged and approved 👍
@@ -54,10 54,16 @@ export class SlackClient { | |||
organizationUuid, | |||
); | |||
|
|||
return new WebClient(installation?.token); | |||
if (!installation) { | |||
throw new Error('Could not find slack installation'); |
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 use an extended LightdashError
instead of a generic 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.
Tested locally, works as expected 👍
e443663
to
f7fdfe0
Compare
@all-contributors add @maxdiplogit for code |
I've put up a pull request to add @maxdiplogit! 🎉 |
thank you @maxdiplogit for your contribution! 😻 |
## [0.1202.17](0.1202.16...0.1202.17) (2024-08-06) ### Bug Fixes * slack auth model throwing an error ([#10991](#10991)) ([03579bb](03579bb))
🎉 This PR is included in version 0.1202.17 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Thank you for the support @IrakliJani 😊 |
Closes: #10473
Closes: #10716
Description:
if organization does not have a slack integration it throws an error when visiting integrations page and returns error 500
two routes were affected by this:
/api/v1/slack/
/api/v1/slack/channels
you can see whole bunch of sentry events here:
https://lightdash.sentry.io/issues/4521767626/events/?project=5959292&sort=-url
THIS PR IS BASED on contributions from @maxdiplogit -> #10716
Reviewer actions