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

Telegram webhook receiver #85

Merged
merged 10 commits into from Jul 20, 2021
Merged

Telegram webhook receiver #85

merged 10 commits into from Jul 20, 2021

Conversation

Tijani-Dia
Copy link
Collaborator

@Tijani-Dia Tijani-Dia commented Jul 19, 2021

PR to add Telegram webhook receiver.

Current limitations
When a message with multiple images is edited on Telegram, we only receive the edited message and the first photo of the message. This means that the other images of the message will be removed from the corresponding live post.

See #80 to find how to setup TelegramWebhookReceiver.

@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2021

Codecov Report

Merging #85 (6b61833) into main (f43c0c1) will increase coverage by 0.68%.
The diff coverage is 99.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #85      +/-   ##
==========================================
+ Coverage   98.11%   98.79%   +0.68%     
==========================================
  Files          17       20       +3     
  Lines         689      833     +144     
  Branches       61       81      +20     
==========================================
+ Hits          676      823     +147     
+ Misses          8        5       -3     
  Partials        5        5              
Impacted Files Coverage Δ
src/wagtail_live/receivers.py 98.47% <50.00%> (+2.20%) ⬆️
src/wagtail_live/adapters/telegram/__init__.py 100.00% <100.00%> (ø)
src/wagtail_live/adapters/telegram/receiver.py 100.00% <100.00%> (ø)
src/wagtail_live/adapters/telegram/utils.py 100.00% <100.00%> (ø)
src/wagtail_live/utils.py 95.83% <100.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f43c0c1...6b61833. Read the comment docs.

@kaedroho kaedroho requested a review from jacobtoppm July 19, 2021 12:40
Copy link
Collaborator

@lucasmoeskops lucasmoeskops left a comment

Choose a reason for hiding this comment

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

Great work! Lots of functionality :-) I tested it and it works very well!

I didn't really understand what was going on in the process_text method, so I looked at it and made a proposal to try and make it simpler / more readable.

src/wagtail_live/adapters/telegram/receiver.py Outdated Show resolved Hide resolved
src/wagtail_live/adapters/telegram/receiver.py Outdated Show resolved Hide resolved
src/wagtail_live/adapters/telegram/receiver.py Outdated Show resolved Hide resolved
text = text[:offset] + link + text[end:]
len_added = len(link) - length
cumulated_offset += len_added
len_text += len_added
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this manual algorithm is a bit too complicated which makes it hard to understand what happens. After some thinking I concluded that no extra variables are needed if the entities are process backwards, because the text will then only be edited at locations which are no longer matches by indices in the entity objects.

I propose the following algorithm:

# Process the entities in reversed order to be able to edit the text in place.
for entity in reversed(entities):
    url = ""
    start = entity['offset']
    end = start + entity['length']

    if entity["type"] == "url":
        url = description = text[start:end]

        if is_embed(url):
            # Check if this can match an embed block, if so no conversion happens.
            # It matches an embed block if it has a line in the text for itself.
            if end == len_text or text[end] == "\n":
                if start == 0 or text[start - 1] == "\n":
                    # This is an embed block, skip to the next entity
                    continue

    if entity["type"] == "text_link":
        url = entity["url"]
        description = text[start:end]

    if url:
        link = f'<a href="{url}">{description}</a>'
        text = text[:start] + link + text[end:]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very nice algorithm. Thanks!

@lucasmoeskops
Copy link
Collaborator

lucasmoeskops commented Jul 19, 2021

Unfortunate that Telegram doesn't send an update when the user deletes a message. Would it be nice to think of a work-around for this, or would this be out of scope?

I'm thinking of some string that would mark the message as deleted if the user puts it at the start, for example by editing the message "Hello world!" to become "DELETED Hello world!".

(It creates a bit of a weird case when the user then removes the "DELETED " part again then, so that makes it a bit harder.)

@Tijani-Dia
Copy link
Collaborator Author

Thanks for your nice review @lucasmoeskops .

Yes, indeed it's unfortunate that they don't send updates when a message is deleted, I'm not sure but I think it's due to the fact that after a certain amount of time you can no longer delete messages in Telegram (Again I'm not 100% sure).

It would be really nice to have a work-around for that. We can maybe fill an issue for it so we can give it a look later?

@lucasmoeskops lucasmoeskops merged commit 2602160 into main Jul 20, 2021
@Tijani-Dia Tijani-Dia deleted the telegram-receiver branch July 20, 2021 08:46
@allcaps allcaps removed the request for review from jacobtoppm July 20, 2021 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants