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

New user can't execute bot command #74

Closed
jonathan-s opened this issue Apr 29, 2016 · 8 comments
Closed

New user can't execute bot command #74

jonathan-s opened this issue Apr 29, 2016 · 8 comments

Comments

@jonathan-s
Copy link

The bot is running and then a new user is being added to the team. When the new user is trying to execute the bot command he/she will trigger the KeyError because server.slack.server.users hasn't been updated when the user got added. -> https://github.com/llimllib/limbo/blob/master/limbo/limbo.py#L131

@llimllib
Copy link
Owner

ooh good catch, I hadn't considered that case!

Thanks so much for reporting, fixing it will be a top priority when I get time to work on this.

@StewPoll
Copy link
Contributor

I'd encountered this before but wasn't sire what caused it. I knew a
restart fixed it.

A similar issue happens when you add the bot to a new private group on the
first time its triggered.

Mind if I have a go at fixing this?

On Sat, 30 Apr 2016 02:20 Bill Mill notifications@github.com wrote:

ooh good catch, I hadn't considered that case!

Thanks so much for reporting, fixing it will be a top priority when I get
time to work on this.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#74 (comment)

@llimllib
Copy link
Owner

Not at all, have at it

@StewPoll
Copy link
Contributor

Looking at the offending code, it appears to be because slackrtm pulls the list of users originally and as such the server object has a static list of users in server.slack.server.users

When a new user enters, slackrtm doesn't update the list of users, which is giving the key error.

The immediately obvious way to fix it would be to make slackrtm update the users first when this KeyError occurs, try again and then if it happens again perform the debugging.

Having said that, what purpose does this line serve? (from limbo/limbo.py ln 131)

msguser = server.slack.server.users[event["user"]]

Can this be replaced with just msguser = event["user"]? As far as I can see this would server the same purpose and solve the issue, or is msguser used elsewhere that I can't find?

@llimllib
Copy link
Owner

Looking at the offending code, it appears to be because slackrtm pulls the list of users originally and as such the server object has a static list of users in server.slack.server.users

When a new user enters, slackrtm doesn't update the list of users, which is giving the key error.

Yup! So I think one part of fixing this is updating slackrtm to catch the team_join message and update the user roster. If it's promising to maintain a user roster, it ought to do so.

Having said that, what purpose does this line serve? (from limbo/limbo.py ln 131)

Very good question! It used to be there to make sure that we didn't get into a loop by responding to bot messages from ourself or from slackbot. However, slack has since added the bot_message subtype which prevents this problem, and I don't seem to have noticed that we didn't need it anymore.

It also relates to issue #40, which was the cause of me starting to check that every message has a user.

Can this be replaced with just msguser = event["user"]

I think this is the correct answer! We no longer need to check that the user is in the slackrtm roster, though we should fix that too because plugins could depend on that behavior.

@StewPoll
Copy link
Contributor

As msguser isn't used at all I've changed the code to the following

if "user" not in event:
        logger.debug("event {0} has no user".format(event))
        return

I'll make the PR soon.

@StewPoll
Copy link
Contributor

Ok, I deviated a bit and kept some of the code closer to what it was, and I've made slackrtm update at the same time as fixing this issue (and another issue)

llimllib pushed a commit to llimllib/slackrtm that referenced this issue May 14, 2016
Right now, new users are crashing limbo because they don't get added to
the user roster. See: llimllib/limbo#74

This commit fixes that issue, as well as moves the _pytest dir to tests
and adds tests for group join and team join.

closes #2
@llimllib
Copy link
Owner

Fixed in 6be28e4

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

No branches or pull requests

3 participants