diff --git a/.eslintrc.js b/.eslintrc.js index 3eb19364..141121d5 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -53,6 +53,9 @@ module.exports = { ecmaVersion: 'latest', project: ['./src/{docs,marketing}/tsconfig.json'], }, + rules: { + '@typescript-eslint/triple-slash-reference': 0, + }, }, { files: ['src/frontend/**/*.ts'], diff --git a/src/backend/api/backend.did b/src/backend/api/backend.did index da93563e..57e1dae2 100644 --- a/src/backend/api/backend.did +++ b/src/backend/api/backend.did @@ -230,7 +230,6 @@ type ProposalReview = record { last_updated_at : opt text; status : ProposalReviewStatus; summary : opt text; - review_duration_mins : opt nat16; build_reproduced : opt bool; images_paths : vec text; proposal_review_commits : vec ProposalReviewCommitWithId; @@ -245,7 +244,6 @@ type ProposalReviewWithId = record { type CreateProposalReviewRequest = record { proposal_id : text; summary : opt text; - review_duration_mins : opt nat16; build_reproduced : opt bool; vote : opt ProposalVote; }; @@ -259,7 +257,6 @@ type UpdateProposalReviewRequest = record { proposal_id : text; status : opt ProposalReviewStatus; summary : opt text; - review_duration_mins : opt nat16; build_reproduced : opt bool; vote : opt ProposalVote; }; @@ -296,7 +293,6 @@ type ReviewCommitState = variant { reviewed : record { matches_description : opt bool; comment : opt text; - highlights : vec text; }; not_reviewed; }; diff --git a/src/backend/api/src/proposal_review.rs b/src/backend/api/src/proposal_review.rs index d14bf93f..452b3da2 100644 --- a/src/backend/api/src/proposal_review.rs +++ b/src/backend/api/src/proposal_review.rs @@ -28,7 +28,6 @@ pub struct ProposalReview { pub last_updated_at: Option, pub status: ProposalReviewStatus, pub summary: Option, - pub review_duration_mins: Option, pub build_reproduced: Option, pub images_paths: Vec, pub proposal_review_commits: Vec, @@ -45,7 +44,6 @@ pub struct ProposalReviewWithId { pub struct CreateProposalReviewRequest { pub proposal_id: String, pub summary: Option, - pub review_duration_mins: Option, pub build_reproduced: Option, pub vote: Option, } @@ -57,7 +55,6 @@ pub struct UpdateProposalReviewRequest { pub proposal_id: String, pub status: Option, pub summary: Option, - pub review_duration_mins: Option, pub build_reproduced: Option, pub vote: Option, } diff --git a/src/backend/api/src/proposal_review_commit.rs b/src/backend/api/src/proposal_review_commit.rs index 3da469f0..c6095628 100644 --- a/src/backend/api/src/proposal_review_commit.rs +++ b/src/backend/api/src/proposal_review_commit.rs @@ -6,7 +6,6 @@ pub enum ReviewCommitState { Reviewed { matches_description: Option, comment: Option, - highlights: Vec, }, #[serde(rename = "not_reviewed")] NotReviewed, diff --git a/src/backend/impl/src/controllers/proposal_review_commit_controller.rs b/src/backend/impl/src/controllers/proposal_review_commit_controller.rs index 764c30a7..66ca8b0d 100644 --- a/src/backend/impl/src/controllers/proposal_review_commit_controller.rs +++ b/src/backend/impl/src/controllers/proposal_review_commit_controller.rs @@ -142,7 +142,6 @@ mod tests { state: backend_api::ReviewCommitState::Reviewed { matches_description: Some(true), comment: Some("comment".to_string()), - highlights: vec![], }, }; let response = CreateProposalReviewCommitResponse { @@ -184,7 +183,6 @@ mod tests { state: backend_api::ReviewCommitState::Reviewed { matches_description: Some(true), comment: Some("comment".to_string()), - highlights: vec![], }, }; let error = ApiError::permission_denied(&format!( @@ -221,7 +219,6 @@ mod tests { state: backend_api::ReviewCommitState::Reviewed { matches_description: Some(true), comment: Some("comment".to_string()), - highlights: vec![], }, }; @@ -255,7 +252,6 @@ mod tests { state: backend_api::ReviewCommitState::Reviewed { matches_description: Some(true), comment: Some("comment".to_string()), - highlights: vec![], }, }; let error = ApiError::permission_denied(&format!( diff --git a/src/backend/impl/src/controllers/proposal_review_controller.rs b/src/backend/impl/src/controllers/proposal_review_controller.rs index 2378eaeb..1158d014 100644 --- a/src/backend/impl/src/controllers/proposal_review_controller.rs +++ b/src/backend/impl/src/controllers/proposal_review_controller.rs @@ -254,7 +254,6 @@ mod tests { let request = CreateProposalReviewRequest { proposal_id: "proposal_id".to_string(), summary: Some("summary".to_string()), - review_duration_mins: Some(10), build_reproduced: Some(true), vote: Some(ProposalVote::Yes), }; @@ -297,7 +296,6 @@ mod tests { let request = CreateProposalReviewRequest { proposal_id: "proposal_id".to_string(), summary: Some("summary".to_string()), - review_duration_mins: Some(10), build_reproduced: Some(true), vote: Some(ProposalVote::No), }; @@ -338,7 +336,6 @@ mod tests { proposal_id: fixtures::proposal_id().to_string(), status: None, summary: Some("summary".to_string()), - review_duration_mins: Some(10), build_reproduced: Some(true), vote: Some(ProposalVote::Yes), }; @@ -374,7 +371,6 @@ mod tests { proposal_id: fixtures::proposal_id().to_string(), status: None, summary: Some("summary".to_string()), - review_duration_mins: Some(10), build_reproduced: Some(true), vote: None, }; diff --git a/src/backend/impl/src/fixtures/id.rs b/src/backend/impl/src/fixtures/id.rs index 4fcc0ecf..0bd53010 100644 --- a/src/backend/impl/src/fixtures/id.rs +++ b/src/backend/impl/src/fixtures/id.rs @@ -14,7 +14,7 @@ pub fn principal_b() -> Principal { #[fixture] pub fn principal_c() -> Principal { - Principal::from_slice(&[1]) + Principal::from_slice(&[2]) } #[fixture] diff --git a/src/backend/impl/src/fixtures/proposal_review.rs b/src/backend/impl/src/fixtures/proposal_review.rs index 2f9b8617..f5a49437 100644 --- a/src/backend/impl/src/fixtures/proposal_review.rs +++ b/src/backend/impl/src/fixtures/proposal_review.rs @@ -13,7 +13,6 @@ pub fn proposal_review_draft() -> ProposalReview { last_updated_at: None, status: ProposalReviewStatus::Draft, summary: Some("Proposal review summary".to_string()), - review_duration_mins: Some(60), build_reproduced: Some(true), images_ids: vec![], vote: ProposalVote::Unspecified, @@ -29,7 +28,6 @@ pub fn proposal_review_published() -> ProposalReview { last_updated_at: None, status: ProposalReviewStatus::Published, summary: Some("Proposal review summary".to_string()), - review_duration_mins: Some(60), build_reproduced: Some(true), images_ids: vec![uuid()], vote: ProposalVote::Yes, diff --git a/src/backend/impl/src/fixtures/proposal_review_commit.rs b/src/backend/impl/src/fixtures/proposal_review_commit.rs index 6a2148e8..661c774e 100644 --- a/src/backend/impl/src/fixtures/proposal_review_commit.rs +++ b/src/backend/impl/src/fixtures/proposal_review_commit.rs @@ -15,7 +15,6 @@ pub fn proposal_review_commit_reviewed() -> ProposalReviewCommit { 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.rs b/src/backend/impl/src/mappings/proposal_review.rs index 2724a5eb..1512a87c 100644 --- a/src/backend/impl/src/mappings/proposal_review.rs +++ b/src/backend/impl/src/mappings/proposal_review.rs @@ -52,7 +52,6 @@ impl From for backend_api::ProposalReview { last_updated_at: proposal_review.last_updated_at.map(|dt| dt.to_string()), status: proposal_review.status.into(), summary: proposal_review.summary, - review_duration_mins: proposal_review.review_duration_mins, build_reproduced: proposal_review.build_reproduced, images_paths: vec![], proposal_review_commits: vec![], diff --git a/src/backend/impl/src/mappings/proposal_review_commit.rs b/src/backend/impl/src/mappings/proposal_review_commit.rs index 8c3960c8..c70c067f 100644 --- a/src/backend/impl/src/mappings/proposal_review_commit.rs +++ b/src/backend/impl/src/mappings/proposal_review_commit.rs @@ -8,11 +8,9 @@ impl From for backend_api::ReviewCommitState { ReviewCommitState::Reviewed(ReviewedCommitState { matches_description, comment, - highlights, }) => backend_api::ReviewCommitState::Reviewed { matches_description, comment, - highlights, }, ReviewCommitState::NotReviewed => backend_api::ReviewCommitState::NotReviewed, } @@ -25,11 +23,9 @@ impl From for ReviewCommitState { backend_api::ReviewCommitState::Reviewed { matches_description, comment, - highlights, } => 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 aa11986b..49812263 100644 --- a/src/backend/impl/src/repositories/proposal_review_commit_repository.rs +++ b/src/backend/impl/src/repositories/proposal_review_commit_repository.rs @@ -510,7 +510,6 @@ mod tests { 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 9f55e7eb..956660a5 100644 --- a/src/backend/impl/src/repositories/types/proposal_review.rs +++ b/src/backend/impl/src/repositories/types/proposal_review.rs @@ -45,7 +45,6 @@ pub struct ProposalReview { pub last_updated_at: Option, pub status: ProposalReviewStatus, pub summary: Option, - pub review_duration_mins: Option, pub build_reproduced: Option, pub images_ids: Vec, pub vote: ProposalVote, 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 02ee95ca..4d5994f7 100644 --- a/src/backend/impl/src/repositories/types/proposal_review_commit.rs +++ b/src/backend/impl/src/repositories/types/proposal_review_commit.rs @@ -15,7 +15,6 @@ pub type ProposalReviewCommitId = Uuid; pub struct ReviewedCommitState { pub matches_description: Option, pub comment: Option, - pub highlights: Vec, } impl Display for ReviewedCommitState { @@ -31,13 +30,6 @@ impl Display for ReviewedCommitState { 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) } } @@ -215,7 +207,6 @@ mod tests { let mut state = ReviewedCommitState { matches_description: None, comment: None, - highlights: vec![], }; assert_eq!(state.to_string(), "Matches description: Unanswered"); @@ -234,17 +225,5 @@ mod tests { 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_commit_service.rs b/src/backend/impl/src/services/proposal_review_commit_service.rs index ffbc3e40..44246b52 100644 --- a/src/backend/impl/src/services/proposal_review_commit_service.rs +++ b/src/backend/impl/src/services/proposal_review_commit_service.rs @@ -17,8 +17,6 @@ use crate::{ const MAX_PROPOSAL_REVIEW_COMMITS_PER_PROPOSAL_REVIEW_PER_USER: usize = 50; const MAX_PROPOSAL_REVIEW_COMMIT_COMMENT_CHARS: usize = 1000; -const MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHTS_COUNT: usize = 5; -const MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHT_CHARS: usize = 100; #[cfg_attr(test, mockall::automock)] pub trait ProposalReviewCommitService { @@ -240,42 +238,19 @@ impl< fn validate_fields(&self, state: &backend_api::ReviewCommitState) -> Result<(), ApiError> { if let backend_api::ReviewCommitState::Reviewed { - comment, - highlights, + comment: Some(comment), .. } = state { - if let Some(comment) = comment { - if comment.is_empty() { - return Err(ApiError::invalid_argument("Comment cannot be empty")); - } - if comment.chars().count() > MAX_PROPOSAL_REVIEW_COMMIT_COMMENT_CHARS { - return Err(ApiError::invalid_argument(&format!( - "Comment must be less than {} characters", - MAX_PROPOSAL_REVIEW_COMMIT_COMMENT_CHARS - ))); - } + if comment.is_empty() { + return Err(ApiError::invalid_argument("Comment cannot be empty")); } - - if highlights.len() > MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHTS_COUNT { + if comment.chars().count() > MAX_PROPOSAL_REVIEW_COMMIT_COMMENT_CHARS { return Err(ApiError::invalid_argument(&format!( - "Number of highlights must be less than {}", - MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHTS_COUNT + "Comment must be less than {} characters", + MAX_PROPOSAL_REVIEW_COMMIT_COMMENT_CHARS ))); } - - for highlight in highlights { - if highlight.is_empty() { - return Err(ApiError::invalid_argument("Highlight cannot be empty")); - } - - if highlight.chars().count() > MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHT_CHARS { - return Err(ApiError::invalid_argument(&format!( - "Each highlight must be less than {} characters", - MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHT_CHARS - ))); - } - } } Ok(()) @@ -747,9 +722,6 @@ mod tests { #[rstest] #[case::comment_empty(proposal_review_commit_create_comment_empty())] #[case::comment_too_long(proposal_review_commit_create_comment_too_long())] - #[case::highlights_too_many(proposal_review_commit_create_highlights_too_many())] - #[case::highlight_item_empty(proposal_review_commit_create_highlights_item_empty())] - #[case::highlight_item_too_long(proposal_review_commit_create_highlights_item_too_long())] async fn create_proposal_review_commit_comment_invalid_fields( #[case] fixture: (CreateProposalReviewCommitRequest, ApiError), ) { @@ -957,7 +929,6 @@ mod tests { state: backend_api::ReviewCommitState::Reviewed { comment: Some("".to_string()), matches_description: None, - highlights: vec![], }, }, ApiError::invalid_argument("Comment cannot be empty"), @@ -976,7 +947,6 @@ mod tests { state: backend_api::ReviewCommitState::Reviewed { comment: Some("a".repeat(MAX_PROPOSAL_REVIEW_COMMIT_COMMENT_CHARS + 1)), matches_description: None, - highlights: vec![], }, }, ApiError::invalid_argument(&format!( @@ -986,74 +956,6 @@ mod tests { ) } - #[fixture] - fn proposal_review_commit_create_highlights_too_many( - ) -> (CreateProposalReviewCommitRequest, ApiError) { - let proposal_review_commit = fixtures::proposal_review_commit_reviewed(); - - ( - CreateProposalReviewCommitRequest { - proposal_review_id: proposal_review_commit.proposal_review_id.to_string(), - commit_sha: proposal_review_commit.commit_sha.to_string(), - state: backend_api::ReviewCommitState::Reviewed { - comment: None, - matches_description: None, - highlights: std::iter::repeat("a".to_string()) - .take(MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHTS_COUNT + 1) - .collect(), - }, - }, - ApiError::invalid_argument(&format!( - "Number of highlights must be less than {}", - MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHTS_COUNT - )), - ) - } - - #[fixture] - fn proposal_review_commit_create_highlights_item_empty( - ) -> (CreateProposalReviewCommitRequest, ApiError) { - let proposal_review_commit = fixtures::proposal_review_commit_reviewed(); - - ( - CreateProposalReviewCommitRequest { - proposal_review_id: proposal_review_commit.proposal_review_id.to_string(), - commit_sha: proposal_review_commit.commit_sha.to_string(), - state: backend_api::ReviewCommitState::Reviewed { - comment: None, - matches_description: None, - highlights: vec!["highlight".to_string(), "".to_string()], - }, - }, - ApiError::invalid_argument("Highlight cannot be empty"), - ) - } - - #[fixture] - fn proposal_review_commit_create_highlights_item_too_long( - ) -> (CreateProposalReviewCommitRequest, ApiError) { - let proposal_review_commit = fixtures::proposal_review_commit_reviewed(); - - ( - CreateProposalReviewCommitRequest { - proposal_review_id: proposal_review_commit.proposal_review_id.to_string(), - commit_sha: proposal_review_commit.commit_sha.to_string(), - state: backend_api::ReviewCommitState::Reviewed { - comment: None, - matches_description: None, - highlights: vec![ - "highlight".to_string(), - "a".repeat(MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHT_CHARS + 1), - ], - }, - }, - ApiError::invalid_argument(&format!( - "Each highlight must be less than {} characters", - MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHT_CHARS - )), - ) - } - #[rstest] #[case::reviewed(proposal_review_commit_update_reviewed())] #[case::not_reviewed(proposal_review_commit_update_not_reviewed())] @@ -1388,9 +1290,6 @@ mod tests { #[rstest] #[case::comment_empty(proposal_review_commit_update_comment_empty())] #[case::comment_too_long(proposal_review_commit_update_comment_too_long())] - #[case::highlights_too_many(proposal_review_commit_update_highlights_too_many())] - #[case::highlight_item_empty(proposal_review_commit_update_highlights_item_empty())] - #[case::highlight_item_too_long(proposal_review_commit_update_highlights_item_too_long())] async fn update_proposal_review_commit_comment_invalid_fields( #[case] fixture: (UpdateProposalReviewCommitRequest, ApiError), ) { @@ -1452,7 +1351,6 @@ mod tests { let state = backend_api::ReviewCommitState::Reviewed { comment: Some("Review commit comment".to_string()), matches_description: Some(true), - highlights: vec![], }; ( @@ -1613,7 +1511,6 @@ mod tests { state: backend_api::ReviewCommitState::Reviewed { comment: Some("".to_string()), matches_description: None, - highlights: vec![], }, }, ApiError::invalid_argument("Comment cannot be empty"), @@ -1631,7 +1528,6 @@ mod tests { state: backend_api::ReviewCommitState::Reviewed { comment: Some("a".repeat(MAX_PROPOSAL_REVIEW_COMMIT_COMMENT_CHARS + 1)), matches_description: None, - highlights: vec![], }, }, ApiError::invalid_argument(&format!( @@ -1641,71 +1537,6 @@ mod tests { ) } - #[fixture] - fn proposal_review_commit_update_highlights_too_many( - ) -> (UpdateProposalReviewCommitRequest, ApiError) { - let id = fixtures::proposal_review_commit_id(); - - ( - UpdateProposalReviewCommitRequest { - id: id.to_string(), - state: backend_api::ReviewCommitState::Reviewed { - comment: None, - matches_description: None, - highlights: std::iter::repeat("a".to_string()) - .take(MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHTS_COUNT + 1) - .collect(), - }, - }, - ApiError::invalid_argument(&format!( - "Number of highlights must be less than {}", - MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHTS_COUNT - )), - ) - } - - #[fixture] - fn proposal_review_commit_update_highlights_item_empty( - ) -> (UpdateProposalReviewCommitRequest, ApiError) { - let id = fixtures::proposal_review_commit_id(); - - ( - UpdateProposalReviewCommitRequest { - id: id.to_string(), - state: backend_api::ReviewCommitState::Reviewed { - comment: None, - matches_description: None, - highlights: vec!["highlight".to_string(), "".to_string()], - }, - }, - ApiError::invalid_argument("Highlight cannot be empty"), - ) - } - - #[fixture] - fn proposal_review_commit_update_highlights_item_too_long( - ) -> (UpdateProposalReviewCommitRequest, ApiError) { - let id = fixtures::proposal_review_commit_id(); - - ( - UpdateProposalReviewCommitRequest { - id: id.to_string(), - state: backend_api::ReviewCommitState::Reviewed { - comment: None, - matches_description: None, - highlights: vec![ - "highlight".to_string(), - "a".repeat(MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHT_CHARS + 1), - ], - }, - }, - ApiError::invalid_argument(&format!( - "Each highlight must be less than {} characters", - MAX_PROPOSAL_REVIEW_COMMIT_HIGHLIGHT_CHARS - )), - ) - } - #[rstest] fn delete_proposal_review_commit() { let calling_principal = fixtures::principal_a(); diff --git a/src/backend/impl/src/services/proposal_review_service.rs b/src/backend/impl/src/services/proposal_review_service.rs index ee7673e7..ffe48d43 100644 --- a/src/backend/impl/src/services/proposal_review_service.rs +++ b/src/backend/impl/src/services/proposal_review_service.rs @@ -22,7 +22,6 @@ use candid::Principal; use std::{path::PathBuf, str::FromStr}; const MAX_PROPOSAL_REVIEW_SUMMARY_CHARS: usize = 1500; -const MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS: u16 = 60 * 3; // 3 hours const PROPOSAL_REVIEW_IMAGES_SUB_PATH: &str = "reviews"; @@ -129,7 +128,7 @@ impl< calling_principal: Principal, request: CreateProposalReviewRequest, ) -> Result { - self.validate_fields(request.summary.as_ref(), request.review_duration_mins)?; + self.validate_fields(request.summary.as_ref())?; let user_id = self .user_profile_repository @@ -181,7 +180,6 @@ impl< created_at: DateTime::new(date_time)?, last_updated_at: None, summary: request.summary, - review_duration_mins: request.review_duration_mins, build_reproduced: request.build_reproduced, images_ids: vec![], vote: request @@ -202,7 +200,7 @@ impl< calling_principal: Principal, request: UpdateProposalReviewRequest, ) -> Result<(), ApiError> { - self.validate_fields(request.summary.as_ref(), request.review_duration_mins)?; + self.validate_fields(request.summary.as_ref())?; let (id, mut current_proposal_review, _) = self.get_current_proposal_review_with_user_id( request.proposal_id, @@ -213,9 +211,6 @@ impl< if request.summary.is_some() { current_proposal_review.summary = request.summary; } - if request.review_duration_mins.is_some() { - current_proposal_review.review_duration_mins = request.review_duration_mins; - } if request.build_reproduced.is_some() { current_proposal_review.build_reproduced = request.build_reproduced; } @@ -229,7 +224,6 @@ impl< // unless the review is set back to draft self.validate_published_fields( current_proposal_review.summary.as_ref(), - current_proposal_review.review_duration_mins, current_proposal_review.build_reproduced, ) .map_err(|e| { @@ -503,11 +497,7 @@ impl< } } - fn validate_fields( - &self, - summary: Option<&String>, - review_duration_mins: Option, - ) -> Result<(), ApiError> { + fn validate_fields(&self, summary: Option<&String>) -> Result<(), ApiError> { if let Some(summary) = summary { if summary.is_empty() { return Err(ApiError::invalid_argument("Summary cannot be empty")); @@ -521,19 +511,6 @@ impl< } } - if let Some(review_duration_mins) = review_duration_mins { - if review_duration_mins == 0 { - return Err(ApiError::invalid_argument("Review duration cannot be 0")); - } - - if review_duration_mins > MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS { - return Err(ApiError::invalid_argument(&format!( - "Review duration must be less than {} minutes", - MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS - ))); - } - } - Ok(()) } @@ -621,26 +598,19 @@ impl< fn validate_published_fields( &self, summary: Option<&String>, - review_duration_mins: Option, build_reproduced: Option, ) -> Result<(), ApiError> { if summary.is_none() { return Err(ApiError::invalid_argument("Summary cannot be empty")); } - if review_duration_mins.is_none() { - return Err(ApiError::invalid_argument( - "Review duration cannot be empty", - )); - } - if build_reproduced.is_none() { return Err(ApiError::invalid_argument( "Build reproduced cannot be empty", )); } - self.validate_fields(summary, review_duration_mins)?; + self.validate_fields(summary)?; Ok(()) } @@ -714,7 +684,6 @@ impl< /// - **[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]**: /// ... /// ... @@ -903,8 +872,6 @@ mod tests { #[rstest] #[case::summary_empty(proposal_review_create_summary_empty())] #[case::summary_too_long(proposal_review_create_summary_too_long())] - #[case::review_duration_zero(proposal_review_create_review_duration_zero())] - #[case::review_duration_too_long(proposal_review_create_review_duration_too_long())] async fn create_proposal_review_invalid( #[case] fixture: (CreateProposalReviewRequest, ApiError), ) { @@ -1160,7 +1127,6 @@ mod tests { CreateProposalReviewRequest { proposal_id: proposal_review.proposal_id.to_string(), summary: proposal_review.summary.clone(), - review_duration_mins: proposal_review.review_duration_mins, build_reproduced: proposal_review.build_reproduced, vote: Some(proposal_review.vote.into()), }, @@ -1173,7 +1139,6 @@ mod tests { let proposal_review = ProposalReview { created_at: DateTime::new(date_time).unwrap(), summary: None, - review_duration_mins: None, build_reproduced: None, ..fixtures::proposal_review_draft() }; @@ -1183,7 +1148,6 @@ mod tests { CreateProposalReviewRequest { proposal_id: proposal_review.proposal_id.to_string(), summary: proposal_review.summary, - review_duration_mins: proposal_review.review_duration_mins, build_reproduced: proposal_review.build_reproduced, vote: Some(proposal_review.vote.into()), }, @@ -1198,7 +1162,6 @@ mod tests { CreateProposalReviewRequest { proposal_id: proposal_review.proposal_id.to_string(), summary: Some("".to_string()), - review_duration_mins: proposal_review.review_duration_mins, build_reproduced: proposal_review.build_reproduced, vote: Some(proposal_review.vote.into()), }, @@ -1214,7 +1177,6 @@ mod tests { CreateProposalReviewRequest { proposal_id: proposal_review.proposal_id.to_string(), summary: Some("a".repeat(MAX_PROPOSAL_REVIEW_SUMMARY_CHARS + 1)), - review_duration_mins: proposal_review.review_duration_mins, build_reproduced: proposal_review.build_reproduced, vote: Some(proposal_review.vote.into()), }, @@ -1225,42 +1187,6 @@ mod tests { ) } - #[fixture] - fn proposal_review_create_review_duration_zero() -> (CreateProposalReviewRequest, ApiError) { - let proposal_review = fixtures::proposal_review_draft(); - - ( - CreateProposalReviewRequest { - proposal_id: proposal_review.proposal_id.to_string(), - summary: proposal_review.summary, - review_duration_mins: Some(0), - build_reproduced: proposal_review.build_reproduced, - vote: Some(proposal_review.vote.into()), - }, - ApiError::invalid_argument("Review duration cannot be 0"), - ) - } - - #[fixture] - fn proposal_review_create_review_duration_too_long() -> (CreateProposalReviewRequest, ApiError) - { - let proposal_review = fixtures::proposal_review_draft(); - - ( - CreateProposalReviewRequest { - proposal_id: proposal_review.proposal_id.to_string(), - summary: proposal_review.summary, - review_duration_mins: Some(MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS + 1), - build_reproduced: proposal_review.build_reproduced, - vote: Some(proposal_review.vote.into()), - }, - ApiError::invalid_argument(&format!( - "Review duration must be less than {} minutes", - MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS - )), - ) - } - #[rstest] #[case::update(proposal_review_update())] #[case::publish(proposal_review_update_publish())] @@ -1581,8 +1507,6 @@ mod tests { #[rstest] #[case::summary_empty(proposal_review_update_summary_empty())] #[case::summary_too_long(proposal_review_update_summary_too_long())] - #[case::review_duration_zero(proposal_review_update_duration_zero())] - #[case::review_duration_too_long(proposal_review_update_duration_too_long())] fn proposal_review_update_invalid(#[case] fixture: (UpdateProposalReviewRequest, ApiError)) { let calling_principal = fixtures::principal_a(); let (request, expected_error) = fixture; @@ -1619,9 +1543,6 @@ mod tests { #[rstest] #[case::summary_empty(proposal_review_update_publish_summary_empty())] #[case::summary_too_long(proposal_review_update_publish_summary_too_long())] - #[case::review_duration_zero(proposal_review_update_publish_duration_zero())] - #[case::no_summary(proposal_review_update_publish_duration_too_long())] - #[case::no_duration(proposal_review_update_publish_no_duration())] #[case::no_build_reproduced(proposal_review_update_publish_no_build_reproduced())] fn proposal_review_update_publish_invalid( #[case] fixture: ( @@ -1690,7 +1611,6 @@ mod tests { ..fixtures::proposal_review_draft() }; let summary = "Updated summary".to_string(); - let review_duration_mins = 120; let build_reproduced = false; ( @@ -1700,13 +1620,11 @@ mod tests { proposal_id: original_proposal_review.proposal_id.to_string(), status: None, summary: Some(summary.clone()), - review_duration_mins: Some(review_duration_mins), build_reproduced: Some(build_reproduced), vote: Some(backend_api::ProposalVote::Yes), }, ProposalReview { summary: Some(summary), - review_duration_mins: Some(review_duration_mins), build_reproduced: Some(build_reproduced), last_updated_at: Some(DateTime::new(date_time).unwrap()), vote: ProposalVote::Yes, @@ -1730,7 +1648,6 @@ mod tests { }; let status = ProposalReviewStatus::Draft; let summary = "Updated summary".to_string(); - let review_duration_mins = 120; let build_reproduced = false; let vote = ProposalVote::No; @@ -1741,14 +1658,12 @@ mod tests { proposal_id: original_proposal_review.proposal_id.to_string(), status: Some(status.clone().into()), summary: Some(summary.clone()), - review_duration_mins: Some(review_duration_mins), build_reproduced: Some(build_reproduced), vote: Some(vote.clone().into()), }, ProposalReview { status, summary: Some(summary), - review_duration_mins: Some(review_duration_mins), build_reproduced: Some(build_reproduced), last_updated_at: Some(DateTime::new(date_time).unwrap()), vote, @@ -1772,7 +1687,6 @@ mod tests { }; let status = ProposalReviewStatus::Published; let summary = "Updated summary".to_string(); - let review_duration_mins = 120; let build_reproduced = false; ( @@ -1782,14 +1696,12 @@ mod tests { proposal_id: original_proposal_review.proposal_id.to_string(), status: Some(status.clone().into()), summary: Some(summary.clone()), - review_duration_mins: Some(review_duration_mins), build_reproduced: Some(build_reproduced), vote: None, }, ProposalReview { status, summary: Some(summary), - review_duration_mins: Some(review_duration_mins), build_reproduced: Some(build_reproduced), last_updated_at: Some(DateTime::new(date_time).unwrap()), ..original_proposal_review @@ -1804,7 +1716,6 @@ mod tests { proposal_id: fixtures::proposal_id().to_string(), status: None, summary: Some("".to_string()), - review_duration_mins: None, build_reproduced: None, vote: None, }, @@ -1819,7 +1730,6 @@ mod tests { proposal_id: fixtures::proposal_id().to_string(), status: None, summary: Some("a".repeat(MAX_PROPOSAL_REVIEW_SUMMARY_CHARS + 1)), - review_duration_mins: None, build_reproduced: None, vote: None, }, @@ -1830,39 +1740,6 @@ mod tests { ) } - #[fixture] - fn proposal_review_update_duration_zero() -> (UpdateProposalReviewRequest, ApiError) { - ( - UpdateProposalReviewRequest { - proposal_id: fixtures::proposal_id().to_string(), - status: None, - summary: None, - review_duration_mins: Some(0), - build_reproduced: None, - vote: None, - }, - ApiError::invalid_argument("Review duration cannot be 0"), - ) - } - - #[fixture] - fn proposal_review_update_duration_too_long() -> (UpdateProposalReviewRequest, ApiError) { - ( - UpdateProposalReviewRequest { - proposal_id: fixtures::proposal_id().to_string(), - status: None, - summary: None, - review_duration_mins: Some(MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS + 1), - build_reproduced: None, - vote: None, - }, - ApiError::invalid_argument(&format!( - "Review duration must be less than {} minutes", - MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS - )), - ) - } - #[fixture] fn proposal_review_update_publish_summary_empty() -> ( ProposalReviewId, @@ -1884,7 +1761,6 @@ mod tests { proposal_id: original_proposal_review.proposal_id.to_string(), status: Some(backend_api::ProposalReviewStatus::Published), summary: None, - review_duration_mins: None, build_reproduced: None, vote: None, }, @@ -1919,74 +1795,6 @@ mod tests { proposal_id: original_proposal_review.proposal_id.to_string(), status: Some(backend_api::ProposalReviewStatus::Published), summary: None, - review_duration_mins: None, - build_reproduced: None, - vote: None, - }, - ApiError::conflict(&format!( - "Proposal review cannot be published due to invalid field: {}", - error_message - )), - ) - } - - #[fixture] - fn proposal_review_update_publish_duration_zero() -> ( - ProposalReviewId, - ProposalReview, - UpdateProposalReviewRequest, - ApiError, - ) { - let id = fixtures::proposal_review_id(); - let original_proposal_review = ProposalReview { - user_id: fixtures::uuid_a(), - review_duration_mins: Some(0), - ..fixtures::proposal_review_draft() - }; - - ( - id, - original_proposal_review.clone(), - UpdateProposalReviewRequest { - proposal_id: original_proposal_review.proposal_id.to_string(), - status: Some(backend_api::ProposalReviewStatus::Published), - summary: None, - review_duration_mins: None, - build_reproduced: None, - vote: None, - }, - ApiError::conflict( - "Proposal review cannot be published due to invalid field: Review duration cannot be 0", - ), - ) - } - - #[fixture] - fn proposal_review_update_publish_duration_too_long() -> ( - ProposalReviewId, - ProposalReview, - UpdateProposalReviewRequest, - ApiError, - ) { - let id = fixtures::proposal_review_id(); - let original_proposal_review = ProposalReview { - user_id: fixtures::uuid_a(), - review_duration_mins: Some(MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS + 1), - ..fixtures::proposal_review_draft() - }; - let error_message = format!( - "Review duration must be less than {} minutes", - MAX_PROPOSAL_REVIEW_REVIEW_DURATION_MINS - ); - - ( - id, - original_proposal_review.clone(), - UpdateProposalReviewRequest { - proposal_id: original_proposal_review.proposal_id.to_string(), - status: Some(backend_api::ProposalReviewStatus::Published), - summary: None, - review_duration_mins: None, build_reproduced: None, vote: None, }, @@ -2417,7 +2225,6 @@ mod tests { proposal_id: original_proposal_review.proposal_id.to_string(), status: Some(backend_api::ProposalReviewStatus::Published), summary: None, - review_duration_mins: None, build_reproduced: None, vote: None, }, @@ -2427,37 +2234,6 @@ mod tests { ) } - #[fixture] - fn proposal_review_update_publish_no_duration() -> ( - ProposalReviewId, - ProposalReview, - UpdateProposalReviewRequest, - ApiError, - ) { - let id = fixtures::proposal_review_id(); - let original_proposal_review = ProposalReview { - user_id: fixtures::uuid_a(), - review_duration_mins: None, - ..fixtures::proposal_review_draft() - }; - - ( - id, - original_proposal_review.clone(), - UpdateProposalReviewRequest { - proposal_id: original_proposal_review.proposal_id.to_string(), - status: Some(backend_api::ProposalReviewStatus::Published), - summary: None, - review_duration_mins: None, - build_reproduced: None, - vote: None, - }, - ApiError::conflict( - "Proposal review cannot be published due to invalid field: Review duration cannot be empty", - ), - ) - } - #[fixture] fn proposal_review_update_publish_no_build_reproduced() -> ( ProposalReviewId, @@ -2479,7 +2255,6 @@ mod tests { proposal_id: original_proposal_review.proposal_id.to_string(), status: Some(backend_api::ProposalReviewStatus::Published), summary: None, - review_duration_mins: None, build_reproduced: None, vote: None, }, @@ -2523,7 +2298,6 @@ mod tests { 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() }, @@ -2535,7 +2309,6 @@ mod tests { 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() }, @@ -2561,7 +2334,6 @@ mod tests { 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() }, @@ -2573,7 +2345,6 @@ mod tests { state: ReviewCommitState::Reviewed(ReviewedCommitState { matches_description: None, comment: Some("Issues found".to_string()), - highlights: vec!["Missing tests".to_string()], }), ..fixtures::proposal_review_commit_reviewed() }, @@ -2599,7 +2370,6 @@ mod tests { 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() }, @@ -2611,7 +2381,6 @@ mod tests { state: ReviewCommitState::Reviewed(ReviewedCommitState { matches_description: None, comment: Some("Issues found".to_string()), - highlights: vec!["Missing tests".to_string()], }), ..fixtures::proposal_review_commit_reviewed() }, @@ -2671,11 +2440,9 @@ 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 ) diff --git a/src/backend/integration/package.json b/src/backend/integration/package.json index 5fd03656..2fdeffe5 100644 --- a/src/backend/integration/package.json +++ b/src/backend/integration/package.json @@ -2,7 +2,7 @@ "name": "backend-integration", "private": true, "scripts": { - "test": "tsc && bun test --timeout 60000" + "test": "tsc && bun test --timeout 120000" }, "devDependencies": { "@cg/backend": "workspace:*", diff --git a/src/backend/integration/src/support/common.ts b/src/backend/integration/src/support/common.ts index 01bd1693..e4c02926 100644 --- a/src/backend/integration/src/support/common.ts +++ b/src/backend/integration/src/support/common.ts @@ -129,7 +129,6 @@ export async function createProposalReview( const res = await actor.create_proposal_review({ proposal_id: proposalId, summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -196,7 +195,6 @@ export async function publishProposalReview( proposal_id: proposalId, status: [{ published: null }], summary: [], - review_duration_mins: [], build_reproduced: [], vote: [], }); @@ -247,7 +245,6 @@ export async function createProposalReviewCommit( reviewed: { matches_description: [true], comment: ['comment'], - highlights: ['first highlight', 'second highlight'], }, }, }); @@ -284,7 +281,6 @@ export function validateProposalReview( created_at: expect.any(String), last_updated_at: expected.lastUpdatedAt ? [expected.lastUpdatedAt] : [], summary: expect.any(Array), - review_duration_mins: expect.any(Array), build_reproduced: expect.any(Array), images_paths: expect.any(Array), vote: expected.vote, 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 index 10f02e24..a4815cba 100644 --- 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 @@ -149,9 +149,6 @@ describe('get my proposal review summary', () => { 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`; } } diff --git a/src/backend/integration/src/tests/proposal_review.spec.ts b/src/backend/integration/src/tests/proposal_review.spec.ts index 326c69ba..d7d75856 100644 --- a/src/backend/integration/src/tests/proposal_review.spec.ts +++ b/src/backend/integration/src/tests/proposal_review.spec.ts @@ -44,7 +44,6 @@ describe('Proposal Review', () => { const res = await driver.actor.create_proposal_review({ proposal_id: 'proposal-id', summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ unspecified: null }], }); @@ -66,7 +65,6 @@ describe('Proposal Review', () => { const resAnonymous = await driver.actor.create_proposal_review({ proposal_id: 'proposal-id', summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ unspecified: null }], }); @@ -85,7 +83,6 @@ describe('Proposal Review', () => { const resAdmin = await driver.actor.create_proposal_review({ proposal_id: 'proposal-id', summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ unspecified: null }], }); @@ -115,7 +112,6 @@ describe('Proposal Review', () => { const resFull = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -130,7 +126,6 @@ describe('Proposal Review', () => { created_at: dateToRfc3339(proposalReviewCreationDate), last_updated_at: [], summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], images_paths: [], proposal_review_commits: [], @@ -155,7 +150,6 @@ describe('Proposal Review', () => { const resEmpty = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: [], - review_duration_mins: [], build_reproduced: [], vote: [], }); @@ -170,7 +164,6 @@ describe('Proposal Review', () => { created_at: dateToRfc3339(proposalReviewCreationDate), last_updated_at: [], summary: [], - review_duration_mins: [], build_reproduced: [], images_paths: [], proposal_review_commits: [], @@ -188,7 +181,6 @@ describe('Proposal Review', () => { const res = await driver.actor.create_proposal_review({ proposal_id: nonExistentProposalId, summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ unspecified: null }], }); @@ -214,7 +206,6 @@ describe('Proposal Review', () => { const res = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -240,7 +231,6 @@ describe('Proposal Review', () => { const resAliceCreated = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -249,7 +239,6 @@ describe('Proposal Review', () => { const res = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -264,7 +253,6 @@ describe('Proposal Review', () => { const resBobCreated = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -285,7 +273,6 @@ describe('Proposal Review', () => { const resEmptySummary = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: [''], - review_duration_mins: [60], build_reproduced: [true], vote: [{ unspecified: null }], }); @@ -299,7 +286,6 @@ describe('Proposal Review', () => { const resLongSummary = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: ['a'.repeat(1501)], - review_duration_mins: [60], build_reproduced: [true], vote: [{ unspecified: null }], }); @@ -309,34 +295,6 @@ describe('Proposal Review', () => { code: 400, message: 'Summary must be less than 1500 characters', }); - - const resZeroDuration = await driver.actor.create_proposal_review({ - proposal_id: proposalId, - summary: ['summary'], - review_duration_mins: [0], - build_reproduced: [true], - vote: [{ unspecified: null }], - }); - const resZeroDurationErr = extractErrResponse(resZeroDuration); - - expect(resZeroDurationErr).toEqual({ - code: 400, - message: 'Review duration cannot be 0', - }); - - const resLongDuration = await driver.actor.create_proposal_review({ - proposal_id: proposalId, - summary: ['summary'], - review_duration_mins: [3 * 60 + 1], - build_reproduced: [true], - vote: [{ unspecified: null }], - }); - const resLongDurationErr = extractErrResponse(resLongDuration); - - expect(resLongDurationErr).toEqual({ - code: 400, - message: 'Review duration must be less than 180 minutes', - }); }); }); @@ -348,7 +306,6 @@ describe('Proposal Review', () => { proposal_id: 'proposal-id', status: [{ draft: null }], summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ unspecified: null }], }); @@ -371,7 +328,6 @@ describe('Proposal Review', () => { proposal_id: 'proposal-id', status: [{ draft: null }], summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -391,7 +347,6 @@ describe('Proposal Review', () => { proposal_id: 'proposal-id', status: [{ draft: null }], summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ no: null }], }); @@ -416,7 +371,6 @@ describe('Proposal Review', () => { proposal_id: nonExistentProposalId, status: [{ draft: null }], summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -444,7 +398,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [{ draft: null }], summary: ['summary'], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -471,7 +424,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [{ draft: null }], summary: ['updated summary'], - review_duration_mins: [120], build_reproduced: [false], vote: [{ no: null }], }); @@ -495,7 +447,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [{ published: null }], summary: [], - review_duration_mins: [], build_reproduced: [], vote: [], }); @@ -517,7 +468,6 @@ describe('Proposal Review', () => { const resProposalReview = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: [], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -529,7 +479,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [{ draft: null }], summary: ['updated summary'], - review_duration_mins: [1], build_reproduced: [false], vote: [{ yes: null }], }); @@ -557,7 +506,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [{ published: null }], summary: [], - review_duration_mins: [], build_reproduced: [], vote: [], }); @@ -567,7 +515,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [], summary: ['updated summary'], - review_duration_mins: [1], build_reproduced: [false], vote: [{ yes: null }], }); @@ -582,7 +529,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [{ published: null }], summary: ['updated summary'], - review_duration_mins: [1], build_reproduced: [false], vote: [{ no: null }], }); @@ -609,7 +555,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [{ published: null }], summary: [], - review_duration_mins: [], build_reproduced: [], vote: [], }); @@ -619,7 +564,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [{ draft: null }], summary: ['updated summary'], - review_duration_mins: [1], build_reproduced: [false], vote: [{ yes: null }], }); @@ -643,7 +587,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [], summary: [''], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -658,7 +601,6 @@ describe('Proposal Review', () => { proposal_id: proposalId, status: [], summary: ['a'.repeat(1501)], - review_duration_mins: [60], build_reproduced: [true], vote: [{ yes: null }], }); @@ -668,43 +610,12 @@ describe('Proposal Review', () => { code: 400, message: 'Summary must be less than 1500 characters', }); - - const resZeroDuration = await driver.actor.update_proposal_review({ - proposal_id: proposalId, - status: [], - summary: ['summary'], - review_duration_mins: [0], - build_reproduced: [true], - vote: [{ yes: null }], - }); - const resZeroDurationErr = extractErrResponse(resZeroDuration); - - expect(resZeroDurationErr).toEqual({ - code: 400, - message: 'Review duration cannot be 0', - }); - - const resLongDuration = await driver.actor.update_proposal_review({ - proposal_id: proposalId, - status: [], - summary: ['summary'], - review_duration_mins: [3 * 60 + 1], - build_reproduced: [true], - vote: [{ yes: null }], - }); - const resLongDurationErr = extractErrResponse(resLongDuration); - - expect(resLongDurationErr).toEqual({ - code: 400, - message: 'Review duration must be less than 180 minutes', - }); }); it.each<[Partial, string]>([ [ { summary: [], - review_duration_mins: [60], build_reproduced: [false], }, 'Summary cannot be empty', @@ -712,19 +623,10 @@ describe('Proposal Review', () => { [ { summary: ['Summary'], - review_duration_mins: [60], build_reproduced: [], }, 'Build reproduced cannot be empty', ], - [ - { - summary: ['Summary'], - review_duration_mins: [], - build_reproduced: [false], - }, - 'Review duration cannot be empty', - ], ])( 'should not allow to publish a review that has invalid fields', async (updateReq, errMsg) => { @@ -741,7 +643,6 @@ describe('Proposal Review', () => { const resProposalReview = await driver.actor.create_proposal_review({ proposal_id: proposalId, summary: [], - review_duration_mins: [], build_reproduced: [], vote: [], }); diff --git a/src/backend/integration/src/tests/proposal_review_commit.spec.ts b/src/backend/integration/src/tests/proposal_review_commit.spec.ts index be798815..7de50157 100644 --- a/src/backend/integration/src/tests/proposal_review_commit.spec.ts +++ b/src/backend/integration/src/tests/proposal_review_commit.spec.ts @@ -52,7 +52,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: [], - highlights: [], }, }, }); @@ -78,7 +77,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: ['comment'], - highlights: [], }, }, }); @@ -101,7 +99,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], }, }, }); @@ -135,7 +132,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], }, }, }); @@ -153,7 +149,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], }, }, }, @@ -200,7 +195,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: [], - highlights: [], }, }, }); @@ -230,7 +224,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], }, }, }); @@ -260,7 +253,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], }, }, }); @@ -290,7 +282,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], }, }, }); @@ -303,7 +294,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], }, }, }); @@ -334,7 +324,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment 1'], - highlights: [], }, }, }); @@ -351,7 +340,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment 1'], - highlights: [], }, }, }, @@ -365,7 +353,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment 2'], - highlights: ['highlight a', 'highlight b'], }, }, }); @@ -382,7 +369,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment 2'], - highlights: ['highlight a', 'highlight b'], }, }, }, @@ -405,7 +391,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment alice'], - highlights: [], }, }, }); @@ -422,7 +407,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment alice'], - highlights: [], }, }, }, @@ -440,7 +424,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment bob'], - highlights: ['highlight bob a', 'highlight bob b'], }, }, }); @@ -457,7 +440,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment bob'], - highlights: ['highlight bob a', 'highlight bob b'], }, }, }, @@ -486,7 +468,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment alice'], - highlights: [], }, }, }); @@ -500,7 +481,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment alice'], - highlights: [], }, }, }); @@ -531,7 +511,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment alice'], - highlights: [], }, }, }); @@ -545,7 +524,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment alice'], - highlights: [], }, }, }); @@ -579,7 +557,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment alice'], - highlights: [], }, }, }); @@ -593,7 +570,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment alice'], - highlights: [], }, }, }); @@ -622,7 +598,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment bob'], - highlights: [], }, }, }); @@ -636,7 +611,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment bob'], - highlights: [], }, }, }); @@ -667,7 +641,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: [], - highlights: [], }, }, }); @@ -687,7 +660,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: [], - highlights: [], }, }, }); @@ -708,7 +680,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: [''], - highlights: [], }, }, }); @@ -725,7 +696,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: ['a'.repeat(1001)], - highlights: [], }, }, }); @@ -734,61 +704,6 @@ describe('Proposal Review Commit', () => { code: 400, message: 'Comment must be less than 1000 characters', }); - - const resTooManyHighlights = - await driver.actor.create_proposal_review_commit({ - proposal_review_id: proposalReviewId, - commit_sha: VALID_COMMIT_SHA_A, - state: { - reviewed: { - matches_description: [], - comment: ['comment'], - highlights: Array(6).fill('highlight'), - }, - }, - }); - const resTooManyHighlightsErr = extractErrResponse(resTooManyHighlights); - expect(resTooManyHighlightsErr).toEqual({ - code: 400, - message: 'Number of highlights must be less than 5', - }); - - const resEmptyHighlight = - await driver.actor.create_proposal_review_commit({ - proposal_review_id: proposalReviewId, - commit_sha: VALID_COMMIT_SHA_A, - state: { - reviewed: { - matches_description: [true], - comment: ['comment'], - highlights: ['valid highlight', ''], - }, - }, - }); - const resEmptyHighlightErr = extractErrResponse(resEmptyHighlight); - expect(resEmptyHighlightErr).toEqual({ - code: 400, - message: 'Highlight cannot be empty', - }); - - const resLongHighlight = await driver.actor.create_proposal_review_commit( - { - proposal_review_id: proposalReviewId, - commit_sha: VALID_COMMIT_SHA_A, - state: { - reviewed: { - matches_description: [true], - comment: ['comment'], - highlights: ['a'.repeat(101), 'valid highlight'], - }, - }, - }, - ); - const resLongHighlightErr = extractErrResponse(resLongHighlight); - expect(resLongHighlightErr).toEqual({ - code: 400, - message: 'Each highlight must be less than 100 characters', - }); }); }); @@ -802,7 +717,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: [], - highlights: [], }, }, }); @@ -827,7 +741,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: ['comment'], - highlights: [], }, }, }); @@ -849,7 +762,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [true], comment: ['comment'], - highlights: [], }, }, }); @@ -880,7 +792,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [false], comment: ['comment'], - highlights: ['highlight a', 'highlight b'], }, }, }); @@ -928,7 +839,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: ['comment'], - highlights: ['highlight a', 'highlight b'], }, }, }); @@ -959,7 +869,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: ['comment'], - highlights: ['highlight a', 'highlight b'], }, }, }); @@ -990,7 +899,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [false], comment: ['comment'], - highlights: ['highlight a', 'highlight b'], }, }, }); @@ -1020,7 +928,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: [''], - highlights: [], }, }, }); @@ -1036,7 +943,6 @@ describe('Proposal Review Commit', () => { reviewed: { matches_description: [], comment: ['a'.repeat(1001)], - highlights: [], }, }, }); @@ -1045,58 +951,6 @@ describe('Proposal Review Commit', () => { code: 400, message: 'Comment must be less than 1000 characters', }); - - const resTooManyHighlights = - await driver.actor.update_proposal_review_commit({ - id: proposalReviewCommitId, - state: { - reviewed: { - matches_description: [], - comment: ['comment'], - highlights: Array(6).fill('highlight'), - }, - }, - }); - const resTooManyHighlightsErr = extractErrResponse(resTooManyHighlights); - expect(resTooManyHighlightsErr).toEqual({ - code: 400, - message: 'Number of highlights must be less than 5', - }); - - const resEmptyHighlight = - await driver.actor.update_proposal_review_commit({ - id: proposalReviewCommitId, - state: { - reviewed: { - matches_description: [], - comment: ['comment'], - highlights: ['valid highlight', ''], - }, - }, - }); - const resEmptyHighlightErr = extractErrResponse(resEmptyHighlight); - expect(resEmptyHighlightErr).toEqual({ - code: 400, - message: 'Highlight cannot be empty', - }); - - const resLongHighlight = await driver.actor.update_proposal_review_commit( - { - id: proposalReviewCommitId, - state: { - reviewed: { - matches_description: [], - comment: ['comment'], - highlights: ['a'.repeat(101), 'valid highlight'], - }, - }, - }, - ); - const resLongHighlightErr = extractErrResponse(resLongHighlight); - expect(resLongHighlightErr).toEqual({ - code: 400, - message: 'Each highlight must be less than 100 characters', - }); }); }); diff --git a/src/frontend/src/app/core/api/commit-review/commit-review-api.mapper.ts b/src/frontend/src/app/core/api/commit-review/commit-review-api.mapper.ts index 55b777ae..e262b418 100644 --- a/src/frontend/src/app/core/api/commit-review/commit-review-api.mapper.ts +++ b/src/frontend/src/app/core/api/commit-review/commit-review-api.mapper.ts @@ -28,7 +28,6 @@ export function mapCreateProposalReviewCommitRequest( state: req.reviewed ? { reviewed: { - highlights: [], comment: [], matches_description: [], }, @@ -76,7 +75,6 @@ function mapReviewCommitRequestDetails( if (req.reviewed) { return { reviewed: { - highlights: req.highlights, comment: toCandidOpt(req.comment), matches_description: toCandidOpt(req.matchesDescription), }, @@ -94,7 +92,6 @@ function mapReviewCommitResponseDetails( reviewed: true, comment: fromCandidOpt(state.reviewed.comment), matchesDescription: fromCandidOpt(state.reviewed.matches_description), - highlights: state.reviewed.highlights, }; } diff --git a/src/frontend/src/app/core/api/commit-review/commit-review-api.model.ts b/src/frontend/src/app/core/api/commit-review/commit-review-api.model.ts index 189ef0bd..1118158f 100644 --- a/src/frontend/src/app/core/api/commit-review/commit-review-api.model.ts +++ b/src/frontend/src/app/core/api/commit-review/commit-review-api.model.ts @@ -30,7 +30,6 @@ export type ReviewCommitDetails = export interface ReviewedCommitDetails { reviewed: true; comment: string | null; - highlights: string[]; matchesDescription: boolean | null; } diff --git a/src/frontend/src/app/core/api/commit-review/commit-review-api.service.spec.ts b/src/frontend/src/app/core/api/commit-review/commit-review-api.service.spec.ts index 218e145f..2d270a1a 100644 --- a/src/frontend/src/app/core/api/commit-review/commit-review-api.service.spec.ts +++ b/src/frontend/src/app/core/api/commit-review/commit-review-api.service.spec.ts @@ -54,7 +54,6 @@ describe('CommitReviewApiService', () => { state: { reviewed: { comment: [], - highlights: [], matches_description: [], }, }, @@ -71,7 +70,6 @@ describe('CommitReviewApiService', () => { state: { reviewed: { comment: [], - highlights: [], matches_description: [], }, }, @@ -88,7 +86,6 @@ describe('CommitReviewApiService', () => { details: { reviewed: true, comment: null, - highlights: [], matchesDescription: null, }, }; @@ -175,7 +172,6 @@ describe('CommitReviewApiService', () => { details: { reviewed: true, comment: null, - highlights: [], matchesDescription: null, }, }; @@ -184,7 +180,6 @@ describe('CommitReviewApiService', () => { state: { reviewed: { comment: [], - highlights: [], matches_description: [], }, }, diff --git a/src/frontend/src/app/core/api/proposal/proposal-api.mapper.ts b/src/frontend/src/app/core/api/proposal/proposal-api.mapper.ts index 8e122be6..d396d05b 100644 --- a/src/frontend/src/app/core/api/proposal/proposal-api.mapper.ts +++ b/src/frontend/src/app/core/api/proposal/proposal-api.mapper.ts @@ -70,7 +70,6 @@ export function mapGetProposalResponse( ProposalLinkBaseUrl.ICLight + res.proposal.nervous_system.network.id, }, ], - codeGovVote: 'ADOPT', }; } diff --git a/src/frontend/src/app/core/api/proposal/proposal-api.model.ts b/src/frontend/src/app/core/api/proposal/proposal-api.model.ts index 4ad4e9f5..f22904ca 100644 --- a/src/frontend/src/app/core/api/proposal/proposal-api.model.ts +++ b/src/frontend/src/app/core/api/proposal/proposal-api.model.ts @@ -23,7 +23,6 @@ export interface GetProposalResponse { decidedAt: Date | null; summary: string; proposalLinks: ProposalVotingLink[]; - codeGovVote: ProposalCodeGovVote | null; } export enum ProposalTopic { diff --git a/src/frontend/src/app/core/api/review/review-api.mapper.ts b/src/frontend/src/app/core/api/review/review-api.mapper.ts index d82857e0..dd7ddbd2 100644 --- a/src/frontend/src/app/core/api/review/review-api.mapper.ts +++ b/src/frontend/src/app/core/api/review/review-api.mapper.ts @@ -31,7 +31,6 @@ export function mapCreateProposalReviewRequest( ): CreateProposalReviewApiRequest { return { proposal_id: req.proposalId, - review_duration_mins: toCandidOpt(req.reviewDurationMins), summary: toCandidOpt(req.summary), build_reproduced: toCandidOpt(req.buildReproduced), vote: toCandidOpt(mapProposalVoteRequest(req.vote)), @@ -44,7 +43,6 @@ export function mapUpdateProposalReviewRequest( return { proposal_id: req.proposalId, status: toCandidOpt(mapProposalReviewStatusRequest(req.status)), - review_duration_mins: toCandidOpt(req.reviewDurationMins), summary: toCandidOpt(req.summary), build_reproduced: toCandidOpt(req.buildReproduced), vote: toCandidOpt(mapProposalVoteRequest(req.vote)), @@ -90,7 +88,6 @@ export function mapGetProposalReviewResponse( lastUpdatedAt: fromCandidOptDate(review.last_updated_at), status: mapProposalReviewStatusResponse(review.status), summary: fromCandidOpt(review.summary), - reviewDurationMins: fromCandidOpt(review.review_duration_mins), buildReproduced: fromCandidOpt(review.build_reproduced), // [TODO] - connect with API once it's implemented reproducedBuildImageId: getReviewImages(), diff --git a/src/frontend/src/app/core/api/review/review-api.model.ts b/src/frontend/src/app/core/api/review/review-api.model.ts index c41d835c..516ed9ca 100644 --- a/src/frontend/src/app/core/api/review/review-api.model.ts +++ b/src/frontend/src/app/core/api/review/review-api.model.ts @@ -4,7 +4,6 @@ import { ImageSet } from '@cg/angular-ui'; export interface CreateProposalReviewRequest { proposalId: string; summary?: string | null; - reviewDurationMins?: number | null; buildReproduced?: boolean | null; vote?: boolean | null; } @@ -12,7 +11,6 @@ export interface CreateProposalReviewRequest { export interface UpdateProposalReviewRequest { proposalId: string; status?: ProposalReviewStatus | null; - reviewDurationMins?: number | null; summary?: string | null; buildReproduced?: boolean | null; vote?: boolean | null; @@ -40,7 +38,6 @@ export interface GetProposalReviewResponse { lastUpdatedAt: Date | null; status: ProposalReviewStatus; summary: string | null; - reviewDurationMins: number | null; buildReproduced: boolean | null; reproducedBuildImageId: ImageSet[]; commits: GetProposalReviewCommitResponse[]; @@ -56,18 +53,3 @@ export enum ProposalReviewVote { Reject = 'Reject', NoVote = 'NoVote', } - -export interface ProposalCommitReviewHighlight { - reviewerId: string; - text: string; -} - -export interface ProposalCommitReviewSummary { - proposalId: string; - commitId: string; - commitSha: string | null; - highlights: ProposalCommitReviewHighlight[]; - totalReviewers: number; - reviewedCount: number; - matchesDescriptionCount: number; -} diff --git a/src/frontend/src/app/core/api/review/review-api.service.spec.ts b/src/frontend/src/app/core/api/review/review-api.service.spec.ts index 66b2567d..e3d78596 100644 --- a/src/frontend/src/app/core/api/review/review-api.service.spec.ts +++ b/src/frontend/src/app/core/api/review/review-api.service.spec.ts @@ -54,13 +54,12 @@ describe('ReviewApiService', () => { const commonRequest: CreateProposalReviewRequest = { proposalId: 'proposalId', summary: null, - reviewDurationMins: null, buildReproduced: null, }; const commonApiRequest: CreateProposalReviewApiRequest = { proposal_id: 'proposalId', summary: [], - review_duration_mins: [], + build_reproduced: [], vote: [], }; @@ -76,7 +75,7 @@ describe('ReviewApiService', () => { draft: null, }, summary: [], - review_duration_mins: [], + build_reproduced: [], images_paths: [], proposal_review_commits: [], @@ -93,7 +92,6 @@ describe('ReviewApiService', () => { lastUpdatedAt: null, status: ProposalReviewStatus.Draft, summary: null, - reviewDurationMins: null, buildReproduced: null, // [TODO] - remove when API is implemented reproducedBuildImageId: jasmine.any(Array) as unknown as ImageSet[], @@ -133,14 +131,13 @@ describe('ReviewApiService', () => { const commonRequest: UpdateProposalReviewRequest = { proposalId: 'proposalId', summary: null, - reviewDurationMins: null, buildReproduced: null, }; const commonApiRequest: UpdateProposalReviewApiRequest = { proposal_id: 'proposalId', status: [], summary: [], - review_duration_mins: [], + build_reproduced: [], vote: [], }; @@ -199,7 +196,7 @@ describe('ReviewApiService', () => { draft: null, }, summary: [], - review_duration_mins: [], + build_reproduced: [], images_paths: [], proposal_review_commits: [], @@ -217,7 +214,7 @@ describe('ReviewApiService', () => { draft: null, }, summary: [], - review_duration_mins: [], + build_reproduced: [], images_paths: [], proposal_review_commits: [], @@ -237,7 +234,6 @@ describe('ReviewApiService', () => { lastUpdatedAt: null, status: ProposalReviewStatus.Draft, summary: null, - reviewDurationMins: null, buildReproduced: null, // [TODO] - remove when API is implemented reproducedBuildImageId: jasmine.any(Array) as unknown as ImageSet[], @@ -252,7 +248,6 @@ describe('ReviewApiService', () => { lastUpdatedAt: null, status: ProposalReviewStatus.Draft, summary: null, - reviewDurationMins: null, buildReproduced: null, // [TODO] - remove when API is implemented reproducedBuildImageId: jasmine.any(Array) as unknown as ImageSet[], @@ -305,7 +300,7 @@ describe('ReviewApiService', () => { draft: null, }, summary: [], - review_duration_mins: [], + build_reproduced: [], images_paths: [], proposal_review_commits: [], @@ -322,7 +317,6 @@ describe('ReviewApiService', () => { lastUpdatedAt: null, status: ProposalReviewStatus.Draft, summary: null, - reviewDurationMins: null, buildReproduced: null, // [TODO] - remove when API is implemented reproducedBuildImageId: jasmine.any(Array) as unknown as ImageSet[], @@ -376,7 +370,7 @@ describe('ReviewApiService', () => { draft: null, }, summary: [], - review_duration_mins: [], + build_reproduced: [], images_paths: [], proposal_review_commits: [], @@ -393,7 +387,6 @@ describe('ReviewApiService', () => { lastUpdatedAt: null, status: ProposalReviewStatus.Draft, summary: null, - reviewDurationMins: null, buildReproduced: null, // [TODO] - remove when API is implemented reproducedBuildImageId: jasmine.any(Array) as unknown as ImageSet[], @@ -429,13 +422,12 @@ describe('ReviewApiService', () => { const commonCreateRequest: CreateProposalReviewRequest = { proposalId: 'proposalId', summary: null, - reviewDurationMins: null, buildReproduced: null, }; const commonApiCreateRequest: CreateProposalReviewApiRequest = { proposal_id: 'proposalId', summary: [], - review_duration_mins: [], + build_reproduced: [], vote: [], }; @@ -454,7 +446,7 @@ describe('ReviewApiService', () => { draft: null, }, summary: [], - review_duration_mins: [], + build_reproduced: [], images_paths: [], proposal_review_commits: [], @@ -471,7 +463,6 @@ describe('ReviewApiService', () => { lastUpdatedAt: null, status: ProposalReviewStatus.Draft, summary: null, - reviewDurationMins: null, buildReproduced: null, // [TODO] - remove when API is implemented reproducedBuildImageId: jasmine.any(Array) as unknown as ImageSet[], diff --git a/src/frontend/src/app/core/state/profile/profile.service.ts b/src/frontend/src/app/core/state/profile/profile.service.ts index f1010be8..9649919d 100644 --- a/src/frontend/src/app/core/state/profile/profile.service.ts +++ b/src/frontend/src/app/core/state/profile/profile.service.ts @@ -32,14 +32,13 @@ export class ProfileService { private readonly reviewerProfilesSubject = new BehaviorSubject< Record >({}); - public readonly reviewerProfiles$ = - this.reviewerProfilesSubject.asObservable(); + public readonly reviewers$ = this.reviewerProfilesSubject.asObservable(); private readonly userProfileSubject = new BehaviorSubject(null); - public readonly currentUserProfile$ = this.userProfileSubject.asObservable(); + public readonly currentUser$ = this.userProfileSubject.asObservable(); - private readonly currentUserRole$ = this.currentUserProfile$.pipe( + private readonly currentUserRole$ = this.currentUser$.pipe( map(profile => profile?.role ?? null), ); public readonly isCurrentUserReviewer$ = this.currentUserRole$.pipe( diff --git a/src/frontend/src/app/core/state/review-submission/review-submission.service.spec.ts b/src/frontend/src/app/core/state/review-submission/review-submission.service.spec.ts index a5ac7fce..56ea5474 100644 --- a/src/frontend/src/app/core/state/review-submission/review-submission.service.spec.ts +++ b/src/frontend/src/app/core/state/review-submission/review-submission.service.spec.ts @@ -179,7 +179,6 @@ describe('ReviewSubmissionService', () => { ]); expect(commitsSpy).toHaveBeenCalledWith([ createUiProposalReviewCommit({ - apiId: '0', uiId: '0', commit: { commitSha, @@ -319,7 +318,6 @@ describe('ReviewSubmissionService', () => { ]); expect(commitsSpy).toHaveBeenCalledWith([ createUiProposalReviewCommit({ - apiId: '0', uiId: '0', commit: { commitSha, @@ -337,7 +335,6 @@ describe('ReviewSubmissionService', () => { reviewed: true, comment: null, matchesDescription: null, - highlights: [], }), ).toBeRejectedWithError( 'Tried to update a commit for a review without selecting a proposal', @@ -360,7 +357,6 @@ describe('ReviewSubmissionService', () => { reviewed: true, comment: null, matchesDescription: null, - highlights: [], }), ).toBeRejectedWithError( `Tried to update a commit with SHA ${commitSha} from proposal with Id ${proposalId} but it does not exist`, @@ -382,7 +378,6 @@ function createProposalReviewResponse( lastUpdatedAt: null, status: ProposalReviewStatus.Draft, summary: null, - reviewDurationMins: null, buildReproduced: null, reproducedBuildImageId: [], commits: [], diff --git a/src/frontend/src/app/core/state/review-submission/review-submission.service.ts b/src/frontend/src/app/core/state/review-submission/review-submission.service.ts index 9c8f1446..e575c081 100644 --- a/src/frontend/src/app/core/state/review-submission/review-submission.service.ts +++ b/src/frontend/src/app/core/state/review-submission/review-submission.service.ts @@ -41,14 +41,6 @@ export class ReviewSubmissionService { public readonly review$ = this.reviewSubject .asObservable() .pipe(distinctUntilChanged()); - private get review(): GetProposalReviewResponse { - const review = this.reviewSubject.value; - if (isNil(review)) { - throw new Error('Review is not loaded'); - } - - return review; - } private readonly commitsSubject = new BehaviorSubject< UiProposalReviewCommit[] | null @@ -117,7 +109,7 @@ export class ReviewSubmissionService { }); this.reviewSubject.next(res); - this.commitsSubject.next(Array.from(this.commits.values())); + this.emitUpdatedComits(); } public updateReview(updatedReview: UpdateProposalReviewRequest): void { @@ -173,6 +165,9 @@ export class ReviewSubmissionService { ); } + // only add the commit locally first, once we have the minimum + // required information to create it, we will make the + // API call to create it in `updateCommit` method this.commits.set(null, { apiId: null, uiId: String(this.listId++), @@ -183,7 +178,7 @@ export class ReviewSubmissionService { }, }, }); - this.commitsSubject.next(Array.from(this.commits.values())); + this.emitUpdatedComits(); } public async removeCommit(commitSha: string | null): Promise { @@ -200,15 +195,19 @@ export class ReviewSubmissionService { ); } + // do the optimistic update first this.commits.delete(commitSha); - this.commitsSubject.next(Array.from(this.commits.values())); + this.emitUpdatedComits(); - if (isNotNil(commit.apiId)) { - commit; - await this.commitReviewApiService.deleteProposalReviewCommit({ - proposalReviewCommitId: commit.apiId, - }); - } + // before deleting the commit, we make sure to wait for any + // pending creation calls to complete, + // otherwise we won't have the API ID to delete it + const createdCommitId = await this.pendingCommitCreation(commit); + + await this.deleteCommit({ + ...commit, + apiId: createdCommitId ?? commit.apiId, + }); } public async updateCommit( @@ -229,10 +228,11 @@ export class ReviewSubmissionService { ); } + // do the optimistic update first if (oldCommitSha !== newCommitSha) { this.commits.delete(oldCommitSha); } - const updatedCommit: UiProposalReviewCommit = { + let updatedCommit: UiProposalReviewCommit = { apiId: oldLocalCommit.apiId, uiId: oldLocalCommit.uiId, commit: { @@ -241,76 +241,59 @@ export class ReviewSubmissionService { }, }; this.commits.set(newCommitSha, updatedCommit); - this.commitsSubject.next(Array.from(this.commits.values())); - - const existingPromise = this.commitCreatePromises.get(oldCommitSha); - if (isNotNil(existingPromise)) { - const oldRemoteCommit = await existingPromise; - - this.commits.set(newCommitSha, { - apiId: oldRemoteCommit.id, - uiId: oldLocalCommit.uiId, - commit: { - commitSha: newCommitSha, - details: newCommit, - }, - }); + this.emitUpdatedComits(); + + // if commit creation is pending, we wait for it to complete + const createdCommitId = await this.pendingCommitCreation(updatedCommit); + if (isNotNil(createdCommitId)) { + const createdCommit = this.updateCommitWithId( + newCommitSha, + createdCommitId, + ); + if (isNotNil(createdCommit)) { + updatedCommit = createdCommit; + this.commits.set(newCommitSha, createdCommit); + } } + // if the commit is not created yet or the commit sha has changed, + // and we have all the required information to create it, + // we proceed with the creation if ( - isNil(oldLocalCommit.apiId) && + (isNil(updatedCommit?.apiId) || oldCommitSha !== newCommitSha) && isNotNil(newCommitSha) && isNotNil(newCommit.reviewed) ) { - const promise = this.commitReviewApiService.createProposalCommitReview({ - commitSha: newCommitSha, - proposalReviewId: this.review.id, - reviewed: newCommit.reviewed, - }); - this.commitCreatePromises.set(oldCommitSha, promise); - - const createdCommit = await promise; - updatedCommit.apiId = createdCommit.id; - - this.commitCreatePromises.delete(oldCommitSha); - this.commits.set(newCommitSha, updatedCommit); + const createdCommitId = await this.createCommit( + newCommitSha, + newCommit.reviewed, + ); - const [observable, subscription] = this.createCommitObservable(); - this.commitObservables.set(oldCommitSha, observable); - this.commitSubscriptions.set(oldCommitSha, subscription); + const createdCommit = this.updateCommitWithId( + newCommitSha, + createdCommitId, + ); + if (isNotNil(createdCommit)) { + updatedCommit = createdCommit; + this.commits.set(newCommitSha, createdCommit); + } } + // if the commit was not previously created, we don't need to make + // any further updates if (isNil(oldLocalCommit.apiId)) { return; } + // if the commit was previously created, and the commit sha has changed, + // we need to delete the old commit on the API if (oldCommitSha !== newCommitSha) { - this.handleCommitShaChange(oldCommitSha, newCommitSha); + await this.deleteCommit(oldLocalCommit); } this.commitObservables.get(newCommitSha)?.next(updatedCommit); } - private handleCommitShaChange( - previousCommitSha: string | null, - newCommitSha: string | null, - ): void { - const prevObservable = this.commitObservables.get(previousCommitSha); - const prevSubscription = this.commitSubscriptions.get(previousCommitSha); - - if (isNil(prevObservable) || isNil(prevSubscription)) { - throw new Error( - `Missing observable or subscription for commit ${previousCommitSha}`, - ); - } - - this.commitObservables.delete(previousCommitSha); - this.commitSubscriptions.delete(previousCommitSha); - - this.commitObservables.set(newCommitSha, prevObservable); - this.commitSubscriptions.set(newCommitSha, prevSubscription); - } - private createCommitObservable(): [ BehaviorSubject, Subscription, @@ -328,7 +311,7 @@ export class ReviewSubmissionService { isNotNil(commit.apiId) ? from( this.commitReviewApiService.updateProposalReviewCommit({ - proposalReviewCommitId: commit.apiId!, + proposalReviewCommitId: commit.apiId, details: commit.commit.details, }), ) @@ -339,4 +322,91 @@ export class ReviewSubmissionService { return [commitSubject, subscription]; } + + private emitUpdatedComits(): void { + this.commitsSubject.next(Array.from(this.commits.values())); + } + + private async deleteCommit( + pendingCommit: UiProposalReviewCommit, + ): Promise { + this.commitSubscriptions.get(pendingCommit.commit.commitSha)?.unsubscribe(); + this.commitSubscriptions.delete(pendingCommit.commit.commitSha); + this.commitObservables.delete(pendingCommit.commit.commitSha); + + // if there's no API ID, + // it means the commit was never created on the API side + if (isNotNil(pendingCommit.apiId)) { + await this.commitReviewApiService.deleteProposalReviewCommit({ + proposalReviewCommitId: pendingCommit.apiId, + }); + } + } + + private async createCommit( + commitSha: string, + reviewed: boolean, + ): Promise { + const review = this.reviewSubject.value; + if (isNil(review)) { + throw new Error( + 'Tried to create a commit for a review before it is loaded', + ); + } + + const crateCommitPromise = + this.commitReviewApiService.createProposalCommitReview({ + commitSha, + reviewed, + proposalReviewId: review.id, + }); + this.commitCreatePromises.set(commitSha, crateCommitPromise); + + const createdCommit = await crateCommitPromise; + + this.commitCreatePromises.delete(commitSha); + + const [observable, subscription] = this.createCommitObservable(); + this.commitObservables.set(commitSha, observable); + this.commitSubscriptions.set(commitSha, subscription); + + return createdCommit.id; + } + + private async pendingCommitCreation( + pendingCommit: UiProposalReviewCommit, + ): Promise { + const existingPromise = this.commitCreatePromises.get( + pendingCommit.commit.commitSha, + ); + + if (isNil(existingPromise)) { + return null; + } + + const createdCommit = await existingPromise; + return createdCommit.id; + } + + private updateCommitWithId( + commitSha: string | null, + apiId: string, + ): UiProposalReviewCommit | null { + // in case there was some optimistic updates, + // we need to make sure we get the latest commit + const currentCommit = this.commits.get(commitSha); + + // if the commit is now null, it means it was optimisitcally deleted, + // so we don't need to update it. it will be deleted by the + // `remoteCommit` method that waits for creation to complete before + // deleting. + if (isNotNil(currentCommit)) { + return { + ...currentCommit, + apiId, + }; + } + + return null; + } } diff --git a/src/frontend/src/app/core/state/review/review.service.mock.ts b/src/frontend/src/app/core/state/review/review.service.mock.ts index 36c095ed..11670da4 100644 --- a/src/frontend/src/app/core/state/review/review.service.mock.ts +++ b/src/frontend/src/app/core/state/review/review.service.mock.ts @@ -4,8 +4,8 @@ export type ReviewServiceMock = jasmine.SpyObj; export function reviewServiceMockFactory(): ReviewServiceMock { return jasmine.createSpyObj('ReviewService', [ - 'loadReviewListByProposalId', - 'loadReviewListByReviewerId', + 'loadReviewsByProposalId', + 'loadReviewsByReviewerId', 'loadReview', ]); } diff --git a/src/frontend/src/app/core/state/review/review.service.ts b/src/frontend/src/app/core/state/review/review.service.ts index cbb1a3af..c79bc334 100644 --- a/src/frontend/src/app/core/state/review/review.service.ts +++ b/src/frontend/src/app/core/state/review/review.service.ts @@ -12,8 +12,7 @@ export class ReviewService { private readonly proposalReviewListSubject = new BehaviorSubject< GetProposalReviewResponse[] >([]); - public readonly proposalReviewList$ = - this.proposalReviewListSubject.asObservable(); + public readonly reviews$ = this.proposalReviewListSubject.asObservable(); private readonly currentReviewSubject = new BehaviorSubject(null); @@ -22,9 +21,10 @@ export class ReviewService { private readonly userReviewListSubject = new BehaviorSubject< GetProposalReviewResponse[] >([]); - public readonly userReviewList$ = this.userReviewListSubject.asObservable(); + public readonly currentUserReviews$ = + this.userReviewListSubject.asObservable(); - public async loadReviewListByProposalId(proposalId: string): Promise { + public async loadReviewsByProposalId(proposalId: string): Promise { const getResponse = await this.reviewApiService.listProposalReviews({ proposalId, }); @@ -32,7 +32,7 @@ export class ReviewService { this.proposalReviewListSubject.next(getResponse); } - public async loadReviewListByReviewerId(userId: string): Promise { + public async loadReviewsByReviewerId(userId: string): Promise { const getResponse = await this.reviewApiService.listProposalReviews({ userId, }); diff --git a/src/frontend/src/app/pages/profile-edit/profile-edit.component.spec.ts b/src/frontend/src/app/pages/profile-edit/profile-edit.component.spec.ts index 133ca1b2..40358967 100644 --- a/src/frontend/src/app/pages/profile-edit/profile-edit.component.spec.ts +++ b/src/frontend/src/app/pages/profile-edit/profile-edit.component.spec.ts @@ -32,7 +32,7 @@ describe('ProfileViewComponent', () => { beforeEach(async () => { profileServiceMock = profileServiceMockFactory(); - defineProp(profileServiceMock, 'currentUserProfile$', of(null)); + defineProp(profileServiceMock, 'currentUser$', of(null)); defineProp(profileServiceMock, 'isCurrentUserReviewer$', of(true)); proposalServiceMock = proposalServiceMockFactory(); @@ -42,8 +42,8 @@ describe('ProfileViewComponent', () => { activatedRouteMock.params = of([{ id: 1 }]); reviewServiceMock = reviewServiceMockFactory(); - defineProp(reviewServiceMock, 'proposalReviewList$', of([])); - defineProp(reviewServiceMock, 'userReviewList$', of([])); + defineProp(reviewServiceMock, 'reviews$', of([])); + defineProp(reviewServiceMock, 'currentUserReviews$', of([])); await TestBed.configureTestingModule({ imports: [ProfileEditComponent, RouterModule], diff --git a/src/frontend/src/app/pages/profile-edit/profile-edit.component.ts b/src/frontend/src/app/pages/profile-edit/profile-edit.component.ts index a1a89548..f010e9c2 100644 --- a/src/frontend/src/app/pages/profile-edit/profile-edit.component.ts +++ b/src/frontend/src/app/pages/profile-edit/profile-edit.component.ts @@ -51,9 +51,7 @@ import { ReviewerProfileComponent } from './reviewer-profile'; `, }) export class ProfileEditComponent implements OnInit { - public readonly userProfile = toSignal( - this.profileService.currentUserProfile$, - ); + public readonly userProfile = toSignal(this.profileService.currentUser$); public readonly UserRole = UserRole; constructor(private readonly profileService: ProfileService) {} diff --git a/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.spec.ts b/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.spec.ts index d14b5ed6..f9899d38 100644 --- a/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.spec.ts +++ b/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.spec.ts @@ -3,16 +3,10 @@ import { ActivatedRoute, provideRouter } from '@angular/router'; import { of } from 'rxjs'; import { - ProposalLinkBaseUrl, - ProposalTopic, - ProposalState, - ProposalVotingLinkType, -} from '~core/api'; -import { ProposalService } from '~core/state'; -import { - ProposalServiceMock, - proposalServiceMockFactory, -} from '~core/state/proposal/proposal.service.mock'; + ProfileServiceMock, + profileServiceMockFactory, +} from '../../../core/state/profile/profile.service.mock'; +import { ProfileService } from '~core/state'; import { ReviewService } from '~core/state/review/review.service'; import { ReviewServiceMock, @@ -28,81 +22,33 @@ import { ClosedProposalSummaryComponent } from './closed-proposal-summary.compon describe('ClosedProposalSummaryComponent', () => { let component: ClosedProposalSummaryComponent; let fixture: ComponentFixture; - let proposalServiceMock: ProposalServiceMock; + + let activatedRouteMock: ActivatedRouteMock; let reviewServiceMock: ReviewServiceMock; - let activatedRoute: ActivatedRouteMock; + let profileServiceMock: ProfileServiceMock; beforeEach(async () => { - proposalServiceMock = proposalServiceMockFactory(); - defineProp( - proposalServiceMock, - 'currentProposal$', - of({ - id: '1', - nsProposalId: 1n, - title: 'title', - topic: ProposalTopic.RVM, - type: 'unknown', - state: ProposalState.InProgress, - reviewPeriodEnd: new Date(2024, 1, 17, 1, 1, 25), - votingPeriodEnd: new Date(2024, 1, 19, 1, 1, 25), - reviewCompletedAt: null, - decidedAt: null, - proposedAt: new Date(2024, 1, 15, 1, 1, 25), - proposedBy: 432432432423n, - summary: 'Elect new replica binary revision', - codeGovVote: null, - proposalLinks: [ - { - type: ProposalVotingLinkType.NNSDApp, - link: ProposalLinkBaseUrl.NNSDApp + 1, - }, - ], - }), - ); + activatedRouteMock = activatedRouteMockFactory(); + activatedRouteMock.params = of([{ proposalId: 1 }]); reviewServiceMock = reviewServiceMockFactory(); - defineProp(reviewServiceMock, 'proposalReviewList$', of([])); + defineProp(reviewServiceMock, 'reviews$', of([])); - activatedRoute = activatedRouteMockFactory(); - activatedRoute.params = of([{ id: 1 }]); + profileServiceMock = profileServiceMockFactory(); + defineProp(profileServiceMock, 'reviewers$', of({})); await TestBed.configureTestingModule({ imports: [ClosedProposalSummaryComponent], providers: [ - { provide: ProposalService, useValue: proposalServiceMock }, { provide: ReviewService, useValue: reviewServiceMock }, - { - provide: ActivatedRoute, - useValue: activatedRoute, - }, + { provide: ProfileService, useValue: profileServiceMock }, + { provide: ActivatedRoute, useValue: activatedRouteMock }, provideRouter([]), ], }).compileComponents(); fixture = TestBed.createComponent(ClosedProposalSummaryComponent); component = fixture.componentInstance; - fixture.componentRef.setInput('proposal', { - id: 1n, - title: 'title', - topic: ProposalTopic.RVM, - type: 'unknown', - state: ProposalState.Completed, - reviewPeriodEnd: new Date(2024, 1, 17, 1, 1, 25), - votingPeriodEnd: new Date(2024, 1, 19, 1, 1, 25), - proposedAt: new Date(2024, 1, 15, 1, 1, 25), - proposedBy: 432432432423n, - reviewCompletedAt: new Date(2024, 1, 19, 1, 1, 25), - decidedAt: new Date(2024, 1, 19, 1, 1, 25), - summary: 'Elect new replica binary revision', - proposalLinks: [ - { - type: ProposalVotingLinkType.NNSDApp, - link: ProposalLinkBaseUrl.NNSDApp + 1, - }, - ], - codeGovVote: 'ADOPT', - }); fixture.detectChanges(); }); diff --git a/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.ts b/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.ts index f4677d8a..ced493f2 100644 --- a/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.ts +++ b/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.ts @@ -4,9 +4,9 @@ import { Component, OnInit, computed, - input, + effect, + inject, } from '@angular/core'; -import { toSignal } from '@angular/core/rxjs-interop'; import { RouterLink } from '@angular/router'; import { @@ -14,13 +14,13 @@ import { DashCircleIconComponent, CheckCircleIconComponent, } from '@cg/angular-ui'; -import { GetProposalResponse, ProposalCommitReviewSummary } from '~core/api'; -import { ReviewService } from '~core/state'; +import { ProfileService, ReviewService } from '~core/state'; import { KeyColComponent, KeyValueGridComponent, ValueColComponent, } from '~core/ui'; +import { isNil, isNotNil, routeParamSignal, toSyncSignal } from '~core/utils'; @Component({ selector: 'app-closed-proposal-summary', @@ -45,11 +45,11 @@ import { margin-bottom: common.size(6); } - .summary__vote--adopt { + .answer--positive { color: common.$success; } - .summary__vote--reject { + .answer--negative { color: common.$error; } @@ -62,26 +62,6 @@ import { flex-direction: row; } - .commit__highlights { - display: flex; - flex-direction: column; - } - - .commit__highlights-label { - color: common.$black; - @include common.dark { - color: common.$white; - } - } - - .commit__highlights-content { - @include common.my(2); - } - - .commit__highlights-quote { - font-style: italic; - } - .reject-icon { width: common.size(6); height: common.size(6); @@ -96,35 +76,23 @@ import { `, ], template: ` - @if (reviewList(); as reviewList) { + @let reviews = this.reviews(); + @let reviewers = this.reviewers(); + @let commits = this.commits(); + @let proposalStats = this.proposalStats(); + + @if (proposalStats) {

Review summary

-

- @if (proposal().codeGovVote === 'NO VOTE') { - No CodeGov vote cast - } @else { - CodeGov voted to - - {{ proposal().codeGovVote }} - - this proposal - } -

- - @if (reviewList.length !== 0) { + @if (reviews.length !== 0) {

- {{ proposalStats()?.adopt }} reviewer(s) voted to - adopt + {{ proposalStats.adopt }} reviewer(s) voted to + adopt this proposal

@@ -132,8 +100,8 @@ import {

- {{ proposalStats()?.reject }} reviewer(s) voted to - reject + {{ proposalStats.reject }} reviewer(s) voted to + reject this proposal

@@ -141,44 +109,60 @@ import {

- {{ commitList().length }} commits reviewed by - {{ reviewList.length }} reviewers + {{ commits.length }} commit(s) reviewed by + {{ reviews.length }} reviewers

Build verified by - {{ proposalStats()?.buildReproduced }} reviewers + {{ proposalStats.buildReproduced }} reviewer(s)

+ } @else { + No reviews were submitted for this proposal }

Reviews

- @for (review of reviewList; track review.id; let i = $index) { + @for (review of reviews; track review.id; let i = $index) {
Reviewer - Reviewer Name Link + {{ reviewers[review.userId].username }} + + + Reviewer vote + + @switch (review.vote) { + @case (true) { + Adopt + } + @case (false) { + Reject + } + @default { + No vote + } + } - Build reproduced + Build reproduction - {{ review.buildReproduced ? 'Yes' : 'No' }} - - - Vote - - {{ review.vote }} + @switch (review.buildReproduced) { + @case (true) { + Successful + } + @case (false) { + Unsuccessful + } + @default { + Not applicable + } + } @@ -186,7 +170,7 @@ import { {{ review.commits.length }} out of - {{ commitList().length }} + {{ commits.length }} @@ -209,7 +193,7 @@ import { }

Commits

- @for (commit of commitList(); track commit.commitId; let i = $index) { + @for (commit of commits; track commit.commitId; let i = $index) {
@@ -231,8 +215,7 @@ import { Reviewed by - {{ commit.reviewedCount }} out of - {{ reviewList.length }} reviewers + {{ commit.reviewedCount }} out of {{ reviews.length }} reviewers @@ -244,23 +227,6 @@ import { }}) - -
-
Reviewer highlights
-
    - @for ( - highlight of commit.highlights; - track highlight.reviewerId - ) { -
  • - Reviewer #{{ highlight.reviewerId }} said: - - "{{ highlight.text }}" - -
    • - } -
-
} @empty { @@ -274,50 +240,54 @@ import { `, }) export class ClosedProposalSummaryComponent implements OnInit { - public readonly proposal = input.required(); - - public readonly reviewList = toSignal(this.reviewService.proposalReviewList$); - - public proposalStats = computed( - () => - this.reviewList()?.reduce( - (accum, review) => ({ - adopt: review.vote ? accum.adopt + 1 : accum.adopt, - reject: review.vote === false ? accum.reject + 1 : accum.reject, - buildReproduced: review.buildReproduced - ? accum.buildReproduced + 1 - : accum.buildReproduced, - }), - { adopt: 0, reject: 0, buildReproduced: 0 }, - ) ?? null, - ); - - public commitList = computed(() => { + private readonly reviewService = inject(ReviewService); + private readonly profileService = inject(ProfileService); + + public readonly currentProposalId = routeParamSignal('proposalId'); + + public readonly reviewers = toSyncSignal(this.profileService.reviewers$); + public readonly reviews = toSyncSignal(this.reviewService.reviews$); + + public proposalStats = computed(() => { + const reviews = this.reviews(); + if (isNil(reviews)) { + return null; + } + + return reviews.reduce( + (accum, review) => ({ + adopt: review.vote ? accum.adopt + 1 : accum.adopt, + reject: review.vote === false ? accum.reject + 1 : accum.reject, + buildReproduced: review.buildReproduced + ? accum.buildReproduced + 1 + : accum.buildReproduced, + }), + { adopt: 0, reject: 0, buildReproduced: 0 }, + ); + }); + + public commits = computed(() => { + const proposalId = this.currentProposalId(); + const reviews = this.reviews(); + if (isNil(proposalId) || isNil(reviews)) { + return []; + } + const map: Map = new Map(); - this.reviewList()?.forEach(review => { + reviews.forEach(review => { for (const commit of review.commits) { const existingCommit = map.get(commit.id) ?? ({ - proposalId: this.proposal().id, + proposalId, commitId: commit.id, commitSha: commit.commitSha, totalReviewers: 0, reviewedCount: 0, matchesDescriptionCount: 0, - highlights: [], } satisfies ProposalCommitReviewSummary); - if (commit.details.reviewed) { - commit.details.highlights.forEach(highlight => { - existingCommit.highlights.push({ - reviewerId: review.userId, - text: highlight, - }); - }); - } - existingCommit.totalReviewers++; if (commit.details.reviewed) { @@ -331,12 +301,36 @@ export class ClosedProposalSummaryComponent implements OnInit { map.set(commit.id, existingCommit); } }); + return Array.from(map.values()); }); - constructor(private readonly reviewService: ReviewService) {} + constructor() { + effect(() => { + const proposalId = this.currentProposalId(); + + if (isNotNil(proposalId)) { + this.reviewService.loadReviewsByProposalId(proposalId); + } + }); + } public ngOnInit(): void { - this.reviewService.loadReviewListByProposalId(this.proposal().id); + this.profileService.loadReviewerProfiles(); } } + +interface ProposalStats { + adopt: number; + reject: number; + buildReproduced: number; +} + +interface ProposalCommitReviewSummary { + proposalId: string; + commitId: string; + commitSha: string | null; + totalReviewers: number; + reviewedCount: number; + matchesDescriptionCount: number; +} diff --git a/src/frontend/src/app/pages/proposal-details/proposal-details.component.spec.ts b/src/frontend/src/app/pages/proposal-details/proposal-details.component.spec.ts index 23b21d87..dee3431c 100644 --- a/src/frontend/src/app/pages/proposal-details/proposal-details.component.spec.ts +++ b/src/frontend/src/app/pages/proposal-details/proposal-details.component.spec.ts @@ -72,7 +72,7 @@ describe('ProposalDetailsComponent', () => { profileServiceMock = profileServiceMockFactory(); defineProp(profileServiceMock, 'isCurrentUserAdmin$', of(false)); defineProp(profileServiceMock, 'isCurrentUserReviewer$', of(true)); - defineProp(profileServiceMock, 'currentUserProfile$', of(null)); + defineProp(profileServiceMock, 'currentUser$', of(null)); activatedRouteMock = activatedRouteMockFactory(); defineProp( @@ -82,8 +82,8 @@ describe('ProposalDetailsComponent', () => { ); reviewServiceMock = reviewServiceMockFactory(); - defineProp(reviewServiceMock, 'proposalReviewList$', of([])); - defineProp(reviewServiceMock, 'userReviewList$', of([])); + defineProp(reviewServiceMock, 'reviews$', of([])); + defineProp(reviewServiceMock, 'currentUserReviews$', of([])); await TestBed.configureTestingModule({ imports: [ProposalDetailsComponent, RouterModule], diff --git a/src/frontend/src/app/pages/proposal-details/proposal-details.component.ts b/src/frontend/src/app/pages/proposal-details/proposal-details.component.ts index ed672d8e..e6e5f9d9 100644 --- a/src/frontend/src/app/pages/proposal-details/proposal-details.component.ts +++ b/src/frontend/src/app/pages/proposal-details/proposal-details.component.ts @@ -63,18 +63,6 @@ import { ClosedProposalSummaryComponent } from './closed-proposal-summary'; .proposal__link { margin-right: common.size(4); } - - .proposal__vote { - font-weight: bold; - } - - .proposal__vote--adopt { - color: common.$success; - } - - .proposal__vote--reject { - color: common.$error; - } `, ], template: ` @@ -165,18 +153,6 @@ import { ClosedProposalSummaryComponent } from './closed-proposal-summary'; : 'Not yet decided' }} - - CodeGov vote - - {{ proposal.codeGovVote }} -
@@ -229,9 +205,10 @@ import { ClosedProposalSummaryComponent } from './closed-proposal-summary'; @if (showSummary() || proposal.state === ProposalState().Completed) { - + } @else {

Proposal summary

+
(() => @@ -267,14 +244,12 @@ export class ProposalDetailsComponent implements OnInit { public readonly isReviewer = toSyncSignal( this.profileService.isCurrentUserReviewer$, ); - public readonly userProfile = toSyncSignal( - this.profileService.currentUserProfile$, - ); + public readonly userProfile = toSyncSignal(this.profileService.currentUser$); public readonly currentProposalId = routeParamSignal('proposalId'); public readonly userReviewList = toSyncSignal( - this.reviewService.userReviewList$, + this.reviewService.currentUserReviews$, ); public readonly userReview = computed( () => @@ -303,7 +278,7 @@ export class ProposalDetailsComponent implements OnInit { const userProfile = this.userProfile(); if (isNotNil(userProfile)) { - this.reviewService.loadReviewListByReviewerId(userProfile.id); + this.reviewService.loadReviewsByReviewerId(userProfile.id); } }); } diff --git a/src/frontend/src/app/pages/proposal-list/proposal-list.component.spec.ts b/src/frontend/src/app/pages/proposal-list/proposal-list.component.spec.ts index b0482802..545e1538 100644 --- a/src/frontend/src/app/pages/proposal-list/proposal-list.component.spec.ts +++ b/src/frontend/src/app/pages/proposal-list/proposal-list.component.spec.ts @@ -31,10 +31,10 @@ describe('ProposalListComponent', () => { profileServiceMock = profileServiceMockFactory(); defineProp(profileServiceMock, 'isCurrentUserReviewer$', of(true)); - defineProp(profileServiceMock, 'currentUserProfile$', of(null)); + defineProp(profileServiceMock, 'currentUser$', of(null)); reviewServiceMock = reviewServiceMockFactory(); - defineProp(reviewServiceMock, 'userReviewList$', of([])); + defineProp(reviewServiceMock, 'currentUserReviews$', of([])); await TestBed.configureTestingModule({ imports: [ProposalListComponent, RouterModule], diff --git a/src/frontend/src/app/pages/proposal-list/proposal-list.component.ts b/src/frontend/src/app/pages/proposal-list/proposal-list.component.ts index d299cba5..ad044647 100644 --- a/src/frontend/src/app/pages/proposal-list/proposal-list.component.ts +++ b/src/frontend/src/app/pages/proposal-list/proposal-list.component.ts @@ -231,20 +231,6 @@ interface FilterForm { : 'Not yet decided' }} - - - CodeGov vote - - - {{ proposal.codeGovVote }} -
@@ -298,10 +284,10 @@ export class ProposalListComponent { public readonly isReviewer = toSignal( this.profileService.isCurrentUserReviewer$, ); - public readonly userProfile = toSignal( - this.profileService.currentUserProfile$, + public readonly userProfile = toSignal(this.profileService.currentUser$); + public readonly userReviewList = toSignal( + this.reviewService.currentUserReviews$, ); - public readonly userReviewList = toSignal(this.reviewService.userReviewList$); public readonly proposalListWithReviewIds = computed(() => { return this.proposalList()?.map(proposal => { @@ -339,7 +325,7 @@ export class ProposalListComponent { const userProfile = this.userProfile(); if (isNotNil(userProfile)) { - this.reviewService.loadReviewListByReviewerId(userProfile.id); + this.reviewService.loadReviewsByReviewerId(userProfile.id); } }); diff --git a/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.ts b/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.ts index aebded7f..c1371880 100644 --- a/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.ts +++ b/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.ts @@ -31,7 +31,13 @@ import { KeyValueGridComponent, ValueColComponent, } from '~core/ui'; -import { boolToRadio, isNil, radioToBool, toSyncSignal } from '~core/utils'; +import { + boolToRadio, + isNil, + isNotNil, + radioToBool, + toSyncSignal, +} from '~core/utils'; @Component({ selector: 'app-review-commits-form', @@ -364,7 +370,9 @@ export class ReviewCommitsFormComponent implements OnDestroy { return; } - commitForm.controls.commitSha.setValue(result, { emitEvent: false }); + if (result !== commitSha) { + commitForm.controls.commitSha.setValue(result, { emitEvent: false }); + } } private focusCommitForm(index: number): void { @@ -404,7 +412,7 @@ export class ReviewCommitsFormComponent implements OnDestroy { (commit, i) => i !== index && commit.commit.commitSha === commitSha, ); - return existingCommit ? { uniqueSha: true } : null; + return isNotNil(existingCommit) ? { uniqueSha: true } : null; }; } } @@ -423,7 +431,7 @@ interface ReviewCommitForm { comment: FormControl; } -const commitShaRegex = /[0-9a-f]{7,40}/; +const commitShaRegex = /^[a-f0-9]{40}$/; function extractCommitSha(commitSha: string): string | null { return commitShaRegex.exec(commitSha)?.[0] ?? null; @@ -435,19 +443,20 @@ function commitToFormValue( ): ReviewCommitFormValue { const reviewed = boolToRadio(commit.reviewed); - let matchesDescription = null; - let comment = null; - if (commit.reviewed) { - matchesDescription = boolToRadio(commit.matchesDescription); - comment = commit.comment; + return { + commitSha, + reviewed, + matchesDescription: boolToRadio(commit.matchesDescription), + comment: commit.comment, + }; } return { - commitSha: commitSha, - matchesDescription, + commitSha, reviewed, - comment, + matchesDescription: null, + comment: null, }; } @@ -461,7 +470,6 @@ function commitFromFormValue( matchesDescription: radioToBool(formValue.matchesDescription), reviewed: true, comment: formValue.comment ?? null, - highlights: [], }; } diff --git a/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.ts b/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.ts index 9c73b1b5..f9451234 100644 --- a/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.ts +++ b/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.ts @@ -42,21 +42,6 @@ import { boolToRadio, isNil, radioToBool, toSyncSignal } from '~core/utils'; template: ` - - - - - - - - -
Summary
 @@ -145,7 +130,6 @@ export class ReviewDetailsFormComponent implements OnDestroy { public readonly reviewForm = signal( new FormGroup({ - reviewDurationMins: new FormControl(null), summary: new FormControl(null), buildReproduced: new FormControl(null), vote: new FormControl(null), @@ -177,7 +161,6 @@ export class ReviewDetailsFormComponent implements OnDestroy { this.reviewForm().setValue( { - reviewDurationMins: review.reviewDurationMins, summary: review.summary, buildReproduced: boolToRadio(review.buildReproduced), vote: boolToRadio(review.vote), @@ -207,7 +190,6 @@ export class ReviewDetailsFormComponent implements OnDestroy { this.reviewSubmissionService.updateReview({ proposalId: review.proposalId, summary: value.summary ?? null, - reviewDurationMins: value.reviewDurationMins ?? null, buildReproduced: radioToBool(value.buildReproduced), vote: radioToBool(value.vote), }); @@ -215,14 +197,12 @@ export class ReviewDetailsFormComponent implements OnDestroy { } interface ReviewFormValue { - reviewDurationMins: number | null; summary: string | null; buildReproduced: 0 | 1 | null; vote: 0 | 1 | null; } interface ReviewForm { - reviewDurationMins: FormControl; summary: FormControl; buildReproduced: FormControl<0 | 1 | null>; vote: FormControl<0 | 1 | null>; diff --git a/src/frontend/src/app/pages/proposal-review/proposal-review.component.spec.ts b/src/frontend/src/app/pages/proposal-review/proposal-review.component.spec.ts index c90b0f92..8c766c05 100644 --- a/src/frontend/src/app/pages/proposal-review/proposal-review.component.spec.ts +++ b/src/frontend/src/app/pages/proposal-review/proposal-review.component.spec.ts @@ -38,8 +38,8 @@ describe('ProposalReviewComponent', () => { defineProp(proposalServiceMock, 'currentProposal$', of(null)); profileServiceMock = profileServiceMockFactory(); - defineProp(profileServiceMock, 'currentUserProfile$', of(null)); - defineProp(profileServiceMock, 'reviewerProfiles$', of({})); + defineProp(profileServiceMock, 'currentUser$', of(null)); + defineProp(profileServiceMock, 'reviewers$', of({})); activatedRouteMock = activatedRouteMockFactory(); defineProp( diff --git a/src/frontend/src/app/pages/proposal-review/proposal-review.component.ts b/src/frontend/src/app/pages/proposal-review/proposal-review.component.ts index ed58a4fb..e3404653 100644 --- a/src/frontend/src/app/pages/proposal-review/proposal-review.component.ts +++ b/src/frontend/src/app/pages/proposal-review/proposal-review.component.ts @@ -4,6 +4,7 @@ import { Component, computed, effect, + inject, OnInit, signal, } from '@angular/core'; @@ -53,10 +54,6 @@ import { isNil, isNotNil, routeParamSignal } from '~core/utils'; color: common.$error; } - .answer--neutral { - color: common.$white; - } - .review__image { height: common.size(10); padding-right: common.size(1); @@ -107,7 +104,7 @@ import { isNil, isNotNil, routeParamSignal } from '~core/utils'; Reject } @default { - No vote + No vote } } @@ -122,7 +119,7 @@ import { isNil, isNotNil, routeParamSignal } from '~core/utils'; Unsuccessful } @default { - Not Applicable + Not applicable } } @@ -131,17 +128,15 @@ import { isNil, isNotNil, routeParamSignal } from '~core/utils'; Build verification images - @if (review.reproducedBuildImageId.length === 0) { + @for ( + image of review.reproducedBuildImageId; + track image.sm.url + ) { + + + + } @empty { No build verification images were provided for this review. - } @else { - @for ( - image of review.reproducedBuildImageId; - track image.sm.url - ) { - - - - } } @@ -200,13 +195,6 @@ import { isNil, isNotNil, routeParamSignal } from '~core/utils'; {{ commit.details.comment }} - - - Commit highlights - - - {{ commit.details.highlights }} - }
@@ -218,12 +206,14 @@ import { isNil, isNotNil, routeParamSignal } from '~core/utils'; `, }) export class ProposalReviewComponent implements OnInit { + private readonly reviewService = inject(ReviewService); + private readonly proposalService = inject(ProposalService); + private readonly profileService = inject(ProfileService); + public readonly proposalReviewStatus = signal(ProposalReviewStatus); public readonly ProposalState = signal(ProposalState); - public readonly userProfile = toSignal( - this.profileService.currentUserProfile$, - ); + public readonly userProfile = toSignal(this.profileService.currentUser$); public readonly isReviewOwner = computed(() => { const userId = this.currentReview()?.userId; @@ -240,7 +230,7 @@ export class ProposalReviewComponent implements OnInit { this.proposalService.currentProposal$, ); - public readonly reviewers = toSignal(this.profileService.reviewerProfiles$); + public readonly reviewers = toSignal(this.profileService.reviewers$); public readonly currentReviewerId = computed( () => this.currentReview()?.userId ?? null, ); @@ -253,11 +243,7 @@ export class ProposalReviewComponent implements OnInit { return this.reviewers()?.[reviewerId] ?? null; }); - constructor( - private readonly reviewService: ReviewService, - private readonly proposalService: ProposalService, - private readonly profileService: ProfileService, - ) { + constructor() { effect(() => { const reviewId = this.currentReviewId(); diff --git a/src/marketing/src/env.d.ts b/src/marketing/src/env.d.ts index f964fe0c..acef35f1 100644 --- a/src/marketing/src/env.d.ts +++ b/src/marketing/src/env.d.ts @@ -1 +1,2 @@ +/// ///