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

Mark Threema outbound messages received/read #1842

Merged
merged 5 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ gem 'rack-attack', '~> 6.6'
# Channel adapters
gem 'postmark-rails'
gem 'telegram-bot'
gem 'threema', git: 'https://github.com/threemarb/threema.git', branch: 'master'
gem 'threema', git: 'https://github.com/threemarb/threema.git', branch: '39_improve_delivery_receipt_class'
gem 'twilio-ruby'

# User management
Expand Down
10 changes: 5 additions & 5 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
GIT
remote: https://github.com/threemarb/threema.git
revision: f29f1f94917a29799c61ac950c31981606ada19d
branch: master
revision: 7f6e30111abee4111bd133960041902f9b683130
branch: 39_improve_delivery_receipt_class
specs:
threema (0.1.0)
case_transform
Expand Down Expand Up @@ -231,9 +231,9 @@ GEM
marcel (1.0.4)
matrix (0.4.2)
method_source (1.0.0)
mime-types (3.4.1)
mime-types (3.5.2)
mime-types-data (~> 3.2015)
mime-types-data (3.2022.0105)
mime-types-data (3.2024.0305)
mimemagic (0.4.3)
nokogiri (~> 1)
rake
Expand All @@ -242,7 +242,7 @@ GEM
mini_portile2 (2.8.5)
minitest (5.16.3)
msgpack (1.7.1)
multipart-post (2.2.3)
multipart-post (2.4.0)
nio4r (2.5.8)
nokogiri (1.16.3)
mini_portile2 (~> 2.8.2)
Expand Down
7 changes: 6 additions & 1 deletion app/adapters/threema_adapter/inbound.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module ThreemaAdapter
UNSUBSCRIBE_CONTRIBUTOR = :unsubscribe_contributor
RESUBSCRIBE_CONTRIBUTOR = :resubscribe_contributor
UNSUPPORTED_CONTENT = :unsupported_content
HANDLE_DELIVERY_RECEIPT = :handle_delivery_receipt

class Inbound
UNSUPPORTED_CONTENT_TYPES = %w[application text/x-vcard].freeze
Expand All @@ -20,11 +21,15 @@ def on(callback, &block)

def consume(threema_message)
decrypted_message = Threema.new.receive(payload: threema_message)
return if delivery_receipt?(decrypted_message)

@sender = initialize_sender(threema_message)
return unless @sender

if delivery_receipt?(decrypted_message)
trigger(HANDLE_DELIVERY_RECEIPT, decrypted_message)
return
end

@message = initialize_message(decrypted_message)
return unless @message

Expand Down
5 changes: 3 additions & 2 deletions app/adapters/threema_adapter/outbound.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,14 @@ def send_files(files, message)
file_path: ActiveStorage::Blob.service.path_for(file.attachment.blob.key),
file_name: file.attachment.blob.filename.to_s,
caption: index.zero? ? message.text : nil,
render_type: :media
render_type: :media,
message: message
)
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

end
end
end
Expand Down
17 changes: 10 additions & 7 deletions app/adapters/threema_adapter/outbound/file.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,19 @@ def self.threema_instance
@threema_instance ||= Threema.new
end

def perform(contributor_id:, file_path:, file_name: nil, caption: nil, render_type: nil)
def perform(contributor_id:, file_path:, file_name: nil, caption: nil, render_type: nil, message: nil)
recipient = Contributor.find_by(id: contributor_id)
return unless recipient

self.class.threema_instance.send(type: :file,
threema_id: recipient.threema_id.upcase,
file: file_path,
render_type: render_type,
file_name: file_name,
caption: caption)
message_id = self.class.threema_instance.send(type: :file,
threema_id: recipient.threema_id.upcase,
file: file_path,
render_type: render_type,
file_name: file_name,
caption: caption)
return unless message

message.update(external_id: message_id)
end
end
end
Expand Down
8 changes: 6 additions & 2 deletions app/adapters/threema_adapter/outbound/text.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,15 @@ def self.threema_instance
@threema_instance ||= Threema.new
end

def perform(contributor_id:, text: nil)
def perform(contributor_id:, text: nil, message: nil)
recipient = Contributor.find(contributor_id)
return unless recipient

self.class.threema_instance.send(type: :text, threema_id: recipient.threema_id.upcase, text: text)
message_id = self.class.threema_instance.send(type: :text, threema_id: recipient.threema_id.upcase, text: text)

return unless message

message.update(external_id: message_id)
end
end
end
Expand Down
25 changes: 25 additions & 0 deletions app/controllers/threema/webhook_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ def message
ResubscribeContributorJob.perform_later(contributor.id, ThreemaAdapter::Outbound)
end

adapter.on(ThreemaAdapter::HANDLE_DELIVERY_RECEIPT) do |delivery_receipt|
handle_delivery_receipt(delivery_receipt)
end

adapter.consume(threema_webhook_params) do |message|
message.contributor.reply(adapter)
end
Expand All @@ -43,4 +47,25 @@ def handle_unknown_contributor(threema_id)
exception = ThreemaAdapter::UnknownContributorError.new(threema_id: threema_id)
ErrorNotifier.report(exception)
end

def handle_delivery_receipt(delivery_receipt)
return if delivery_receipt.message_ids.blank?

messages = Message.where(external_id: delivery_receipt.message_ids)
messages.each do |message|
delivery_receipt.message_ids.each do |message_id|
next unless message.external_id.eql?(message_id)

local_datetime = Time.zone.at(delivery_receipt.timestamp).to_datetime

message.update(received_at: local_datetime) if delivery_receipt.status.eql?(:received)

next unless delivery_receipt.status.eql?(:read)

message.read_at = local_datetime
message.received_at = local_datetime if message.received_at.blank?
message.save
end
end
end
end
36 changes: 35 additions & 1 deletion spec/adapters/threema_adapter/inbound_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
end
let(:threema_mock) { instance_double(Threema::Receive::Text, content: 'Hello World!') }
let(:threema) { instance_double(Threema) }
let(:messages) { [create(:message, external_id: SecureRandom.alphanumeric(16))] }
let(:message_ids) { messages.pluck(:external_id) }
let(:status) { :received }
let(:timestamp) { Time.current.to_i }

before do
allow(Threema).to receive(:new).and_return(threema)
allow(threema).to receive(:receive).with({ payload: threema_message }).and_return(threema_mock)
Expand All @@ -42,7 +47,11 @@
before { allow(threema_mock).to receive(:instance_of?).with(Threema::Receive::DeliveryReceipt).and_return(true) }

context 'Threema::Receive::DeliveryReceipt' do
let(:threema_mock) { instance_double(Threema::Receive::DeliveryReceipt, content: 'x\00x\\0') }
let(:threema_mock) do
instance_double(
Threema::Receive::DeliveryReceipt, content: 'x\00x\\0', message_ids: message_ids, status: status, timestamp: timestamp
)
end

it { is_expected.to be(nil) }
end
Expand Down Expand Up @@ -302,5 +311,30 @@
end
end
end

describe 'HANDLE_DELIVERY_RECEIPT' do
let(:handle_delivery_receipt_callback) { spy('handle_delivery_receipt_callback') }
let(:threema_mock) do
instance_double(
Threema::Receive::DeliveryReceipt, content: 'x\00x\\0', message_ids: message_ids, status: status, timestamp: timestamp
)
end

before do
allow(threema_mock).to receive(:instance_of?).with(Threema::Receive::DeliveryReceipt).and_return(true)
adapter.on(ThreemaAdapter::HANDLE_DELIVERY_RECEIPT) do |delivery_receipt|
handle_delivery_receipt_callback.call(delivery_receipt)
end
end

subject do
adapter.consume(threema_message)
handle_delivery_receipt_callback
end

describe 'if the message is a delivery receipt' do
it { should have_received(:call) }
end
end
end
end
28 changes: 27 additions & 1 deletion spec/adapters/threema_adapter/outbound/file_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,16 @@
caption: message.text
}
end
let(:message_id) { SecureRandom.alphanumeric(16) }
let(:threema_lookup_double) { instance_double(Threema::Lookup) }

describe '#perform' do
before do
message.reload
allow(Threema).to receive(:new).and_return(threema_double)
allow(ThreemaAdapter::Outbound::File).to receive(:threema_instance).and_return(threema_double)
allow(Threema::Lookup).to receive(:new).and_call_original
allow(Threema::Lookup).to receive(:new).with({ threema: threema_double }).and_return(threema_lookup_double)
allow(threema_lookup_double).to receive(:key).and_return('PUBLIC_KEY_HEX_ENCODED')
end
subject do
lambda {
Expand All @@ -42,5 +47,26 @@

subject.call
end

context 'when a message is passed in' do
subject do
lambda {
adapter.perform(contributor_id: message.recipient.id,
file_path: file_path,
file_name: message.files.first.attachment.blob.filename.to_s,
caption: message.text,
render_type: :media,
message: message)
}
end

before do
allow(threema_double).to receive(:send).with(expected_params).and_return(message_id)
end

it "saves the returned message id to the message's external_id" do
expect { subject.call }.to change { message.reload.external_id }.from(nil).to(message_id)
end
end
end
end
25 changes: 23 additions & 2 deletions spec/adapters/threema_adapter/outbound/text_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,22 +9,43 @@
build(:contributor, threema_id: threema_id, email: nil).tap { |contributor| contributor.save(validate: false) }
end
let(:message) { create(:message, recipient: contributor) }
let(:threema_double) { instance_double(Threema) }
let(:threema_lookup_double) { instance_double(Threema::Lookup) }
let(:message_id) { SecureRandom.alphanumeric(16) }

before do
allow(ThreemaAdapter::Outbound::Text).to receive(:threema_instance).and_return(threema_double)
allow(Threema::Lookup).to receive(:new).and_call_original
allow(Threema::Lookup).to receive(:new).with({ threema: threema_double }).and_return(threema_lookup_double)
allow(threema_lookup_double).to receive(:key).and_return('PUBLIC_KEY_HEX_ENCODED')
allow(threema_double).to receive(:send).with(type: :text, threema_id: threema_id, text: message.text).and_return(message_id)
end

describe '#perform' do
subject { -> { adapter.perform(text: message.text, contributor_id: message.recipient.id) } }

it 'sends the message' do
expect_any_instance_of(Threema).to receive(:send).with(type: :text, threema_id: 'V5EA564T', text: message.text)
expect(threema_double).to receive(:send).with(type: :text, threema_id: threema_id, text: message.text)

subject.call
end

context 'with lowercase Threema ID' do
let(:threema_id) { 'v5ea564t' }

it 'converts ID to uppercase' do
expect_any_instance_of(Threema).to receive(:send).with(type: :text, threema_id: 'V5EA564T', text: message.text)
expect(threema_double).to receive(:send).with(type: :text, threema_id: threema_id.upcase, text: message.text)

subject.call
end
end

context 'when a message is passed in' do
subject { -> { adapter.perform(text: message.text, contributor_id: message.recipient.id, message: message) } }

it "saves the returned message id to the message's external_id" do
expect { subject.call }.to change { message.reload.external_id }.from(nil).to(message_id)
end
end
end
end
40 changes: 39 additions & 1 deletion spec/requests/threema/webhook_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,51 @@
it_behaves_like 'an ActivityNotification', 'MessageReceived'

describe 'DeliveryReceipt' do
let(:threema_mock) { instance_double(Threema::Receive::DeliveryReceipt, content: 'x\00x\\0') }
let(:threema_mock) do
instance_double(
Threema::Receive::DeliveryReceipt, content: 'x\00x\\0', message_ids: message_ids, status: status, timestamp: timestamp
)
end
let(:messages) { [create(:message, external_id: SecureRandom.alphanumeric(16))] }
let(:message_ids) { messages.pluck(:external_id) }
let(:status) { :received }
let(:timestamp) { Time.current.to_i }
before { allow(threema_mock).to receive(:instance_of?).with(Threema::Receive::DeliveryReceipt).and_return(true) }

it 'returns 200 to avoid retries' do
subject
expect(response).to have_http_status(200)
end

context 'given a received status for a known message' do
it 'updates the received_at attr' do
expect { subject }.to change { messages.first.reload.received_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone))
end
end

context 'given a read status for a known message' do
let(:status) { :read }

it 'updates the read_at attr' do
expect { subject }.to change { messages.first.reload.read_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone))
end

it 'updates receive_at if blank' do
expect { subject }.to change { messages.first.reload.received_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone))
end
end

context 'given multiple message_ids' do
let(:messages) { create_list(:message, 3, external_id: SecureRandom.alphanumeric(16)) }
let(:message_ids) { messages.pluck(:external_id) }
let(:status) { :read }

it 'updates all messages' do
expect { subject }.to change { messages.first.reload.read_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone)).and \
change { messages.second.reload.read_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone)).and \
change { messages.third.reload.read_at }.from(nil).to(kind_of(ActiveSupport::TimeWithZone))
end
end
end

describe 'Threema::Receive::File' do
Expand Down
Loading