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.

Sign up for GitHub

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

Jump to bottom

fix: slack auth model throwing an error #10991

Merged
merged 5 commits into from
Aug 6, 2024
Merged

fix: slack auth model throwing an error #10991

merged 5 commits into from
Aug 6, 2024

Conversation

IrakliJani
Copy link
Contributor

@IrakliJani IrakliJani commented Aug 2, 2024 •
edited
Loading

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

This PR deals with improving developer productivity by not relying on network errors on the Frontend and also not requesting /slack/channels endpoint if slack is not enabled.

Reviewer actions

  • I have manually tested the changes in the preview environment
  • I have reviewed the code
  • I understand that "request changes" will block this PR from merging

@owlas owlas requested a deployment to irakli/fix/slack-auth-model-throwing-an-error - jaffle_db_pg_13 PR #10991 August 2, 2024 12:26 — with Render Abandoned
@owlas owlas deployed to irakli/fix/slack-auth-model-throwing-an-error - headless-browser PR #10991 August 2, 2024 12:27 — with Render Active
@netlify Netlify
Copy link

netlify bot commented Aug 2, 2024 •
edited
Loading

✅ Deploy Preview for peaceful-bassi-cbf284 canceled.

Name Link
🔨 Latest commit a1ade49
🔍 Latest deploy log https://app.netlify.com/sites/peaceful-bassi-cbf284/deploys/66b2155eefeed30008860124

@owlas owlas temporarily deployed to irakli/fix/slack-auth-model-throwing-an-error - lightdash PR #10991 August 2, 2024 12:28 — with Render Destroyed
@IrakliJani IrakliJani force-pushed the irakli/fix/slack-auth-model-throwing-an-error branch from a2f807c to eb95852 Compare August 2, 2024 12:28
@owlas owlas deployed to irakli/fix/slack-auth-model-throwing-an-error - headless-browser PR #10991 August 2, 2024 12:28 — with Render Active
@IrakliJani IrakliJani requested review from a team and ZeRego and removed request for a team August 2, 2024 12:28
@IrakliJani IrakliJani self-assigned this Aug 2, 2024
@ZeRego
Copy link
Contributor

ZeRego commented Aug 5, 2024

@IrakliJani do you want to review this external PR that fixes the same problem?
#10716

@IrakliJani
Copy link
Contributor Author

@ZeRego I'll have a look, yes

@owlas owlas deployed to irakli/fix/slack-auth-model-throwing-an-error - headless-browser PR #10991 August 5, 2024 17:09 — with Render Active
@IrakliJani IrakliJani force-pushed the irakli/fix/slack-auth-model-throwing-an-error branch from eb95852 to e443663 Compare August 5, 2024 17:12
@owlas owlas deployed to irakli/fix/slack-auth-model-throwing-an-error - headless-browser PR #10991 August 5, 2024 17:12 — with Render Active
IrakliJani
IrakliJani commented Aug 5, 2024
View reviewed changes
packages/backend/src/clients/Slack/SlackClient.ts Outdated
Comment on lines 57 to 59
if (!installation) {
throw new Error('Could not find slack installation');
}
Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me 👍

packages/backend/src/clients/Slack/SlackClient.ts Outdated
Comment on lines 195 to 197
if (!installation) {
throw new Error('Could not find slack installation');
}
Copy link
Contributor Author

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.

packages/backend/src/clients/Slack/Slackbot.ts Outdated
Comment on lines 192 to 194
if (!installation) {
throw new Error('Could not find slack installation');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here

@IrakliJani IrakliJani mentioned this pull request Aug 5, 2024
Closed
3 tasks
maxdiplogit
maxdiplogit approved these changes Aug 5, 2024
View reviewed changes
Copy link
Contributor

@maxdiplogit maxdiplogit left a 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 👍

rephus
rephus reviewed Aug 6, 2024
View reviewed changes
packages/backend/src/clients/Slack/SlackClient.ts Outdated
@@ -54,10 54,16 @@ export class SlackClient {
organizationUuid,
);

return new WebClient(installation?.token);
if (!installation) {
throw new Error('Could not find slack installation');
Copy link
Collaborator

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 ?

rephus
rephus approved these changes Aug 6, 2024
View reviewed changes
Copy link
Collaborator

@rephus rephus left a 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 👍

@IrakliJani IrakliJani force-pushed the irakli/fix/slack-auth-model-throwing-an-error branch from e443663 to f7fdfe0 Compare August 6, 2024 12:14
@owlas owlas temporarily deployed to irakli/fix/slack-auth-model-throwing-an-error - lightdash PR #10991 August 6, 2024 12:14 — with Render Destroyed
@owlas owlas deployed to irakli/fix/slack-auth-model-throwing-an-error - headless-browser PR #10991 August 6, 2024 12:14 — with Render Active
@owlas owlas temporarily deployed to irakli/fix/slack-auth-model-throwing-an-error - headless-browser PR #10991 August 6, 2024 12:21 — with Render Destroyed
@owlas owlas temporarily deployed to irakli/fix/slack-auth-model-throwing-an-error - lightdash PR #10991 August 6, 2024 12:21 — with Render Destroyed
@IrakliJani
Copy link
Contributor Author

@all-contributors add @maxdiplogit for code

@allcontributors allcontributors bot mentioned this pull request Aug 6, 2024
Merged
@allcontributors AllContributors
Copy link
Contributor

@IrakliJani

I've put up a pull request to add @maxdiplogit! 🎉

@IrakliJani IrakliJani merged commit 03579bb into main Aug 6, 2024
45 of 52 checks passed
@IrakliJani IrakliJani deleted the irakli/fix/slack-auth-model-throwing-an-error branch August 6, 2024 12:54
@IrakliJani
Copy link
Contributor Author

thank you @maxdiplogit for your contribution! 😻

lightdash-bot pushed a commit that referenced this pull request Aug 6, 2024
@semantic-release-bot
chore(release): 0.1202.17
909f2c0 
## [0.1202.17](0.1202.16...0.1202.17) (2024-08-06)

### Bug Fixes

* slack auth model throwing an error ([#10991](#10991)) ([03579bb](03579bb))
@lightdash-bot
Copy link
Collaborator

🎉 This PR is included in version 0.1202.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

@maxdiplogit
Copy link
Contributor

thank you @maxdiplogit for your contribution! 😻

Thank you for the support @IrakliJani 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rephus rephus rephus approved these changes

@ZeRego ZeRego ZeRego approved these changes

@maxdiplogit maxdiplogit maxdiplogit approved these changes

Assignees

@IrakliJani IrakliJani

Labels
released
Projects
Selected for Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slack network errors
6 participants
@IrakliJani @ZeRego @lightdash-bot @maxdiplogit @rephus @owlas