From 8abb51fbecbf7d2e962bfb21cb8e78b884dcbb47 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 | 96 +++++++++++++++---- crates/matrix-sdk/src/send_queue.rs | 12 ++- 4 files changed, 113 insertions(+), 18 deletions(-) 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 b6fb11bfca3..6b8605ba7e2 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 index ef5cb20d163..a6065e5f3d9 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/media.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/media.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{fs::File, io::Write as _, time::Duration}; +use std::{fs::File, io::Write as _, path::PathBuf, time::Duration}; use assert_matches::assert_matches; use assert_matches2::assert_let; @@ -35,6 +35,24 @@ use tempfile::TempDir; use tokio::time::sleep; use wiremock::ResponseTemplate; +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_send_attachment() { let mock = MatrixMockServer::new().await; @@ -48,6 +66,7 @@ async fn test_send_attachment() { let (items, mut timeline_stream) = timeline.subscribe_filter_map(|item| item.as_event().cloned()).await; + assert!(items.is_empty()); let f = EventFactory::new(); @@ -66,13 +85,7 @@ async fn test_send_attachment() { assert!(timeline_stream.next().now_or_never().is_none()); // Store a file in a temporary directory. - let tmp_dir = TempDir::new().unwrap(); - - let file_path = tmp_dir.path().join("test.bin"); - { - let mut file = File::create(&file_path).unwrap(); - file.write_all(b"hello world").unwrap(); - } + let (_tmp_dir, file_path) = create_temporary_file("test.bin"); // Set up mocks for the file upload. mock.mock_upload() @@ -94,16 +107,14 @@ async fn test_send_attachment() { { assert_let_timeout!(Some(VectorDiff::PushBack { value: item }) = timeline_stream.next()); assert_matches!(item.send_state(), Some(EventSendState::NotSentYet)); - assert_let!(TimelineItemContent::Message(msg) = item.content()); // Body is the caption, because there's both a caption and filename. assert_eq!(msg.body(), "caption"); - assert_let!(MessageType::File(file) = msg.msgtype()); - assert_eq!(file.filename(), "test.bin"); - assert_eq!(file.caption(), Some("caption")); + assert_eq!(get_filename_and_caption(msg.msgtype()), ("test.bin", Some("caption"))); // The URI refers to the local cache. + assert_let!(MessageType::File(file) = msg.msgtype()); assert_let!(MediaSource::Plain(uri) = &file.source); assert!(uri.to_string().contains("localhost")); } @@ -116,12 +127,11 @@ async fn test_send_attachment() { Some(VectorDiff::Set { index: 1, value: item }) = timeline_stream.next() ); assert_let!(TimelineItemContent::Message(msg) = item.content()); - - assert_let!(MessageType::File(file) = msg.msgtype()); - assert_eq!(file.filename(), "test.bin"); - assert_eq!(file.caption(), Some("caption")); + assert_matches!(item.send_state(), Some(EventSendState::NotSentYet)); + assert_eq!(get_filename_and_caption(msg.msgtype()), ("test.bin", Some("caption"))); // The URI now refers to the final MXC URI. + assert_let!(MessageType::File(file) = msg.msgtype()); assert_let!(MediaSource::Plain(uri) = &file.source); assert_eq!(uri.to_string(), "mxc://sdk.rs/media"); } @@ -139,3 +149,57 @@ async fn test_send_attachment() { // That's all, folks! assert!(timeline_stream.next().now_or_never().is_none()); } + +#[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/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.