diff --git a/radicle/src/cob/issue.rs b/radicle/src/cob/issue.rs index 6dc811f2..86398bd2 100644 --- a/radicle/src/cob/issue.rs +++ b/radicle/src/cob/issue.rs @@ -209,14 +209,22 @@ impl store::Transaction { self.push(Action::Lifecycle { state }) } - /// Comment on an issue. - pub fn comment(&mut self, body: S) -> CommentId { + /// Create the issue thread. + pub fn thread(&mut self, body: S) -> CommentId { self.push(Action::from(thread::Action::Comment { body: body.to_string(), reply_to: None, })) } + /// Comment on an issue. + pub fn comment(&mut self, body: S, reply_to: CommentId) -> CommentId { + self.push(Action::from(thread::Action::Comment { + body: body.to_string(), + reply_to: Some(reply_to), + })) + } + /// Tag an issue. pub fn tag( &mut self, @@ -229,16 +237,6 @@ impl store::Transaction { self.push(Action::Tag { add, remove }) } - /// Reply to on an issue comment. - pub fn reply(&mut self, parent: CommentId, body: S) -> OpId { - let body = body.to_string(); - - self.push(Action::from(thread::Action::Comment { - body, - reply_to: Some(parent), - })) - } - /// React to an issue comment. pub fn react(&mut self, to: CommentId, reaction: Reaction) -> OpId { self.push(Action::Thread { @@ -269,13 +267,24 @@ impl<'a, 'g> IssueMut<'a, 'g> { self.transaction("Lifecycle", signer, |tx| tx.lifecycle(state)) } - /// Comment on an issue. - pub fn comment( + /// Create the issue thread. + pub fn thread( &mut self, body: S, signer: &G, ) -> Result { - self.transaction("Comment", signer, |tx| tx.comment(body)) + self.transaction("Create thread", signer, |tx| tx.thread(body)) + } + + /// Comment on an issue. + pub fn comment( + &mut self, + body: S, + reply_to: CommentId, + signer: &G, + ) -> Result { + assert!(self.thread.comment(&reply_to).is_some()); + self.transaction("Comment", signer, |tx| tx.comment(body, reply_to)) } /// Tag an issue. @@ -288,17 +297,6 @@ impl<'a, 'g> IssueMut<'a, 'g> { self.transaction("Tag", signer, |tx| tx.tag(add, remove)) } - /// Reply to on an issue comment. - pub fn reply( - &mut self, - parent: CommentId, - body: S, - signer: &G, - ) -> Result { - assert!(self.thread.comment(&parent).is_some()); - self.transaction("Reply", signer, |tx| tx.reply(parent, body)) - } - /// React to an issue comment. pub fn react( &mut self, @@ -391,8 +389,8 @@ impl<'a> Issues<'a> { ) -> Result, Error> { let (id, issue, clock) = Transaction::initial("Create issue", &mut self.raw, signer, |tx| { + tx.thread(description); tx.edit(title); - tx.comment(description); tx.tag(tags.to_owned(), []); })?; // Just a sanity check that our clock is advancing as expected. @@ -524,22 +522,37 @@ mod test { fn test_issue_reply() { let tmp = tempfile::tempdir().unwrap(); let (_, signer, project) = test::setup::context(&tmp); + let author = *signer.public_key(); let mut issues = Issues::open(*signer.public_key(), &project).unwrap(); let mut issue = issues .create("My first issue", "Blah blah blah.", &[], &signer) .unwrap(); - let comment = issue.comment("Ho ho ho.", &signer).unwrap(); + let root = (clock::Lamport::default(), author); - issue.reply(comment, "Hi hi hi.", &signer).unwrap(); - issue.reply(comment, "Ha ha ha.", &signer).unwrap(); + let c1 = issue.comment("Hi hi hi.", root, &signer).unwrap(); + let c2 = issue.comment("Ha ha ha.", root, &signer).unwrap(); let id = issue.id; - let issue = issues.get(&id).unwrap().unwrap(); - let (_, reply1) = &issue.replies(&comment).nth(0).unwrap(); - let (_, reply2) = &issue.replies(&comment).nth(1).unwrap(); + let mut issue = issues.get_mut(&id).unwrap(); + let (_, reply1) = &issue.replies(&root).nth(0).unwrap(); + let (_, reply2) = &issue.replies(&root).nth(1).unwrap(); assert_eq!(reply1.body, "Hi hi hi."); assert_eq!(reply2.body, "Ha ha ha."); + + issue.comment("Re: Hi.", c1, &signer).unwrap(); + issue.comment("Re: Ha.", c2, &signer).unwrap(); + issue.comment("Re: Ha. Ha.", c2, &signer).unwrap(); + issue.comment("Re: Ha. Ha. Ha.", c2, &signer).unwrap(); + + let issue = issues.get(&id).unwrap().unwrap(); + assert_eq!(&issue.replies(&c1).nth(0).unwrap().1.body, "Re: Hi."); + assert_eq!(&issue.replies(&c2).nth(0).unwrap().1.body, "Re: Ha."); + assert_eq!(&issue.replies(&c2).nth(1).unwrap().1.body, "Re: Ha. Ha."); + assert_eq!( + &issue.replies(&c2).nth(2).unwrap().1.body, + "Re: Ha. Ha. Ha." + ); } #[test] @@ -575,8 +588,11 @@ mod test { .create("My first issue", "Blah blah blah.", &[], &signer) .unwrap(); - issue.comment("Ho ho ho.", &signer).unwrap(); - issue.comment("Ha ha ha.", &signer).unwrap(); + // The initial thread op id is always the same. + let c0 = (clock::Lamport::initial(), author); + + issue.comment("Ho ho ho.", c0, &signer).unwrap(); + issue.comment("Ha ha ha.", c0, &signer).unwrap(); let id = issue.id; let issue = issues.get(&id).unwrap().unwrap(); diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs index e8ea5fd2..1a1d2689 100644 --- a/radicle/src/cob/patch.rs +++ b/radicle/src/cob/patch.rs @@ -372,7 +372,8 @@ impl Revision { } pub fn description(&self) -> Option<&str> { - self.discussion.first() + let (_, comment) = self.discussion.root()?; + Some(comment.body.as_str()) } } @@ -534,8 +535,8 @@ impl store::Transaction { }) } - /// Comment on a patch revision. - pub fn comment(&mut self, revision: RevisionId, body: S) -> OpId { + /// Start a patch revision discussion. + pub fn thread(&mut self, revision: RevisionId, body: S) -> OpId { self.push(Action::Thread { revision, action: thread::Action::Comment { @@ -545,6 +546,22 @@ impl store::Transaction { }) } + /// Comment on a patch revision. + pub fn comment( + &mut self, + revision: RevisionId, + body: S, + reply_to: CommentId, + ) -> OpId { + self.push(Action::Thread { + revision, + action: thread::Action::Comment { + body: body.to_string(), + reply_to: Some(reply_to), + }, + }) + } + /// Review a patch revision. pub fn review( &mut self, @@ -582,7 +599,7 @@ impl store::Transaction { oid: impl Into, ) -> (OpId, OpId) { let revision = self.revision(base, oid); - let comment = self.comment(revision, description); + let comment = self.thread(revision, description); (revision, comment) } @@ -664,9 +681,10 @@ impl<'a, 'g> PatchMut<'a, 'g> { &mut self, revision: RevisionId, body: S, + reply_to: CommentId, signer: &G, ) -> Result { - self.transaction("Comment", signer, |tx| tx.comment(revision, body)) + self.transaction("Comment", signer, |tx| tx.comment(revision, body, reply_to)) } /// Review a patch revision. @@ -703,7 +721,7 @@ impl<'a, 'g> PatchMut<'a, 'g> { ) -> Result<(OpId, OpId), Error> { self.transaction("Add revision", signer, |tx| { let r = tx.revision(base, oid); - let c = tx.comment(r, description); + let c = tx.thread(r, description); (r, c) }) @@ -950,21 +968,18 @@ mod test { let [p1, p2, p3] = log.permutations; let mut t1 = t.clone(); - match t1.apply(p1) { - Ok(()) => {} - Err(ApplyError::Missing(_)) => return TestResult::discard(), + if t1.apply(p1).is_err() { + return TestResult::discard(); } let mut t2 = t.clone(); - match t2.apply(p2) { - Ok(()) => {} - Err(ApplyError::Missing(_)) => return TestResult::discard(), + if t2.apply(p2).is_err() { + return TestResult::discard(); } let mut t3 = t; - match t3.apply(p3) { - Ok(()) => {} - Err(ApplyError::Missing(_)) => return TestResult::discard(), + if t3.apply(p3).is_err() { + return TestResult::discard(); } assert_eq!(t1, t2); @@ -976,7 +991,7 @@ mod test { qcheck::QuickCheck::new() .min_tests_passed(100) - .gen(qcheck::Gen::new(8)) + .gen(qcheck::Gen::new(7)) .quickcheck(property as fn(Changes<3>) -> TestResult); } diff --git a/radicle/src/cob/thread.rs b/radicle/src/cob/thread.rs index 6af20652..d91a7337 100644 --- a/radicle/src/cob/thread.rs +++ b/radicle/src/cob/thread.rs @@ -29,19 +29,20 @@ pub static TYPENAME: Lazy = pub type CommentId = OpId; /// A comment on a discussion thread. -#[derive(Default, Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct Comment { /// The comment body. pub body: String, - /// Thread or comment this is a reply to. - pub reply_to: Option, + /// Comment this is a reply to. + /// Should always be set, except for the root comment. + pub reply_to: Option, /// When the comment was authored. pub timestamp: Timestamp, } impl Comment { /// Create a new comment. - pub fn new(body: String, reply_to: Option, timestamp: Timestamp) -> Self { + pub fn new(body: String, reply_to: Option, timestamp: Timestamp) -> Self { Self { body, reply_to, @@ -68,7 +69,9 @@ pub enum Action { Comment { /// Comment body. body: String, - /// Another comment this is a reply to. + /// Comment this is a reply to. + /// Should be [`None`] if it's the top-level comment. + /// Should be the root [`CommentId`] if it's a top-level comment. reply_to: Option, }, /// Redact a change. Not all changes can be redacted. @@ -105,8 +108,8 @@ impl store::FromHistory for Thread { fn from_history(history: &History) -> Result<(Self, Lamport), store::Error> { let obj = history.traverse(Thread::default(), |mut acc, entry| { - if let Ok(Ops(changes)) = Ops::try_from(entry) { - acc.apply(changes); + if let Ok(Ops(ops)) = Ops::try_from(entry) { + acc.apply(ops); ControlFlow::Continue(acc) } else { ControlFlow::Break(acc) @@ -132,6 +135,10 @@ impl Deref for Thread { } impl Thread { + pub fn is_initialized(&self) -> bool { + !self.comments.is_empty() + } + pub fn comment(&self, id: &CommentId) -> Option<&Comment> { if let Some(Redactable::Present(comment)) = self.comments.get(id) { Some(comment) @@ -140,11 +147,10 @@ impl Thread { } } - pub fn first(&self) -> Option<&str> { + pub fn root(&self) -> Option<(&CommentId, &Comment)> { self.comments - .values() - .filter_map(|r| r.get()) - .map(|c| c.body.as_str()) + .iter() + .filter_map(|(id, r)| r.get().map(|comment| (id, comment))) .next() } @@ -153,8 +159,8 @@ impl Thread { to: &'a CommentId, ) -> impl Iterator { self.comments().filter_map(move |(id, c)| { - if let Some(parent) = &c.reply_to { - if parent == to { + if let Some(reply_to) = c.reply_to { + if &reply_to == to { return Some((id, c)); } } @@ -179,8 +185,10 @@ impl Thread { match op.action { Action::Comment { body, reply_to } => { - let present = Redactable::Present(Comment::new(body, reply_to, op.timestamp)); - self.comments.insert(id, present); + self.comments.insert( + id, + Redactable::Present(Comment::new(body, reply_to, op.timestamp)), + ); } Action::Redact { id } => { self.comments.insert(id, Redactable::Redacted); @@ -273,9 +281,9 @@ mod tests { use std::{array, iter}; use crate::crypto::test::signer::MockSigner; + use nonempty::NonEmpty; use pretty_assertions::assert_eq; - use qcheck::Arbitrary; - use qcheck_macros::quickcheck; + use qcheck::{Arbitrary, TestResult}; use super::*; use crate as radicle; @@ -303,6 +311,7 @@ mod tests { fn arbitrary(g: &mut qcheck::Gen) -> Self { let author = ActorId::from([0; 32]); let rng = fastrand::Rng::with_seed(u64::arbitrary(g)); + let root = (Lamport::initial(), author); let gen = WeightedGenerator::<(Lamport, Action), (Lamport, Vec)>::new(rng.clone()) .variant(3, |(clock, changes), rng| { @@ -312,7 +321,7 @@ mod tests { *clock, Action::Comment { body: iter::repeat_with(|| rng.alphabetic()).take(16).collect(), - reply_to: None, + reply_to: Some(root), }, )) }) @@ -340,11 +349,19 @@ mod tests { Some((clock.tick(), Action::Redact { id })) }); - let mut changes = Vec::new(); + let mut changes = vec![Op { + action: Action::Comment { + body: String::default(), + reply_to: None, + }, + author, + clock: Lamport::initial(), + timestamp: Timestamp::now(), + }]; let mut permutations: [Vec>; N] = array::from_fn(|_| Vec::new()); - let timestamp = Timestamp::now() + rng.u64(..60); - for (clock, action) in gen.take(g.size().min(8)) { + for (clock, action) in gen.take(g.size()) { + let timestamp = Timestamp::now() + rng.u64(..60); changes.push(Op { action, author, @@ -370,23 +387,24 @@ mod tests { radicle::cob::store::Store::::open(*signer.public_key(), &repository).unwrap(); let mut alice = Actor::new(signer); - let a1 = alice.comment("First comment", None); - let a2 = alice.comment("Second comment", None); - let a3 = alice.comment("Third comment", None); + let a0 = alice.comment("First comment", None); + let a1 = alice.comment("Second comment", Some(a0.id())); + let a2 = alice.comment("Third comment", Some(a0.id())); let (id, _, _) = store - .create("Thread created", a1.action, &alice.signer) + .create("Thread created", a0.action, &alice.signer) .unwrap(); - let second = store + let comment = store + .update(id, "Thread updated", a1.action, &alice.signer) + .unwrap(); + store .update(id, "Thread updated", a2.action, &alice.signer) .unwrap(); - store - .update(id, "Thread updated", a3.action, &alice.signer) - .unwrap(); - let a4 = alice.redact((second.history().clock().into(), *alice.signer.public_key())); + // Redact the second comment. + let a3 = alice.redact((comment.history().clock().into(), *alice.signer.public_key())); store - .update(id, "Comment redacted", a4.action, &alice.signer) + .update(id, "Comment redacted", a3.action, &alice.signer) .unwrap(); let (thread, _) = store.get(&id).unwrap().unwrap(); @@ -407,17 +425,20 @@ mod tests { let mut alice = Actor::new(signer); - let a1 = alice.comment("First comment", None); - let a2 = alice.comment("Second comment", None); + let a0 = alice.comment("Thread root", None); + let a1 = alice.comment("First comment", Some(a0.id())); + let a2 = alice.comment("Second comment", Some(a0.id())); let mut expected = Thread::default(); - expected.apply([a1.clone(), a2.clone()]); + expected.apply([a0.clone(), a1.clone(), a2.clone()]); let (id, _, _) = store - .create("Thread created", a1.action, &alice.signer) + .create("Thread created", a0.action, &alice.signer) .unwrap(); + + let actions = NonEmpty::from_vec(vec![a1.action, a2.action]).unwrap(); store - .update(id, "Thread updated", a2.action, &alice.signer) + .update(id, "Thread updated", actions, &alice.signer) .unwrap(); let (actual, _) = store.get(&id).unwrap().unwrap(); @@ -430,18 +451,19 @@ mod tests { let mut alice = Actor::::default(); let mut bob = Actor::::default(); - let a1 = alice.comment("First comment", None); - let a2 = alice.comment("Second comment", None); + let a0 = alice.comment("Thread root", None); + let a1 = alice.comment("First comment", Some(a0.id())); + let a2 = alice.comment("Second comment", Some(a0.id())); - bob.receive([a1.clone(), a2.clone()]); + bob.receive([a0.clone(), a1.clone(), a2.clone()]); assert_eq!( bob.timeline().collect::>(), alice.timeline().collect::>() ); - assert_eq!(alice.timeline().collect::>(), vec![&a1, &a2]); + assert_eq!(alice.timeline().collect::>(), vec![&a0, &a1, &a2]); bob.reset(); - bob.receive([a2, a1]); + bob.receive([a0, a2, a1]); assert_eq!( bob.timeline().collect::>(), alice.timeline().collect::>() @@ -454,32 +476,33 @@ mod tests { let mut bob = Actor::::default(); let mut eve = Actor::::default(); - let a1 = alice.comment("First comment", None); + let a0 = alice.comment("Thread root", None); + let a1 = alice.comment("First comment", Some(a0.id())); - bob.receive([a1.clone()]); + bob.receive([a0.clone(), a1.clone()]); - let b0 = bob.comment("Bob's first reply to Alice", None); - let b1 = bob.comment("Bob's second reply to Alice", None); + let b0 = bob.comment("Bob's first reply to Alice", Some(a0.id())); + let b1 = bob.comment("Bob's second reply to Alice", Some(a0.id())); - eve.receive([b1.clone(), b0.clone()]); - let e0 = eve.comment("Eve's first reply to Alice", None); + eve.receive([a0.clone(), b1.clone(), b0.clone()]); + let e0 = eve.comment("Eve's first reply to Alice", Some(a0.id())); bob.receive([e0.clone()]); - let b2 = bob.comment("Bob's third reply to Alice", None); + let b2 = bob.comment("Bob's third reply to Alice", Some(a0.id())); eve.receive([b2.clone(), a1.clone()]); - let e1 = eve.comment("Eve's second reply to Alice", None); + let e1 = eve.comment("Eve's second reply to Alice", Some(a0.id())); alice.receive([b0.clone(), b1.clone(), b2.clone(), e0.clone(), e1.clone()]); bob.receive([e1.clone()]); - let a2 = alice.comment("Second comment", None); + let a2 = alice.comment("Second comment", Some(a0.id())); eve.receive([a2.clone()]); bob.receive([a2.clone()]); - assert_eq!(alice.ops.len(), 7); - assert_eq!(bob.ops.len(), 7); - assert_eq!(eve.ops.len(), 7); + assert_eq!(alice.ops.len(), 8); + assert_eq!(bob.ops.len(), 8); + assert_eq!(eve.ops.len(), 8); assert_eq!( bob.timeline().collect::>(), @@ -490,27 +513,36 @@ mod tests { alice.timeline().collect::>() ); assert_eq!( - vec![&a1, &b0, &b1, &e0, &b2, &e1, &a2], + vec![&a0, &a1, &b0, &b1, &e0, &b2, &e1, &a2], alice.timeline().collect::>(), ); } - #[quickcheck] - fn prop_invariants(log: Changes<3>) { - let t = Thread::default(); - let [p1, p2, p3] = log.permutations; + #[test] + fn prop_invariants() { + fn property(log: Changes<3>) -> TestResult { + let t = Thread::default(); + let [p1, p2, p3] = log.permutations; - let mut t1 = t.clone(); - t1.apply(p1); + let mut t1 = t.clone(); + t1.apply(p1); - let mut t2 = t.clone(); - t2.apply(p2); + let mut t2 = t.clone(); + t2.apply(p2); - let mut t3 = t; - t3.apply(p3); + let mut t3 = t; + t3.apply(p3); - assert_eq!(t1, t2); - assert_eq!(t2, t3); - assert_laws(&t1, &t2, &t3); + assert_eq!(t1, t2); + assert_eq!(t2, t3); + assert_laws(&t1, &t2, &t3); + + TestResult::passed() + } + qcheck::QuickCheck::new() + .min_tests_passed(100) + .max_tests(10000) + .gen(qcheck::Gen::new(7)) + .quickcheck(property as fn(Changes<3>) -> TestResult); } }