From b852a6866d47d2ad5847e7d2a014b65f5c3cb085 Mon Sep 17 00:00:00 2001 From: Poolitzer Date: Sat, 6 Nov 2021 15:19:21 +0100 Subject: [PATCH] Refactor Warnings in ConversationHandler (#2755, #2784) --- .github/pull_request_template.md | 1 + telegram/ext/_conversationhandler.py | 119 +++++++++----- tests/test_conversationhandler.py | 236 +++++++++++++++++---------- 3 files changed, 229 insertions(+), 127 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 332fdde09..ca3b7bf4f 100644 --- a/.github/pull_request_template.md +++ b/.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` diff --git a/telegram/ext/_conversationhandler.py b/telegram/ext/_conversationhandler.py index f322ece9f..269bfd32c 100644 --- a/telegram/ext/_conversationhandler.py +++ b/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: - for handler in all_handlers: - 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 + # this loop is going to warn the user about handlers which can work unexpected + # in conversations - if self.per_chat: - for handler in all_handlers: - if isinstance(handler, (InlineQueryHandler, ChosenInlineResultHandler)): - warn( - "If 'per_chat=True', 'InlineQueryHandler' can not be used, " - "since inline queries have no chat context.", - stacklevel=2, - ) - break + # this link will be added to all warnings tied to per_* setting + per_faq_link = ( + " Read this FAQ entry to learn more about the per_* settings: https://git.io/JtcyU." + ) - if self.conversation_timeout: - for handler in all_handlers: - if 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, - ) - break + for handler in all_handlers: + if isinstance(handler, (StringCommandHandler, StringRegexHandler)): + warn( + "The `ConversationHandler` only handles updates of type `telegram.Update`. " + f"{handler.__class__.__name__} handles updates of type `str`.", + stacklevel=2, + ) + elif isinstance(handler, TypeHandler) and not issubclass(handler.type, Update): + warn( + "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: - for handler in all_handlers: + elif self.per_chat and ( + 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 diff --git a/tests/test_conversationhandler.py b/tests/test_conversationhandler.py index 781b3704f..772127e39 100644 --- a/tests/test_conversationhandler.py +++ b/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): - ConversationHandler( - entry_points=self.entry_points, - 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_handlers_generate_warning(self, recwarn): + """ + this function tests all handler + per_* setting combinations. + """ - 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: [CommandHandler('startCoding', self.code)], + self.BREWING: [ + 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): - ConversationHandler( - entry_points=self.entry_points, - 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!" + # this for loop checks if the correct stacklevel is used when generating the warning + for warning in recwarn: + assert warning.filename == __file__, "incorrect stacklevel!" def test_nested_conversation_handler(self, dp, bot, user1, user2): self.nested_states[self.DRINKING] = [