diff --git a/Cargo.lock b/Cargo.lock index 72d15614..cac8e2ac 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2176,6 +2176,7 @@ dependencies = [ "log", "multibase", "nonempty 0.8.0", + "num-traits", "olpc-cjson", "once_cell", "pretty_assertions", diff --git a/radicle-crdt/src/clock.rs b/radicle-crdt/src/clock.rs index 6202bdea..c72d0c1d 100644 --- a/radicle-crdt/src/clock.rs +++ b/radicle-crdt/src/clock.rs @@ -1,3 +1,4 @@ +use num_traits::Bounded; use serde::{Deserialize, Serialize}; use crate::ord::Max; @@ -43,3 +44,13 @@ impl From for Lamport { } } } + +impl Bounded for Lamport { + fn min_value() -> Self { + Self::from(u64::min_value()) + } + + fn max_value() -> Self { + Self::from(u64::max_value()) + } +} diff --git a/radicle-crdt/src/lwwmap.rs b/radicle-crdt/src/lwwmap.rs index cecc6ded..a5ffaad5 100644 --- a/radicle-crdt/src/lwwmap.rs +++ b/radicle-crdt/src/lwwmap.rs @@ -64,6 +64,10 @@ impl LWWMap { .filter_map(|(k, v)| v.get().as_ref().map(|v| (k, v))) } + pub fn len(&self) -> usize { + self.iter().count() + } + pub fn is_empty(&self) -> bool { self.iter().next().is_none() } diff --git a/radicle-crdt/src/ord.rs b/radicle-crdt/src/ord.rs index 948ec8b4..fa3fb2f9 100644 --- a/radicle-crdt/src/ord.rs +++ b/radicle-crdt/src/ord.rs @@ -56,6 +56,16 @@ impl Semilattice for Max { } } +impl Bounded for Max { + fn min_value() -> Self { + Self::from(T::min_value()) + } + + fn max_value() -> Self { + Self::from(T::max_value()) + } +} + #[allow(clippy::derive_ord_xor_partial_ord)] #[derive(Clone, Copy, Debug, PartialEq, Eq, Ord, Serialize, Deserialize)] pub struct Min(pub T); diff --git a/radicle/Cargo.toml b/radicle/Cargo.toml index 71c9fa3a..9ab2c575 100644 --- a/radicle/Cargo.toml +++ b/radicle/Cargo.toml @@ -19,6 +19,7 @@ cyphernet = { version = "0", optional = true } fastrand = { version = "1.8.0" } git-ref-format = { version = "0", features = ["serde", "macro"] } multibase = { version = "0.9.1" } +num-traits = { version = "0.2.15", default-features = false, features = ["std"] } log = { version = "0.4.17", features = ["std"] } once_cell = { version = "1.13" } olpc-cjson = { version = "0.1.1" } diff --git a/radicle/src/cob/common.rs b/radicle/src/cob/common.rs index e89ab2d3..e7c5b551 100644 --- a/radicle/src/cob/common.rs +++ b/radicle/src/cob/common.rs @@ -2,6 +2,7 @@ use std::fmt; use std::str::FromStr; use std::time::{SystemTime, UNIX_EPOCH}; +use num_traits::Bounded; use serde::{Deserialize, Serialize}; use crate::prelude::*; @@ -57,6 +58,20 @@ impl std::ops::Add for Timestamp { } } +impl Bounded for Timestamp { + fn min_value() -> Self { + Self { + seconds: u64::min_value(), + } + } + + fn max_value() -> Self { + Self { + seconds: u64::max_value(), + } + } +} + #[derive(thiserror::Error, Debug)] pub enum ReactionError { #[error("invalid reaction")] diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index 0eab3e41..24687f68 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -9,7 +9,7 @@ use std::str::FromStr; use once_cell::sync::Lazy; use radicle_crdt as crdt; use radicle_crdt::clock; -use radicle_crdt::{ActorId, ChangeId, LWWMap, LWWReg, LWWSet, Max, Redactable, Semilattice}; +use radicle_crdt::{ActorId, ChangeId, LWWReg, LWWSet, Max, Redactable, Semilattice}; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -271,14 +271,17 @@ impl Patch { ref inline, } => { // TODO(cloudhead): Test review on redacted revision. - // TODO(cloudhead): Test that updating a review only requires the fields that we - // want to update. if let Some(Redactable::Present(revision)) = self.revisions.get_mut(&revision) { - revision.reviews.insert( - change.author, - Review::new(verdict, comment.to_owned(), inline.to_owned(), timestamp), - change.clock, - ); + revision + .reviews + .entry(change.author) + .or_default() + .merge(Review::new( + verdict, + comment.to_owned(), + inline.to_owned(), + timestamp, + )); } else { return Err(ApplyError::Missing(revision)); } @@ -360,7 +363,7 @@ pub struct Revision { /// Merges of this revision into other repositories. pub merges: LWWSet>, /// Reviews of this revision's changes (one per actor). - pub reviews: LWWMap, + pub reviews: BTreeMap, /// When this revision was created. pub timestamp: Timestamp, } @@ -373,7 +376,7 @@ impl Revision { oid, discussion: Thread::default(), merges: LWWSet::default(), - reviews: LWWMap::default(), + reviews: BTreeMap::default(), timestamp, } } @@ -470,7 +473,7 @@ pub struct CodeComment { } /// A patch review on a revision. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Review { /// Review verdict. pub verdict: LWWReg>, @@ -1019,13 +1022,66 @@ mod test { let id = patch.id; let patch = patches.get(&id).unwrap().unwrap(); let (_, revision) = patch.latest().unwrap(); - assert_eq!(revision.reviews.iter().count(), 1); + assert_eq!(revision.reviews.len(), 1); let review = revision.reviews.get(signer.public_key()).unwrap(); assert_eq!(review.verdict(), Some(Verdict::Accept)); assert_eq!(review.comment(), Some("LGTM")); } + #[test] + fn test_patch_review_edit() { + let tmp = tempfile::tempdir().unwrap(); + let (_, signer, project) = test::setup::context(&tmp); + let base = git::Oid::from_str("cb18e95ada2bb38aadd8e6cef0963ce37a87add3").unwrap(); + let oid = git::Oid::from_str("518d5069f94c03427f694bb494ac1cd7d1339380").unwrap(); + let mut patches = Patches::open(*signer.public_key(), &project).unwrap(); + let mut patch = patches + .create( + "My first patch", + "Blah blah blah.", + MergeTarget::Delegates, + base, + oid, + &[], + &signer, + ) + .unwrap(); + + let (rid, _) = patch.latest().unwrap(); + let rid = *rid; + + patch + .review( + rid, + Some(Verdict::Accept), + Some("LGTM".to_owned()), + vec![], + &signer, + ) + .unwrap(); + patch + .review(rid, Some(Verdict::Reject), None, vec![], &signer) + .unwrap(); // Overwrite the verdict. + + let id = patch.id; + let mut patch = patches.get_mut(&id).unwrap(); + let (_, revision) = patch.latest().unwrap(); + assert_eq!(revision.reviews.len(), 1, "the reviews were merged"); + + let review = revision.reviews.get(signer.public_key()).unwrap(); + assert_eq!(review.verdict(), Some(Verdict::Reject)); + assert_eq!(review.comment(), Some("LGTM")); + + patch + .review(rid, None, Some("Whoops!".to_owned()), vec![], &signer) + .unwrap(); // Overwrite the comment. + let (_, revision) = patch.latest().unwrap(); + let review = revision.reviews.get(signer.public_key()).unwrap(); + assert_eq!(review.verdict(), Some(Verdict::Reject)); + assert_eq!(review.comment(), Some("Whoops!")); + } + #[test] fn test_patch_update() { let tmp = tempfile::tempdir().unwrap();