From fa15ca59f18ad6c4714cd47b592e0d0f6628cdbc Mon Sep 17 00:00:00 2001
From: Sebastian Martinez <me@sebastinez.dev>
Date: Wed, 29 Mar 2023 13:44:13 +0200
Subject: [PATCH] cob: Use `LWWReg::new` instead of `LWWReg::from`

Using LWWReg::from was giving subsequent actions
the same clock value as their predecessors.

This resulted in a Reject not being able
to turn into an Accept on a second review of a patch.

The solution is to use the clock value that was produced for the Op
and pass that into new, avoiding the call to from.

Co-authored-by: Fintan Halpenny <fintan.halpenny@gmail.com>
Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
Signed-off-by: Sebastian Martinez <me@sebastinez.dev>
---
 radicle/src/cob/patch.rs | 70 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 6 deletions(-)

diff --git a/radicle/src/cob/patch.rs b/radicle/src/cob/patch.rs
index a12f36e8..0cbad7fb 100644
--- a/radicle/src/cob/patch.rs
+++ b/radicle/src/cob/patch.rs
@@ -311,7 +311,13 @@ impl store::FromHistory for Patch {
                     if let Some(Redactable::Present(revision)) = self.revisions.get_mut(&revision) {
                         revision.reviews.insert(
                             op.author,
-                            Review::new(verdict, comment.to_owned(), inline.to_owned(), timestamp),
+                            Review::new(
+                                verdict,
+                                comment.to_owned(),
+                                inline.to_owned(),
+                                timestamp,
+                                op.clock,
+                            ),
                         );
                     } else {
                         return Err(ApplyError::Missing(revision));
@@ -554,15 +560,16 @@ impl Review {
         comment: Option<String>,
         inline: Vec<CodeComment>,
         timestamp: Timestamp,
+        clock: Clock,
     ) -> Self {
         Self {
-            verdict: LWWReg::from(verdict),
-            comment: LWWReg::from(comment.map(Max::from)),
+            verdict: LWWReg::new(verdict, clock),
+            comment: LWWReg::new(comment.map(Max::from), clock),
             inline: LWWSet::from_iter(
                 inline
                     .into_iter()
                     .map(Max::from)
-                    .zip(std::iter::repeat(clock::Lamport::default())),
+                    .zip(std::iter::repeat(clock)),
             ),
             timestamp: Max::from(timestamp),
         }
@@ -1373,17 +1380,68 @@ mod test {
 
         let review = revision.reviews.get(signer.public_key()).unwrap();
         assert_eq!(review.verdict(), Some(Verdict::Reject));
-        assert_eq!(review.comment(), Some("LGTM"));
+        assert_eq!(review.comment(), None);
 
         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.verdict(), None);
         assert_eq!(review.comment(), Some("Whoops!"));
     }
 
+    #[test]
+    fn test_patch_reject_to_accept() {
+        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(&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::Reject),
+                Some("Nah".to_owned()),
+                vec![],
+                &signer,
+            )
+            .unwrap();
+        patch
+            .review(
+                rid,
+                Some(Verdict::Accept),
+                Some("LGTM".to_owned()),
+                vec![],
+                &signer,
+            )
+            .unwrap();
+
+        let id = patch.id;
+        let 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::Accept));
+        assert_eq!(review.comment(), Some("LGTM"));
+    }
+
     #[test]
     fn test_patch_update() {
         let tmp = tempfile::tempdir().unwrap();