diff --git a/.ci/magician/cmd/scheduled_pr_reminders.go b/.ci/magician/cmd/scheduled_pr_reminders.go index 884b494c806a..df1f6cd64ac4 100644 --- a/.ci/magician/cmd/scheduled_pr_reminders.go +++ b/.ci/magician/cmd/scheduled_pr_reminders.go @@ -267,29 +267,20 @@ func (s pullRequestReviewState) String() string { // We don't specially handle cases where the contributor has "acted" because it would be // significant additional effort, and this case is already handled by re-requesting review // automatically based on contributor actions. -func notificationState(pr *github.PullRequest, issueEvents []*github.IssueEvent, reviews []*github.PullRequestReview) (pullRequestReviewState, time.Time, error) { - slices.SortFunc(issueEvents, func(a, b *github.IssueEvent) int { - if a.CreatedAt.Before(*b.CreatedAt.GetTime()) { - return 1 +func notificationState(pr *github.PullRequest, issueEventsDesc []*github.IssueEvent, reviewsDesc []*github.PullRequestReview) (pullRequestReviewState, time.Time, error) { + issueEventsDesc = sortedEventsDesc(issueEventsDesc) + reviewsDesc = sortedReviewsDesc(reviewsDesc) + + var readyForReviewTime = *pr.CreatedAt.GetTime() + for _, event := range issueEventsDesc { + if "ready_for_review" == *event.Event { + readyForReviewTime = maxTime(*event.CreatedAt.GetTime(), readyForReviewTime) } - if a.CreatedAt.After(*b.CreatedAt.GetTime()) { - return -1 - } - return 0 - }) - slices.SortFunc(reviews, func(a, b *github.PullRequestReview) int { - if a.SubmittedAt.Before(*b.SubmittedAt.GetTime()) { - return 1 - } - if a.SubmittedAt.After(*b.SubmittedAt.GetTime()) { - return -1 - } - return 0 - }) + } var latestReviewRequest *github.IssueEvent removedRequests := map[string]struct{}{} - for _, event := range issueEvents { + for _, event := range issueEventsDesc { if *event.Event == "review_request_removed" && event.RequestedReviewer != nil { removedRequests[*event.RequestedReviewer.Login] = struct{}{} continue @@ -320,7 +311,8 @@ func notificationState(pr *github.PullRequest, issueEvents []*github.IssueEvent, var earliestCommented *github.PullRequestReview ignoreBy := map[string]struct{}{} - for _, review := range reviews { + dismissedReviewer := map[string]struct{}{} + for _, review := range reviewsDesc { if review.SubmittedAt.Before(*latestReviewRequest.CreatedAt.GetTime()) { break } @@ -340,12 +332,16 @@ func notificationState(pr *github.PullRequest, issueEvents []*github.IssueEvent, switch *review.State { case "DISMISSED": // ignore dismissed reviews + dismissedReviewer[reviewer] = struct{}{} continue case "APPROVED": earliestApproved = review // ignore all earlier reviews from this reviewer ignoreBy[reviewer] = struct{}{} case "CHANGES_REQUESTED": + if _, ok := dismissedReviewer[reviewer]; ok { + continue + } earliestChangesRequested = review // ignore all earlier reviews from this reviewer ignoreBy[reviewer] = struct{}{} @@ -355,15 +351,59 @@ func notificationState(pr *github.PullRequest, issueEvents []*github.IssueEvent, } if earliestChangesRequested != nil { - return waitingForContributor, *earliestChangesRequested.SubmittedAt.GetTime(), nil + timeState := maxTime(*earliestChangesRequested.SubmittedAt.GetTime(), readyForReviewTime) + return waitingForContributor, timeState, nil } if earliestApproved != nil { - return waitingForMerge, *earliestApproved.SubmittedAt.GetTime(), nil + timeState := maxTime(*earliestApproved.SubmittedAt.GetTime(), readyForReviewTime) + return waitingForMerge, timeState, nil } if earliestCommented != nil { - return waitingForContributor, *earliestCommented.SubmittedAt.GetTime(), nil + timeState := maxTime(*earliestCommented.SubmittedAt.GetTime(), readyForReviewTime) + return waitingForContributor, timeState, nil + } + timeState := maxTime(*latestReviewRequest.CreatedAt.GetTime(), readyForReviewTime) + return waitingForReview, timeState, nil +} + +// compareTimeDesc returns sort ordering for descending time comparison (newest first) +func compareTimeDesc(a, b time.Time) int { + if a.Before(b) { + return 1 + } + if a.After(b) { + return -1 + } + return 0 +} + +// sortedEventsDesc returns events sorted by creation time, newest first +func sortedEventsDesc(events []*github.IssueEvent) []*github.IssueEvent { + sortedEvents := make([]*github.IssueEvent, len(events)) + copy(sortedEvents, events) + slices.SortFunc(sortedEvents, func(a, b *github.IssueEvent) int { + return compareTimeDesc(*a.CreatedAt.GetTime(), *b.CreatedAt.GetTime()) + }) + return sortedEvents +} + +// sortedReviewsDesc returns reviews sorted by submission time, newest first +func sortedReviewsDesc(reviews []*github.PullRequestReview) []*github.PullRequestReview { + sortedReviews := make([]*github.PullRequestReview, len(reviews)) + copy(sortedReviews, reviews) + slices.SortFunc(sortedReviews, func(a, b *github.PullRequestReview) int { + return compareTimeDesc(*a.SubmittedAt.GetTime(), *b.SubmittedAt.GetTime()) + }) + return sortedReviews +} + +// maxTime - returns whichever time occured after the other +func maxTime(time1, time2 time.Time) time.Time { + // Return the later time + if time1.After(time2) { + return time1 } - return waitingForReview, *latestReviewRequest.CreatedAt.GetTime(), nil + return time2 } // Calculates the number of PDT days between from and to (by calendar date, not # of hours). diff --git a/.ci/magician/cmd/scheduled_pr_reminders_test.go b/.ci/magician/cmd/scheduled_pr_reminders_test.go index 1777130cc623..c7cfdf49b52e 100644 --- a/.ci/magician/cmd/scheduled_pr_reminders_test.go +++ b/.ci/magician/cmd/scheduled_pr_reminders_test.go @@ -442,6 +442,223 @@ func TestNotificationState(t *testing.T) { expectState: waitingForMerge, expectSince: time.Date(2024, 2, 1, 0, 0, 0, 0, time.UTC), }, + "ready_for_review event after creation": { + pullRequest: &github.PullRequest{ + User: &github.User{Login: github.String("author")}, + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + issueEvents: []*github.IssueEvent{ + &github.IssueEvent{ + Event: github.String("review_requested"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)}, + RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)}, + }, + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, + }, + }, + expectState: waitingForReview, + expectSince: time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC), + }, + + "ready_for_review event before review request": { + pullRequest: &github.PullRequest{ + User: &github.User{Login: github.String("author")}, + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + issueEvents: []*github.IssueEvent{ + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)}, + }, + &github.IssueEvent{ + Event: github.String("review_requested"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, + RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)}, + }, + }, + expectState: waitingForReview, + expectSince: time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC), + }, + + "review after ready_for_review": { + pullRequest: &github.PullRequest{ + User: &github.User{Login: github.String("author")}, + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + issueEvents: []*github.IssueEvent{ + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)}, + }, + &github.IssueEvent{ + Event: github.String("review_requested"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, + RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)}, + }, + }, + reviews: []*github.PullRequestReview{ + &github.PullRequestReview{ + User: &github.User{Login: github.String(firstCoreReviewer)}, + State: github.String("APPROVED"), + SubmittedAt: &github.Timestamp{time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC)}, + }, + }, + expectState: waitingForMerge, + expectSince: time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC), + }, + "changes_requested after ready_for_review": { + pullRequest: &github.PullRequest{ + User: &github.User{Login: github.String("author")}, + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + issueEvents: []*github.IssueEvent{ + &github.IssueEvent{ + Event: github.String("review_requested"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)}, // Earlier request + RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)}, + }, + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, + }, + }, + reviews: []*github.PullRequestReview{ + &github.PullRequestReview{ + User: &github.User{Login: github.String(firstCoreReviewer)}, + State: github.String("CHANGES_REQUESTED"), + SubmittedAt: &github.Timestamp{time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC)}, // Early changes requested + }, + }, + expectState: waitingForContributor, + expectSince: time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC), // Should use ready_for_review time + }, + + "changes_requested before ready_for_review": { + pullRequest: &github.PullRequest{ + User: &github.User{Login: github.String("author")}, + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + issueEvents: []*github.IssueEvent{ + &github.IssueEvent{ + Event: github.String("review_requested"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)}, + }, + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, // Earlier ready + }, + }, + reviews: []*github.PullRequestReview{ + &github.PullRequestReview{ + User: &github.User{Login: github.String(firstCoreReviewer)}, + State: github.String("CHANGES_REQUESTED"), + SubmittedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)}, + }, + }, + expectState: waitingForContributor, + expectSince: time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC), // Should use changes requested time since it's later + }, + + "comment review after ready_for_review": { + pullRequest: &github.PullRequest{ + User: &github.User{Login: github.String("author")}, + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + issueEvents: []*github.IssueEvent{ + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + &github.IssueEvent{ + Event: github.String("review_requested"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)}, + RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)}, + }, + }, + reviews: []*github.PullRequestReview{ + &github.PullRequestReview{ + User: &github.User{Login: github.String(firstCoreReviewer)}, + State: github.String("COMMENTED"), + SubmittedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, + }, + }, + expectState: waitingForContributor, + expectSince: time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC), // Should use ready_for_review time + }, + + "multiple ready_for_review events": { + pullRequest: &github.PullRequest{ + User: &github.User{Login: github.String("author")}, + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + issueEvents: []*github.IssueEvent{ + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)}, + }, + &github.IssueEvent{ + Event: github.String("review_requested"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, + RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)}, + }, + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 5, 0, 0, 0, 0, time.UTC)}, // Later ready_for_review + }, + }, + reviews: []*github.PullRequestReview{ + &github.PullRequestReview{ + User: &github.User{Login: github.String(firstCoreReviewer)}, + State: github.String("CHANGES_REQUESTED"), + SubmittedAt: &github.Timestamp{time.Date(2024, 1, 4, 0, 0, 0, 0, time.UTC)}, + }, + }, + expectState: waitingForContributor, + expectSince: time.Date(2024, 1, 5, 0, 0, 0, 0, time.UTC), // Should use latest ready_for_review time + }, + "if reviewer dismissed, should ignore earlier review": { + pullRequest: &github.PullRequest{ + User: &github.User{Login: github.String("author")}, + CreatedAt: &github.Timestamp{time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)}, + }, + issueEvents: []*github.IssueEvent{ + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 2, 0, 0, 0, 0, time.UTC)}, + }, + &github.IssueEvent{ + Event: github.String("review_requested"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 3, 0, 0, 0, 0, time.UTC)}, + RequestedReviewer: &github.User{Login: github.String(firstCoreReviewer)}, + }, + &github.IssueEvent{ + Event: github.String("ready_for_review"), + CreatedAt: &github.Timestamp{time.Date(2024, 1, 5, 0, 0, 0, 0, time.UTC)}, // Later ready_for_review + }, + }, + reviews: []*github.PullRequestReview{ + &github.PullRequestReview{ + User: &github.User{Login: github.String(firstCoreReviewer)}, + State: github.String("CHANGES_REQUESTED"), + SubmittedAt: &github.Timestamp{time.Date(2024, 1, 6, 0, 0, 0, 0, time.UTC)}, + }, + &github.PullRequestReview{ + User: &github.User{Login: github.String(firstCoreReviewer)}, + State: github.String("DISMISSED"), + SubmittedAt: &github.Timestamp{time.Date(2024, 1, 7, 0, 0, 0, 0, time.UTC)}, + }, + &github.PullRequestReview{ + User: &github.User{Login: github.String(secondCoreReviewer)}, + State: github.String("APPROVED"), + SubmittedAt: &github.Timestamp{time.Date(2024, 1, 9, 0, 0, 0, 0, time.UTC)}, + }, + }, + expectState: waitingForMerge, + expectSince: time.Date(2024, 1, 9, 0, 0, 0, 0, time.UTC), // Should use latest ready_for_review time + }, } for tn, tc := range cases {