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

Errbot doesn't recognise you're talking to it when you use its name #86

Open
colincoghill opened this issue Sep 28, 2022 · 4 comments
Open

Comments

@colincoghill
Copy link

colincoghill commented Sep 28, 2022

We recently switched from the old slackclient to the slackv3 backend and it generally works pretty well, however:

when we try to issue errbot a command by mentioning it @errbot help instead of with the prefix !help it doesn't
recognize that it's being spoken to and doesn't respond.

I think the fault is in line 173 of slackv3.py:

converted_prefixes = []
for prefix in bot_prefixes:
try:
converted_prefixes.append(f"<@{self.username_to_userid(prefix)}>")
except Exception as e:
log.error(
f'Failed to look up Slack userid for alternate prefix "{prefix}": {str(e)}'
)
self.bot_alt_prefixes = tuple(
x.lower() for x in self.bot_config.BOT_ALT_PREFIXES
)

If I change it to: self.bot_alt_prefixes = tuple(x.lower() for x in converted_prefixes) we can talk to errbot again.

@nzlosh
Copy link
Collaborator

nzlosh commented Sep 29, 2022

@colincoghill @samhopwell Thank you both for offering fixes for this issue.

For a bit of history, this function was brought directly over from the slack backend. After looking at it closer there are a few issues with it.

  1. The function doesn't use the configured BOT_ALT_PREFIX_SEPARATORS variable to split.
    https://github.com/errbotio/errbot/blob/890b28e433b2d9f50238467cd639476067d6f7a2/errbot/bootstrap.py#L39
    1.1 Attempting to split BOT_ALT_PREFIX doesn't make sense when it's expected to be a list type.
        # convert BOT_ALT_PREFIXES to a list
        try:
            bot_prefixes = self.bot_config.BOT_ALT_PREFIXES.split(",")
        except AttributeError:
            bot_prefixes = list(self.bot_config.BOT_ALT_PREFIXES)

I question if this code should be completely removed.

  1. The alternative prefix is meant to be able to support arbitrary text that can be set as case sensitive or not. https://github.com/errbotio/errbot/blob/890b28e433b2d9f50238467cd639476067d6f7a2/errbot/bootstrap.py#L40

This means that if the slackv3 backend is going to process @ mentions to inject slack usernames into the alternative prefix list, it will need to handle plain text (case sensitive and insensitive) as well as @ mentions.

Would either or both of you be interested in taking a crack at implementing that? If you're interested in working on a solution together, there's the gitter errbot community channel that you could use to communicate directly. (timezones overlap permitting)

@colincoghill
Copy link
Author

The function doesn't use the configured BOT_ALT_PREFIX_SEPARATORS variable to split.

I got the impression this was for during conversation, errbot, hello rather than when it's parsing the config list of alt prefixes.

@colincoghill
Copy link
Author

colincoghill commented Oct 12, 2022

The alternative prefix is meant to be able to support arbitrary text that can be set as case sensitive or not.

I suppose the difficult part here is that because slack uses the "userid" now, instead of the username, the thing errbot (core.py) is comparing against is the user id ("@U10234") instead of the original username, so case insensitivity isn't really helpful.

We'd have to override process_message() which feels messy.

@nzlosh
Copy link
Collaborator

nzlosh commented Oct 12, 2022

I dug more into the code and you're quite right about the BOT_ALT_PREFIX_SERPARATORS.

From what I can tell, the self.bot_config.BOT_ALT_PREFIXES.split(",") is to catch configurations that defined a single string tuple ("alt_prefix") which becomes a single string "alt_prefix".

I also agree that trying to override process_message is not an ideal path. I can think of some common use cases for BOT_ALT_PREFIXES that errbot should be able to support:

  1. Alternative characters ("!", ".", ":") to trigger commands !help, .help :help.
  2. Bot Aliases ("jim", "james", "jimmy", "j", "jay") to trigger commands Jim help or jay help
  3. Mentions ("@someone", "U2345") to trigger @someone help or @someone_else help

In the 2nd case, the notion of case sensitive/insensitive is a valid and useful feature. Especially when a backend other than slack is being used.

I ended coming up with this as a way to update the alternate prefixes. Do you want to give it a try on your end?

from errbot.backends.base import (
    UserNotUniqueError,
    UserDoesNotExistError,
)

    def update_alternate_prefixes(self):
        """Converts BOT_ALT_PREFIXES to use the slack ID instead of name

        Slack only acknowledges direct callouts `@username` in chat if referred
        by using the ID of that user.
        """
        # convert BOT_ALT_PREFIXES to a list
        try:
            bot_prefixes = self.bot_config.BOT_ALT_PREFIXES.split(",")
        except AttributeError:
            bot_prefixes = list(self.bot_config.BOT_ALT_PREFIXES)

        converted_prefixes = []
        for prefix in bot_prefixes:
            try:
                new_prefix = self.username_to_userid(prefix)
                if new_prefix != prefix:
                    new_prefix = f"<@{new_prefix}>"
            except UserDoesNotExistError as e:
                new_prefix = prefix
                log.warning('"%s" was not found: %s', prefix, str(e))
            except UserNotUniqueError as e:
                log.warning("Can't add prefix because multiple users were found using %s", prefix)
                continue

            if self.bot_config.BOT_ALT_PREFIX_CASEINSENSITIVE:
                new_prefix = new_prefix.lower()
            converted_prefixes.append(new_prefix)
        
        self.bot_alt_prefixes = tuple(converted_prefixes)

        log.debug(f"Converted bot_alt_prefixes: {self.bot_config.BOT_ALT_PREFIXES} to {self.bot_alt_prefixes}")

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

2 participants