Feature/smartinterface

[MBoretto]

I renamed the known tracking feature with conversation. With this new name every thing is more readable.
I test a lot and seem that works fine, and i would like to have a feeedback also from you guy @akalongman @noplanman before continue.
Here some things to know

• Conversation need a new table in the database. The table holds the 'coordinates' of the conversation such as 'user_id', 'chat_id'. It store some data related to the conversation, is something like a php $_SESSION where you can drop useful var that you need to bring with you during the user navigation.
• A Conversation can have one of those three status:
  • active : conversation has been initialize with ->start() and is active
  • stopped : conversation has reach a ->stop() endpoint and it's terminated
  • cancelled : conversation has been interrupted by the creation of another conversation
• The system is build around the concept that a user in a chat can have only one conversation active
• At each conversation is bound a command that is executed for each message that arrive from the user until the conversation is stopped.
• Command that exploit this feature are /survey (User Command) and /Sendtochannel (Admin Command)
• Commad are implemented as machine states
• The system is build on top of the new system command introduced by @noplanman

[noplanman]

Regarding the new sendtochannel command, there are a few ways to "break" it:

• Sending the command when no (or an incorrect) commandConfig has been set.
• Sending the command in between the states makes it jump to the last step which it can't recover from, unless you actually type "Yes" or "No".

Also, it would be great if we could find out if the bot is an admin of the entered channels before showing the channel selection keyboard, so it doesn't go through all the steps and at the end it says "Sorry, can't send...".

Can the Telegram keyboard be set to accept multiple selections like checkboxes? Would be cool to add that when managing multiple channels, to allow the same message to be sent to multiple ones 😃 Similarly, if there is only one channel, it should select it automatically I think.

For now I've just had a look at this, will check out the main conversation feature soon 👍

[noplanman]

Before continuing, make sure that at least one channel has been set in the config.

[MBoretto]

done

[noplanman]

@MBoretto Great work so far, it's coming along really nicely!!

After combing through the code and trying different things, here a few thoughts I've come up with so far. Let me know what you think!

Multiple parallel conversations

When mixing different conversations, any old one is set to cancelled and only the new one becomes active.
Would it maybe be possible to set any existing ones on pause and reactivate them when calling the appropriate command?
For example:

• /survey
• Enter name: Johnny
• /sendtochannel (set's the /survey one on pause)
• Select @my_channel
• Enter message to send to channel
• /survey (set's the /sendtochannel one on pause, resumes /survey)
• Enter surname: Johnson
• etc.

The way the commands work at the moment, it's not possible to send /anything to a channel, as it gets interpreted as a command, not just text. Do we keep it like this or does this need to change?

Subcommands

Would it be an idea to add subcommands to conversations to make it easier to control or alter them?
What I mean is something along the lines of:

• /survey cancel (which would cancel the current survey)
• /survey restart (which cancels and restarts the current survey)
• etc.

This could obviously be used for other stuff too.

Short-hands

It might be an idea to consider short-hand commands for the conversations.
For the /sendtochannel command, I feel it would make sense to be able to create a "1-liner" command (for the CLI), so that it can be executed from the command line without the need of any interaction:

• /sendtochannel @my_channel My text message!

This specific example doesn't allow images or other media, just text.

Other conversation commands could implement their own versions if they wanted them.

[MBoretto]

• You can understand if the bot is admin of the channel just when you're trying to send a message to it. I add some feedback if the bot fail sending the message.
• For what concern multiple sending, Conversation can stay in the same state and select more channels and just when you press the button 'ok' you can continue in the flow. We can implement it maybe in the next version of the command!
• I fixed the bug that break the conversation after resending the command

Multiple parallel conversation

I prefer to avoid this, because user don't care about what is typing in the chat. Can happen that the user jumps between commands and start nesting conversation.
Of course if we need to develop this feature for a specific task I will implement it!

• You are right so far you can't send any text that begin with /.
  I think that implement this feature can be very difficult because you have to touch the command handling system in the deep. We can take this in consideration implementing the new command class. (This is not a priority in my opinion)

Subcommands & short-hands

As ux designer (we can talk about ux design in bot??) :) , I think that commands just have to be pressed. Adding params increase the complexity for the mean user in bot interaction.
The main purpose of conversation is to avoid params.

But! We can have care, as you said, that all commands can work both with conversation and paramar (at least in minimal function).

• I would like to introduce the command /cancel that just cancel the active convesation and clean all the keyboard, this can be used with every command that exploits conversation and not just in /survey
• I was thinking to implement /sendtochannel also if db is not setted (1-liner)

[noplanman]

Assigning true or false explicitly doesn't make sense.
Instead, it's easier to just put:

$this->command_is_provided = isset($command);

or (which is faster performance-wise)

$this->command_is_provided = ($command !== null);

[MBoretto]

i will fix it

[noplanman]

Apart from the few things I commented on inline above, I think we're good to go 😃

I'll see if I can manage to write some tests for this before we merge.
Then, we can start the final release branch, woohoo!

Nice work 👍

[MBoretto]

I put the var to public because it seem's to me easy to menage, like the var $_SESSION. That's all!
ready to go! 😃

[noplanman]

I'm busy writing some tests and have come across a few things we need to discuss.

• Instead of exists() it would make more sense to rename the method to isActive(), because the conversation actually does "exist" in the DB, the status is just not set to "active".
• The conversation needs to be "re-loaded" after updating its data. e.g. after calling cancel() or stop() the conversation shouldn't be active any more, but it still returns true when calling exists(). You need to unset() and reinstantiate the conversation first for the change to take effect.
• Updating the data on destruct is not intuitive. It makes a lot more sense to have it as it was, by calling update() to actually update the data in the DB. The problem with using __destruct() is that you first need to call unset($conversation); to update the data inline. Also, it makes it impossible to "cancel the changes". I think it's better to require the user / command to explicitly call update() to actually save the changes. Is there any specific reason why you changed this? Calling update() is like clicking "Save" in an application, I think it's ok to require that.

Remember, KISS 😉

The simpler it all is, the easier it is to test and maintain.

When making changes to something that already works as it should, please explain why exactly you're changing it. That way we can discuss and possibly improve it.

[MBoretto]

Intro

I'm still thinking about a way to break in piece the long switch in the /sendtochannels command.
One thing that really challenge me is that conversation data/notes have to be loaded and downloaded from the class.
So I decided to store and manipulate them inside.
The current implementation has been inspired (a bit) from the ORM database library.
That's how it works:

//This is pseudo code! 
$my_conversation = MyConversationTable::find($id); //retrive a conversation
$my_conversation->column_command = 'weather'; //Set data to update
$my_conversation->column_status = 1;
$my_conversation->update(); //update the data

Inside the class the fetched data are stored twice.

• One copy is protected and is used as reference
• The other is public and can be edited inside the program without call class methods

What I like about this implementation is:

• You can discard changes made in the public part copying them back from the private part
• If you set a variable as it was before, an easy check can be done by the class in order to save a query to the database (i don't know if is implemented in orm but i would like to implement it in the library)
  So I decided to implement a similar mechanism for notes.

Answers

• For what concern the update() on the destructor maybe is a bit excessive, i like the idea because it remind to me the $_SESSION mechanism where you just drop inside everything and forget about it .
  Anyway thanks for point this out, we can easily revert id back as it was before. Maybe using update() methods can also simplify unit test, i didn't think about this when i changed it sorry!
• About renaming, yes could be good to do it but I would suggest to make two methods exist() and isActive() (this need some change also in CoversationDB)
• When you stop() or cancel() a conversation i think that conversation should exist(), but isActive() must return false.
• I want to avoid as much as i can unuseful query to the database, I don't need to fetch back data after update(), stop()or cancel(). I already know what has been changed.
  If I need to reload data i can call load(), we should put it public if you agree.

Further next things in mind

I would like to create a protected var for each database column instead of store everything here: https://github.com/MBoretto/php-telegram-bot/blob/feature/smartinterface/src/Conversation.php#L26
This can simplify the creation of a methods isActive().

Sorry for this long comment, I hope I have clarified my latest change in this feature!
Let me know your opinion and if i miss something! Thanks for your feedback!
ps. We're getting closer to 300! 🎉

[noplanman]

Data management

Looking at the pseudo code, I really like the first line to load a conversation. I had thought about this already but left it as you had first implemented it. Like the example, I think it's better to have a static method (find or load) to check the database if a conversation exists and then return a Conversation object if it does, or false if it doesn't. Instantiating a new conversation with new would always create a new one, even if it already exists, so any conversation with the matching user_id and chat_id would automatically be cancelled and a new one inserted. I guess that's what the word new would literally mean.

Do you think this is a good idea?

I totally agree with you on trying to keep database queries to an absolute minimum. We can do this by having the update() method public again. So to stop a conversation for example, one would write:

if ($conversation = Conversation::load($user_id, $chat_id)) {
  $conversation->stop();
  $conversation->update();
}

Obviously only the update() makes a call to the database to update it. And as you say, this makes testing a lot easier, as each method does only a certain thing, not multiple things at the same time 👍
I'm fine with having 2 variables saving the data, one keeps the original database state, the other one keeps the "current" state (until update() is called). We could also introduce a rollback() or revert() method to undo any changes made to a conversation before update() is called. This would simply overwrite the current notes with the protected notes.

About having 2 methods, exists() and isActive(), why both? There can only be 1 active conversation anyway. This would make sense if we would allow pausing and resuming of conversations, then we could see that it exists but is inactive.
Maybe you could explain a bit more what both of these would be used for?

I would like to create a protected var for each database column instead of store everything here

I think if we introduce good getter/setter methods, this isn't necessary. What would be the advantage of this?
To me, $conversation->setStatus(1); is more intuitive than $conversation->column_status = 1;

Hope I responded to all your point in some way.