Improve Threema text receiving workflow #700
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattwr18
commented
Feb 10, 2021
mattwr18
commented
Feb 10, 2021
Gemfile.lock
Outdated
| revision: 0819d4099b751a8d0bbda6bb7e14e2ee54b73d85 | ||
| branch: master | ||
| revision: 1eb86559a6572d22c3d16d800e664bff8483c48f | ||
| branch: feature-add-delivery-receipt-support |
mattwr18
commented
Feb 10, 2021
| @@ -3,21 +3,35 @@ | |||
| require 'openssl' | |||
|
|
|||
| class Threema::WebhookController < ApplicationController | |||
| protect_from_forgery with: :null_session | |||
| skip_before_action :require_login | |||
| skip_before_action :require_login, :verify_authenticity_token | |||
There was a problem hiding this comment.
we can skip the verify_authenticity_token here since it's a webhook and there would not be such a token.
mattwr18
commented
Feb 10, 2021
mattwr18
commented
Feb 10, 2021
roschaefer
approved these changes
Feb 11, 2021
| end | ||
|
|
||
| private | ||
|
|
||
| def threema_webhook_params | ||
| params.permit(:from, :to, :messageId, :date, :nonce, :box, :mac, :nickname) | ||
| end | ||
|
|
||
| def respond_to_unknown_content_and_prevent_retries(contributor) | ||
| ThreemaAdapter.new(message: Message.new(text: Setting.telegram_unknown_content_message, recipient: contributor)).send! |
There was a problem hiding this comment.
It would be better to have a dedicated Setting.threema_unknown_content_message because I saw that we mentioned Astrid's personal account in there. This would certainly be different for Threema then.
There was a problem hiding this comment.
Cool, didn't catch that part... I've added that in 8c0217a ... I noticed that our Telegram message says we don't support voice, though we do... I'll have to create another ticket to update that, and also remember to do the same for Threema when it gets out of date.
- Threema retry logic will attempt to send the webhook 3 times, 5 minutes apart, if the server does not respond with 200. After that, the message is lost.
- Rescue NameError returned from Threema::Receive::DeliveryReceipt not being declared in threema gem - respond to contributors trying to send files/images with message... return 200 to avoid Threema Gateway API retry logic
036de6d to
957c941
Compare
- to be able to access the message retrospectively - to keep consistent across the code base
- with Astrid's contact info
- to avoid false positives since is_a? would return true for classes which are inherited from, and even module namespaces
- Calling JSON.generate throws an error, decrypted_message.content is already ready to be used by StringIO.new
- this will be changed in future to send back a message, probably, but for now it's sufficient to avoid retries if we don't know who the user is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
minutes apart, if the server does not respond with 200. After that,
the message is lost.
I'd carry on working on this
PR, I guess... Unless, we want to merge this in, then I could open antother PR.
Next steps:
Threema::Receive::TextThreema::Receive::Fileseparate issue created: Add support for Threema::Receive::File type #706Threema::Receive::Imageseparate issue created: Add support for Threema::Receive::Image #707Threema::Receive::DeliveryReceiptseparate issue created: Add support for Threema::Receive::DeliveryReceipt #708