Merge pull request #6 from noplanman/conversations_code_review

Conversations code review

src/Commands/AdminCommands/SendtochannelCommand.php

@@ -26,7 +26,7 @@ class SendtochannelCommand extends AdminCommand
     protected $name = 'sendtochannel';
     protected $description = 'Send message to a channel';
     protected $usage = '/sendtochannel <message to send>';
-    protected $version = '0.1.3';
+    protected $version = '0.1.4';
     protected $need_mysql = true;
     /**#@-*/
 
@@ -56,7 +56,7 @@ class SendtochannelCommand extends AdminCommand
 
         // Conversation
         $this->conversation = new Conversation($user_id, $chat_id, $this->getName());
-        $this->conversation->start();
+
         $session = $this->conversation->getData();
 
         $channels = (array) $this->getConfig('your_channel');
@@ -70,7 +70,6 @@ class SendtochannelCommand extends AdminCommand
             case -1:
                 // getConfig has not been configured asking for channel to administer
                 if ($type != 'Message' || empty($text)) {
-
                     $session['state'] = -1;
                     $this->conversation->update($session);
 

src/Commands/SystemCommands/GenericmessageCommand.php

@@ -24,7 +24,7 @@ class GenericmessageCommand extends SystemCommand
      */
     protected $name = 'Genericmessage';
     protected $description = 'Handle generic message';
-    protected $version = '1.0.1';
+    protected $version = '1.0.2';
     protected $need_mysql = true;
     /**#@-*/
 
@@ -46,15 +46,16 @@ class GenericmessageCommand extends SystemCommand
      */
     public function execute()
     {
-        //System command, fetch command to execute if conversation exist
-        $message = $this->getMessage();
-        $chat_id = $message->getChat()->getId();
-        $user_id = $message->getFrom()->getId();
-        //Fetch Conversation if exist
-        $command = (new Conversation($user_id, $chat_id))->getConversationCommand();
-        if (! is_null($command)) {
+        //If a conversation is busy, execute the conversation command after handling the message
+        $conversation = new Conversation(
+            $this->getMessage()->getChat()->getId(),
+            $this->getMessage()->getFrom()->getId()
+        );
+        //Fetch conversation command if it exists and execute it
+        if ($conversation->exists() && ($command = $conversation->getCommand())) {
             return $this->telegram->executeCommand($command, $this->update);
         }
+
         return Request::emptyResponse();
     }
 }

src/Commands/UserCommands/CancelCommand.php

@@ -30,7 +30,7 @@ class CancelCommand extends UserCommand
     protected $name = 'cancel';
     protected $description = 'Cancel the currently active conversation';
     protected $usage = '/cancel';
-    protected $version = '0.1.0';
+    protected $version = '0.1.1';
     protected $need_mysql = true;
     /**#@-*/
 
@@ -47,7 +47,7 @@ class CancelCommand extends UserCommand
             $this->getMessage()->getChat()->getId()
         );
 
-        if ($conversation_command = $conversation->getConversationCommand()) {
+        if ($conversation_command = $conversation->getCommand()) {
             $conversation->cancel();
             $text = 'Conversation "' . $conversation_command . '" cancelled!';
         }

src/Commands/UserCommands/SurveyCommand.php

@@ -27,7 +27,7 @@ class SurveyCommand extends UserCommand
     protected $name = 'survey';
     protected $description = 'Survery for bot users';
     protected $usage = '/survey';
-    protected $version = '0.1.0';
+    protected $version = '0.1.1';
     protected $need_mysql = true;
     /**#@-*/
 
@@ -58,10 +58,9 @@ class SurveyCommand extends UserCommand
 
         //tracking
         $conversation = new Conversation($user_id, $chat_id, $this->getName());
-        $conversation->start();
 
         //cache data from the tracking session if any
-        $session = $conversation->GetData();
+        $session = $conversation->getData();
         if (!isset($session['state'])) {
             $state = '0';
         } else {

src/Conversation.php

@@ -10,23 +10,14 @@
 
 namespace Longman\TelegramBot;
 
-use Longman\TelegramBot\Command;
-use Longman\TelegramBot\Request;
-use Longman\TelegramBot\ConversationDB;
-use Longman\TelegramBot\Entities\Update;
-
 /**
  * Class Conversation
+ *
+ * Only one conversation can be active at any one time.
+ * A conversation is directly linked to a user, chat and the command that is managing the conversation.
  */
 class Conversation
 {
-    /**
-     * Conversation has been fetched true false
-     *
-     * @var bool
-     */
-    protected $is_fetched = false;
-
     /**
      * All information fetched from the database
      *
@@ -35,7 +26,7 @@ class Conversation
     protected $conversation = null;
 
     /**
-     * Data stored inside the Conversation
+     * Data stored inside the conversation
      *
      * @var array
      */
@@ -55,14 +46,6 @@ class Conversation
      */
     protected $chat_id;
 
-    /**
-     * Group name let you share the session among commands
-     * Call this as the same name of the command if you don't need to share the conversation
-     *
-     * @var string
-     */
-    protected $group_name;
-
     /**
      * Command to be executed if the conversation is active
      *
@@ -75,117 +58,153 @@ class Conversation
      *
      * @param int    $user_id
      * @param int    $chat_id
-     * @param string $group_name
      * @param string $command
      */
-    public function __construct($user_id, $chat_id, $group_name = null, $command = null)
+    public function __construct($user_id, $chat_id, $command = null)
     {
-        if (is_null($command)) {
-            $command = $group_name;
-        }
-
         $this->user_id = $user_id;
         $this->chat_id = $chat_id;
         $this->command = $command;
-        $this->group_name = $group_name;
+
+        //Try to load an existing conversation if possible
+        $this->load();
     }
 
     /**
-     * Check if the conversation already exists
+     * Load the conversation from the database
      *
      * @return bool
      */
-    protected function exist()
+    protected function load()
     {
-        //Conversation info already fetched
-        if ($this->is_fetched) {
-            return true;
-        }
+        $this->conversation = null;
+        $this->data = null;
+
         //Select an active conversation
         $conversation = ConversationDB::selectConversation($this->user_id, $this->chat_id, 1);
-        $this->is_fetched = true;
-
         if (isset($conversation[0])) {
             //Pick only the first element
             $this->conversation = $conversation[0];
 
-            if (is_null($this->group_name)) {
-                //Conversation name and command has not been specified. command has to be retrieved
-                return true;
+            //Load the command from the conversation if it hasn't been passed
+            $this->command = $this->command ?: $this->conversation['command'];
+
+            if ($this->command !== $this->conversation['command']) {
+                $this->cancel();
+                return false;
             }
 
-            //A conversation with the same name was already opened, store the data inside the class
-            if ($this->conversation['conversation_name'] == $this->group_name) {
+            //Load the conversation data
             $this->data = json_decode($this->conversation['data'], true);
-                return true;
         }
 
-            //A conversation with a different name has been opened, unset the DB one and recreate a new one
-            $this->cancel();
-            return false;
+        return $this->exists();
     }
 
-        $this->conversation = null;
-        return false;
+    /**
+     * Check if the conversation already exists
+     *
+     * @return bool
+     */
+    public function exists()
+    {
+        return ($this->conversation !== null);
     }
 
     /**
-     * Check if a conversation has already been created in the database. If the conversation is not found, a new conversation is created. Start fetches the data stored in the database.
+     * Start a new conversation if the current command doesn't have one yet
      *
      * @return bool
      */
-    public function start()
+    protected function start()
     {
-        if (!$this->exist()) {
-            $status = ConversationDB::insertConversation($this->command, $this->group_name, $this->user_id, $this->chat_id);
-            $this->is_fetched = true;
+        if (!$this->exists() && $this->command) {
+            if (ConversationDB::insertConversation(
+                $this->user_id,
+                $this->chat_id,
+                $this->command
+            )) {
+                return $this->load();
             }
-        return true;
+        }
+
+        return false;
     }
 
     /**
-     * Delete the conversation from the database
+     * Delete the current conversation
      *
      * Currently the Conversation is not deleted but just set to 'stopped'
      *
-     * @todo should return something
+     * @return bool
      */
     public function stop()
     {
-        if ($this->exist()) {
-            ConversationDB::updateConversation(['status' => 'stopped'], ['chat_id' => $this->chat_id, 'user_id' => $this->user_id, 'status' => 'active']);
-        }
+        return $this->updateStatus('stopped');
     }
 
     /**
-     * Set to Cancelled the conversation in the database
+     * Cancel the current conversation
      *
-     * @todo should return something
+     * @return bool
      */
     public function cancel()
     {
-        if ($this->exist()) {
-            ConversationDB::updateConversation(['status' => 'cancelled'], ['chat_id' => $this->chat_id, 'user_id' => $this->user_id, 'status' => 'active']);
+        return $this->updateStatus('cancelled');
     }
+
+    /**
+     * Update the status of the current conversation
+     *
+     * @param string $status
+     *
+     * @return bool
+     */
+    protected function updateStatus($status)
+    {
+        if ($this->exists()) {
+            $fields = ['status' => $status];
+            $where  = [
+                'status'  => 'active',
+                'user_id' => $this->user_id,
+                'chat_id' => $this->chat_id,
+            ];
+            if (ConversationDB::updateConversation($fields, $where)) {
+                //Reload the data
+                $this->load();
+                return true;
+            }
+        }
+
+        return false;
     }
 
     /**
      * Store the array/variable in the database with json_encode() function
      *
-     * @todo Verify the query before assigning the $data member variable
-     *
      * @param array $data
+     *
+     * @return bool
      */
     public function update($data)
     {
-        //Conversation must exist!
-        if ($this->exist()) {
-            $fields['data'] = json_encode($data);
-
-            ConversationDB::updateConversation($fields, ['chat_id' => $this->chat_id, 'user_id' => $this->user_id, 'status' => 'active']);
-            //TODO verify query success before convert the private var
-            $this->data = $data;
+        if ($this->exists()) {
+            $fields = ['data' => json_encode($data)];
+            $where  = [
+                'status'  => 'active',
+                'user_id' => $this->user_id,
+                'chat_id' => $this->chat_id,
+            ];
+            if (ConversationDB::updateConversation($fields, $where)) {
+                //Reload the data
+                $this->load();
+                return true;
             }
+        } elseif ($this->start()) {
+            return $this->update($data);
+        }
+
+        return false;
     }
 
     /**
@@ -193,18 +212,15 @@ class Conversation
      *
      * @return string|null
      */
-    public function getConversationCommand()
+    public function getCommand()
     {
-        if ($this->exist()) {
-            return $this->conversation['conversation_command'];
-        }
-        return null;
+        return $this->command;
     }
 
     /**
      * Retrieve the data stored in the conversation
      *
-     * @return array
+     * @return array|null
      */
     public function getData()
     {

src/ConversationDB.php

@@ -72,14 +72,13 @@ class ConversationDB extends DB
     /**
      * Insert the conversation in the database
      *
-     * @param string $conversation_command
-     * @param string $conversation_group_name
      * @param int    $user_id
      * @param int    $chat_id
+     * @param string $command
      *
      * @return bool
      */
-    public static function insertConversation($conversation_command, $conversation_group_name, $user_id, $chat_id)
+    public static function insertConversation($user_id, $chat_id, $command)
     {
         if (!self::isDbConnected()) {
             return false;
@@ -88,10 +87,10 @@ class ConversationDB extends DB
         try {
             $sth = self::$pdo->prepare('INSERT INTO `' . TB_CONVERSATION . '`
                 (
-                `status`, `conversation_command`, `conversation_name`, `user_id`, `chat_id`, `data`, `created_at`, `updated_at`
+                `status`, `user_id`, `chat_id`, `command`, `data`, `created_at`, `updated_at`
                 )
                 VALUES (
-                :status, :conversation_command, :conversation_name, :user_id, :chat_id, :data, :date, :date
+                :status, :user_id, :chat_id, :command, :data, :date, :date
                 )
                ');
             $active = 'active';
@@ -100,8 +99,7 @@ class ConversationDB extends DB
             $created_at = self::getTimestamp();
 
             $sth->bindParam(':status', $active);
-            $sth->bindParam(':conversation_command', $conversation_command);
-            $sth->bindParam(':conversation_name', $conversation_group_name);
+            $sth->bindParam(':command', $command);
             $sth->bindParam(':user_id', $user_id);
             $sth->bindParam(':chat_id', $chat_id);
             $sth->bindParam(':data', $data);

structure.sql

@@ -140,8 +140,7 @@ CREATE TABLE IF NOT EXISTS `conversation` (
   `user_id` bigint NULL DEFAULT NULL COMMENT 'User id',
   `chat_id` bigint NULL DEFAULT NULL COMMENT 'Telegram chat_id can be a the user id or the chat id ',
   `status` ENUM('active', 'cancelled', 'stopped') NOT NULL DEFAULT 'active' COMMENT 'active conversation is active, cancelled conversation has been truncated before end, stopped conversation has end',
-  `conversation_command` varchar(160) DEFAULT '' COMMENT 'Default Command to execute',
-  `conversation_name` varchar(160) NOT NULL DEFAULT '' COMMENT 'Name of the conversation can be the command name or a generic name for conversation between multiple commands',
+  `command` varchar(160) DEFAULT '' COMMENT 'Default Command to execute',
   `data` varchar(1000) DEFAULT 'NULL' COMMENT 'Data stored from command',
   `created_at` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
   `updated_at` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
@@ -150,7 +149,6 @@ CREATE TABLE IF NOT EXISTS `conversation` (
   KEY `user_id` (`user_id`),
   KEY `chat_id` (`chat_id`),
   KEY `status` (`status`),
-  KEY `conversation_name` (`conversation_name`), 
 
   FOREIGN KEY (`user_id`)
   REFERENCES `user` (`id`),