diff --git a/src/backend/api/backend.did b/src/backend/api/backend.did index fd307727..156e7082 100644 --- a/src/backend/api/backend.did +++ b/src/backend/api/backend.did @@ -317,11 +317,22 @@ type GetMyProposalReviewRequest = record { proposal_id : text; }; +type GetMyProposalReviewSummaryRequest = record { + proposal_id : text; +}; + type GetMyProposalReviewResponse = variant { ok : ProposalReviewWithId; err : Err; }; +type GetMyProposalReviewSummaryResponse = variant { + ok : record { + summary_markdown : text; + }; + err : Err; +}; + type ProposalReviewCommit = record { proposal_review_id : text; user_id : text; @@ -404,6 +415,7 @@ service : { create_proposal_review_image : (CreateProposalReviewImageRequest) -> (CreateProposalReviewImageResponse); delete_proposal_review_image : (DeleteProposalReviewImageRequest) -> (DeleteProposalReviewImageResponse); get_my_proposal_review : (GetMyProposalReviewRequest) -> (GetMyProposalReviewResponse) query; + get_my_proposal_review_summary : (GetMyProposalReviewSummaryRequest) -> (GetMyProposalReviewSummaryResponse) query; create_proposal_review_commit : (CreateProposalReviewCommitRequest) -> (CreateProposalReviewCommitResponse); update_proposal_review_commit : (UpdateProposalReviewCommitRequest) -> (UpdateProposalReviewCommitResponse); delete_proposal_review_commit : (DeleteProposalReviewCommitRequest) -> (DeleteProposalReviewCommitResponse); diff --git a/src/backend/api/src/lib.rs b/src/backend/api/src/lib.rs index 660b1e7c..9f181590 100644 --- a/src/backend/api/src/lib.rs +++ b/src/backend/api/src/lib.rs @@ -4,6 +4,7 @@ mod log; mod proposal; mod proposal_review; mod proposal_review_commit; +mod proposal_review_summary; mod result; mod user_profile; @@ -13,5 +14,6 @@ pub use log::*; pub use proposal::*; pub use proposal_review::*; pub use proposal_review_commit::*; +pub use proposal_review_summary::*; pub use result::*; pub use user_profile::*; diff --git a/src/backend/api/src/proposal_review_summary.rs b/src/backend/api/src/proposal_review_summary.rs new file mode 100644 index 00000000..33b3bfd7 --- /dev/null +++ b/src/backend/api/src/proposal_review_summary.rs @@ -0,0 +1,11 @@ +use candid::{CandidType, Deserialize}; + +#[derive(Debug, Clone, CandidType, Deserialize, PartialEq, Eq)] +pub struct GetMyProposalReviewSummaryRequest { + pub proposal_id: String, +} + +#[derive(Debug, Clone, CandidType, Deserialize, PartialEq, Eq)] +pub struct GetMyProposalReviewSummaryResponse { + pub summary_markdown: String, +} diff --git a/src/backend/impl/src/controllers/proposal_review_controller.rs b/src/backend/impl/src/controllers/proposal_review_controller.rs index 6b5e0cd0..610300c5 100644 --- a/src/backend/impl/src/controllers/proposal_review_controller.rs +++ b/src/backend/impl/src/controllers/proposal_review_controller.rs @@ -12,9 +12,9 @@ use crate::{ use backend_api::{ ApiError, ApiResult, CreateProposalReviewImageRequest, CreateProposalReviewImageResponse, CreateProposalReviewRequest, CreateProposalReviewResponse, DeleteProposalReviewImageRequest, - GetMyProposalReviewRequest, GetMyProposalReviewResponse, GetProposalReviewRequest, - GetProposalReviewResponse, ListProposalReviewsRequest, ListProposalReviewsResponse, - UpdateProposalReviewRequest, + GetMyProposalReviewRequest, GetMyProposalReviewResponse, GetMyProposalReviewSummaryRequest, + GetMyProposalReviewSummaryResponse, GetProposalReviewRequest, GetProposalReviewResponse, + ListProposalReviewsRequest, ListProposalReviewsResponse, UpdateProposalReviewRequest, }; use candid::Principal; use ic_cdk::*; @@ -92,6 +92,17 @@ fn get_my_proposal_review( .into() } +#[query] +fn get_my_proposal_review_summary( + request: GetMyProposalReviewSummaryRequest, +) -> ApiResult { + let calling_principal = caller(); + + ProposalReviewController::default() + .get_my_proposal_review_summary(calling_principal, request) + .into() +} + struct ProposalReviewController { access_control_service: A, proposal_review_service: P, @@ -199,6 +210,20 @@ impl ProposalReviewController .get_my_proposal_review(calling_principal, request) } + fn get_my_proposal_review_summary( + &self, + calling_principal: Principal, + request: GetMyProposalReviewSummaryRequest, + ) -> Result { + self.access_control_service + .assert_principal_not_anonymous(&calling_principal)?; + self.access_control_service + .assert_principal_is_reviewer(&calling_principal)?; + + self.proposal_review_service + .get_my_proposal_review_summary(calling_principal, request) + } + fn delete_proposal_review_image( &self, calling_principal: Principal, diff --git a/src/backend/impl/src/fixtures/commit_sha.rs b/src/backend/impl/src/fixtures/commit_sha.rs index b6299717..6f260a4e 100644 --- a/src/backend/impl/src/fixtures/commit_sha.rs +++ b/src/backend/impl/src/fixtures/commit_sha.rs @@ -11,3 +11,8 @@ pub fn commit_sha_a() -> CommitSha { pub fn commit_sha_b() -> CommitSha { CommitSha::try_from("47d98477c6c59e570e2220aab433b0943b326ef8").unwrap() } + +#[fixture] +pub fn commit_sha_c() -> CommitSha { + CommitSha::try_from("e62e328fa193e73fbb7562458d719f311c25d455").unwrap() +} diff --git a/src/backend/impl/src/fixtures/proposal_review_commit.rs b/src/backend/impl/src/fixtures/proposal_review_commit.rs index 95c61fc3..6a2148e8 100644 --- a/src/backend/impl/src/fixtures/proposal_review_commit.rs +++ b/src/backend/impl/src/fixtures/proposal_review_commit.rs @@ -1,6 +1,6 @@ use rstest::*; -use crate::repositories::{ProposalReviewCommit, ReviewCommitState}; +use crate::repositories::{ProposalReviewCommit, ReviewCommitState, ReviewedCommitState}; use super::{commit_sha_a, commit_sha_b, date_time_a, proposal_review_id, user_id}; @@ -12,11 +12,11 @@ pub fn proposal_review_commit_reviewed() -> ProposalReviewCommit { created_at: date_time_a(), last_updated_at: None, commit_sha: commit_sha_a(), - state: ReviewCommitState::Reviewed { + state: ReviewCommitState::Reviewed(ReviewedCommitState { matches_description: Some(true), comment: Some("Review commit comment".to_string()), highlights: vec![], - }, + }), } } diff --git a/src/backend/impl/src/mappings/proposal_review_commit.rs b/src/backend/impl/src/mappings/proposal_review_commit.rs index c0f84b1a..8c3960c8 100644 --- a/src/backend/impl/src/mappings/proposal_review_commit.rs +++ b/src/backend/impl/src/mappings/proposal_review_commit.rs @@ -1,13 +1,15 @@ -use crate::repositories::{ProposalReviewCommit, ProposalReviewCommitId, ReviewCommitState}; +use crate::repositories::{ + ProposalReviewCommit, ProposalReviewCommitId, ReviewCommitState, ReviewedCommitState, +}; impl From for backend_api::ReviewCommitState { fn from(proposal_review_commit_state: ReviewCommitState) -> Self { match proposal_review_commit_state { - ReviewCommitState::Reviewed { + ReviewCommitState::Reviewed(ReviewedCommitState { matches_description, comment, highlights, - } => backend_api::ReviewCommitState::Reviewed { + }) => backend_api::ReviewCommitState::Reviewed { matches_description, comment, highlights, @@ -24,11 +26,11 @@ impl From for ReviewCommitState { matches_description, comment, highlights, - } => ReviewCommitState::Reviewed { + } => ReviewCommitState::Reviewed(ReviewedCommitState { matches_description, comment, highlights, - }, + }), backend_api::ReviewCommitState::NotReviewed => ReviewCommitState::NotReviewed, } } diff --git a/src/backend/impl/src/repositories/proposal_review_commit_repository.rs b/src/backend/impl/src/repositories/proposal_review_commit_repository.rs index d6b676e7..aa11986b 100644 --- a/src/backend/impl/src/repositories/proposal_review_commit_repository.rs +++ b/src/backend/impl/src/repositories/proposal_review_commit_repository.rs @@ -233,7 +233,7 @@ mod tests { use super::*; use crate::{ fixtures::{self, commit_sha_a, commit_sha_b, uuid_a, uuid_b}, - repositories::ReviewCommitState, + repositories::{ReviewCommitState, ReviewedCommitState}, }; use rstest::*; @@ -507,11 +507,11 @@ mod tests { #[fixture] fn updated_proposal_review_commit() -> ProposalReviewCommit { ProposalReviewCommit { - state: ReviewCommitState::Reviewed { + state: ReviewCommitState::Reviewed(ReviewedCommitState { matches_description: Some(false), comment: Some("Updated comment".to_string()), highlights: vec![], - }, + }), ..fixtures::proposal_review_commit_not_reviewed() } } diff --git a/src/backend/impl/src/repositories/types/proposal_review.rs b/src/backend/impl/src/repositories/types/proposal_review.rs index e2edfa19..9f55e7eb 100644 --- a/src/backend/impl/src/repositories/types/proposal_review.rs +++ b/src/backend/impl/src/repositories/types/proposal_review.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, ops::RangeBounds}; +use std::{borrow::Cow, fmt::Display, ops::RangeBounds}; use backend_api::ApiError; use candid::{CandidType, Decode, Deserialize, Encode}; @@ -26,6 +26,17 @@ pub enum ProposalVote { No = 2, } +impl Display for ProposalVote { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let val = match self { + ProposalVote::Unspecified => "-", + ProposalVote::Yes => "ADOPTED", + ProposalVote::No => "REJECTED", + }; + write!(f, "{}", val) + } +} + #[derive(Debug, CandidType, Deserialize, Clone, PartialEq, Eq)] pub struct ProposalReview { pub proposal_id: ProposalId, diff --git a/src/backend/impl/src/repositories/types/proposal_review_commit.rs b/src/backend/impl/src/repositories/types/proposal_review_commit.rs index 056ccbad..02ee95ca 100644 --- a/src/backend/impl/src/repositories/types/proposal_review_commit.rs +++ b/src/backend/impl/src/repositories/types/proposal_review_commit.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, ops::RangeBounds}; +use std::{borrow::Cow, fmt::Display, ops::RangeBounds}; use backend_api::ApiError; use candid::{CandidType, Decode, Deserialize, Encode}; @@ -11,13 +11,40 @@ use super::{CommitSha, DateTime, ProposalReviewId, UserId, Uuid}; pub type ProposalReviewCommitId = Uuid; +#[derive(Debug, CandidType, Deserialize, Clone, PartialEq, Eq)] +pub struct ReviewedCommitState { + pub matches_description: Option, + pub comment: Option, + pub highlights: Vec, +} + +impl Display for ReviewedCommitState { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut commit_details = format!( + "Matches description: {}", + self.matches_description + .map(|b| b.to_string()) + .unwrap_or_else(|| "Unanswered".to_string()), + ); + if let Some(comment) = self.comment.as_ref() { + if !comment.is_empty() { + commit_details = format!("{}\nComment: {}", commit_details, comment); + } + } + if !self.highlights.is_empty() { + commit_details = format!( + "{}\nHighlights: {}", + commit_details, + self.highlights.join(", ") + ); + } + write!(f, "{}", commit_details) + } +} + #[derive(Debug, CandidType, Deserialize, Clone, PartialEq, Eq)] pub enum ReviewCommitState { - Reviewed { - matches_description: Option, - comment: Option, - highlights: Vec, - }, + Reviewed(ReviewedCommitState), NotReviewed, } @@ -39,6 +66,14 @@ impl ProposalReviewCommit { pub fn is_not_reviewed(&self) -> bool { matches!(&self.state, ReviewCommitState::NotReviewed) } + + pub fn reviewed_state(&self) -> Option<&ReviewedCommitState> { + if let ReviewCommitState::Reviewed(state) = &self.state { + Some(state) + } else { + None + } + } } impl Storable for ProposalReviewCommit { @@ -174,4 +209,42 @@ mod tests { assert_eq!(key, deserialized_key); } + + #[rstest] + fn reviewed_commit_state_display_impl() { + let mut state = ReviewedCommitState { + matches_description: None, + comment: None, + highlights: vec![], + }; + + assert_eq!(state.to_string(), "Matches description: Unanswered"); + + state.matches_description = Some(false); + assert_eq!(state.to_string(), "Matches description: false"); + + state.matches_description = Some(true); + assert_eq!(state.to_string(), "Matches description: true"); + + state.comment = Some("".to_string()); + assert_eq!(state.to_string(), "Matches description: true"); + + state.comment = Some("test".to_string()); + assert_eq!( + state.to_string(), + "Matches description: true\nComment: test" + ); + + state.highlights = vec!["test".to_string()]; + assert_eq!( + state.to_string(), + "Matches description: true\nComment: test\nHighlights: test" + ); + + state.highlights = vec!["test1".to_string(), "test2".to_string()]; + assert_eq!( + state.to_string(), + "Matches description: true\nComment: test\nHighlights: test1, test2" + ); + } } diff --git a/src/backend/impl/src/services/proposal_review_service.rs b/src/backend/impl/src/services/proposal_review_service.rs index a99967d7..a4b72637 100644 --- a/src/backend/impl/src/services/proposal_review_service.rs +++ b/src/backend/impl/src/services/proposal_review_service.rs @@ -3,20 +3,20 @@ use crate::{ mappings::map_proposal_review, repositories::{ CertificationRepository, CertificationRepositoryImpl, CreateImageRequest, DateTime, Image, - ImageRepository, ImageRepositoryImpl, ProposalId, ProposalRepository, - ProposalRepositoryImpl, ProposalReview, ProposalReviewCommitRepository, - ProposalReviewCommitRepositoryImpl, ProposalReviewId, ProposalReviewRepository, - ProposalReviewRepositoryImpl, ProposalReviewStatus, ProposalVote, UserId, - UserProfileRepository, UserProfileRepositoryImpl, + ImageRepository, ImageRepositoryImpl, Proposal, ProposalId, ProposalRepository, + ProposalRepositoryImpl, ProposalReview, ProposalReviewCommit, ProposalReviewCommitId, + ProposalReviewCommitRepository, ProposalReviewCommitRepositoryImpl, ProposalReviewId, + ProposalReviewRepository, ProposalReviewRepositoryImpl, ProposalReviewStatus, ProposalVote, + UserId, UserProfileRepository, UserProfileRepositoryImpl, }, system_api::get_date_time, }; use backend_api::{ ApiError, CreateProposalReviewImageRequest, CreateProposalReviewImageResponse, CreateProposalReviewRequest, CreateProposalReviewResponse, DeleteProposalReviewImageRequest, - GetMyProposalReviewRequest, GetMyProposalReviewResponse, GetProposalReviewRequest, - GetProposalReviewResponse, ListProposalReviewsRequest, ListProposalReviewsResponse, - UpdateProposalReviewRequest, + GetMyProposalReviewRequest, GetMyProposalReviewResponse, GetMyProposalReviewSummaryRequest, + GetMyProposalReviewSummaryResponse, GetProposalReviewRequest, GetProposalReviewResponse, + ListProposalReviewsRequest, ListProposalReviewsResponse, UpdateProposalReviewRequest, }; use candid::Principal; use std::{path::PathBuf, str::FromStr}; @@ -69,6 +69,12 @@ pub trait ProposalReviewService { calling_principal: Principal, request: GetMyProposalReviewRequest, ) -> Result; + + fn get_my_proposal_review_summary( + &self, + calling_principal: Principal, + request: GetMyProposalReviewSummaryRequest, + ) -> Result; } pub struct ProposalReviewServiceImpl< @@ -198,19 +204,9 @@ impl< ) -> Result<(), ApiError> { self.validate_fields(request.summary.as_ref(), request.review_duration_mins)?; - let user_id = self - .user_profile_repository - .get_user_id_by_principal(&calling_principal) - .ok_or_else(|| { - ApiError::not_found(&format!( - "User id for principal {} not found", - calling_principal.to_text() - )) - })?; - - let (id, mut current_proposal_review) = self.get_current_proposal_review( + let (id, mut current_proposal_review, _) = self.get_current_proposal_review_with_user_id( request.proposal_id, - user_id, + &calling_principal, request.status.as_ref(), )?; @@ -344,18 +340,12 @@ impl< ) -> Result { request.validate_fields()?; - let user_id = self - .user_profile_repository - .get_user_id_by_principal(&calling_principal) - .ok_or_else(|| { - ApiError::not_found(&format!( - "User id for principal {} not found", - calling_principal.to_text() - )) - })?; - - let (id, mut current_proposal_review) = - self.get_current_proposal_review(request.proposal_id.clone(), user_id, None)?; + let (id, mut current_proposal_review, user_id) = self + .get_current_proposal_review_with_user_id( + request.proposal_id.clone(), + &calling_principal, + None, + )?; // for now, we only allow one image if !current_proposal_review.images_ids.is_empty() { @@ -399,18 +389,11 @@ impl< calling_principal: Principal, request: DeleteProposalReviewImageRequest, ) -> Result<(), ApiError> { - let user_id = self - .user_profile_repository - .get_user_id_by_principal(&calling_principal) - .ok_or_else(|| { - ApiError::not_found(&format!( - "User id for principal {} not found", - calling_principal.to_text() - )) - })?; - - let (id, mut current_proposal_review) = - self.get_current_proposal_review(request.proposal_id.clone(), user_id, None)?; + let (id, mut current_proposal_review, _) = self.get_current_proposal_review_with_user_id( + request.proposal_id.clone(), + &calling_principal, + None, + )?; let image_id_to_delete = PathBuf::from(request.image_path.clone()) .iter() @@ -450,35 +433,49 @@ impl< calling_principal: Principal, request: GetMyProposalReviewRequest, ) -> Result { - let proposal_id = ProposalId::try_from(request.proposal_id.as_str())?; - - let user_id = self - .user_profile_repository - .get_user_id_by_principal(&calling_principal) - .ok_or_else(|| { - ApiError::not_found(&format!( - "User id for principal {} not found", - calling_principal.to_text() - )) - })?; - - let (proposal_review_id, proposal_review) = self - .proposal_review_repository - .get_proposal_review_by_proposal_id_and_user_id(proposal_id, user_id) - .ok_or_else(|| { - ApiError::not_found(&format!( - "Proposal review for proposal {} for principal {} not found", - request.proposal_id, - calling_principal.to_text() - )) - })?; + let (proposal_review_id, proposal_review, _, _) = self + .get_proposal_review_with_proposal_id_and_user_id( + request.proposal_id, + &calling_principal, + )?; let response = self.map_proposal_review(proposal_review_id, proposal_review)?; Ok(response) } + + fn get_my_proposal_review_summary( + &self, + calling_principal: Principal, + request: GetMyProposalReviewSummaryRequest, + ) -> Result { + let (proposal_review_id, proposal_review, proposal, _) = self + .get_proposal_review_with_proposal_and_user_id( + request.proposal_id, + &calling_principal, + )?; + let (proposal_review_commits, images_paths) = self + .get_proposal_review_commits_and_images_paths(proposal_review_id, &proposal_review)?; + + let summary_markdown = proposal_review_summary_markdown( + &proposal, + &proposal_review, + &proposal_review_commits, + &images_paths, + ); + + Ok(GetMyProposalReviewSummaryResponse { summary_markdown }) + } } +type GetProposalReviewCommitsAndImagesPathsResult = Result< + ( + Vec<(ProposalReviewCommitId, ProposalReviewCommit)>, + Vec, + ), + ApiError, +>; + impl< PR: ProposalReviewRepository, U: UserProfileRepository, @@ -540,22 +537,17 @@ impl< Ok(()) } - fn get_current_proposal_review( + fn get_current_proposal_review_with_user_id( &self, raw_proposal_id: String, - user_id: UserId, + user_principal: &Principal, request_status: Option<&backend_api::ProposalReviewStatus>, - ) -> Result<(ProposalReviewId, ProposalReview), ApiError> { - let proposal_id = ProposalId::try_from(raw_proposal_id.as_str())?; - let (id, current_proposal_review) = self - .proposal_review_repository - .get_proposal_review_by_proposal_id_and_user_id(proposal_id, user_id) - .ok_or_else(|| { - ApiError::not_found(&format!( - "Proposal review for proposal with Id {} not found", - raw_proposal_id - )) - })?; + ) -> Result<(ProposalReviewId, ProposalReview, UserId), ApiError> { + let (id, current_proposal_review, proposal, user_id) = self + .get_proposal_review_with_proposal_and_user_id( + raw_proposal_id.clone(), + user_principal, + )?; if current_proposal_review.is_published() && !request_status.is_some_and(|s| s == &backend_api::ProposalReviewStatus::Draft) @@ -566,31 +558,20 @@ impl< ))); } - match self.proposal_repository.get_proposal_by_id(&proposal_id) { - Some(proposal) => { - if proposal.is_completed() { - return Err(ApiError::conflict( - "The proposal associated with this review is already completed", - )); - } - } - None => { - // this should never happen - return Err(ApiError::not_found(&format!( - "Proposal with Id {} not found", - proposal_id.to_string() - ))); - } + if proposal.is_completed() { + return Err(ApiError::conflict( + "The proposal associated with this review is already completed", + )); } - Ok((id, current_proposal_review)) + Ok((id, current_proposal_review, user_id)) } - fn map_proposal_review( + fn get_proposal_review_commits_and_images_paths( &self, id: ProposalReviewId, - proposal_review: ProposalReview, - ) -> Result { + proposal_review: &ProposalReview, + ) -> GetProposalReviewCommitsAndImagesPathsResult { let proposal_review_commits = self .proposal_review_commit_repository .get_proposal_review_commits_by_proposal_review_id(id)?; @@ -606,6 +587,17 @@ impl< }) .collect(); + Ok((proposal_review_commits, images_paths)) + } + + fn map_proposal_review( + &self, + id: ProposalReviewId, + proposal_review: ProposalReview, + ) -> Result { + let (proposal_review_commits, images_paths) = + self.get_proposal_review_commits_and_images_paths(id, &proposal_review)?; + Ok(map_proposal_review( id, proposal_review, @@ -652,6 +644,177 @@ impl< Ok(()) } + + fn get_proposal_review_with_proposal_id_and_user_id( + &self, + raw_proposal_id: String, + user_principal: &Principal, + ) -> Result<(ProposalReviewId, ProposalReview, ProposalId, UserId), ApiError> { + let proposal_id = ProposalId::try_from(raw_proposal_id.as_str())?; + let user_id = self + .user_profile_repository + .get_user_id_by_principal(user_principal) + .ok_or_else(|| { + ApiError::not_found(&format!( + "User id for principal {} not found", + user_principal.to_text() + )) + })?; + let (id, current_proposal_review) = self + .proposal_review_repository + .get_proposal_review_by_proposal_id_and_user_id(proposal_id, user_id) + .ok_or_else(|| { + ApiError::not_found(&format!( + "Proposal review for proposal {} for principal {} not found", + raw_proposal_id, + user_principal.to_text() + )) + })?; + + Ok((id, current_proposal_review, proposal_id, user_id)) + } + + fn get_proposal_review_with_proposal_and_user_id( + &self, + raw_proposal_id: String, + user_principal: &Principal, + ) -> Result<(ProposalReviewId, ProposalReview, Proposal, UserId), ApiError> { + let (id, current_proposal_review, proposal_id, user_id) = + self.get_proposal_review_with_proposal_id_and_user_id(raw_proposal_id, user_principal)?; + let proposal = self + .proposal_repository + .get_proposal_by_id(&proposal_id) + .ok_or_else(|| { + // this should never happen, as the proposal review is associated with a proposal that must exist + ApiError::not_found(&format!( + "Proposal with Id {} not found", + proposal_id.to_string() + )) + })?; + + Ok((id, current_proposal_review, proposal, user_id)) + } +} + +/// Returns the markdown representation for the proposal review. +/// +/// Template: +/// ```markdown +/// # Proposal [nervous system proposal id] +/// +/// Hashes match: [true or false] +/// All reviewed commits match their descriptions: [true or false] +/// +/// [proposal review images if any] +/// +/// Summary: +/// [proposal review summary if any] +/// +/// Commits review: +/// - **[commit sha truncated to 9 characters]**: +/// Matches description: [true or false] +/// Comment: [commit comment] +/// Highlights: [comma-separated list of commit highlights] +/// - **[commit sha truncated to 9 characters]**: +/// ... +/// ... +/// ``` +fn proposal_review_summary_markdown( + proposal: &Proposal, + proposal_review: &ProposalReview, + proposal_review_commits: &[(ProposalReviewCommitId, ProposalReviewCommit)], + images_paths: &[String], +) -> String { + let mut md_content = String::new(); + let reviewed_commits: Vec<&(ProposalReviewCommitId, ProposalReviewCommit)> = + proposal_review_commits + .iter() + .filter(|(_, commit)| commit.is_reviewed()) + .collect(); + + // header + { + md_content.push_str(&format!( + "# Proposal {}\n\n", + proposal.nervous_system.proposal_id() + )); + } + // info + { + md_content.push_str(&format!("Vote: {}\n", proposal_review.vote)); + md_content.push_str(&format!( + "Hashes match: {}\n", + proposal_review + .build_reproduced + .map(|b| b.to_string()) + .unwrap_or("Unanswered".to_string()) + )); + md_content.push_str(&format!( + "All reviewed commits match their descriptions: {}\n", + { + let mut no_answer_commits_count = 0; + let mut all_reviewed_commits_match = true; + + for (_, commit) in &reviewed_commits { + let state = commit.reviewed_state().expect("should be reviewed"); + match state.matches_description { + None => no_answer_commits_count += 1, + Some(b) => { + all_reviewed_commits_match = b; + } + } + } + + if no_answer_commits_count == reviewed_commits.len() { + "Unanswered".to_string() + } else if no_answer_commits_count > 0 { + "Only partially answered (see individual reports below)".to_string() + } else { + all_reviewed_commits_match.to_string() + } + }, + )); + } + // images + { + for image_path in images_paths { + md_content.push_str(&format!("\n![]({})\n", image_path)); + } + } + // summary + { + if let Some(summary) = proposal_review.summary.as_ref() { + md_content.push_str(&format!("\nSummary:\n{}\n", summary)); + } + } + // commits + { + if !reviewed_commits.is_empty() { + md_content.push_str("\nCommits review:\n"); + + const INDENT: &str = " "; + + let mut commits_list = String::new(); + for (_, commit) in reviewed_commits { + if let Some(state) = commit.reviewed_state() { + let mut commit_sha = commit.commit_sha.to_string(); + commit_sha.truncate(9); + commits_list.push_str(&format!( + "- **{}**:\n{INDENT}{}\n", + commit_sha, + state + .to_string() + .lines() + .collect::>() + .join(&format!("\n{INDENT}")) + )); + } + } + md_content.push_str(&commits_list); + } + } + + md_content } #[cfg(test)] @@ -662,7 +825,8 @@ mod tests { repositories::{ ImageId, MockCertificationRepository, MockImageRepository, MockProposalRepository, MockProposalReviewCommitRepository, MockProposalReviewRepository, - MockUserProfileRepository, ProposalReviewId, IMAGES_BASE_PATH, + MockUserProfileRepository, NervousSystem, ProposalReviewId, ReviewCommitState, + ReviewedCommitState, IMAGES_BASE_PATH, }, }; use backend_api::{ @@ -1240,8 +1404,9 @@ mod tests { assert_eq!( result, ApiError::not_found(&format!( - "Proposal review for proposal with Id {} not found", - request.proposal_id + "Proposal review for proposal {} for principal {} not found", + request.proposal_id, + calling_principal.to_text() )) ) } @@ -1327,10 +1492,16 @@ mod tests { .expect_get_proposal_review_by_proposal_id_and_user_id() .once() .with(eq(original_proposal_review.proposal_id), eq(user_id)) - .return_const(Some((id, original_proposal_review))); + .return_const(Some((id, original_proposal_review.clone()))); pr_repository_mock.expect_update_proposal_review().never(); let mut p_repository_mock = MockProposalRepository::new(); - p_repository_mock.expect_get_proposal_by_id().never(); + p_repository_mock + .expect_get_proposal_by_id() + .once() + .with(eq(original_proposal_review.proposal_id)) + .return_const(Some(fixtures::nns_replica_version_management_proposal( + None, None, + ))); let prc_repository_mock = MockProposalReviewCommitRepository::new(); let image_repository_mock = MockImageRepository::new(); let certification_repository_mock = MockCertificationRepository::new(); @@ -2317,4 +2488,254 @@ mod tests { ), ) } + + #[fixture] + fn basic_proposal() -> Proposal { + let proposal = fixtures::nns_replica_version_management_proposal(None, None); + Proposal { + nervous_system: match proposal.nervous_system { + NervousSystem::Network { proposal_info, .. } => NervousSystem::Network { + proposal_id: 123, + proposal_info, + }, + }, + ..proposal + } + } + + #[fixture] + fn basic_review() -> ProposalReview { + ProposalReview { + vote: ProposalVote::Yes, + summary: Some("Test summary".to_string()), + build_reproduced: Some(true), + ..fixtures::proposal_review_published() + } + } + + #[fixture] + fn all_reviewed_commits_match() -> Vec<(ProposalReviewCommitId, ProposalReviewCommit)> { + vec![ + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_a(), + state: ReviewCommitState::Reviewed(ReviewedCommitState { + matches_description: Some(true), + comment: Some("Good commit".to_string()), + highlights: vec!["Feature A".to_string(), "Bug fix B".to_string()], + }), + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_b(), + state: ReviewCommitState::Reviewed(ReviewedCommitState { + matches_description: Some(false), + comment: Some("Issues found".to_string()), + highlights: vec!["Missing tests".to_string()], + }), + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_c(), + state: ReviewCommitState::NotReviewed, + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ] + } + + #[fixture] + fn all_reviewed_commits_not_answered() -> Vec<(ProposalReviewCommitId, ProposalReviewCommit)> { + vec![ + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_a(), + state: ReviewCommitState::Reviewed(ReviewedCommitState { + matches_description: None, + comment: Some("Good commit".to_string()), + highlights: vec!["Feature A".to_string(), "Bug fix B".to_string()], + }), + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_b(), + state: ReviewCommitState::Reviewed(ReviewedCommitState { + matches_description: None, + comment: Some("Issues found".to_string()), + highlights: vec!["Missing tests".to_string()], + }), + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_c(), + state: ReviewCommitState::NotReviewed, + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ] + } + + #[fixture] + fn some_reviewed_commits_not_answered() -> Vec<(ProposalReviewCommitId, ProposalReviewCommit)> { + vec![ + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_a(), + state: ReviewCommitState::Reviewed(ReviewedCommitState { + matches_description: Some(true), + comment: Some("Good commit".to_string()), + highlights: vec!["Feature A".to_string(), "Bug fix B".to_string()], + }), + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_b(), + state: ReviewCommitState::Reviewed(ReviewedCommitState { + matches_description: None, + comment: Some("Issues found".to_string()), + highlights: vec!["Missing tests".to_string()], + }), + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ( + fixtures::uuid(), + ProposalReviewCommit { + commit_sha: fixtures::commit_sha_c(), + state: ReviewCommitState::NotReviewed, + ..fixtures::proposal_review_commit_reviewed() + }, + ), + ] + } + + #[fixture] + fn images_paths() -> Vec { + vec![ + "/images/13449503-1b2f-4b92-8346-8e843253e842.png".to_string(), + "/images/68362227-6f0f-4fb4-b80a-0bfa53a9e99b.png".to_string(), + ] + } + + #[rstest] + fn test_proposal_review_summary_markdown() { + let basic_proposal = basic_proposal(); + let mut basic_review = basic_review(); + let mut review_commits = all_reviewed_commits_match(); + let images_paths = images_paths(); + let markdown = proposal_review_summary_markdown( + &basic_proposal, + &basic_review, + &review_commits, + &images_paths, + ); + + fn expected_markdown( + hashes_match: &str, + all_reviewed_commits_match: &str, + commits_matches: (&str, &str), + ) -> String { + format!( + r#"# Proposal 123 + +Vote: ADOPTED +Hashes match: {hashes_match} +All reviewed commits match their descriptions: {all_reviewed_commits_match} + +![](/images/13449503-1b2f-4b92-8346-8e843253e842.png) + +![](/images/68362227-6f0f-4fb4-b80a-0bfa53a9e99b.png) + +Summary: +Test summary + +Commits review: +- **28111ed23**: + Matches description: {} + Comment: Good commit + Highlights: Feature A, Bug fix B +- **47d98477c**: + Matches description: {} + Comment: Issues found + Highlights: Missing tests +"#, + commits_matches.0, commits_matches.1 + ) + } + + assert_eq!( + markdown, + expected_markdown("true", "false", ("true", "false")) + ); + + basic_review.build_reproduced = Some(false); + let markdown = proposal_review_summary_markdown( + &basic_proposal, + &basic_review, + &review_commits, + &images_paths, + ); + assert_eq!( + markdown, + expected_markdown("false", "false", ("true", "false")) + ); + + basic_review.build_reproduced = None; + let markdown = proposal_review_summary_markdown( + &basic_proposal, + &basic_review, + &review_commits, + &images_paths, + ); + assert_eq!( + markdown, + expected_markdown("Unanswered", "false", ("true", "false")) + ); + + review_commits = some_reviewed_commits_not_answered(); + let markdown = proposal_review_summary_markdown( + &basic_proposal, + &basic_review, + &review_commits, + &images_paths, + ); + assert_eq!( + markdown, + expected_markdown( + "Unanswered", + "Only partially answered (see individual reports below)", + ("true", "Unanswered") + ) + ); + + review_commits = all_reviewed_commits_not_answered(); + let markdown = proposal_review_summary_markdown( + &basic_proposal, + &basic_review, + &review_commits, + &images_paths, + ); + assert_eq!( + markdown, + expected_markdown("Unanswered", "Unanswered", ("Unanswered", "Unanswered")) + ); + } } diff --git a/src/backend/integration/src/support/common.ts b/src/backend/integration/src/support/common.ts index ff3d5bdd..01bd1693 100644 --- a/src/backend/integration/src/support/common.ts +++ b/src/backend/integration/src/support/common.ts @@ -23,7 +23,10 @@ export async function createProposal( actor: BackendActorService, governance: Governance, title: string, -): Promise { +): Promise<{ + nnsProposalId: bigint; + proposalId: string; +}> { const neuronId = await governance.createNeuron(nnsProposerIdentity); const rvmProposalId = await governance.createRvmProposal( @@ -55,7 +58,10 @@ export async function createProposal( ); } - return proposal.id; + return { + nnsProposalId: rvmProposalId, + proposalId: proposal.id, + }; } /** @@ -101,17 +107,22 @@ export async function createProposalReview( reviewer: Identity, existingProposalId?: string, ): Promise<{ + nnsProposalId?: bigint; proposalId: string; proposalReviewId: string; }> { let proposalId = existingProposalId; + let nnsProposalId: bigint | undefined; if (!proposalId) { // randomize the proposal title to make it unique - proposalId = await createProposal( + const res = await createProposal( actor, governance, 'Test proposal ' + Math.random(), ); + + proposalId = res.proposalId; + nnsProposalId = res.nnsProposalId; } actor.setIdentity(reviewer); @@ -124,7 +135,7 @@ export async function createProposalReview( }); const { id: proposalReviewId } = extractOkResponse(res); - return { proposalId, proposalReviewId }; + return { nnsProposalId, proposalId, proposalReviewId }; } /** @@ -208,6 +219,7 @@ export async function createProposalReviewCommit( reviewer: Identity, commitSha: string, existingProposalReviewData?: { + nnsProposalId?: bigint; proposalId: string; proposalReviewId: string; }, @@ -235,7 +247,7 @@ export async function createProposalReviewCommit( reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], + highlights: ['first highlight', 'second highlight'], }, }, }); diff --git a/src/backend/integration/src/tests/get_my_proposal_review_summary.spec.ts b/src/backend/integration/src/tests/get_my_proposal_review_summary.spec.ts new file mode 100644 index 00000000..10f02e24 --- /dev/null +++ b/src/backend/integration/src/tests/get_my_proposal_review_summary.spec.ts @@ -0,0 +1,176 @@ +import { Identity } from '@dfinity/agent'; +import { describe, beforeAll, afterAll, expect, it } from 'bun:test'; +import { + Governance, + TestDriver, + VALID_COMMIT_SHA_A, + VALID_COMMIT_SHA_B, + anonymousIdentity, + createProposalReview, + createProposalReviewCommit, + extractErrResponse, + extractOkResponse, +} from '../support'; +import { CODEGOV_LOGO_PNG } from '../fixtures'; + +describe('get my proposal review summary', () => { + let driver: TestDriver; + + let alice: Identity; + let bob: Identity; + let charlie: Identity; + + let governance: Governance; + + let proposalReviewData: { + nnsProposalId?: bigint; + proposalId: string; + proposalReviewId: string; + }; + + beforeAll(async () => { + driver = await TestDriver.createWithNnsState(); + + governance = new Governance(driver.pic); + }); + + afterAll(async () => { + await driver.tearDown(); + }); + + beforeAll(async () => { + [alice] = await driver.users.createReviewer(); + [bob] = await driver.users.createAdmin(); + [charlie] = await driver.users.createAnonymous(); + + proposalReviewData = await createProposalReview( + driver.actor, + governance, + alice, + ); + for (const commitSha of [VALID_COMMIT_SHA_A, VALID_COMMIT_SHA_B]) { + await createProposalReviewCommit( + driver.actor, + governance, + alice, + commitSha, + proposalReviewData, + ); + } + await driver.actor.create_proposal_review_image({ + proposal_id: proposalReviewData.proposalId, + content_type: 'image/png', + content_bytes: CODEGOV_LOGO_PNG, + }); + }); + + it('should not allow anonymous principals to get their own proposal review summary', async () => { + driver.actor.setIdentity(anonymousIdentity); + + const res = await driver.actor.get_my_proposal_review_summary({ + proposal_id: proposalReviewData.proposalId, + }); + const resErr = extractErrResponse(res); + + expect(resErr).toEqual({ + code: 401, + message: 'Anonymous principals are not allowed to call this endpoint', + }); + }); + + it('should not allow admin users to get their own proposal review summary', async () => { + driver.actor.setIdentity(bob); + + const res = await driver.actor.get_my_proposal_review_summary({ + proposal_id: proposalReviewData.proposalId, + }); + const resErr = extractErrResponse(res); + + expect(resErr).toEqual({ + code: 403, + message: `Principal ${bob.getPrincipal()} must be a reviewer to call this endpoint`, + }); + }); + + it('should not allow anonymous users to get their own proposal review summary', async () => { + driver.actor.setIdentity(charlie); + + const res = await driver.actor.get_my_proposal_review_summary({ + proposal_id: proposalReviewData.proposalId, + }); + const resErr = extractErrResponse(res); + + expect(resErr).toEqual({ + code: 403, + message: `Principal ${charlie.getPrincipal()} must be a reviewer to call this endpoint`, + }); + }); + + it('should allow reviewers to get their own proposal review summary', async () => { + driver.actor.setIdentity(alice); + + const resReview = await driver.actor.get_my_proposal_review({ + proposal_id: proposalReviewData.proposalId, + }); + const resReviewOk = extractOkResponse(resReview); + const proposalReview = resReviewOk.proposal_review; + // make sure the proposal review has at least one image and one commit + expect(proposalReview.images_paths.length).toBeGreaterThan(0); + expect(proposalReview.proposal_review_commits.length).toBeGreaterThan(0); + + const resReviewSummary = await driver.actor.get_my_proposal_review_summary({ + proposal_id: proposalReviewData.proposalId, + }); + const resReviewSummaryOk = extractOkResponse(resReviewSummary); + const summaryMarkdown = resReviewSummaryOk.summary_markdown; + + let expectedMarkdown = `# Proposal ${proposalReviewData.nnsProposalId?.toString()}\n\n`; + expectedMarkdown += `Vote: ${'yes' in proposalReview.vote ? 'ADOPTED' : 'no' in proposalReview.vote ? 'REJECTED' : '-'}\n`; + expectedMarkdown += `Hashes match: ${proposalReview.build_reproduced[0] ? 'true' : 'false'}\n`; + const allCommitsMatchDescription = + proposalReview.proposal_review_commits.every( + ({ proposal_review_commit: commit }) => + 'reviewed' in commit.state && + commit.state.reviewed.matches_description[0], + ); + expectedMarkdown += `All reviewed commits match their descriptions: ${allCommitsMatchDescription ? 'true' : 'false'}\n`; + expectedMarkdown += `${proposalReview.images_paths + .map(imagePath => `\n![](${imagePath})`) + .join('\n')}\n`; + expectedMarkdown += `\nSummary:\n${proposalReview.summary[0]}\n`; + expectedMarkdown += '\nCommits review:\n'; + + const INDENT = ' '; + for (const { + proposal_review_commit: commit, + } of proposalReview.proposal_review_commits) { + if ('reviewed' in commit.state) { + let commitReview = `${INDENT}Matches description: ${commit.state.reviewed.matches_description}`; + if (commit.state.reviewed.comment) { + commitReview += `\n${INDENT}Comment: ${commit.state.reviewed.comment}`; + } + if (commit.state.reviewed.highlights.length > 0) { + commitReview += `\n${INDENT}Highlights: ${commit.state.reviewed.highlights.join(', ')}`; + } + expectedMarkdown += `- **${commit.commit_sha.slice(0, 9)}**:\n${commitReview}\n`; + } + } + + expect(summaryMarkdown).toEqual(expectedMarkdown); + }); + + it('should return a 404 if the review does not exist', async () => { + driver.actor.setIdentity(alice); + const proposalId = '269a316e-589b-4c17-bca7-2ef47bea48fe'; + + const res = await driver.actor.get_my_proposal_review_summary({ + proposal_id: proposalId, + }); + const resErr = extractErrResponse(res); + + expect(resErr).toEqual({ + code: 404, + message: `Proposal review for proposal ${proposalId} for principal ${alice.getPrincipal()} not found`, + }); + }); +}); diff --git a/src/backend/integration/src/tests/list_proposal_reviews.spec.ts b/src/backend/integration/src/tests/list_proposal_reviews.spec.ts index 196d6bd9..9dc443b7 100644 --- a/src/backend/integration/src/tests/list_proposal_reviews.spec.ts +++ b/src/backend/integration/src/tests/list_proposal_reviews.spec.ts @@ -65,16 +65,12 @@ describe('list proposal reviews', () => { [alice, { id: aliceId }] = await driver.users.createReviewer(); [bob, { id: bobId }] = await driver.users.createReviewer(); - proposal1Id = await createProposal( - driver.actor, - governance, - 'Test proposal 1', - ); - proposal2Id = await createProposal( - driver.actor, - governance, - 'Test proposal 2', - ); + proposal1Id = ( + await createProposal(driver.actor, governance, 'Test proposal 1') + ).proposalId; + proposal2Id = ( + await createProposal(driver.actor, governance, 'Test proposal 2') + ).proposalId; const proposal1ReviewAliceData = await createProposalReview( driver.actor, @@ -335,7 +331,7 @@ describe('list proposal reviews', () => { }); it('should return an empty list when there are no reviews', async () => { - const proposalId = await createProposal( + const { proposalId } = await createProposal( driver.actor, governance, 'Test Proposal with no reviews', diff --git a/src/backend/integration/src/tests/proposal_review.spec.ts b/src/backend/integration/src/tests/proposal_review.spec.ts index 705a1bc9..326c69ba 100644 --- a/src/backend/integration/src/tests/proposal_review.spec.ts +++ b/src/backend/integration/src/tests/proposal_review.spec.ts @@ -103,7 +103,7 @@ describe('Proposal Review', () => { const [reviewer, { id: reviewerId }] = await driver.users.createReviewer(); - const proposalId = await createProposal( + const { proposalId } = await createProposal( driver.actor, governance, 'Test proposal', @@ -143,7 +143,7 @@ describe('Proposal Review', () => { const [reviewer, { id: reviewerId }] = await driver.users.createReviewer(); - const proposalId = await createProposal( + const { proposalId } = await createProposal( driver.actor, governance, 'Test proposal', @@ -203,7 +203,7 @@ describe('Proposal Review', () => { it('should not allow to create a review for a proposal that is completed', async () => { const [reviewer] = await driver.users.createReviewer(); - const proposalId = await createProposal( + const { proposalId } = await createProposal( driver.actor, governance, 'Test proposal', @@ -229,7 +229,7 @@ describe('Proposal Review', () => { it('should not allow to create multiple reviews for the same reviewer and proposal', async () => { const [alice, { id: aliceId }] = await driver.users.createReviewer(); - const proposalId = await createProposal( + const { proposalId } = await createProposal( driver.actor, governance, 'Test proposal', @@ -274,7 +274,7 @@ describe('Proposal Review', () => { it('should not allow to create an invalid review', async () => { const [reviewer] = await driver.users.createReviewer(); - const proposalId = await createProposal( + const { proposalId } = await createProposal( driver.actor, governance, 'Test proposal', @@ -424,7 +424,7 @@ describe('Proposal Review', () => { expect(resErr).toEqual({ code: 404, - message: `Proposal review for proposal with Id ${nonExistentProposalId} not found`, + message: `Proposal review for proposal ${nonExistentProposalId} for principal ${reviewer.getPrincipal().toText()} not found`, }); }); @@ -452,7 +452,7 @@ describe('Proposal Review', () => { expect(resErr).toEqual({ code: 404, - message: `Proposal review for proposal with Id ${proposalId} not found`, + message: `Proposal review for proposal ${proposalId} for principal ${bob.getPrincipal().toText()} not found`, }); }); @@ -507,7 +507,7 @@ describe('Proposal Review', () => { it('should not allow a reviewer to update a proposal review for a completed proposal', async () => { const [reviewer] = await driver.users.createReviewer(); - const proposalId = await createProposal( + const { proposalId } = await createProposal( driver.actor, governance, 'Test proposal', @@ -730,7 +730,7 @@ describe('Proposal Review', () => { async (updateReq, errMsg) => { const [reviewer] = await driver.users.createReviewer(); - const proposalId = await createProposal( + const { proposalId } = await createProposal( driver.actor, governance, 'Test proposal', diff --git a/src/backend/integration/src/tests/proposal_review_image.spec.ts b/src/backend/integration/src/tests/proposal_review_image.spec.ts index f2ebc4f6..79a21104 100644 --- a/src/backend/integration/src/tests/proposal_review_image.spec.ts +++ b/src/backend/integration/src/tests/proposal_review_image.spec.ts @@ -115,7 +115,7 @@ describe('Proposal Review Image', () => { expect(resErr).toEqual({ code: 404, - message: `Proposal review for proposal with Id ${nonExistentProposalId} not found`, + message: `Proposal review for proposal ${nonExistentProposalId} for principal ${reviewer.getPrincipal().toText()} not found`, }); }); @@ -140,7 +140,7 @@ describe('Proposal Review Image', () => { expect(resErr).toEqual({ code: 404, - message: `Proposal review for proposal with Id ${proposalId} not found`, + message: `Proposal review for proposal ${proposalId} for principal ${bob.getPrincipal().toText()} not found`, }); });