From 003a45b51a363f5771cde5446553f9d3d6e46636 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Tue, 19 Nov 2024 14:27:21 +0100 Subject: [PATCH] fix(send queue): when adding a local reaction, look for media events in dependent requests too --- .../matrix-sdk-base/src/store/send_queue.rs | 21 ++++ .../src/timeline/controller/mod.rs | 2 +- .../tests/integration/timeline/media.rs | 98 +++++++++++++++++++ .../tests/integration/timeline/mod.rs | 1 + crates/matrix-sdk/src/send_queue.rs | 12 ++- 5 files changed, 132 insertions(+), 2 deletions(-) create mode 100644 crates/matrix-sdk-ui/tests/integration/timeline/media.rs diff --git a/crates/matrix-sdk-base/src/store/send_queue.rs b/crates/matrix-sdk-base/src/store/send_queue.rs index 4d3b4a76bb8..6e7aac5f072 100644 --- a/crates/matrix-sdk-base/src/store/send_queue.rs +++ b/crates/matrix-sdk-base/src/store/send_queue.rs @@ -367,6 +367,27 @@ pub struct DependentQueuedRequest { pub parent_key: Option, } +impl DependentQueuedRequest { + /// Does the dependent request represent a new event that is *not* + /// aggregated, aka it is going to be its own item in a timeline? + pub fn is_own_event(&self) -> bool { + match self.kind { + DependentQueuedRequestKind::EditEvent { .. } + | DependentQueuedRequestKind::RedactEvent + | DependentQueuedRequestKind::ReactEvent { .. } + | DependentQueuedRequestKind::UploadFileWithThumbnail { .. } => { + // These are all aggregated events, or non-visible items (file upload producing + // a new MXC ID). + false + } + DependentQueuedRequestKind::FinishUpload { .. } => { + // This one graduates into a new media event. + true + } + } + } +} + #[cfg(not(tarpaulin_include))] impl fmt::Debug for QueuedRequest { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 8f09d4f3095..b95aeec81f5 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -505,7 +505,7 @@ impl TimelineController

{ let Some(prev_status) = prev_status else { match &item.kind { EventTimelineItemKind::Local(local) => { - if let Some(send_handle) = local.send_handle.clone() { + if let Some(send_handle) = &local.send_handle { if send_handle .react(key.to_owned()) .await diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/media.rs b/crates/matrix-sdk-ui/tests/integration/timeline/media.rs new file mode 100644 index 00000000000..684decb9cae --- /dev/null +++ b/crates/matrix-sdk-ui/tests/integration/timeline/media.rs @@ -0,0 +1,98 @@ +// Copyright 2024 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::{fs::File, io::Write as _, path::PathBuf}; + +use assert_matches2::assert_let; +use eyeball_im::VectorDiff; +use futures_util::{FutureExt, StreamExt}; +use matrix_sdk::{ + assert_let_timeout, attachment::AttachmentConfig, test_utils::mocks::MatrixMockServer, +}; +use matrix_sdk_test::async_test; +use matrix_sdk_ui::timeline::{RoomExt, TimelineItemContent}; +use ruma::{events::room::message::MessageType, room_id}; +use tempfile::TempDir; + +fn create_temporary_file(filename: &str) -> (TempDir, PathBuf) { + let tmp_dir = TempDir::new().unwrap(); + let file_path = tmp_dir.path().join(filename); + let mut file = File::create(&file_path).unwrap(); + file.write_all(b"hello world").unwrap(); + (tmp_dir, file_path) +} + +fn get_filename_and_caption(msg: &MessageType) -> (&str, Option<&str>) { + match msg { + MessageType::File(event) => (event.filename(), event.caption()), + MessageType::Image(event) => (event.filename(), event.caption()), + MessageType::Video(event) => (event.filename(), event.caption()), + MessageType::Audio(event) => (event.filename(), event.caption()), + _ => panic!("unexpected message type"), + } +} + +#[async_test] +async fn test_react_to_local_media() { + let mock = MatrixMockServer::new().await; + let client = mock.client_builder().build().await; + + // Disable the sending queue, to simulate offline mode. + client.send_queue().set_enabled(false).await; + + mock.mock_room_state_encryption().plain().mount().await; + + let room_id = room_id!("!a98sd12bjh:example.org"); + let room = mock.sync_joined_room(&client, room_id).await; + let timeline = room.timeline().await.unwrap(); + + let (items, mut timeline_stream) = + timeline.subscribe_filter_map(|item| item.as_event().cloned()).await; + + assert!(items.is_empty()); + assert!(timeline_stream.next().now_or_never().is_none()); + + // Store a file in a temporary directory. + let (_tmp_dir, file_path) = create_temporary_file("test.bin"); + + // Queue sending of an attachment (no captions). + let config = AttachmentConfig::new(); + timeline.send_attachment(&file_path, mime::TEXT_PLAIN, config).use_send_queue().await.unwrap(); + + let item_id = { + assert_let_timeout!(Some(VectorDiff::PushBack { value: item }) = timeline_stream.next()); + assert_let!(TimelineItemContent::Message(msg) = item.content()); + assert_eq!(get_filename_and_caption(msg.msgtype()), ("test.bin", None)); + + // The item starts with no reactions. + assert!(item.reactions().is_empty()); + + item.identifier() + }; + + // Add a reaction to the file media event. + timeline.toggle_reaction(&item_id, "🤪").await.unwrap(); + + assert_let_timeout!(Some(VectorDiff::Set { index: 0, value: item }) = timeline_stream.next()); + assert_let!(TimelineItemContent::Message(msg) = item.content()); + assert_eq!(get_filename_and_caption(msg.msgtype()), ("test.bin", None)); + + // There's a reaction for the current user for the given emoji. + let reactions = item.reactions(); + let own_user_id = client.user_id().unwrap(); + reactions.get("🤪").unwrap().get(own_user_id).unwrap(); + + // That's all, folks! + assert!(timeline_stream.next().now_or_never().is_none()); +} diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs index b6bc4211140..a70f3a499ff 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs @@ -53,6 +53,7 @@ use crate::mock_sync; mod echo; mod edit; mod focus_event; +mod media; mod pagination; mod pinned_event; mod profiles; diff --git a/crates/matrix-sdk/src/send_queue.rs b/crates/matrix-sdk/src/send_queue.rs index c248dc841e9..3ab04db900a 100644 --- a/crates/matrix-sdk/src/send_queue.rs +++ b/crates/matrix-sdk/src/send_queue.rs @@ -1270,7 +1270,17 @@ impl QueueStorage { // If the target event has been already sent, abort immediately. if !requests.iter().any(|item| item.transaction_id == transaction_id) { - return Ok(None); + // We didn't find it as a queued request; try to find it as a dependent queued + // request. + let dependent_requests = store.load_dependent_queued_requests(&self.room_id).await?; + if !dependent_requests + .into_iter() + .filter_map(|item| item.is_own_event().then_some(item.own_transaction_id)) + .any(|child_txn| *child_txn == *transaction_id) + { + // We didn't find it as either a request or a dependent request, abort. + return Ok(None); + } } // Record the dependent request.