Add `invoice_payload` filtering #4005

aelkheir · 2023-12-12T13:42:43Z

Added .. versionadded:: NEXT.VERSION, .. versionchanged:: NEXT.VERSION or .. deprecated:: NEXT.VERSION to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
Created new or adapted existing unit tests
Documented code changes according to the CSI standard <https://standards.mousepawmedia.com/en/stable/csi.html>__
~~Added myself alphabetically to AUTHORS.rst (optional)~~
Added new classes & modules to the docs and all suitable __all__ s
Checked the Stability Policy <https://docs.python-telegram-bot.org/stability_policy.html>_ in case of deprecations or changes to documented behavior

why not add collect_additional_context in PreCheckoutQueryHandler to store the match since we're adding a pattern argument. i can do it if you approve.

Add class `filters.SuccessfulPayment` with support for `invoice_payload` filtering.

Bibo-Joshi · 2023-12-12T16:41:33Z

Thanks for the PR! I have two general comments that I'd like you to address before I review in details:

the pattern argument for PreCheckoutQueryHanlder accepting a type makes little sense, as the invoice payload will alsways be a string. I would even argue that we don't need to allow callables. This was added to CallbackQueryHandler only for usage with arbitrary callback_data and not for string data
Please add unit tests for the code changes and ensure that the existing ones don't fail. See also Filter support for invoice_payload in PreCheckoutQueryHandler and successful_payment message #3752 (comment). If you're new to writing unit tests and have problems getting started, that's ofc ok and I recommend to reach out via https://t.me/pythontelegrambotchannel/105 in this case.

aelkheir · 2023-12-12T17:48:16Z

Thanks for the feedback.

This was added to CallbackQueryHandler only for usage with arbitrary callback_data

Aha got it, I did not understand why it was there

I would even argue that we don't need to allow callables.

Ok. I thought it was needed because of this #3773 (comment)

Please add unit tests for the code changes and ensure that the existing ones don't fail.

On it.

aelkheir · 2023-12-12T20:47:02Z

I created some tests. I see some checks are still failing. Doc build is failing, although it built fine locally.

Fix an indentation error that was preventing sphinx-build from exiting with code 0.

aelkheir · 2023-12-14T17:55:24Z

Couldn't figure out why pytest (3.8, macos-latest) is still failing. But I think its ready for review. @Bibo-Joshi

…iltering

Bibo-Joshi

Thanks for the nice PR! I left a few smaller comments below :) the tests on macOS recently tend to fail due to flakyness in time-based jobs …

Bibo-Joshi · 2023-12-14T19:57:13Z

telegram/ext/_precheckoutqueryhandler.py

+    def __init__(
+        self,
+        callback: HandlerCallback[Update, CCT, RT],
+        pattern: Optional[Union[str, Pattern[str]]] = None,


please move the argument to the end of the list. Otherwise this would be a breaking change as PCQH(callback, False) would no longer work as expected. Please also move the docstring entries for argument and attribute above.

Bibo-Joshi · 2023-12-14T19:59:50Z

telegram/ext/_precheckoutqueryhandler.py

+        if isinstance(pattern, str):
+            pattern = re.compile(pattern)
+
+        self.pattern: Optional[Union[str, Pattern[str]]] = pattern


Suggested change

if isinstance(pattern, str):

pattern = re.compile(pattern)

self.pattern: Optional[Union[str, Pattern[str]]] = pattern

self.pattern: Optional[Pattern[str]] = re.compile(pattern) if pattern is not None else None

Just a bit shorter and also gives the attribute a more narrow type.
This way we can also use self.pattern.match(…) in check_update below :)

tests/ext/test_filters.py

Bibo-Joshi · 2023-12-14T20:06:33Z

tests/ext/test_precheckoutqueryhandler.py

@@ -103,6 +106,14 @@ async def callback(self, update, context):
            and isinstance(update.pre_checkout_query, PreCheckoutQuery)
        )

+    def test_with_pattern(self, pre_checkout_query):
+        handler = PreCheckoutQueryHandler(self.callback, pattern=".*voice.*")


please also test passing a compiled pattern :)

tests/ext/test_precheckoutqueryhandler.py

…iltering

aelkheir · 2023-12-15T12:49:11Z

Applied changes, ready for more

Bibo-Joshi

Thanks for the updates and sorry for the late review. LGTM now :) Let me check if one of the other team members can also have a second look.

Bibo-Joshi · 2023-12-22T14:47:31Z

telegram/ext/_precheckoutqueryhandler.py

        block: DVType[bool] = DEFAULT_TRUE,
+        pattern: Optional[Pattern[str]] = None,


Suggested change

pattern: Optional[Pattern[str]] = None,

pattern: Optional[Union[str, Pattern[str]]] = None,

accepting a string as input is okay, my point in #4005 (comment) was just that we can convert strings to patterns on init already :) sorry if that was unclear.

tests/ext/test_filters.py

tests/ext/test_precheckoutqueryhandler.py

Poolitzer

LGTM

Bibo-Joshi · 2024-01-02T17:35:50Z

Thanks very much for the nice contribution :)

aelkheir · 2024-01-03T06:52:52Z

Ha! No really, thank you. I learnt something new.

aelkheir added 2 commits December 12, 2023 15:49

Add 'SuccessfulPayment` message filter

96715df

Add class `filters.SuccessfulPayment` with support for `invoice_payload` filtering.

Add pattern arg to PreCheckoutQueryHandler

1259f69

aelkheir mentioned this pull request Dec 12, 2023

Filter support for invoice_payload in PreCheckoutQueryHandler and successful_payment message #3752

Closed

aelkheir added 3 commits December 12, 2023 22:58

Disallow types callable, type in PreCheckoutQueryHandler.pattern

954167d

Add 'SuccessfulPayment' to ext.filters.__all__

576f9d5

Add tests for filters.SuccessfulPayment and PreCheckoutQueryHandler

e7153cd

aelkheir added 3 commits December 14, 2023 00:27

Fix tests of _precheckoutqueryhandler.py

5324ab1

Fix docstring of telegram.ext.filters.SuccessfulPayment

3c8b054

Fix an indentation error that was preventing sphinx-build from exiting with code 0.

Fix: import Pattern from typing for python backward compatibility

84fd314

aelkheir changed the title ~~invoice_payload filtering in (new) filters.SuccessfulPayment and (existing) PreCheckoutQueryHandler~~ Add invoice_payload filtering Dec 14, 2023

aelkheir added 2 commits December 14, 2023 21:47

Fix docstring of arg PreCheckoutQueryHandler.pattern

75b05fa

Merge remote-tracking branch 'upstream/master' into invoice-payload-f…

c21e86d

…iltering

Bibo-Joshi requested changes Dec 14, 2023

View reviewed changes

Apply code review suggestions

25276a9

aelkheir commented Dec 14, 2023

View reviewed changes

tests/ext/test_precheckoutqueryhandler.py Show resolved Hide resolved

aelkheir requested a review from Bibo-Joshi December 14, 2023 23:49

Merge remote-tracking branch 'upstream/master' into invoice-payload-f…

f093609

…iltering

Bibo-Joshi approved these changes Dec 22, 2023

View reviewed changes

Improve typing

8b677e1

harshil21 self-requested a review December 30, 2023 02:24

harshil21 added this to the v20.8 milestone Dec 30, 2023

harshil21 added the enhancement label Dec 30, 2023

Poolitzer approved these changes Jan 2, 2024

View reviewed changes

Merge branch 'master' into invoice-payload-filtering

07492ea

Bibo-Joshi merged commit f3479cd into python-telegram-bot:master Jan 2, 2024
22 of 23 checks passed

aelkheir deleted the invoice-payload-filtering branch January 10, 2024 21:27

github-actions bot locked and limited conversation to collaborators Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `invoice_payload` filtering #4005

Add `invoice_payload` filtering #4005

aelkheir commented Dec 12, 2023 •

edited

Bibo-Joshi commented Dec 12, 2023

aelkheir commented Dec 12, 2023

aelkheir commented Dec 12, 2023 •

edited

aelkheir commented Dec 14, 2023

Bibo-Joshi left a comment

Bibo-Joshi Dec 14, 2023

Bibo-Joshi Dec 14, 2023

Bibo-Joshi Dec 14, 2023

aelkheir commented Dec 15, 2023

Bibo-Joshi left a comment

Bibo-Joshi Dec 22, 2023

Poolitzer left a comment

Bibo-Joshi commented Jan 2, 2024

aelkheir commented Jan 3, 2024

		block: DVType[bool] = DEFAULT_TRUE,
		pattern: Optional[Pattern[str]] = None,

	pattern: Optional[Pattern[str]] = None,
	pattern: Optional[Union[str, Pattern[str]]] = None,

Add invoice_payload filtering #4005

Add invoice_payload filtering #4005

Conversation

aelkheir commented Dec 12, 2023 • edited

Bibo-Joshi commented Dec 12, 2023

aelkheir commented Dec 12, 2023

aelkheir commented Dec 12, 2023 • edited

aelkheir commented Dec 14, 2023

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Bibo-Joshi Dec 14, 2023

Choose a reason for hiding this comment

Bibo-Joshi Dec 14, 2023

Choose a reason for hiding this comment

Bibo-Joshi Dec 14, 2023

Choose a reason for hiding this comment

aelkheir commented Dec 15, 2023

Bibo-Joshi left a comment

Choose a reason for hiding this comment

Bibo-Joshi Dec 22, 2023

Choose a reason for hiding this comment

Poolitzer left a comment

Choose a reason for hiding this comment

Bibo-Joshi commented Jan 2, 2024

aelkheir commented Jan 3, 2024

Add `invoice_payload` filtering #4005

Add `invoice_payload` filtering #4005

aelkheir commented Dec 12, 2023 •

edited

aelkheir commented Dec 12, 2023 •

edited