From c4dd2d192ea3116529ff62fc1e1f1caae741f6d1 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Tue, 15 Oct 2024 16:36:34 +0200 Subject: [PATCH] refactor(timeline): fuse `redact_by_id()` within `redact()` Changelog: `Timeline::redact_by_id` has been fused into `Timeline::redact`, which now takes a `TimelineEventItemId` as an identifier of the item (local or remote) to redact. --- bindings/matrix-sdk-ffi/src/timeline/mod.rs | 9 +- crates/matrix-sdk-ui/src/timeline/error.rs | 5 +- crates/matrix-sdk-ui/src/timeline/mod.rs | 76 ++------ .../tests/integration/timeline/mod.rs | 183 +----------------- 4 files changed, 35 insertions(+), 238 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 87dbb8593..ea4395ee5 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -610,10 +610,11 @@ impl Timeline { event_or_transaction_id: EventOrTransactionId, reason: Option, ) -> Result<(), ClientError> { - self.inner - .redact_by_id(&(event_or_transaction_id.try_into()?), reason.as_deref()) - .await - .map_err(Into::into) + if !self.inner.redact(&(event_or_transaction_id.try_into()?), reason.as_deref()).await? { + // TODO make it a hard error instead + warn!("Couldn't redact item"); + } + Ok(()) } /// Load the reply details for the given event id. diff --git a/crates/matrix-sdk-ui/src/timeline/error.rs b/crates/matrix-sdk-ui/src/timeline/error.rs index b1a899564..bc881bc97 100644 --- a/crates/matrix-sdk-ui/src/timeline/error.rs +++ b/crates/matrix-sdk-ui/src/timeline/error.rs @@ -18,7 +18,6 @@ use matrix_sdk::{ send_queue::RoomSendQueueError, HttpError, }; -use ruma::OwnedTransactionId; use thiserror::Error; use crate::timeline::{pinned_events_loader::PinnedEventsLoaderError, TimelineEventItemId}; @@ -83,8 +82,8 @@ pub enum Error { #[derive(Error, Debug)] pub enum RedactError { /// Local event to redact wasn't found for transaction id - #[error("Local event to redact wasn't found for transaction {0}")] - LocalEventNotFound(OwnedTransactionId), + #[error("Event to redact wasn't found for item id {0:?}")] + ItemNotFound(TimelineEventItemId), /// An error happened while attempting to redact an event. #[error(transparent)] diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index 35adf7e87..4fee860a3 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -54,6 +54,7 @@ use ruma::{ }; use thiserror::Error; use tracing::{error, instrument, trace, warn}; +use util::rfind_event_by_item_id; use crate::timeline::pinned_events_loader::PinnedEventsRoom; @@ -602,41 +603,6 @@ impl Timeline { /// Redact an event given its [`TimelineEventItemId`] and an optional /// reason. /// - /// See [`Self::redact`] for more info. - pub async fn redact_by_id( - &self, - id: &TimelineEventItemId, - reason: Option<&str>, - ) -> Result<(), Error> { - let event_id = match id { - TimelineEventItemId::TransactionId(transaction_id) => { - let Some(item) = self.item_by_transaction_id(transaction_id).await else { - return Err(Error::RedactError(RedactError::LocalEventNotFound( - transaction_id.to_owned(), - ))); - }; - - match item.handle() { - TimelineItemHandle::Local(handle) => { - // If there is a local item that hasn't been sent yet, abort the upload - handle.abort().await.map_err(RoomSendQueueError::StorageError)?; - return Ok(()); - } - TimelineItemHandle::Remote(event_id) => event_id.to_owned(), - } - } - TimelineEventItemId::EventId(event_id) => event_id.to_owned(), - }; - self.room() - .redact(&event_id, reason, None) - .await - .map_err(|e| Error::RedactError(RedactError::HttpError(e)))?; - - Ok(()) - } - - /// Redact an event. - /// /// # Returns /// /// - Returns `Ok(true)` if the redact happened. @@ -646,33 +612,27 @@ impl Timeline { /// interacting with the sending queue. pub async fn redact( &self, - event: &EventTimelineItem, + item_id: &TimelineEventItemId, reason: Option<&str>, ) -> Result { - let event_id = match event.identifier() { - TimelineEventItemId::TransactionId(_) => { - // See if we have an up-to-date timeline item with that transaction id. - match event.handle() { - TimelineItemHandle::Remote(event_id) => event_id.to_owned(), - TimelineItemHandle::Local(handle) => { - return Ok(handle - .abort() - .await - .map_err(RoomSendQueueError::StorageError)?); - } - } - } - - TimelineEventItemId::EventId(event_id) => event_id, + let items = self.items().await; + let Some((_pos, event)) = rfind_event_by_item_id(&items, item_id) else { + return Err(Error::RedactError(RedactError::ItemNotFound(item_id.clone()))); }; - self.room() - .redact(&event_id, reason, None) - .await - .map_err(RedactError::HttpError) - .map_err(Error::RedactError)?; - - Ok(true) + match event.handle() { + TimelineItemHandle::Remote(event_id) => { + self.room() + .redact(event_id, reason, None) + .await + .map_err(RedactError::HttpError) + .map_err(Error::RedactError)?; + Ok(true) + } + TimelineItemHandle::Local(handle) => { + Ok(handle.abort().await.map_err(RoomSendQueueError::StorageError)?) + } + } } /// Fetch unavailable details about the event with the given ID. diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs index 76fb281f9..f2d151e5b 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/mod.rs @@ -276,9 +276,8 @@ async fn test_redact_message() { // Redacting a remote event works. mock_redaction(event_id!("$42")).mount(&server).await; - let event_id = first.as_event().unwrap(); - - let did_redact = timeline.redact(event_id, Some("inapprops")).await.unwrap(); + let did_redact = + timeline.redact(&first.as_event().unwrap().identifier(), Some("inapprops")).await.unwrap(); assert!(did_redact); // Redacting a local event works. @@ -301,7 +300,7 @@ async fn test_redact_message() { assert_matches!(second.send_state(), Some(EventSendState::SendingFailed { .. })); // Let's redact the local echo. - let did_redact = timeline.redact(second, None).await.unwrap(); + let did_redact = timeline.redact(&second.identifier(), None).await.unwrap(); assert!(did_redact); // Observe local echo being removed. @@ -371,151 +370,11 @@ async fn test_redact_local_sent_message() { mock_redaction(event.event_id().unwrap()).expect(1).mount(&server).await; // Let's redact the local echo with the remote handle. - timeline.redact(event, None).await.unwrap(); + timeline.redact(&event.identifier(), None).await.unwrap(); } #[async_test] -async fn test_redact_by_id_message() { - let room_id = room_id!("!a98sd12bjh:example.org"); - let (client, server) = logged_in_client_with_server().await; - let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000)); - - let mut sync_builder = SyncResponseBuilder::new(); - sync_builder.add_joined_room(JoinedRoomBuilder::new(room_id)); - - mock_sync(&server, sync_builder.build_json_sync_response(), None).await; - let _response = client.sync_once(sync_settings.clone()).await.unwrap(); - server.reset().await; - - mock_encryption_state(&server, false).await; - - let room = client.get_room(room_id).unwrap(); - let timeline = room.timeline().await.unwrap(); - let (_, mut timeline_stream) = timeline.subscribe().await; - - let factory = EventFactory::new(); - factory.set_next_ts(MilliSecondsSinceUnixEpoch::now().get().into()); - - sync_builder.add_joined_room( - JoinedRoomBuilder::new(room_id).add_timeline_event( - factory.sender(user_id!("@a:b.com")).text_msg("buy my bitcoins bro"), - ), - ); - - mock_sync(&server, sync_builder.build_json_sync_response(), None).await; - let _response = client.sync_once(sync_settings.clone()).await.unwrap(); - server.reset().await; - - assert_let!(Some(VectorDiff::PushBack { value: first }) = timeline_stream.next().await); - assert_eq!( - first.as_event().unwrap().content().as_message().unwrap().body(), - "buy my bitcoins bro" - ); - - assert_let!(Some(VectorDiff::PushFront { value: day_divider }) = timeline_stream.next().await); - assert!(day_divider.is_day_divider()); - - // Redacting a remote event works. - mock_redaction(event_id!("$42")).mount(&server).await; - - let event = first.as_event().unwrap(); - - timeline.redact_by_id(&event.identifier(), Some("inapprops")).await.unwrap(); - - // Redacting a local event works. - timeline - .send(RoomMessageEventContent::text_plain("i will disappear soon").into()) - .await - .unwrap(); - - assert_let!(Some(VectorDiff::PushBack { value: second }) = timeline_stream.next().await); - - let second = second.as_event().unwrap(); - assert_matches!(second.send_state(), Some(EventSendState::NotSentYet)); - - // We haven't set a route for sending events, so this will fail. - assert_let!(Some(VectorDiff::Set { index, value: second }) = timeline_stream.next().await); - assert_eq!(index, 2); - - let second = second.as_event().unwrap(); - assert!(second.is_local_echo()); - assert_matches!(second.send_state(), Some(EventSendState::SendingFailed { .. })); - - // Let's redact the local echo. - timeline.redact_by_id(&second.identifier(), None).await.unwrap(); - - // Observe local echo being removed. - assert_matches!(timeline_stream.next().await, Some(VectorDiff::Remove { index: 2 })); -} - -#[async_test] -async fn test_redact_by_local_sent_message() { - let room_id = room_id!("!a98sd12bjh:example.org"); - let (client, server) = logged_in_client_with_server().await; - let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000)); - - let mut sync_builder = SyncResponseBuilder::new(); - sync_builder.add_joined_room(JoinedRoomBuilder::new(room_id)); - - mock_sync(&server, sync_builder.build_json_sync_response(), None).await; - let _response = client.sync_once(sync_settings.clone()).await.unwrap(); - server.reset().await; - - mock_encryption_state(&server, false).await; - - let room = client.get_room(room_id).unwrap(); - let timeline = room.timeline().await.unwrap(); - let (_, mut timeline_stream) = timeline.subscribe().await; - - // Mock event sending. - Mock::given(method("PUT")) - .and(path_regex(r"^/_matrix/client/r0/rooms/.*/send/.*")) - .and(header("authorization", "Bearer 1234")) - .respond_with( - ResponseTemplate::new(200) - .set_body_json(json!({ "event_id": "$wWgymRfo7ri1uQx0NXO40vLJ" })), - ) - .expect(1) - .mount(&server) - .await; - - // Send the event so it's added to the send queue as a local event. - timeline - .send(RoomMessageEventContent::text_plain("i will disappear soon").into()) - .await - .unwrap(); - - // Assert the local event is in the timeline now and is not sent yet. - assert_let_timeout!(Some(VectorDiff::PushBack { value: item }) = timeline_stream.next()); - let event = item.as_event().unwrap(); - assert!(event.is_local_echo()); - assert_matches!(event.send_state(), Some(EventSendState::NotSentYet)); - - // As well as a day divider. - assert_let_timeout!( - Some(VectorDiff::PushFront { value: day_divider }) = timeline_stream.next() - ); - assert!(day_divider.is_day_divider()); - - // We receive an update in the timeline from the send queue. - assert_let_timeout!(Some(VectorDiff::Set { index, value: item }) = timeline_stream.next()); - assert_eq!(index, 1); - - // Check the event is sent but still considered local. - let event = item.as_event().unwrap(); - assert!(event.is_local_echo()); - assert_matches!(event.send_state(), Some(EventSendState::Sent { .. })); - - // Mock the redaction response for the event we just sent. Ensure it's called - // once. - mock_redaction(event.event_id().unwrap()).expect(1).mount(&server).await; - - // Let's redact the local echo with the remote handle. - timeline.redact_by_id(&event.identifier(), None).await.unwrap(); -} - -#[async_test] -async fn test_redact_by_id_message_with_no_remote_message_present() { +async fn test_redact_nonexisting_item() { let room_id = room_id!("!a98sd12bjh:example.org"); let (client, server) = logged_in_client_with_server().await; @@ -534,36 +393,14 @@ async fn test_redact_by_id_message_with_no_remote_message_present() { let timeline = room.timeline().await.unwrap(); let error = timeline - .redact_by_id(&TimelineEventItemId::EventId(owned_event_id!("$123:example.com")), None) + .redact(&TimelineEventItemId::EventId(owned_event_id!("$123:example.com")), None) .await .err(); - assert_matches!(error, Some(Error::RedactError(RedactError::HttpError(_)))) -} + assert_matches!(error, Some(Error::RedactError(RedactError::ItemNotFound(_)))); -#[async_test] -async fn test_redact_by_id_message_with_no_local_message_present() { - let room_id = room_id!("!a98sd12bjh:example.org"); - let (client, server) = logged_in_client_with_server().await; - - let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000)); - - let mut sync_builder = SyncResponseBuilder::new(); - sync_builder.add_joined_room(JoinedRoomBuilder::new(room_id)); - - mock_sync(&server, sync_builder.build_json_sync_response(), None).await; - let _response = client.sync_once(sync_settings.clone()).await.unwrap(); - server.reset().await; - - mock_encryption_state(&server, false).await; - - let room = client.get_room(room_id).unwrap(); - let timeline = room.timeline().await.unwrap(); - - let error = timeline - .redact_by_id(&TimelineEventItemId::TransactionId("something".into()), None) - .await - .err(); - assert_matches!(error, Some(Error::RedactError(RedactError::LocalEventNotFound(_)))) + let error = + timeline.redact(&TimelineEventItemId::TransactionId("something".into()), None).await.err(); + assert_matches!(error, Some(Error::RedactError(RedactError::ItemNotFound(_)))); } #[async_test]