Room-11 / Jeeves

Chatbot for Stack Overflow
MIT License
42 stars 19 forks source link

Add ability to redirect commands to other users #26

Open PeeHaa opened 8 years ago

PeeHaa commented 8 years ago

It would be nice to have a way to redirect commands to other users.

This prevent people from having to run the command and ping the person afterwards like here:

Danack - !!package rdlowrey/auryn

Jeeves - [ rdlowrey/auryn ] Auryn is a dependency injector for bootstrapping object-oriented PHP applications.

Danack - @someFolk I'd suggest checking that out.

gooh commented 8 years ago

This would result in double pings, wouldn't it?

Danack - !!package(@PeeHaa) rdlowrey/auryn

Jeeves - @PeeHaa [ rdlowrey/auryn ] Auryn is a dependency injector for bootstrapping object-oriented PHP applications.

PeeHaa commented 8 years ago

Yeah that would double ping

gooh commented 8 years ago

Can we find a way that wouldn't double ping the receiver?

PeeHaa commented 8 years ago

JS does !!tell name !!command [params] I think:

!!tell gooh !!command
kelunik commented 8 years ago

I think that's fine.

DaveRandom commented 8 years ago

I love how you pinged some github randomer with this thread...

The main issue I see with !!tell is that it won't tab-complete, which is often an issue with visiting users for me as I only have a UK keyboard and some of those bastards use non ascii chars in their names. As if languages other than English exist or something.

If I can't tab complete I probably won't use it and I suspect that's true for others as well, so maybe a browser ext or user script to work with it would be in order?

PeeHaa commented 8 years ago

I am thinking trying to find the best match based on http://chat.stackoverflow.com/rooms/pingable/11 somehow.

Will investigate the feasibility.

Nessworthy commented 6 years ago

TL;DR is that using !!tell to target a message that might not exist isn't possible. Pinging users directly is possible but only with some MAJOR tomfoolery AND the caveat that multiline messages are prefixed with an additional line and multiline preformatted text just doesn't work with pings. I wouldn't recommend.

The resolution of a chat command works but the tell part seems like it can't be done - the chat client is already injected into builtin + plugin commands so we can't override that to force the message to be a reply or @ ping instead.

If we're talking redirecting direct replies by overriding the original poster then permissions are also a problem - rerouting the message would make it look like the person it has routed to was the original sender, allowing unprivileged users to exploit admin only commands e.g. !!tell peeHaa plugin disable php.RFC

You could override Chat\Command's getId to point at the target's last message but what if they don't have one, and if they did, how hard would it be to have to crawl back through the room's history to find it?

I tested creating new PluginManager / BuiltInManager instances within the tell plugin & registering new instances of plugins specifically for !!tell - the tell plugin then had control over the ChatClient passed to those plugins by leveraging Auryn to override what was injected as the chatClient argument. Plugins could be a white list of existing plugins defined separately in config.yml. The code looks mental and it sort of worked but it also highlighted how multiline responses look messed up when prefixing with @ ping.

Additionally, forcing an @ ping override with the PostFlag then switches it on across all text content in the response - not great. A solution would be to maybe have a separate flag to allow a single ping at the beginning of a message (useful in general) but it still doesn't resolve the formatting issues on things like !!tell user uptime. I guess a fix for that would be to detect if it's multiline and start the ping on the first line, then immediately break. However even that doesn't work with things like !!uptime where it returns as a preformatted block - pings don't work there!

Nessworthy commented 6 years ago

Here's an absolutely stupid proof of concept which also involves modification of StackChat code to add and handle the ALLOW_SINGLE_PING Post Flag. Auryn also needed to be told to share itself, and notice the way BuiltInActionManager had to be instantiated because it is shared.


<?php declare(strict_types = 1);

namespace Room11\Jeeves\Plugins;

use Amp\Promise;
use Amp\Success;
use Auryn\Injector;
use Psr\Log\LoggerInterface;
use Room11\Jeeves\BuiltIn\Commands\Version;
use Room11\Jeeves\Chat\Command;
use Room11\Jeeves\Chat\RoomStatusManager;
use Room11\Jeeves\Storage\Ban;
use Room11\Jeeves\System\BuiltInActionManager;
use Room11\Jeeves\System\PluginCommandEndpoint;
use Room11\Jeeves\System\PluginManager;
use Room11\StackChat\Client\ChatClient;
use Room11\StackChat\Client\Client as ChatClientInterface;
use Room11\StackChat\Client\PostFlags;

class DumbChatClient extends ChatClient
{
    private $username;

    public function setUserTarget(string $username) {
        $this->username = $username;
    }

    public function postMessage($target, string $text, int $flags = PostFlags::NONE): Promise
    {
        $user = '@' . $this->username;
        if(strpos($text, "\n") !== false) {
            $text = $user . "\n" . $text;
        } else {
            $text = $user . ' ' . $text;
        }
        return parent::postMessage($target, $text, $flags | PostFlags::ALLOW_SINGLE_PING);
    }
}

class Tell extends BasePlugin
{
    private $chatClient;
    /**
     * @var LoggerInterface
     */
    private $logger;
    /**
     * @var Injector
     */
    private $injector;
    private $builtInActionManager;
    private $pluginManager;
    private $stupidChatClient;

    public function __construct(
        ChatClientInterface $chatClient,
        LoggerInterface $logger,
        Injector $injector
    ) {
        $this->chatClient  = $chatClient;
        $this->logger = $logger;
        $this->injector = $injector;

        $this->pluginManager = $this->injector->make(PluginManager::class);
        $this->builtInActionManager =
            new BuiltInActionManager(
                $injector->make(Ban::class),
                $injector->make(RoomStatusManager::class),
                $logger
            );
        $this->stupidChatClient = $injector->make(DumbChatClient::class);

        $this->builtInActionManager->registerCommand($injector->make(Version::class, [
            ':chatClient' => $this->stupidChatClient
        ]));
    }

    public function tell(Command $command)
    {
        if (!$command->hasParameters()) {
            return $this->chatClient->postMessage($command, '`!!tell user command [...args]` - To prevent double pings, make sure you don\'t include the @!');
        }

        $user = $command->getParameter(0);

        $this->stupidChatClient->setUserTarget($user);

        $tellCommand = $command->getParameter(1);
        $tellCommandArguments = $command->getParameters(2);

        $newCommand = new Command($command->getRoom(), $tellCommand, $tellCommandArguments, $command->getOriginatingMessage());

        yield $this->builtInActionManager->handleCommand($newCommand);
        yield $this->pluginManager->handleCommand($newCommand);

        return new Success();
    }

    public function getDescription(): string
    {
        return 'Ping a user with the result of running another command.';
    }

    /**
     * @return PluginCommandEndpoint[]
     */
    public function getCommandEndpoints(): array
    {
        return [new PluginCommandEndpoint('Tell', [$this, 'tell'], 'tell')];
    }
}