mab122
/
radicle-heartwood-lfs
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Add `id` to `Revision`, forbid duplicate reviews 

Some changes to the `Patch` state needed for code review.
This commit is contained in:
cloudhead 2024-07-23 10:13:04 +02:00
parent 81a3bc3d1b
commit 7cecabed42
No known key found for this signature in database
4 changed files with 53 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
radicle-cli/examples/rad-cob-show.md
View File

@ -113,6 +113,7 @@ $ rad cob show --repo rad:z42hL2jL4XNk6K8oHQaSWfMgCL7ji --type xyz.radicle.patch
"merges": {}, "merges": {},
"revisions": { "revisions": {
"d1f7f869fde9fac19c1779c4c2e77e8361333f91": { "d1f7f869fde9fac19c1779c4c2e77e8361333f91": {
"id": "d1f7f869fde9fac19c1779c4c2e77e8361333f91",
"author": { "author": {
"id": "did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi" "id": "did:key:z6MknSLrJoTcukLrE435hVNQT4JUhbvWLX4kUzqkEStBU8Vi"
}, },

2
radicle/src/cob/cache.rs
View File

@ -47,7 +47,7 @@ pub struct Read;
#[derive(Clone)] #[derive(Clone)]
pub struct Write; pub struct Write;
/// A file-backed database storing information about the network. /// A file-backed database storing materialized COBs.
#[derive(Clone)] #[derive(Clone)]
pub struct Store<T> { pub struct Store<T> {
pub(super) db: Arc<sql::ConnectionThreadSafe>, pub(super) db: Arc<sql::ConnectionThreadSafe>,

60
radicle/src/cob/patch.rs
View File

@ -1,5 +1,6 @@
pub mod cache; pub mod cache;
use std::collections::btree_map::Entry;
use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::collections::{BTreeMap, BTreeSet, HashMap};
use std::fmt; use std::fmt;
use std::ops::Deref; use std::ops::Deref;
@ -106,6 +107,9 @@ pub enum Error {
/// Review is empty. /// Review is empty.
#[error("empty review; verdict or summary not provided")] #[error("empty review; verdict or summary not provided")]
EmptyReview, EmptyReview,
/// Duplicate review.
#[error("review {0} of {1} already exists by author {2}")]
DuplicateReview(ReviewId, RevisionId, NodeId),
/// Error loading the document payload. /// Error loading the document payload.
#[error("payload failed to load: {0}")] #[error("payload failed to load: {0}")]
Payload(#[from] PayloadError), Payload(#[from] PayloadError),
@ -521,7 +525,7 @@ impl Patch {
t.and_then(|(rev_id, pk)| { t.and_then(|(rev_id, pk)| {
if rev == rev_id { if rev == rev_id {
self.revision(&rev_id) self.revision(&rev_id)
.and_then(|r| r.review(&pk)) .and_then(|r| r.review_by(&pk))
.map(|r| (review_id, r)) .map(|r| (review_id, r))
} else { } else {
None None
@ -825,10 +829,12 @@ impl Patch {
resolves, resolves,
} => { } => {
debug_assert!(!self.revisions.contains_key(&entry)); debug_assert!(!self.revisions.contains_key(&entry));
let id = RevisionId(entry);
self.revisions.insert( self.revisions.insert(
RevisionId(entry), id,
Some(Revision::new( Some(Revision::new(
id,
author.into(), author.into(),
description, description,
base, base,
@ -886,21 +892,25 @@ impl Patch {
return Err(Error::EmptyReview); return Err(Error::EmptyReview);
} }
if let Some(rev) = rev { if let Some(rev) = rev {
// Nb. Applying two reviews by the same author is not allowed and let id = ReviewId(entry);
// results in the review being redacted. // Nb. Applying two reviews by the same author is not allowed.
rev.reviews.insert( match rev.reviews.entry(author) {
author, Entry::Occupied(_) => {
Some(Review::new( return Err(Error::DuplicateReview(id, revision, author));
}
Entry::Vacant(e) => {
e.insert(Some(Review::new(
id,
Author::new(author), Author::new(author),
verdict, verdict,
summary.to_owned(), summary.to_owned(),
labels, labels,
timestamp, timestamp,
)), )));
); }
}
// Update reviews index. // Update reviews index.
self.reviews self.reviews.insert(id, Some((revision, author)));
.insert(ReviewId(entry), Some((revision, author)));
} }
} }
Action::ReviewCommentReact { Action::ReviewCommentReact {
@ -1150,6 +1160,7 @@ impl store::Cob for Patch {
return Err(Error::Init("the second action must be of type `edit`")); return Err(Error::Init("the second action must be of type `edit`"));
}; };
let revision = Revision::new( let revision = Revision::new(
RevisionId(op.id),
op.author.into(), op.author.into(),
description, description,
base, base,
@ -1332,6 +1343,8 @@ mod lookup {
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")] #[serde(rename_all = "camelCase")]
pub struct Revision { pub struct Revision {
/// Revision identifier.
pub(super) id: RevisionId,
/// Author of the revision. /// Author of the revision.
pub(super) author: Author, pub(super) author: Author,
/// Revision description. /// Revision description.
@ -1358,6 +1371,7 @@ pub struct Revision {
impl Revision { impl Revision {
pub fn new( pub fn new(
id: RevisionId,
author: Author, author: Author,
description: String, description: String,
base: git::Oid, base: git::Oid,
@ -1368,6 +1382,7 @@ impl Revision {
let description = Edit::new(*author.public_key(), description, timestamp, Vec::default()); let description = Edit::new(*author.public_key(), description, timestamp, Vec::default());
Self { Self {
id,
author, author,
description: NonEmpty::new(description), description: NonEmpty::new(description),
base, base,
@ -1380,6 +1395,10 @@ impl Revision {
} }
} }
pub fn id(&self) -> RevisionId {
self.id
}
pub fn description(&self) -> &str { pub fn description(&self) -> &str {
self.description.last().body.as_str() self.description.last().body.as_str()
} }
@ -1439,7 +1458,7 @@ impl Revision {
} }
/// Get a review by author. /// Get a review by author.
pub fn review(&self, author: &ActorId) -> Option<&Review> { pub fn review_by(&self, author: &ActorId) -> Option<&Review> {
self.reviews.get(author).and_then(|o| o.as_ref()) self.reviews.get(author).and_then(|o| o.as_ref())
} }
} }
@ -1559,6 +1578,8 @@ impl fmt::Display for Verdict {
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")] #[serde(rename_all = "camelCase")]
pub struct Review { pub struct Review {
/// Review identifier.
pub(super) id: ReviewId,
/// Review author. /// Review author.
pub(super) author: Author, pub(super) author: Author,
/// Review verdict. /// Review verdict.
@ -1580,6 +1601,7 @@ pub struct Review {
impl Review { impl Review {
pub fn new( pub fn new(
id: ReviewId,
author: Author, author: Author,
verdict: Option<Verdict>, verdict: Option<Verdict>,
summary: Option<String>, summary: Option<String>,
@ -1587,6 +1609,7 @@ impl Review {
timestamp: Timestamp, timestamp: Timestamp,
) -> Self { ) -> Self {
Self { Self {
id,
author, author,
verdict, verdict,
summary, summary,
@ -1596,6 +1619,11 @@ impl Review {
} }
} }
/// Review identifier.
pub fn id(&self) -> ReviewId {
self.id
}
/// Review author. /// Review author.
pub fn author(&self) -> &Author { pub fn author(&self) -> &Author {
&self.author &self.author
@ -2891,7 +2919,7 @@ mod test {
let (_, revision) = patch.latest(); let (_, revision) = patch.latest();
assert_eq!(revision.reviews.len(), 1); assert_eq!(revision.reviews.len(), 1);
let review = revision.review(alice.signer.public_key()).unwrap(); let review = revision.review_by(alice.signer.public_key()).unwrap();
assert_eq!(review.verdict(), Some(Verdict::Accept)); assert_eq!(review.verdict(), Some(Verdict::Accept));
assert_eq!(review.summary(), Some("LGTM")); assert_eq!(review.summary(), Some("LGTM"));
@ -3135,7 +3163,7 @@ mod test {
.unwrap(); // Overwrite the comment. .unwrap(); // Overwrite the comment.
// //
let (_, revision) = patch.latest(); let (_, revision) = patch.latest();
let review = revision.review(alice.signer.public_key()).unwrap(); let review = revision.review_by(alice.signer.public_key()).unwrap();
assert_eq!(review.verdict(), Some(Verdict::Reject)); assert_eq!(review.verdict(), Some(Verdict::Reject));
assert_eq!(review.summary(), Some("Whoops!")); assert_eq!(review.summary(), Some("Whoops!"));
} }
@ -3180,7 +3208,7 @@ mod test {
.unwrap(); .unwrap();
let (_, revision) = patch.latest(); let (_, revision) = patch.latest();
let review = revision.review(alice.signer.public_key()).unwrap(); let review = revision.review_by(alice.signer.public_key()).unwrap();
let (_, comment) = review.comments().next().unwrap(); let (_, comment) = review.comments().next().unwrap();
assert_eq!(comment.body(), "I like these lines of code"); assert_eq!(comment.body(), "I like these lines of code");
@ -3216,7 +3244,7 @@ mod test {
let id = patch.id; let id = patch.id;
let patch = patches.get_mut(&id).unwrap(); let patch = patches.get_mut(&id).unwrap();
let (_, revision) = patch.latest(); let (_, revision) = patch.latest();
let review = revision.review(alice.signer.public_key()).unwrap(); let review = revision.review_by(alice.signer.public_key()).unwrap();
assert_eq!(review.summary(), None); assert_eq!(review.summary(), None);
} }

3
radicle/src/cob/patch/cache.rs
View File

@ -712,7 +712,9 @@ mod tests {
let oid = arbitrary::oid(); let oid = arbitrary::oid();
let timestamp = env::local_time(); let timestamp = env::local_time();
let resolves = BTreeSet::new(); let resolves = BTreeSet::new();
let id = RevisionId::from(arbitrary::oid());
let mut revision = Revision::new( let mut revision = Revision::new(
id,
Author { id: author }, Author { id: author },
description, description,
base, base,
@ -730,7 +732,6 @@ mod tests {
); );
let thread = Thread::new(arbitrary::oid(), comment); let thread = Thread::new(arbitrary::oid(), comment);
revision.discussion = thread; revision.discussion = thread;
let id = RevisionId::from(arbitrary::oid());
(id, revision) (id, revision)
} }