Add both handlers for queries from new Payment API by jeffffc · Pull Request #631 · python-telegram-bot/python-telegram-bot

jeffffc · 2017-05-22T12:10:30Z

Added PreCheckoutQueryHandler and ShippingQueryHandler

(Re-open after merging #630 to beta branch)

# Conflicts: # telegram/__init__.py # telegram/message.py

* add handlers for new payment API * fix typo * fix docstring mistakes * added missing 'from_user'

bugfixes added payment-related handlers

jsmnbom · 2017-05-24T14:21:08Z

Oh I'd already handed adding all the needed filters in #622, but no worries, we'll just fix it in the merge I guess. Also it would probably have been better if you'd opened a new PR with all this extra stuff that did not get merged to the beta branch, instead of just piling it on, but we'll figure something out :)
EDIT: sorry, it seems you did commit the other changes to the beta branch, pardon me

tsnoam · 2017-06-02T08:46:24Z

@jeffffc

Would you mind merging from master?
When (& after) you merge please note the convention we use at functions like answer_shipping_query():
When checking if an optional parameter was supplied we compare it to None like this:

if ok is not None:
    do_something()

It's an important semantic difference as you might want to know that ok is actually False in contrary to never supplied.

# Conflicts: # telegram/bot.py # telegram/message.py # telegram/precheckoutquery.py # telegram/shippingquery.py

Merge from master and resolve conflicts

jeffffc · 2017-06-02T17:22:40Z

@tsnoam I have just resolved the conflicts and merged from master, please do tell me if there is any more problem
and yea it was my first time doing such coding, thanks for the advice, it helps me a lot also

jsmnbom

All in all looks good ^^
I've added a few comments in the code

jsmnbom · 2017-06-02T17:24:47Z


    game = _Game()

+    class _SuccessfulPayment(BaseFilter):


This filter seems to be duplicated as it's been added in another commit too ( see further down)

jsmnbom · 2017-06-02T17:26:23Z

        left_chat_participant  (:class:`telegram.User`): Use `left_chat_member`
        instead.

+<<<<<<<<< Temporary merge branch 1


Something weird happened here....
I think it can all just be deleted tbh :/

jsmnbom · 2017-06-02T17:28:06Z

+        self.assertTrue(Filters.successful_payment(self.message))
+        self.message.successful_payment = None
+        self.assertFalse(Filters.successful_payment(self.message))
+


Could you add a test for invoice too? I know you didn't actually add the filter, but i just saw that it was missing.

sure, meanwhile please check with the updated codes again, thanks a lot!

tsnoam

@jeffffc
Thank you for your contribution, it is highly appreciated.
I have left several comments on the code. Please take a look. Some of them are nitpicks, but should be quick fixes while you're fixing the more substantial parts.

tsnoam · 2017-06-02T20:30:05Z

            data['need_name'] = need_name
        if need_phone_number is not None:
            data['need_phone_number'] = need_phone_number
+        if need_email:


Please fix to: if need_email is not None

tsnoam · 2017-06-02T20:30:56Z

 from .shippingquery import ShippingQuery
 from .webhookinfo import WebhookInfo
 from .gamehighscore import GameHighScore
-from .videonote import VideoNote


nitpick:
why remove here and add above? (I prefer the minimal change in diff...)

That's on me... I moved it long ago to fix a circular import issue I had... not sure why it's showing up in this diff though...

well, what every git voodoo happened here, I prefer it not to be part of this PR

tsnoam · 2017-06-02T20:42:31Z

-        url = '{0]/answerPreCheckoutQuery'.format(self.base_url)
+
+        if ((ok is not True and error_message is None) or
+                (ok is True and error_message is not None)):


In here, for readability I would prefer a shorter form like this:

if not (ok ^ (error_message is None)):

sure i could not think of such short version, thank you

tsnoam · 2017-06-02T20:44:01Z

+            raise TelegramError(
+                'answerPreCheckoutQuery: If ok is True, there should '
+                'not be error_message; if ok is False, error_message '
+                'should not be empty')


Open for discussion:
I don't think that we should raise an exception here. If user wants to do unorthodox things, he might know better than us. At most I would issue a warning here.

Warning seems better... especially since we don't raise errors anywhere else if arguments are wrong, so it would be a bit strange to do it here imo

I'm ok with a warning.

okay for this one i believe i checked with the other existing methods beforehand about these mutual exclusive parameters
i raise exception there was because i was following edit_message_caption and similar ones with either inline messages or normal messages
what do you guys think?

@tsnoam @bomjacob what do you guys think about this?

Hmm.. you seen to be correct... I don't really have a preference either way then... It's your call @tsnoam

@jeffffc
lol... edit_message_caption exception is my doing. lets keep it that way.

tsnoam · 2017-06-02T20:46:14Z

+
+    def check_update(self, update):
+        if isinstance(update, Update) and update.pre_checkout_query:
+            return True


too long.... this would be sufficient:

return isinstance(update, Update) and update.pre_checkout_query

tsnoam · 2017-06-02T20:46:59Z

+
+    def check_update(self, update):
+        if isinstance(update, Update) and update.shipping_query:
+            return True


same comments like above (too long....)

tsnoam · 2017-06-02T20:47:27Z


        left_chat_participant  (:class:`telegram.User`): Use `left_chat_member`
        instead.
-


please don't remove that line.

tsnoam · 2017-06-02T20:48:16Z

        total_amount (int): Total price in the smallest units of the currency (integer)
        invoice_payload (str): Bot specified invoice payload
+
+    Keyword Args:


These are not Keyword Args in the sense of how documentation is generated. Please remove.
Also, bot is missing from the docstring, please add it..

tsnoam · 2017-06-02T20:54:11Z

                 chosen_inline_result=None,
                 callback_query=None,
+                 shipping_query=None,
+                 pre_checkout_query=None,


For backward compatibility (if some1 for any reason, decided to use Update directly) please add the new parameters to the end (just before **kwargs).

tsnoam

I removed some of my comments. They were wrong.

tsnoam · 2017-06-07T20:38:28Z

@jeffffc
I'm sorry for the delay in my response, I had several busy days.
I responded to your question above.

jeffffc · 2017-06-08T01:51:06Z

@tsnoam It's alright!
and @bomjacob I pushed the fixes already, please have a brief check, thanks!

tsnoam · 2017-06-09T10:09:31Z

LGTM. Waiting for travis before merging.

tsnoam · 2017-06-09T15:23:46Z

@jeffffc
Thank you for your contribution.

jeffffc · 2017-06-10T09:51:20Z

great, thank you all ;)

jsmnbom and others added 21 commits May 21, 2017 15:42

Merge branch 'payment' into beta

d220ff4

Merge branch 'videonote' into beta

48fa3d9

# Conflicts: # telegram/__init__.py # telegram/message.py

Merge branch 'may18minor' into beta

acda19b

Import order fix

01430a2

add handlers for new payment API

2c05b03

fix typo

ee057e7

fix docstring mistakes

cc03649

added missing 'from_user'

a78f72c

Add both handlers for queries from new Payment API (#630)

3767d26

* add handlers for new payment API * fix typo * fix docstring mistakes * added missing 'from_user'

fix typo, add bot and kwargs to class init

8f2f29c

add kwargs to answer* methods

1e250f2

add checks for answer* methods

1142953

fix answer* methods not sending api requests

43f4128

add successful_payment filter under message

27238c5

fix crucial typo in filters

9617111

fix typo...

73ac259

Merge branch 'fix-paymenthandlers' into paymenthandlers-final

18822d1

fix authors typo

05ed693

Finalize Payment API

1210e4e

bugfixes added payment-related handlers

add missing "need_email" in sendInvoice, fix pep8/flake

1bf0078

fix typo again

f314915

jsmnbom mentioned this pull request Jun 2, 2017

Payment stuff #622

Merged

jeffffc added 2 commits June 3, 2017 01:09

Merge branch 'master' into paymenthandlers-new

e9d08c6

# Conflicts: # telegram/bot.py # telegram/message.py # telegram/precheckoutquery.py # telegram/shippingquery.py

Merge from master and resolve conflicts

f735a37

Merge from master and resolve conflicts

jsmnbom requested changes Jun 2, 2017

View reviewed changes

Remove duplicated/useless codes, added Filters.invoice test

eaf765d

jsmnbom approved these changes Jun 2, 2017

View reviewed changes

Fix typo in filter test

b6ba66b

tsnoam requested changes Jun 2, 2017

View reviewed changes

tsnoam reviewed Jun 2, 2017

View reviewed changes

tiny changes upon PR review

4247dc0

tsnoam approved these changes Jun 9, 2017

View reviewed changes

tsnoam merged commit da8a3ce into python-telegram-bot:master Jun 9, 2017

github-actions Bot locked and limited conversation to collaborators Aug 23, 2020


		left_chat_participant (:class:`telegram.User`): Use `left_chat_member`
		instead.

Uh oh!

Conversation

jeffffc commented May 22, 2017

Uh oh!

jsmnbom commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsnoam commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffffc commented Jun 2, 2017

Uh oh!

jsmnbom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsmnbom Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsnoam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsnoam left a comment

Choose a reason for hiding this comment

Uh oh!

tsnoam commented Jun 7, 2017

Uh oh!

jeffffc commented Jun 8, 2017

Uh oh!

tsnoam commented Jun 9, 2017

Uh oh!

tsnoam commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffffc commented Jun 10, 2017

Uh oh!

Reviewers

Assignees

jsmnbom commented May 24, 2017 •

edited

Loading

tsnoam commented Jun 2, 2017 •

edited

Loading

jsmnbom Jun 2, 2017 •

edited

Loading

tsnoam commented Jun 9, 2017 •

edited

Loading