diff --git a/src/bin/pr-metadata-validator.rs b/src/bin/pr-metadata-validator.rs index d553802..9132a44 100644 --- a/src/bin/pr-metadata-validator.rs +++ b/src/bin/pr-metadata-validator.rs @@ -8,9 +8,9 @@ use regex::Regex; use trainee_tracker::{ Error, config::{CourseSchedule, CourseScheduleWithRegisterSheetId}, - course::match_prs_to_assignments, + course::{get_descriptor_id_for_pr, match_prs_to_assignments}, newtypes::Region, - octocrab::octocrab_for_token, + octocrab::{all_pages, octocrab_for_token}, pr_comments::{PullRequest, close_existing_comments, leave_tagged_comment}, prs::get_prs, }; @@ -74,6 +74,10 @@ async fn main() { &format!("{}{}", BAD_TITLE_COMMENT_PREFIX, reason) } ValidationResult::UnknownRegion => UNKNOWN_REGION_COMMENT, + ValidationResult::WrongFiles { + expected_files_pattern, + } => &format!("{}`{}`", WRONG_FILES, expected_files_pattern), + ValidationResult::NoFiles => NO_FILES, }; let full_message = format!( @@ -131,6 +135,16 @@ const UNKNOWN_REGION_COMMENT: &str = r#"Your PR's title didn't contain a known r Please check the expected title format, and make sure your region is in the correct place and spelled correctly."#; +const WRONG_FILES: &str = r#"The changed files in this PR don't match what is expected for this task. + +Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. + +Please review the changed files tab at the top of the page, we are only expecting changes in this directory: "#; + +const NO_FILES: &str = r#"This PR is missing any submitted files. + +Please check that you committed the right files and pushed to the repository"#; + #[derive(strum_macros::Display)] enum ValidationResult { Ok, @@ -138,6 +152,8 @@ enum ValidationResult { CouldNotMatch, BadTitleFormat { reason: String }, UnknownRegion, + WrongFiles { expected_files_pattern: String }, + NoFiles, } async fn validate_pr( @@ -228,6 +244,78 @@ async fn validate_pr( return Ok(ValidationResult::BodyTemplateNotFilledOut); } + let pr_assignment_descriptor_id = + get_descriptor_id_for_pr(&matched.sprints, pr_number).expect("This PR does not exist"); + // This should never error, as a PR by this point in code must have been matched + // with an assignment, and PR assignments must have an associated issue descriptor + + check_pr_file_changes( + octocrab, + github_org_name, + module_name, + pr_number, + pr_assignment_descriptor_id, + ) + .await +} + +// Check the changed files in a pull request match what is expected for that sprint task +async fn check_pr_file_changes( + octocrab: &Octocrab, + org_name: &str, + module_name: &str, + pr_number: u64, + task_issue_number: u64, +) -> Result { + // Get the Sprint Task's description of expected changes + let Ok(task_issue) = octocrab + .issues(org_name, module_name) + .get(task_issue_number) + .await + else { + return Ok(ValidationResult::CouldNotMatch); // Failed to find the right task + }; + + let task_issue_body = task_issue.body.unwrap_or_default(); + + let directory_description = Regex::new("CHANGE_DIR=(.+)\\n") + .map_err(|err| Error::UserFacing(format!("Known good regex failed to compile: {}", err)))?; + let Some(directory_regex_captures) = directory_description.captures(&task_issue_body) else { + return Ok(ValidationResult::Ok); // There is no match defined for this task, don't do any more checks + }; + let directory_description_regex = directory_regex_captures + .get(1) + .expect("Regex capture failed to return string match") + .as_str(); // Only allows a single directory for now + + let directory_matcher = Regex::new(directory_description_regex).map_err(|err| { + Error::UserFacing(format!( + "Failed to compile regex from {}, check the CHANGE_DIR declaration: {}", + task_issue.html_url, err + )) + })?; + + // Get all of the changed files + let pr_files = all_pages("changed files in pull request", octocrab, async || { + octocrab + .pulls(org_name, module_name) + .list_files(pr_number) + .await + }) + .await?; + if pr_files.is_empty() { + return Ok(ValidationResult::NoFiles); // no files committed + } + + // check each file and error if one is in unexpected place + for pr_file in pr_files { + if !directory_matcher.is_match(&pr_file.filename) { + return Ok(ValidationResult::WrongFiles { + expected_files_pattern: directory_description_regex.to_string(), + }); + } + } + Ok(ValidationResult::Ok) } diff --git a/src/course.rs b/src/course.rs index 9d9a1f7..997bbca 100644 --- a/src/course.rs +++ b/src/course.rs @@ -138,6 +138,7 @@ fn parse_issue(issue: &Issue) -> Result, Erro labels, title, html_url, + number, .. } = issue; @@ -209,6 +210,7 @@ fn parse_issue(issue: &Issue) -> Result, Erro title: title.clone(), html_url: html_url.clone(), optionality, + assignment_issue_id: *number, }), "Codility" => { // TODO: Handle these. @@ -310,6 +312,7 @@ pub enum Assignment { ExpectedPullRequest { title: String, html_url: Url, + assignment_issue_id: u64, optionality: AssignmentOptionality, }, } @@ -436,6 +439,7 @@ impl TraineeWithSubmissions { SubmissionState::Some(Submission::PullRequest { pull_request, optionality, + .. }) => { let max = match optionality { AssignmentOptionality::Mandatory => 10, @@ -537,6 +541,7 @@ pub enum Submission { PullRequest { pull_request: Pr, optionality: AssignmentOptionality, + assignment_issue_id: u64, }, } @@ -930,6 +935,7 @@ fn match_pr_to_assignment( sprint_index: usize, assignment_index: usize, optionality: AssignmentOptionality, + assignment_issue_id: u64, } let mut best_match: Option = None; @@ -950,6 +956,7 @@ fn match_pr_to_assignment( Assignment::ExpectedPullRequest { title: expected_title, optionality, + assignment_issue_id, .. } => { let mut assignment_title_words = make_title_more_matchable(expected_title); @@ -974,6 +981,7 @@ fn match_pr_to_assignment( sprint_index, assignment_index, optionality: optionality.clone(), + assignment_issue_id: *assignment_issue_id, }); } } @@ -981,10 +989,12 @@ fn match_pr_to_assignment( } } } + if let Some(Match { sprint_index, assignment_index, optionality, + assignment_issue_id, .. }) = best_match { @@ -992,12 +1002,33 @@ fn match_pr_to_assignment( SubmissionState::Some(Submission::PullRequest { pull_request: pr, optionality, + assignment_issue_id, }); } else if !pr.is_closed { unknown_prs.push(pr); } } +// Given a vector of sprints, and a target pr number, for a given person +// return the issue ID for the associated assignment descriptor +pub fn get_descriptor_id_for_pr( + sprints: &[SprintWithSubmissions], + target_pr_number: u64, +) -> Option { + sprints + .iter() + .flat_map(|sprint_with_subs| sprint_with_subs.submissions.iter()) + .filter_map(|submission_state| match submission_state { + SubmissionState::Some(Submission::PullRequest { + pull_request, + assignment_issue_id, + .. + }) if pull_request.number == target_pr_number => Some(*assignment_issue_id), + _ => None, + }) + .next() +} + fn make_title_more_matchable(title: &str) -> IndexSet { use itertools::Itertools; diff --git a/src/octocrab.rs b/src/octocrab.rs index 480cb45..b05dc52 100644 --- a/src/octocrab.rs +++ b/src/octocrab.rs @@ -81,7 +81,7 @@ pub fn octocrab_for_token(token: String) -> Result { Ok(octocrab) } -pub(crate) async fn all_pages( +pub async fn all_pages( description: &str, octocrab: &Octocrab, func: impl AsyncFnOnce() -> Result, octocrab::Error>,