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

Throws a Runtime Error if team is already regstered. #139

Open
amar-sharma opened this issue May 29, 2021 · 6 comments
Open

Throws a Runtime Error if team is already regstered. #139

amar-sharma opened this issue May 29, 2021 · 6 comments
Labels

Comments

@amar-sharma
Copy link

02:09:59 web.1 | E, [2021-05-30T02:09:59.774355 #15140] ERROR -- : RuntimeError: Team Enlite is already registered. 02:09:59 web.1 | F:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/slack-ruby-bot-server-1.2.0/lib/slack-ruby-bot-server/api/endpoints/teams_endpoint.rb:95:in 'block (2 levels) in <class:TeamsEndpoint>'

If I want to let users re install the app with custom parameter, it can't handle that, the flow just crashes.

raise "Team #{team.name} is already registered." if team.active?
Shouldn't throw an error and let developer handle it in instance.on :error.

@dblock
Copy link
Collaborator

dblock commented May 31, 2021

I think you could just implement the re-install workflow and succeed in that case instead of raising an error?

@dblock dblock added the bug? label May 31, 2021
@amar-sharma
Copy link
Author

I've to re implement the whole teams_endpoint or have to maintain two redirect URLs? Not sure if it is ideal, I am wondering what that exception's purpose is? Because we would want new tokens to be updated in Teams model in case user re-installs?
Sorry if that doesn't make sense, new to Slack bot Dev.

@dblock
Copy link
Collaborator

dblock commented Jun 4, 2021

@amar-sharma

You definitely don't want to reimplement the endpoint and maintain 2 URLs, just get rid of the exception. But you can't remove it, its original intent is to avoid calling activate! which in RTM starts a new instance of a bot. If you do that, you'll end up with 2 connections both responding, which is ... bad. For RTM implementations you'd need to find a way to shutdown the existing connection, and start a new one, which is non-trivial, or raise as we do now but elsewhere. For event-based bots this is no longer useful because there's no "start" of any kind of worker and you could theoretically call activate! as many times as you want because it does nothing.

All of this needs to be tested and confirmed.

@amar-sharma
Copy link
Author

amar-sharma commented Jun 4, 2021

So the activate is only used for RTM, If I am not using RTM I can just get rid of activate! all together?
How about team.deactivate id if team.active? instead of raise and let it proceed?

@dblock
Copy link
Collaborator

dblock commented Jun 4, 2021

does not do what you think it does, it marks the team inactive, doesn't shutdown the connection, etc.

In your own code I think it's safe to just remove both the exception and activate!, but I would double-check. You should try and make this work in a PR here though or you'll have a hard time tracking this in the future since it's sort of an interface.

@amar-sharma
Copy link
Author

okay thank you for the response, I'll look into the code, find a way to reopen the connection and create a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants