fix: editing the same reminder doesn't work (AppFlowy-IO#336)

* fix: editing the same reminder doesn't work * chore: clippy --------- Co-authored-by: nathan <[email protected]>
richardshiue · Nov 6, 2024 · 6cb0fcb · 6cb0fcb
1 parent be6bb90
commit 6cb0fcb
Show file tree

Hide file tree

Showing 4 changed files with 141 additions and 26 deletions.
diff --git a/collab-entity/src/reminder.rs b/collab-entity/src/reminder.rs
@@ -23,7 +23,7 @@ pub struct Reminder {
   pub object_id: String,
 }
 
-#[derive(Clone, Debug, Eq, PartialEq, Serialize_repr, Deserialize_repr)]
+#[derive(Clone, Debug, Eq, PartialEq, Serialize_repr, Deserialize_repr, Copy)]
 #[repr(i64)]
 pub enum ObjectType {
   Unknown = 0,
@@ -41,6 +41,16 @@ impl From<i64> for ObjectType {
   }
 }
 
+impl From<ObjectType> for i64 {
+  fn from(value: ObjectType) -> Self {
+    match value {
+      ObjectType::Unknown => 0,
+      ObjectType::Document => 1,
+      ObjectType::Database => 2,
+    }
+  }
+}
+
 impl Reminder {
   pub fn new(id: String, object_id: String, scheduled_at: i64, ty: ObjectType) -> Self {
     Self {

diff --git a/collab-user/src/reminder.rs b/collab-user/src/reminder.rs
@@ -1,9 +1,14 @@
-use collab::preclude::encoding::serde::{from_any, to_any};
+use std::collections::HashMap;
+
+use collab::preclude::encoding::serde::from_any;
 use collab::preclude::{
-  Any, Array, ArrayRef, Change, DeepObservable, Event, Map, MapPrelim, Out, ReadTxn, Subscription,
-  ToJson, TransactionMut, YrsValue,
+  Any, Array, ArrayRef, Change, DeepObservable, Event, Map, MapPrelim, MapRef, Out, ReadTxn,
+  Subscription, ToJson, TransactionMut, YrsValue,
+};
+use collab_entity::reminder::{
+  Reminder, REMINDER_ID, REMINDER_IS_ACK, REMINDER_MESSAGE, REMINDER_META, REMINDER_OBJECT_ID,
+  REMINDER_SCHEDULED_AT, REMINDER_TITLE, REMINDER_TY,
 };
-use collab_entity::reminder::{Reminder, REMINDER_ID};
 use tokio::sync::broadcast;
 
 pub type RemindersChangeSender = broadcast::Sender<ReminderChange>;
@@ -45,8 +50,9 @@ impl Reminders {
   }
 
   pub fn remove(&self, txn: &mut TransactionMut, id: &str) {
-    let (i, _) = self.find(txn, id).unwrap();
-    self.container.remove(txn, i);
+    if let Some((i, _value)) = self.find(txn, id) {
+      self.container.remove(txn, i);
+    }
   }
 
   pub fn add(&self, txn: &mut TransactionMut, reminder: Reminder) {
@@ -56,19 +62,14 @@ impl Reminders {
 
   pub fn update_reminder<F>(&self, txn: &mut TransactionMut, reminder_id: &str, f: F)
   where
-    F: FnOnce(&mut Reminder),
+    F: FnOnce(ReminderUpdate),
   {
-    if let Some((i, value)) = self.find(txn, reminder_id) {
-      if let Ok(mut reminder) = from_any::<Reminder>(&value.to_json(txn)) {
-        // FIXME: this is wrong. This doesn't "update" the reminder, instead it replaces it,
-        //   with all of the unchanged fields included. That means, that if two people will be
-        //   updating the same reminder at the same time, the last one will overwrite the changes
-        //   of the first one, even if there was no edit collisions.
-        f(&mut reminder);
-        self.container.remove(txn, i);
-        let any = to_any(&reminder).unwrap();
-        self.container.insert(txn, i, any);
-      }
+    if let Some((_, Out::YMap(mut map))) = self.find(txn, reminder_id) {
+      let update = ReminderUpdate {
+        map_ref: &mut map,
+        txn,
+      };
+      f(update)
     }
   }
 
@@ -136,3 +137,58 @@ fn subscribe_reminder_change(
     }
   })
 }
+
+pub struct ReminderUpdate<'a, 'b> {
+  map_ref: &'a mut MapRef,
+  txn: &'a mut TransactionMut<'b>,
+}
+
+impl<'a, 'b> ReminderUpdate<'a, 'b> {
+  pub fn set_object_id<T: AsRef<str>>(self, value: T) -> Self {
+    self
+      .map_ref
+      .try_update(self.txn, REMINDER_OBJECT_ID, value.as_ref());
+    self
+  }
+
+  pub fn set_title<T: AsRef<str>>(self, value: T) -> Self {
+    self
+      .map_ref
+      .try_update(self.txn, REMINDER_TITLE, value.as_ref());
+    self
+  }
+
+  pub fn set_message<T: AsRef<str>>(self, value: T) -> Self {
+    self
+      .map_ref
+      .try_update(self.txn, REMINDER_MESSAGE, value.as_ref());
+    self
+  }
+
+  pub fn set_is_ack(self, value: bool) -> Self {
+    self.map_ref.try_update(self.txn, REMINDER_IS_ACK, value);
+    self
+  }
+
+  pub fn set_is_read(self, value: bool) -> Self {
+    self.map_ref.try_update(self.txn, REMINDER_IS_ACK, value);
+    self
+  }
+
+  pub fn set_type<T: Into<i64>>(self, value: T) -> Self {
+    self.map_ref.try_update(self.txn, REMINDER_TY, value.into());
+    self
+  }
+
+  pub fn set_scheduled_at<T: Into<i64>>(self, value: T) -> Self {
+    self
+      .map_ref
+      .try_update(self.txn, REMINDER_SCHEDULED_AT, value.into());
+    self
+  }
+
+  pub fn set_meta(self, value: HashMap<String, String>) -> Self {
+    self.map_ref.try_update(self.txn, REMINDER_META, value);
+    self
+  }
+}
diff --git a/collab-user/src/user_awareness.rs b/collab-user/src/user_awareness.rs
@@ -2,6 +2,7 @@ use std::borrow::{Borrow, BorrowMut};
 use std::collections::HashMap;
 use std::ops::{Deref, DerefMut};
 
+use crate::core::ReminderUpdate;
 use crate::reminder::{Reminders, RemindersChangeSender};
 use anyhow::{Error, Result};
 use collab::core::origin::CollabOrigin;
@@ -133,7 +134,7 @@ impl UserAwareness {
   /// * `f` - A function or closure that takes `ReminderUpdate` as its argument and implements the changes to the reminder.
   pub fn update_reminder<F>(&mut self, reminder_id: &str, f: F)
   where
-    F: FnOnce(&mut Reminder),
+    F: FnOnce(ReminderUpdate),
   {
     let mut txn = self.collab.transact_mut();
     self

diff --git a/collab-user/tests/reminder_test/test.rs b/collab-user/tests/reminder_test/test.rs
@@ -1,3 +1,5 @@
+use std::collections::HashMap;
+
 use collab_entity::reminder::{ObjectType, Reminder};
 
 use crate::util::UserAwarenessTest;
@@ -45,12 +47,14 @@ fn update_reminder_test() {
     .with_key_value("id", "fake_id");
   test.add_reminder(reminder);
 
-  test.update_reminder("1", |reminder| {
-    reminder.title = "new title".to_string();
-    reminder.message = "new message".to_string();
-    reminder
-      .meta
-      .insert("block_id".to_string(), "fake_block_id2".to_string());
+  test.update_reminder("1", |update| {
+    update
+      .set_title("new title")
+      .set_message("new message")
+      .set_meta(HashMap::from([
+        ("block_id".to_string(), "fake_block_id2".to_string()),
+        ("id".to_string(), "fake_id".to_string()),
+      ]));
   });
   let json = test.to_json().unwrap();
   assert_json_eq!(
@@ -77,6 +81,50 @@ fn update_reminder_test() {
   )
 }
 
+#[test]
+fn update_reminder_multiple_times_test() {
+  let mut test = UserAwarenessTest::new(1);
+  let reminder = Reminder::new("1".to_string(), "o1".to_string(), 123, ObjectType::Document)
+    .with_key_value("block_id", "fake_block_id")
+    .with_key_value("id", "fake_id");
+  test.add_reminder(reminder);
+
+  test.update_reminder("1", |update| {
+    update
+      .set_title("new title")
+      .set_message("new message")
+      .set_meta(HashMap::from([(
+        "block_id".to_string(),
+        "fake_block_id2".to_string(),
+      )]));
+  });
+  test.update_reminder("1", |update| {
+    update.set_title("another title");
+  });
+  let json = test.to_json().unwrap();
+  assert_json_eq!(
+    json,
+    json!({
+      "appearance_settings": {},
+      "reminders": [
+        {
+          "id": "1",
+          "object_id": "o1",
+          "is_ack": false,
+          "is_read": false,
+          "message": "new message",
+          "meta": {
+            "block_id": "fake_block_id2",
+          },
+          "scheduled_at": 123,
+          "title": "another title",
+          "ty": 1
+        }
+      ]
+    })
+  )
+}
+
 #[test]
 fn delete_reminder_test() {
   let mut test = UserAwarenessTest::new(1);