Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bot API 4.0 #1168

Merged
merged 94 commits into from
Aug 29, 2018
Merged

Bot API 4.0 #1168

merged 94 commits into from
Aug 29, 2018

Conversation

Eldinnie
Copy link
Member

@Eldinnie Eldinnie commented Jul 26, 2018

Closes #1165

'&public_key={}'.format(quote(public_key)) + \
'&payload={}'.format(quote(payload))
if callback_url:
url += '&callback_url={}'.format(escape(callback_url))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

escape is not correct here, because we need url encoding, not html escape. quote is the correct function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, thanks :)

callback_url (:obj:`str`, optional): URL to which the user will be redirected.

"""
url = 'tg://resolve?domain=telegrampassport' + \
Copy link
Contributor

@spontanurlaub spontanurlaub Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tg://resolve url is no longer clickable on Telegram. We can use t.me/telegrampassport to send a clickable url with the same functionality on telegram. But this url only works on Telegram Desktop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to do it on mobile at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, no. The t.me/telegrampassport url seems more like a bug in the username resolve process of Telegram Desktop. I already contacted Telegram Support and the bot Support. Telegram support didn't know anything about why the links are no longer clickable. Bot support did not reply yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@code1mountain Any updates on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just gonna remove the function for now. We can always add it in the future if we figure out how to do it properly.
(this should be okay since we haven't released it yet)

"""
url = 'tg://resolve?domain=telegrampassport' + \
'&bot_id={}'.format(bot_id) + \
'&scope={}'.format(quote(scope)) + \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quote somehow doesn't work to escape a list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we'll probably have to do like ','.join([quote(s) for s in scope]) it seems

digest.update(data)
data_hash = digest.finalize()
if data_hash != hash:
raise TelegramDecryptionError("Hashes are not equal! {} != {}".format(data_hash, hash))
Copy link
Contributor

@spontanurlaub spontanurlaub Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bot completely hangs up in polling mode if this error occurs, because it is raised when receiving the update and and it can't go on with the other updates. Even a bot restart does not fix it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's real bad :/ I'll do some debugging when I get time. Thanks :D I should probably write a test for that error too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a 100% sure that the test I wrote tests this completely... could you try your test with my latest commits?

Copy link
Member Author

@Eldinnie Eldinnie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I went over it all.
Made some changes to the docstrings I will push. Some small other changes too.

I think automa(t|g)ically decrypting the passportdata should be done explicit instead of implicit with a previously supplied key. I would like @python-telegram-bot/maintainers opinions on that please.

The docstrings for Passportstuff are still very unclear and copied from one item to another. These need to be fixed. Also the wiki doc needs to be written. (@jsmnbom already had that planned),

Lastly I'[ll try to improve testing. I would prefer the diff to be covered at least 90+%


class SecureData(TelegramObject):
"""
This object represents the credentials required to decrypt encrypted data.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstrings seems off


class SecureValue(TelegramObject):
"""
This object represents the credentials required to decrypt encrypted value.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@@ -19,6 +19,10 @@
"""This module contains helper functions."""
from html import escape

try:
from urllib import quote # noqa: F401
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing F401 is unused import. But it seems unused. Why is it imported?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good question.... removed :)

@@ -0,0 +1,82 @@
#!/usr/bin/env python
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the docstrings here. As far as I can see it currently decrypts everything when comming in. So docstrings are off...

@jsmnbom
Copy link
Member

jsmnbom commented Aug 29, 2018

Merging. Remaining errors are due to non-compliance with official API (missing bot API 4.1).

@jsmnbom jsmnbom merged commit 4689a80 into master Aug 29, 2018
@jsmnbom jsmnbom deleted the bot-api-4.0 branch September 1, 2018 17:45
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Telegram BotAPI Update July 26
4 participants