slack-ruby-bot-server icon indicating copy to clipboard operation
slack-ruby-bot-server copied to clipboard

Throws a Runtime Error if team is already regstered.

Open amar-sharma opened this issue 4 years ago • 6 comments

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.

https://github.com/slack-ruby/slack-ruby-bot-server/blob/99e91edbc3092dc9f9665fb8c9be3eebff3252f1/lib/slack-ruby-bot-server/api/endpoints/teams_endpoint.rb#L95 Shouldn't throw an error and let developer handle it in instance.on :error.

amar-sharma avatar May 29 '21 20:05 amar-sharma

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

dblock avatar May 31 '21 14:05 dblock

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.

amar-sharma avatar Jun 03 '21 13:06 amar-sharma

@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.

dblock avatar Jun 04 '21 14:06 dblock

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?

amar-sharma avatar Jun 04 '21 15:06 amar-sharma

https://github.com/slack-ruby/slack-ruby-bot-server/blob/99e91edbc3092dc9f9665fb8c9be3eebff3252f1/lib/slack-ruby-bot-server/models/team/methods.rb#L15 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.

dblock avatar Jun 04 '21 17:06 dblock

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

amar-sharma avatar Jun 05 '21 07:06 amar-sharma