Fix review dismissals bug #485

devinburnette · 2022-10-13T22:47:35Z

Closes #480

In #463 we introduced a feature to dismiss stale reviews, while iterating on it some additional logic slipped in to also dismiss reviews that didn't match review patterns if people were using the github_review_comment_patterns field in their policies. This worked well until a predicate was included in a policy causing certain reviews to be skipped from evaluation and therefore not added to the list of approved candidates. This manifested in buggy/broken behavior with a misleading dismissal reason until the predicate was satisfied.

After deliberating in the issue above, I think we agreed that the dismissal of non-matching patterns is just a little too heavy handed and doesn't really fit well with dismissing "stale" reviews. In other words, if we aren't also dismissing all other reviews that can never satisfy a rule under any condition than the feature seems broken. So this PR removes dismissal of non-matching patterns, but keeps dismissal for both edited and invalidated reviews, classifying those as "stale" if people have the ignore_edited_comments or invalidate_on_push fields set to true in their policies. In addition, this PR also highlights which patterns will satisfy which rules on the details page in hopes to clear up confusion for folks using approval comments.

devinburnette · 2022-10-13T22:49:50Z

policy/approval/approve.go

@@ -111,7 +111,8 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 	res.Name = r.Name
 	res.Description = r.Description
 	res.Status = common.StatusSkipped
-	res.Requires = r.Requires.Actors
+	res.Requires = common.Requires{Count: r.Requires.Count, Actors: r.Requires.Actors}


this felt a little funny, but i kept getting cyclical dependency issues when just trying to use the existing approval.Requires

Moving types to common is normal as they end up used in common.Result, so I think this is fine.

devinburnette · 2022-10-13T22:55:26Z

server/handler/frontend.go

-		"getRequires": func(results *common.Result) map[string][]Membership {
-			return getRequires(results, strings.TrimSuffix(githubURL, "/"))
+		"hasPatterns": func(results *common.Result) bool {
+			return !results.Methods.IsEmpty()


there's one thing I think I'm missing here and that's what if a github review approval (without comments) would satisfy the policy just fine? I think i can make it only show the Patterns if github_review: true and github_review_comment_patterns is not set.

but then it's like the logic is slipping into the frontend a little bit.

What if instead of thinking of this as patterns-only, we show all methods? I think that removes the need for hasPatterns (every rule will have methods) and then we can change getPatterns to getMethods and update the template accordingly (see later comments)

devinburnette · 2022-10-14T14:57:37Z

policy/common/methods.go

@@ -179,3 +179,7 @@ func (m *Methods) BodyMatches(prBody string) bool {
 	}
 	return false
 }
+
+func (m *Methods) IsEmpty() bool {
+	return m == nil || (len(m.Comments) == 0 && len(m.CommentPatterns) == 0 && len(m.GithubReviewCommentPatterns) == 0 && len(m.BodyPatterns) == 0)


oh i can just put it here, and maybe call this something different.

I think this also needs to check that m.GithubReview is non-nil to account for all fields.

yea i realized that, but with the new approach, i think we don't need it anymore.

bluekeyes

Thanks for fixing this and expanding the details view! I had some suggestions on the UI part, but the rest of the change looks good.

bluekeyes · 2022-10-14T21:48:25Z

policy/approval/approve.go

@@ -111,7 +111,8 @@ func (r *Rule) Evaluate(ctx context.Context, prctx pull.Context) (res common.Res
 	res.Name = r.Name
 	res.Description = r.Description
 	res.Status = common.StatusSkipped
-	res.Requires = r.Requires.Actors
+	res.Requires = common.Requires{Count: r.Requires.Count, Actors: r.Requires.Actors}


Moving types to common is normal as they end up used in common.Result, so I think this is fine.

bluekeyes · 2022-10-14T21:49:33Z

policy/common/result.go

@@ -59,14 +59,20 @@ type ReviewRequestRule struct {
 	Mode RequestMode
 }

+type Requires struct {


Instead of duplicating Requires in this package, I think it might be better to move approval.Requires to this package and then fix the references in the approval packages. That should reduce confusion between the two types in the future since they're otherwise the same.

bluekeyes · 2022-10-14T21:52:03Z

policy/common/methods.go

@@ -179,3 +179,7 @@ func (m *Methods) BodyMatches(prBody string) bool {
 	}
 	return false
 }
+
+func (m *Methods) IsEmpty() bool {
+	return m == nil || (len(m.Comments) == 0 && len(m.CommentPatterns) == 0 && len(m.GithubReviewCommentPatterns) == 0 && len(m.BodyPatterns) == 0)


I think this also needs to check that m.GithubReview is non-nil to account for all fields.

server/handler/frontend.go

bluekeyes · 2022-10-14T22:01:13Z

server/handler/frontend.go

-		"getRequires": func(results *common.Result) map[string][]Membership {
-			return getRequires(results, strings.TrimSuffix(githubURL, "/"))
+		"hasPatterns": func(results *common.Result) bool {
+			return !results.Methods.IsEmpty()


What if instead of thinking of this as patterns-only, we show all methods? I think that removes the need for hasPatterns (every rule will have methods) and then we can change getPatterns to getMethods and update the template accordingly (see later comments)

bluekeyes · 2022-10-14T22:07:26Z

server/templates/details.html.tmpl

          <div class="pt-2">
-          {{template "result-requires-permissions-details" .}}
+          {{template "result-patterns-details" .}}


I think it would make sense to reorder these templates so it reads something like:

This rule needs N approvals from people in these groups or people with these permissions. Approvals can use these patterns.

As currently structured, it kind of implies that the sections are independent so that the pattern section doesn't also require the approvers have certain team membership/permissions.

bluekeyes · 2022-10-14T22:10:22Z

server/templates/details.html.tmpl

-{{define "result-requires-details"}}
-  <b class="font-bold text-sm">This rule requires review from</b>
+{{define "result-patterns-details"}}
+  <b class="font-bold text-sm">This rule requires {{.Requires.Count}} review(s) using any of these patterns</b>


Good call adding the count to all of these - I'm not sure why we didn't add it earlier. I think it would be better to only show the count once before all the other approval details. With the count repeated, people could interpret it as if they need N reviews for each section instead of N reviews total for the rule.

yea that's fair, i was trying to play with the wording so that wouldn't be the case, but I think this is a better idea.

devinburnette · 2022-10-15T01:00:13Z

server/handler/frontend.go

+			}
+		} else {
+			patternInfo["Github Review State"] = append(patternInfo["Github Review State"], string(result.Methods.GithubReviewState))
+		}


this is the only part where i feel like the logic slipped into the view a little, but i thought the result would be more helpful for humans to read.

Disapproval does not support counts, so using the common type is misleading. Add an new comment explaining this.

This shouldn't be nil in practice, but would prefer to not panic over this if something changes in the future.

bluekeyes

Thanks for addressing the comments and sorry about the delayed review, this looks good to merge

devinburnette added 2 commits October 13, 2022 18:25

only dismiss edited or invalidated reviews

05ec17b

add patterns to details page

6168933

devinburnette changed the title ~~Devin fix dismiss bug~~ Fix review dismissals bug Oct 13, 2022

devinburnette commented Oct 13, 2022

View reviewed changes

devinburnette commented Oct 14, 2022

View reviewed changes

bluekeyes reviewed Oct 14, 2022

View reviewed changes

implement feedback

384abc1

devinburnette force-pushed the devin-fix-dismiss-bug branch from d7530b6 to 384abc1 Compare October 15, 2022 00:53

devinburnette commented Oct 15, 2022

View reviewed changes

devinburnette requested a review from bluekeyes October 24, 2022 19:28

bluekeyes added 3 commits October 25, 2022 21:36

Restore custom Requires type in disapproval

c7acebb

Disapproval does not support counts, so using the common type is misleading. Add an new comment explaining this.

Add nil check

33bf77a

This shouldn't be nil in practice, but would prefer to not panic over this if something changes in the future.

Fix typo in field name

4661ff0

bluekeyes approved these changes Oct 26, 2022

View reviewed changes

bluekeyes merged commit 4690ee8 into palantir:develop Oct 26, 2022

devinburnette deleted the devin-fix-dismiss-bug branch October 29, 2022 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix review dismissals bug #485

Fix review dismissals bug #485

devinburnette commented Oct 13, 2022 •

edited

Loading

devinburnette Oct 13, 2022

bluekeyes Oct 14, 2022

devinburnette Oct 13, 2022

devinburnette Oct 14, 2022

bluekeyes Oct 14, 2022

devinburnette Oct 15, 2022

devinburnette Oct 14, 2022

bluekeyes Oct 14, 2022

devinburnette Oct 15, 2022

bluekeyes left a comment

bluekeyes Oct 14, 2022

bluekeyes Oct 14, 2022

bluekeyes Oct 14, 2022

bluekeyes Oct 14, 2022

bluekeyes Oct 14, 2022

bluekeyes Oct 14, 2022

devinburnette Oct 15, 2022

devinburnette Oct 15, 2022

bluekeyes left a comment

Fix review dismissals bug #485

Fix review dismissals bug #485

Conversation

devinburnette commented Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluekeyes left a comment

Choose a reason for hiding this comment

devinburnette commented Oct 13, 2022 •

edited

Loading