Mark Threema outbound messages received/read #1842
Conversation
| ) | ||
| end | ||
| end | ||
|
|
||
| def send_text(message) | ||
| ThreemaAdapter::Outbound::Text.perform_later(contributor_id: message.recipient.id, text: message.text) | ||
| ThreemaAdapter::Outbound::Text.perform_later(contributor_id: message.recipient.id, text: message.text, message: message) |
There was a problem hiding this comment.
The methods signature looks a bit strange now as you hand over some message attributes and now also the message itself. I've looked through the code and have seen that also on other adapters, so I wouldn't block that here. But in general I would either input only some attributes (preferred) or the object itself.
There was a problem hiding this comment.
I came to the same conclusion. The gap in understanding on my part was from the fact that these outbound text classes also handle sending text messages that have no message object in the db.
This happens for welcome messages, for example. See https://github.com/tactilenews/100eyes/blob/main/app/adapters/threema_adapter/outbound.rb#L25
There was a problem hiding this comment.
One final note on this before I merge. With the solution proposed in #1869, we could easily refactor to send out the welcome message, which going forward should always actually be saved in the db.
This would have the benefits of being able to refactor to always expect a message object be passed in to these outbound classes and we would be able to personalize our welcome message by inserting the contributors first name into the placeholder created in this welcome message request.
using DeliveryReceipts
Closes #1748
What's changed in this PR and why
We now save the return of the
Threem.new.sendcalls in ThreemaAdapter::Outbound::Text(File) classes and update the message's external id, if it exists. For the welcome message, for example, there is no message record created to update.Instead of actively ignoring
Threema::Receive::DeliveryReceiptin our TheemaAdapter::Inbound class, we now process them by triggering aHANDLE_DELIVERY_RECEIPTcallback, then checking if we can find the message records by their external_id and updating them if we do in our Theema::WebhookController.Only received and read receipts are considered, even though we can receive receipts with statuses
:explicitly_acknowlededand:explicitly_declinedas well. See What is not included in this PR and why for an explanation of why.We handle the case where we receive a single delivery receipt for multiple different message ids with the same status. This happens when someone receives several messages and later opens threema and reads them at the same time. I suspect it also happens if one has their phone turned off and receives several at the same time, though I have not manually tested this.
If we cannot find the message by its
external_id, it is a no-op asmessages = Message.where(external_id: delivery_receipt.message_ids)will return an empty array.We also check if the message record has not had the
received_atset while we mark theread_at. There was one time during manually testing that it seems we did not receive a received receipt for a message, but we did receive a read one. I think it's safe to say that if they have read the message, they also received it.The changes made to Gemfile* can be considered as temporary and will be reverted once the changes from Improve Threema::Receive::DeliveryReceipt class #40 have been merged in.
What's not included in this PR and why
:explicitly_acknowledgedand:explicitly_declinedreceipts. I have not tested this, but it is my understanding that this is when a threema user long clicks on the message and gives it a thumbs up or down respectively. The reason we did not do anything in this PR is that the messengers have different ways of handling these interactions. Our contributors should not be expected to know that these interactions will be ignored, so I do not want to lose this information by ignoring it.I think further discussion on how we should handle it is warranted. I don't think adding other attributes
:explicitly_acknowledged_atand:explicitly_declined_atwould be warranted since it only happens for Threema.In my opinion, we should not treat them as delivery receipts at all, but treat them as text with the emojis 👍 👎 . This is similar to how we receive the information from Signal, which also provides a way to react with emojis. See SignalAdapter::Inbound specs. Telegram does not offer this feature, it seems. WhatsApp does, but it's unclear how it's handled.
Testing
As long as we don't merge anything in that gets automatically deployed, one can test this at staging.100ey.es ... I've deployed a custom image there that allows testing.