From 01787080966ccaf2733a21d6f5d7e5b481f4b5e7 Mon Sep 17 00:00:00 2001 From: Richard Shiue <71320345+richardshiue@users.noreply.github.com> Date: Wed, 6 Nov 2024 15:11:15 +0300 Subject: [PATCH] fix: editing the same reminder doesn't work --- collab-user/src/reminder.rs | 89 ++++++++++++++++++++----- collab-user/src/user_awareness.rs | 3 +- collab-user/tests/reminder_test/test.rs | 60 +++++++++++++++-- 3 files changed, 128 insertions(+), 24 deletions(-) diff --git a/collab-user/src/reminder.rs b/collab-user/src/reminder.rs index b012fe8c4..e49001637 100644 --- 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; @@ -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,18 +62,12 @@ impl Reminders { pub fn update_reminder(&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::(&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((_, mut value)) = self.find(txn, reminder_id) { + if let Out::YMap(map) = &mut value { + let update = ReminderUpdate { map_ref: map, txn }; + f(update) } } } @@ -136,3 +136,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>(self, value: T) -> Self { + self + .map_ref + .try_update(self.txn, REMINDER_OBJECT_ID, value.as_ref()); + self + } + + pub fn set_title>(self, value: T) -> Self { + self + .map_ref + .try_update(self.txn, REMINDER_TITLE, value.as_ref()); + self + } + + pub fn set_message>(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>(self, value: T) -> Self { + self.map_ref.try_update(self.txn, REMINDER_TY, value.into()); + self + } + + pub fn set_scheduled_at>(self, value: T) -> Self { + self + .map_ref + .try_update(self.txn, REMINDER_SCHEDULED_AT, value.into()); + self + } + + pub fn set_meta(self, value: HashMap) -> 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 index 7789f2f41..363a80020 100644 --- 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(&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 index 1b94ca060..1f184c601 100644 --- 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);