Fix help command not working in group chats #10

0xEAB · 2017-09-03T22:01:13Z

Reply was sent to sender's chat instead of the group.

noplanman · 2017-09-03T23:25:24Z

@0xEAB This is intentional, to make sure that no admin commands get leaked by mistake.

If you want it be available in group chats too, then make that change to your own /help command 👍

Remember, this is just an example repository, trying to focus on the safest defaults. You will need to manually copy any commands you want to use, and modify them if necessary.

Anyway thanks for your contribution!
Next time maybe start by opening an issue to discuss the situation first 😊

0xEAB · 2017-09-04T13:37:16Z

Especially because it is an example, it shouldn't have any unexpected behaviour imho.

I admit I haven't thought of accidently leaking any commands, but I'll look into it, as I assume there is a better solution.

0xEAB · 2017-09-04T14:04:26Z

Don't ge wrong, but doesn't the help example "leak" all commands to everyone anyway?

noplanman · 2017-09-04T15:34:28Z

Especially because it is an example, it shouldn't have any unexpected behaviour imho.

I totally agree with that, but:

doesn't the help example "leak" all commands to everyone anyway?

Yes it does, in this example! Thing is, that many users seem to just copy paste various commands from here, adjusting them only a little bit, without actually understanding certain implications.

So to be safe, the default is "no-leak". If a user then wants to be able to use it in public groups, that's a very small change to make 👍

I really appreciate your thoughts on this, please do keep the ideas / suggestions / critique coming!

0xEAB · 2017-09-04T15:59:49Z

Thanks for your reply 😃

I understand your concerns, of course. So, I've added an additional check that ensures that no admin commands are sent to any non-admin users (which happens atm if there aren't any admins) or groups.
0xEAB@daec7ea

0xEAB · 2017-09-04T16:15:34Z

This doesn't cover the command-specific help yet. I'm gonna fix that, too.

noplanman · 2017-09-04T16:25:39Z

So, I've added an additional check that ensures that no admin commands are sent to any non-admin users

You'd be happy to know about the $private_only parameter you can add to a command for this exact reason.

Look at the PR here: php-telegram-bot/core#580

Or are you referring to something (slightly) different?

0xEAB · 2017-09-04T16:36:02Z

I don't want to make /help private_only. The result would be the same as it is now.

Sending the notification into user's private chat (like private_only does), is still a bad idea. If the user hasn't started a private conversation with the bot, he'll just experience a broken-looking bot.

My idea is to just hide the admin-section from the help command if the receiver is a group or a non-admin user. 😊

noplanman · 2017-09-04T16:42:03Z

Right, now I understand, thanks for explaining!

In that case, you could probably just change what you wanted to initially in this PR, from getFrom() to getChat(). It should be rare that a user is an admin of a bot without having talked to the bot ever.

But having said that, being safe-safe is always good 👍

So go ahead and make that fix 🎉

0xEAB · 2017-09-04T16:50:47Z

I'm glad you finally got what my intention was. I should have written a more detailed opener for this PR. 🙈

Done! If you find any further issues introduced by my changes, don't hestiate to tell me ...

0xEAB · 2017-09-04T17:08:58Z

Wait ... I haven't taken care of this yet: 0xEAB@daec7ea

0xEAB · 2017-09-04T17:26:57Z

May I ask you for your opinion on that? 0xEAB@daec7ea#commitcomment-24075658

noplanman · 2017-09-04T18:25:15Z

Ok, have written a response there. Let's try to keep the discussion here, to keep it easier to follow.

Now, regarding your latest changes here. Have you heard about the DRY principle? If not, have a quick read to know what it's all about.

Namely, I'm pointing at the new conditions you've added to the two if statements, which are almost the same. Best would be to save the identical part to a variable, say $safe_to_show, and then reuse that variable.
Also, in the second if, make sure to set parenthesis to force the proper interpretation of the conditions. e.g. $a && $b || $c could be viewed by the user as ($a && $b) || $c or $a && ($b || $c), which are quite different.

Having said all that, it's not relevant to check if a user in an admin or not, or if it's an admin command we're busy looping through. If (s)he's an admin, the admin commands are available, no need to double check that. The only thing that needs checking, is if we're in a private or group chat. Best just check if $chat->isPrivateChat(), as that will include both !$chat->isGroupChat() and !$chat->isSuperGroup().

Apart from that it looks ok to me 🎉

Hope this makes sense, otherwise I'm happy to open a PR onto your PR, to explain it better 👍

(As you may have noticed by now, I try to keep all code that's going into this project as clean and simple as possible 😇)

noplanman · 2017-09-04T18:38:02Z

Just to throw it in the room, we could also implement things like this as a command config.

Maybe another time though 😁

0xEAB · 2017-09-04T18:59:45Z

Yeah, I know the DRY principle, and I admit that my changes are a bit dirty. I'll improve that.

Of course, I've noticed that you care about cleaness and plainness.

0xEAB · 2017-09-04T20:54:25Z

anything else to do? 🙈

noplanman · 2017-09-04T21:04:12Z

Commands/HelpCommand.php

@@ -39,17 +39,20 @@ class HelpCommand extends UserCommand
    /**
     * @var string
     */
-    protected $version = '1.2.0';
+    protected $version = '1.2.1';


Go for 1.3.0 here, as this isn't just a bug fix, but a nice functionality change.

noplanman · 2017-09-04T21:07:52Z

Commands/HelpCommand.php

@@ -64,7 +67,7 @@ public function execute()
                $data['text'] .= '/' . $user_command->getName() . ' - ' . $user_command->getDescription() . PHP_EOL;
            }

-            if (count($admin_commands) > 0) {
+            if ((count($admin_commands) > 0) && $safe_to_show) {


Swap these, as a boolean is quicker to evaluate. If it's not safe to show anyway, no need to count the admin commands.

Small speed improvement, because every millisecond counts 😇

(also, you can drop the parenthesis around count(...) > 0)

noplanman · 2017-09-04T21:08:43Z

Commands/HelpCommand.php

@@ -77,7 +80,7 @@ public function execute()
        }

        $command_str = str_replace('/', '', $command_str);
-        if (isset($all_commands[$command_str])) {
+        if (isset($all_commands[$command_str]) && ($safe_to_show || !$all_commands[$command_str]->isAdminCommand())) {


Perfect! Way clearer to understand what's happening here 👍

noplanman · 2017-09-05T11:00:08Z

Wonderful! Thanks a lot for your contribution @0xEAB 🎉

0xEAB · 2017-09-05T11:06:51Z

You're welcome. I'm always happy when I can give something back to projects whose software I use. 😊

Fix help command not working in group chats

a0b9040

Reply was sent to sender's chat instead of the group.

noplanman closed this Sep 3, 2017

Fix /help leaking admin commands in group chats

daec7ea

noplanman reopened this Sep 4, 2017

Fix /help <command> leaking in group chats

7177550

Refactor help command fixes

38585c8

noplanman reviewed Sep 4, 2017

View reviewed changes

Improve performance (slightly)

8a52baa

noplanman merged commit 48a29b2 into php-telegram-bot:master Sep 5, 2017

Uh oh!

Fix help command not working in group chats #10

Fix help command not working in group chats #10

Uh oh!

Conversation

0xEAB commented Sep 3, 2017

Uh oh!

noplanman commented Sep 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

noplanman commented Sep 4, 2017

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

noplanman commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

noplanman commented Sep 4, 2017

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

noplanman commented Sep 4, 2017

Uh oh!

noplanman commented Sep 4, 2017

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

0xEAB commented Sep 4, 2017

Uh oh!

noplanman Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noplanman Sep 4, 2017

Choose a reason for hiding this comment

Uh oh!

noplanman Sep 4, 2017

Choose a reason for hiding this comment

Uh oh!

noplanman commented Sep 5, 2017

Uh oh!

0xEAB commented Sep 5, 2017

Uh oh!

Uh oh!

noplanman commented Sep 3, 2017 •

edited

Loading

noplanman commented Sep 4, 2017 •

edited

Loading

noplanman Sep 4, 2017 •

edited

Loading