Refactor Warnings in ConversationHandler (#2755, #2784)

.github/pull_request_template.md

@@ -29,6 +29,7 @@ Hey! You're PRing? Cool! Please have a look at the below checklist. It's here to
     - [ ] Link new and existing constants in docstrings instead of hard coded number and strings
     - [ ] Add new message types to `Message.effective_attachment`
     - [ ] Added new handlers for new update types
+      - [ ] Add the handlers to the warning loop in the `ConversationHandler`
     - [ ] Added new filters for new message (sub)types
     - [ ] Added or updated documentation for the changed class(es) and/or method(s)
     - [ ] Updated the Bot API version number in all places: `README.rst` and `README_RAW.rst` (including the badge), as well as `telegram.constants.BOT_API_VERSION`

telegram/ext/_conversationhandler.py

@@ -45,6 +45,9 @@ from telegram.ext import (
     DispatcherHandlerStop,
     Handler,
     InlineQueryHandler,
+    StringCommandHandler,
+    StringRegexHandler,
+    TypeHandler,
 )
 from telegram._utils.warnings import warn
 from telegram.ext._utils.promise import Promise
@@ -239,6 +242,14 @@ class ConversationHandler(Handler[Update, CCT]):
         map_to_parent: Dict[object, object] = None,
         run_async: bool = False,
     ):
+        # these imports need to be here because of circular import error otherwise
+        from telegram.ext import (  # pylint: disable=import-outside-toplevel
+            ShippingQueryHandler,
+            PreCheckoutQueryHandler,
+            PollHandler,
+            PollAnswerHandler,
+        )
+
         self.run_async = run_async
 
         self._entry_points = entry_points
@@ -283,49 +294,77 @@ class ConversationHandler(Handler[Update, CCT]):
         for state_handlers in states.values():
             all_handlers.extend(state_handlers)
 
-        if self.per_message:
+        # this loop is going to warn the user about handlers which can work unexpected
-            for handler in all_handlers:
+        # in conversations
-                if not isinstance(handler, CallbackQueryHandler):
-                    warn(
-                        "If 'per_message=True', all entry points, state handlers, and fallbacks"
-                        " must be 'CallbackQueryHandler', since no other handlers "
-                        "have a message context.",
-                        stacklevel=2,
-                    )
-                    break
-        else:
-            for handler in all_handlers:
-                if isinstance(handler, CallbackQueryHandler):
-                    warn(
-                        "If 'per_message=False', 'CallbackQueryHandler' will not be "
-                        "tracked for every message.",
-                        stacklevel=2,
-                    )
-                    break
 
-        if self.per_chat:
+        # this link will be added to all warnings tied to per_* setting
-            for handler in all_handlers:
+        per_faq_link = (
-                if isinstance(handler, (InlineQueryHandler, ChosenInlineResultHandler)):
+            " Read this FAQ entry to learn more about the per_* settings: https://git.io/JtcyU."
-                    warn(
+        )
-                        "If 'per_chat=True', 'InlineQueryHandler' can not be used, "
-                        "since inline queries have no chat context.",
-                        stacklevel=2,
-                    )
-                    break
 
-        if self.conversation_timeout:
+        for handler in all_handlers:
-            for handler in all_handlers:
+            if isinstance(handler, (StringCommandHandler, StringRegexHandler)):
-                if isinstance(handler, self.__class__):
+                warn(
-                    warn(
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`. "
-                        "Using `conversation_timeout` with nested conversations is currently not "
+                    f"{handler.__class__.__name__} handles updates of type `str`.",
-                        "supported. You can still try to use it, but it will likely behave "
+                    stacklevel=2,
-                        "differently from what you expect.",
+                )
-                        stacklevel=2,
+            elif isinstance(handler, TypeHandler) and not issubclass(handler.type, Update):
-                    )
+                warn(
-                    break
+                    "The `ConversationHandler` only handles updates of type `telegram.Update`."
+                    f" The TypeHandler is set to handle {handler.type.__name__}.",
+                    stacklevel=2,
+                )
+            elif isinstance(handler, PollHandler):
+                warn(
+                    "PollHandler will never trigger in a conversation since it has no information "
+                    "about the chat or the user who voted in it. Do you mean the "
+                    "`PollAnswerHandler`?",
+                    stacklevel=2,
+                )
 
-        if self.run_async:
+            elif self.per_chat and (
-            for handler in all_handlers:
+                isinstance(
+                    handler,
+                    (
+                        ShippingQueryHandler,
+                        InlineQueryHandler,
+                        ChosenInlineResultHandler,
+                        PreCheckoutQueryHandler,
+                        PollAnswerHandler,
+                    ),
+                )
+            ):
+                warn(
+                    f"Updates handled by {handler.__class__.__name__} only have information about "
+                    f"the user, so this handler won't ever be triggered if `per_chat=True`."
+                    f"{per_faq_link}",
+                    stacklevel=2,
+                )
+
+            elif self.per_message and not isinstance(handler, CallbackQueryHandler):
+                warn(
+                    "If 'per_message=True', all entry points, state handlers, and fallbacks"
+                    " must be 'CallbackQueryHandler', since no other handlers "
+                    f"have a message context.{per_faq_link}",
+                    stacklevel=2,
+                )
+            elif not self.per_message and isinstance(handler, CallbackQueryHandler):
+                warn(
+                    "If 'per_message=False', 'CallbackQueryHandler' will not be "
+                    f"tracked for every message.{per_faq_link}",
+                    stacklevel=2,
+                )
+
+            if self.conversation_timeout and isinstance(handler, self.__class__):
+                warn(
+                    "Using `conversation_timeout` with nested conversations is currently not "
+                    "supported. You can still try to use it, but it will likely behave "
+                    "differently from what you expect.",
+                    stacklevel=2,
+                )
+
+            if self.run_async:
                 handler.run_async = True
 
     @property

tests/test_conversationhandler.py

@@ -18,6 +18,7 @@
 # along with this program.  If not, see [http://www.gnu.org/licenses/].
 import logging
 from time import sleep
+from warnings import filterwarnings
 
 import pytest
 from flaky import flaky
@@ -45,7 +46,15 @@ from telegram.ext import (
     DispatcherHandlerStop,
     TypeHandler,
     JobQueue,
+    StringCommandHandler,
+    StringRegexHandler,
+    PollHandler,
+    ShippingQueryHandler,
+    ChosenInlineResultHandler,
+    PreCheckoutQueryHandler,
+    PollAnswerHandler,
 )
+from telegram.warnings import PTBUserWarning
 
 
 @pytest.fixture(scope='class')
@@ -1325,59 +1334,88 @@ class TestConversationHandler:
         assert handler.conversations.get((self.group.id, user1.id)) is None
         assert self.is_timeout
 
-    def test_conversation_timeout_warning_only_shown_once(self, recwarn):
+    def test_handlers_generate_warning(self, recwarn):
-        ConversationHandler(
+        """
-            entry_points=self.entry_points,
+        this function tests all handler + per_* setting combinations.
-            states={
+        """
-                self.THIRSTY: [
-                    ConversationHandler(
-                        entry_points=self.entry_points,
-                        states={
-                            self.BREWING: [CommandHandler('pourCoffee', self.drink)],
-                        },
-                        fallbacks=self.fallbacks,
-                    )
-                ],
-                self.DRINKING: [
-                    ConversationHandler(
-                        entry_points=self.entry_points,
-                        states={
-                            self.CODING: [CommandHandler('startCoding', self.code)],
-                        },
-                        fallbacks=self.fallbacks,
-                    )
-                ],
-            },
-            fallbacks=self.fallbacks,
-            conversation_timeout=100,
-        )
-        assert len(recwarn) == 1
-        assert str(recwarn[0].message) == (
-            "Using `conversation_timeout` with nested conversations is currently not "
-            "supported. You can still try to use it, but it will likely behave "
-            "differently from what you expect."
-        )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 
-    def test_per_message_warning_is_only_shown_once(self, recwarn):
+        # the warning message action needs to be set to always,
+        # otherwise only the first occurrence will be issued
+        filterwarnings(action="always", category=PTBUserWarning)
+
+        # this class doesn't do anything, its just not the Update class
+        class NotUpdate:
+            pass
+
+        # this conversation handler has the string, string_regex, Pollhandler and TypeHandler
+        # which should all generate a warning no matter the per_* setting. TypeHandler should
+        # not when the class is Update
         ConversationHandler(
-            entry_points=self.entry_points,
+            entry_points=[StringCommandHandler("code", self.code)],
             states={
-                self.THIRSTY: [CommandHandler('pourCoffee', self.drink)],
+                self.BREWING: [
-                self.BREWING: [CommandHandler('startCoding', self.code)],
+                    StringRegexHandler("code", self.code),
+                    PollHandler(self.code),
+                    TypeHandler(NotUpdate, self.code),
+                ],
             },
-            fallbacks=self.fallbacks,
+            fallbacks=[TypeHandler(Update, self.code)],
+        )
+
+        # these handlers should all raise a warning when per_chat is True
+        ConversationHandler(
+            entry_points=[ShippingQueryHandler(self.code)],
+            states={
+                self.BREWING: [
+                    InlineQueryHandler(self.code),
+                    PreCheckoutQueryHandler(self.code),
+                    PollAnswerHandler(self.code),
+                ],
+            },
+            fallbacks=[ChosenInlineResultHandler(self.code)],
+            per_chat=True,
+        )
+
+        # the CallbackQueryHandler should *not* raise when per_message is True,
+        # but any other one should
+        ConversationHandler(
+            entry_points=[CallbackQueryHandler(self.code)],
+            states={
+                self.BREWING: [CommandHandler("code", self.code)],
+            },
+            fallbacks=[CallbackQueryHandler(self.code)],
             per_message=True,
         )
-        assert len(recwarn) == 1
-        assert str(recwarn[0].message) == (
-            "If 'per_message=True', all entry points, state handlers, and fallbacks"
-            " must be 'CallbackQueryHandler', since no other handlers"
-            " have a message context."
-        )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 
-    def test_per_message_but_not_per_chat_warning(self, recwarn):
+        # the CallbackQueryHandler should raise when per_message is False
+        ConversationHandler(
+            entry_points=[CommandHandler("code", self.code)],
+            states={
+                self.BREWING: [CommandHandler("code", self.code)],
+            },
+            fallbacks=[CallbackQueryHandler(self.code)],
+            per_message=False,
+        )
+
+        # adding a nested conv to a conversation with timeout should warn
+        child = ConversationHandler(
+            entry_points=[CommandHandler("code", self.code)],
+            states={
+                self.BREWING: [CommandHandler("code", self.code)],
+            },
+            fallbacks=[CommandHandler("code", self.code)],
+        )
+
+        ConversationHandler(
+            entry_points=[CommandHandler("code", self.code)],
+            states={
+                self.BREWING: [child],
+            },
+            fallbacks=[CommandHandler("code", self.code)],
+            conversation_timeout=42,
+        )
+
+        # If per_message is True, per_chat should also be True, since msg ids are not unique
         ConversationHandler(
             entry_points=[CallbackQueryHandler(self.code, "code")],
             states={
@@ -1387,52 +1425,76 @@ class TestConversationHandler:
             per_message=True,
             per_chat=False,
         )
-        assert len(recwarn) == 1
+
+        # the overall number of handlers throwing a warning is 13
+        assert len(recwarn) == 13
+        # now we test the messages, they are raised in the order they are inserted
+        # into the conversation handler
         assert str(recwarn[0].message) == (
+            "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+            "StringCommandHandler handles updates of type `str`."
+        )
+        assert str(recwarn[1].message) == (
+            "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+            "StringRegexHandler handles updates of type `str`."
+        )
+        assert str(recwarn[2].message) == (
+            "PollHandler will never trigger in a conversation since it has no information "
+            "about the chat or the user who voted in it. Do you mean the "
+            "`PollAnswerHandler`?"
+        )
+        assert str(recwarn[3].message) == (
+            "The `ConversationHandler` only handles updates of type `telegram.Update`. "
+            "The TypeHandler is set to handle NotUpdate."
+        )
+
+        per_faq_link = (
+            " Read this FAQ entry to learn more about the per_* settings: https://git.io/JtcyU."
+        )
+
+        assert str(recwarn[4].message) == (
+            "Updates handled by ShippingQueryHandler only have information about the user,"
+            " so this handler won't ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[5].message) == (
+            "Updates handled by ChosenInlineResultHandler only have information about the user,"
+            " so this handler won't ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[6].message) == (
+            "Updates handled by InlineQueryHandler only have information about the user,"
+            " so this handler won't ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[7].message) == (
+            "Updates handled by PreCheckoutQueryHandler only have information about the user,"
+            " so this handler won't ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[8].message) == (
+            "Updates handled by PollAnswerHandler only have information about the user,"
+            " so this handler won't ever be triggered if `per_chat=True`." + per_faq_link
+        )
+        assert str(recwarn[9].message) == (
+            "If 'per_message=True', all entry points, state handlers, and fallbacks must be "
+            "'CallbackQueryHandler', since no other handlers have a message context."
+            + per_faq_link
+        )
+        assert str(recwarn[10].message) == (
+            "If 'per_message=False', 'CallbackQueryHandler' will not be tracked for "
+            "every message." + per_faq_link
+        )
+        assert str(recwarn[11].message) == (
+            "Using `conversation_timeout` with nested conversations is currently not "
+            "supported. You can still try to use it, but it will likely behave differently"
+            " from what you expect."
+        )
+
+        assert str(recwarn[12].message) == (
             "If 'per_message=True' is used, 'per_chat=True' should also be used, "
             "since message IDs are not globally unique."
         )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 
-    def test_per_message_false_warning_is_only_shown_once(self, recwarn):
+        # this for loop checks if the correct stacklevel is used when generating the warning
-        ConversationHandler(
+        for warning in recwarn:
-            entry_points=self.entry_points,
+            assert warning.filename == __file__, "incorrect stacklevel!"
-            states={
-                self.THIRSTY: [CallbackQueryHandler(self.drink)],
-                self.BREWING: [CallbackQueryHandler(self.code)],
-            },
-            fallbacks=self.fallbacks,
-            per_message=False,
-        )
-        assert len(recwarn) == 1
-        assert str(recwarn[0].message) == (
-            "If 'per_message=False', 'CallbackQueryHandler' will not be "
-            "tracked for every message."
-        )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
-
-    def test_warnings_per_chat_is_only_shown_once(self, recwarn):
-        def hello(update, context):
-            return self.BREWING
-
-        def bye(update, context):
-            return ConversationHandler.END
-
-        ConversationHandler(
-            entry_points=self.entry_points,
-            states={
-                self.THIRSTY: [InlineQueryHandler(hello)],
-                self.BREWING: [InlineQueryHandler(bye)],
-            },
-            fallbacks=self.fallbacks,
-            per_chat=True,
-        )
-        assert len(recwarn) == 1
-        assert str(recwarn[0].message) == (
-            "If 'per_chat=True', 'InlineQueryHandler' can not be used,"
-            " since inline queries have no chat context."
-        )
-        assert recwarn[0].filename == __file__, "incorrect stacklevel!"
 
     def test_nested_conversation_handler(self, dp, bot, user1, user2):
         self.nested_states[self.DRINKING] = [