Repository: python-telegram-bot/python-telegram-bot
Pull request: Fix conversationhandler
As found by @nmlorg and described in #1031
Went for the easiest fix.

fixes #1031

Problem statement:
Repeating job AttributeError
Hi!

I'm having a small def which is a repeating job, the code actually looks like this:
```
dp = updater.dispatcher
j = updater.job_queue
j.run_repeating(rss, 300)
```

After upgrading to 10.0.0 I get an exception in logs.

### Configuration
**Operating System:** Ubuntu


**Version of Python, python-telegram-bot & dependencies:**

python-telegram-bot 10.0.0
certifi 2018.01.18
future 0.16.0
Python 3.6.3 (default, Oct  6 2017, 08:44:35)

### Logs
```
AttributeError: 'NoneType' object has no attribute 'schedule_removal'
[03-03 23:51:50] [ERROR] [bot]
An uncaught error was raised while processing the update
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/telegram/ext/dispatcher.py", line 279, in process_update
    handler.handle_update(update, self)
  File "/usr/local/lib/python3.6/dist-packages/telegram/ext/conversationhandler.py", line 309, in handle_update
    timeout_job.schedule_removal()
AttributeError: 'NoneType' object has no attribute 'schedule_removal'
```

Hints from issue discussion:
@alexbagirov HI,

I would like to investigate what seems to be the problem. Can you please show the code for the Job in question aswell?
@Eldinnie Hi, sure, you can find it [here](https://github.com/MoarCatz/nmaps-telegram/blob/ptb10/rss.py). Also my latest suggestion is that this exception is not connected with repeating job but something else. But I'm not sure about this :smiley: 
It looks like [conversationhandler.py:308](https://github.com/python-telegram-bot/python-telegram-bot/blob/master/telegram/ext/conversationhandler.py#L308) was [changed](https://github.com/python-telegram-bot/python-telegram-bot/pull/895/commits/f68fb0387230a9e29fe9d636d263ddecf2a7ce03) to no longer protect 309-310 properly. It should definitely be `timeout_job is not None and` any other conditions, not `or`; I guess it depends on if the intent was to timeout from the beginning of the conversation (which would require a couple other changes) or timeout from last interaction, and the doc strings suggest to me the latter, so I think it should just be reverted to `if timeout_job is not None:` or maybe simplified a little to:
```python
        new_state = self.current_handler.handle_update(update, dispatcher)
!       timeout_job = self.timeout_jobs.pop(self.current_conversation, None)

!       if timeout_job is not None:
            timeout_job.schedule_removal()
-           del self.timeout_jobs[self.current_conversation]
        if self.conversation_timeout and new_state != self.END:
```
I think you;re absolutely right. We can take your snippet indeed.
Are you in the possibility to make a PR? And @tsnoam we should push a patch (10.1) for this. It makes every conversationhandler crash if timeout is not set and `ConversationHandler.END` is returned from a callback

Seeing this has some priority IMO I';m picking this up.

Commit message: Fix conversationhandler

As found by @nmlorg and described in #1031
Went for the easiest fix.

closen #1031

Patch to review:
diff --git a/telegram/ext/conversationhandler.py b/telegram/ext/conversationhandler.py
index c97c5991504..8d93cd0f176 100644
--- a/telegram/ext/conversationhandler.py
+++ b/telegram/ext/conversationhandler.py
@@ -305,7 +305,7 @@ def handle_update(self, update, dispatcher):
         new_state = self.current_handler.handle_update(update, dispatcher)
         timeout_job = self.timeout_jobs.get(self.current_conversation)
 
-        if timeout_job is not None or new_state == self.END:
+        if timeout_job is not None and new_state == self.END:
             timeout_job.schedule_removal()
             del self.timeout_jobs[self.current_conversation]
         if self.conversation_timeout and new_state != self.END:
diff --git a/tests/test_conversationhandler.py b/tests/test_conversationhandler.py
index 9dd258c8cb3..f00ff7b1f45 100644
--- a/tests/test_conversationhandler.py
+++ b/tests/test_conversationhandler.py
@@ -16,6 +16,7 @@
 #
 # You should have received a copy of the GNU Lesser Public License
 # along with this program.  If not, see [http://www.gnu.org/licenses/].
+import logging
 from time import sleep
 
 import pytest
@@ -55,7 +56,8 @@ def reset(self):
             self.BREWING: [CommandHandler('pourCoffee', self.drink)],
             self.DRINKING:
                 [CommandHandler('startCoding', self.code),
-                 CommandHandler('drinkMore', self.drink)],
+                 CommandHandler('drinkMore', self.drink),
+                 CommandHandler('end', self.end)],
             self.CODING: [
                 CommandHandler('keepCoding', self.code),
                 CommandHandler('gettingThirsty', self.start),
@@ -73,6 +75,9 @@ def _set_state(self, update, state):
     def start(self, bot, update):
         return self._set_state(update, self.THIRSTY)
 
+    def end(self, bot, update):
+        return self._set_state(update, self.END)
+
     def start_end(self, bot, update):
         return self._set_state(update, self.END)
 
@@ -123,6 +128,25 @@ def test_conversation_handler(self, dp, bot, user1, user2):
         with pytest.raises(KeyError):
             self.current_state[user2.id]
 
+    def test_conversation_handler_end(self, caplog, dp, bot, user1):
+        handler = ConversationHandler(entry_points=self.entry_points, states=self.states,
+                                      fallbacks=self.fallbacks)
+        dp.add_handler(handler)
+
+        message = Message(0, user1, None, self.group, text='/start', bot=bot)
+        dp.process_update(Update(update_id=0, message=message))
+        message.text = '/brew'
+        dp.process_update(Update(update_id=0, message=message))
+        message.text = '/pourCoffee'
+        dp.process_update(Update(update_id=0, message=message))
+        message.text = '/end'
+        with caplog.at_level(logging.ERROR):
+            dp.process_update(Update(update_id=0, message=message))
+        assert len(caplog.records) == 0
+        assert self.current_state[user1.id] == self.END
+        with pytest.raises(KeyError):
+            print(handler.conversations[(self.group.id, user1.id)])
+
     def test_conversation_handler_fallback(self, dp, bot, user1, user2):
         handler = ConversationHandler(entry_points=self.entry_points, states=self.states,
                                       fallbacks=self.fallbacks)


Code review comments:
diff_hunk: @@ -305,7 +305,7 @@ def handle_update(self, update, dispatcher):
         new_state = self.current_handler.handle_update(update, dispatcher)
         timeout_job = self.timeout_jobs.get(self.current_conversation)
 
-        if timeout_job is not None or new_state == self.END:
+        if timeout_job is not None and new_state == self.END:
path: telegram/ext/conversationhandler.py
text: @user1:
So with this logic, this new test:
```python
   def test_conversation_timeout_keeps_extending(self, dp, bot, user1):
        handler = ConversationHandler(entry_points=self.entry_points, states=self.states,
                                      fallbacks=self.fallbacks, conversation_timeout=0.5)
        dp.add_handler(handler)

        # Start state machine, wait, do something, verify the timeout is extended.
        # t=0 /start (timeout=.5)
        # t=.25 /brew (timeout=.75)
        # t=.5 original timeout
        # t=.6 /pourCoffee (timeout=1.1)
        # t=.75 second timeout
        # t=1.1 actual timeout
        message = Message(0, user1, None, self.group, text='/start', bot=bot)
        dp.process_update(Update(update_id=0, message=message))
        assert handler.conversations.get((self.group.id, user1.id)) == self.THIRSTY
        sleep(0.25)  # t=.25
        dp.job_queue.tick()
        assert handler.conversations.get((self.group.id, user1.id)) == self.THIRSTY
        message.text = '/brew'
        dp.process_update(Update(update_id=0, message=message))
        assert handler.conversations.get((self.group.id, user1.id)) == self.BREWING
        sleep(0.35)  # t=.6
        dp.job_queue.tick()
        assert handler.conversations.get((self.group.id, user1.id)) == self.BREWING
        message.text = '/pourCoffee'
        dp.process_update(Update(update_id=0, message=message))
        assert handler.conversations.get((self.group.id, user1.id)) == self.DRINKING
        sleep(.4)  # t=1
        dp.job_queue.tick()
        assert handler.conversations.get((self.group.id, user1.id)) == self.DRINKING
        sleep(.1)  # t=1.1
        dp.job_queue.tick()
        assert handler.conversations.get((self.group.id, user1.id)) is None
```
fails at the t=.6 test:
```
        assert handler.conversations.get((self.group.id, user1.id)) == self.BREWING
        sleep(0.35)  # t=.6
        dp.job_queue.tick()
>       assert handler.conversations.get((self.group.id, user1.id)) == self.BREWING
E       assert None == 1
```
However, if this is changed to just `if timeout_job is not None:`, everything's happy.