Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update the reviewer bot to notify based on draft reopen time in addition to event time #12428

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update the reviewer bot to notify based on draft reopen time in addition to event time #12428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9096db5
Update the reviewer bot to notify based on draft reopen time in addit…
ScottSuarez Nov 23, 2024
99b4291
fix test case logic to more align with teststep name
ScottSuarez Nov 25, 2024
4693b9f
handle dismissed reviews gracefully
ScottSuarez Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 64 additions & 24 deletions .ci/magician/cmd/scheduled_pr_reminders.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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{}{}
Expand All @@ -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
}
Comment on lines 353 to +364
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic here isn't quite right - for example, earliestChangesRequested could end up holding a change request from before the PR was reopened, which would cause the func to return early - potentially missing an earliestApproved that comes after the PR was reopened.

I think that the reviews need additional filtering to skip these events instead.

Copy link
Contributor Author

@ScottSuarez ScottSuarez Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only consider the most recent review from a core reviewer. See the case:switch on line 340.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior I described does theoretically exist - here's a test case that demonstrates it:

		"changes_requested before ready_for_review with approved after": {
			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)},
				},
				&github.PullRequestReview{
					User:        &github.User{Login: github.String(secondCoreReviewer)},
					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),
		},

However, after thinking it over, I think the behavior you have here makes sense... if a change was requested, and then the PR was closed, left for a long time, and then reopened (somehow without a review being requested) we'd want to keep it in "Changes requested" but just reset the clock. (But actually ready_for_review requests a new review, so the only case that can actually happen here is that a review was requested and we reset the clock for when the review should have started.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think there actually is an issue here with dismissed reviews not clearing an earlier changes_requested.

Theoretically the new reviewer should dismiss the old reviewer if their feedback is no-longer applicable and/or they can no longer approve.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an issue with dismissed reviews that was not already present? I don't remember exactly why I implemented this as is; I'm happy to change the behavior around dismissed reviews but I'd rather hold it for a separate PR unless there's a specific impact to dismissed reviews from this work, since it may have additional implications.

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).
Expand Down
217 changes: 217 additions & 0 deletions .ci/magician/cmd/scheduled_pr_reminders_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading