cli: Fix panic on `rad patch review` without options

When invoked without any of `--patch`, `--delete`, `--accept`,
or `--reject`, the command panicked.

It will now correctly return an error to use `--accept`, `--reject`,
or to supply a message.
Note that `--patch` and `--delete` are removed as these commands are
now obsolete.

The new doc test also documents two previously-untested behaviors
of `rad patch review`: neutral reviews (verdict=None with summary)
and the COB layer's silent no-op when a duplicate review is submitted
(see `test_patch_review_duplicate` in radicle-cli/tests/commands/patch.rs).

Fixes 8ceff5d4b8fa94bcbe243f9f8ef52138f65e495d

Signed-off-by: Arthur Mariano <arthvm@proton.me>
Co-authored-by: Fintan Halpenny <fintan.halpenny@gmail.com>

CHANGELOG.md

@@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   merged. The Git remote helper will now fallback to the canonical default
   branch, if the default branch of the delegate could not be found, and
   correctly mark the patch as merged.
+- `rad patch review` presents an appropriate error to the user when the user
+  does not provide an option, one of `--accept` or `--reject`, or a summary
+  message.
 
 ## 1.9.1
 

crates/radicle-cli/examples/rad-patch-review-no-options.md

@@ -0,0 +1,24 @@
+The `rad patch review` command must provide an option or a review message, since
+a review must have a verdict or a review summary:
+
+``` (fail)
+$ rad patch review aa45913 --no-message --no-announce
+✗ Error: expected one of `--accept` or `--reject`, or supply a review message
+```
+
+This time we will supply a message:
+
+```
+$ rad patch review aa45913 -m "Looks reasonable, no strong opinion." --no-announce
+✓ Patch aa45913 reviewed
+```
+
+Submitting a second review on the same revision by the same author is ignored.
+Currently, there is no way to edit a review from the CLI.
+
+Note that the review command will still succeed, but with no effect:
+
+```
+$ rad patch review aa45913 -m "Changed my mind." --no-announce
+✓ Patch aa45913 reviewed
+```

crates/radicle-cli/src/commands/patch.rs

@@ -17,6 +17,7 @@ mod review;
 mod show;
 mod update;
 
+use core::convert::TryInto;
 use std::collections::BTreeSet;
 
 use anyhow::anyhow;
@@ -196,7 +197,13 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> {
                 .map(|rev| rev.resolve::<radicle::git::Oid>(&repository.backend))
                 .transpose()?
                 .map(patch::RevisionId::from);
-            review::run(patch_id, revision_id, options.into(), &profile, &repository)?;
+            review::run(
+                patch_id,
+                revision_id,
+                options.try_into()?,
+                &profile,
+                &repository,
+            )?;
         }
 
         Command::Resolve {

crates/radicle-cli/src/commands/patch/args.rs

@@ -1,3 +1,5 @@
+use core::result::Result::Err;
+
 use clap::{Parser, Subcommand};
 
 use radicle::cob::Label;
@@ -588,8 +590,15 @@ pub(super) struct ReviewArgs {
     message_args: MessageArgs,
 }
 
+#[derive(Debug, thiserror::Error)]
+#[non_exhaustive]
+pub enum OperationError {
+    #[error("expected one of `--accept` or `--reject`, or supply a review message")]
+    MissingOption,
+}
+
 impl ReviewArgs {
-    fn as_operation(&self) -> review::Operation {
+    fn as_operation(&self, message: &Message) -> Result<review::Operation, OperationError> {
         let Self {
             patch,
             accept,
@@ -606,51 +615,60 @@ impl ReviewArgs {
             } else {
                 None
             };
-            return review::Operation::Review(review::ReviewOptions {
+            return Ok(review::Operation::Review(review::ReviewOptions {
                 by_hunk: true,
                 unified: self.unified,
                 hunk: self.hunk,
                 verdict,
-            });
+            }));
         }
 
         if *delete {
-            return review::Operation::Delete;
+            return Ok(review::Operation::Delete);
         }
 
         if *accept {
-            return review::Operation::Review(review::ReviewOptions {
+            return Ok(review::Operation::Review(review::ReviewOptions {
                 by_hunk: false,
                 unified: 3,
                 hunk: None,
                 verdict: Some(Verdict::Accept),
-            });
+            }));
         }
 
         if *reject {
-            return review::Operation::Review(review::ReviewOptions {
+            return Ok(review::Operation::Review(review::ReviewOptions {
                 by_hunk: false,
                 unified: 3,
                 hunk: None,
                 verdict: Some(Verdict::Reject),
-            });
+            }));
         }
 
-        panic!("expected one of `--patch`, `--delete`, `--accept`, or `--reject`");
+        if matches!(message, Message::Edit | Message::Text(_)) {
+            return Ok(review::Operation::Review(review::ReviewOptions {
+                by_hunk: false,
+                unified: self.unified,
+                hunk: self.hunk,
+                verdict: None,
+            }));
+        }
+
+        Err(OperationError::MissingOption)
     }
 }
 
-impl From<ReviewArgs> for review::Options {
+impl TryFrom<ReviewArgs> for review::Options {
-    fn from(args: ReviewArgs) -> Self {
+    type Error = OperationError;
-        let op = args.as_operation();
+
-        Self {
+    fn try_from(args: ReviewArgs) -> Result<Self, Self::Error> {
-            message: Message::from(args.message_args),
+        let message = Message::from(args.message_args.clone());
-            op,
+        let op = args.as_operation(&message)?;
-        }
+        Ok(Self { message, op })
     }
 }
 
-#[derive(Debug, clap::Args)]
+#[derive(Clone, Debug, clap::Args)]
 #[group(required = false, multiple = false)]
 pub(super) struct MessageArgs {
     /// Provide a message (default: prompt)

crates/radicle-cli/tests/commands/patch.rs

@@ -27,6 +27,11 @@ fn rad_patch_checkout() {
     Environment::alice(["rad-init", "rad-patch-checkout"]);
 }
 
+#[test]
+fn rad_patch_review_no_options() {
+    Environment::alice(["rad-init", "rad-patch", "rad-patch-review-no-options"]);
+}
+
 #[test]
 fn rad_patch_checkout_revision() {
     Environment::alice([