fix: do not apply approved label if change requested

pkg/plugins/approve/approve.go

@@ -590,15 +590,21 @@ func addApprovers(approversHandler *approvers.Approvers, approveComments []*comm
 			continue
 		}
 
-		if reviewActsAsApprove && c.ReviewState == github.ReviewStateApproved {
-			approversHandler.AddApprover(
-				c.Author,
-				c.HTMLURL,
-				false,
-			)
+		if c.ReviewState == github.ReviewStateApproved {
+			approversHandler.RemoveChangeRequested(c.Author)
+			if reviewActsAsApprove {
+				approversHandler.AddApprover(
+					c.Author,
+					c.HTMLURL,
+					false,
+				)
+			}
 		}
-		if reviewActsAsApprove && c.ReviewState == github.ReviewStateChangesRequested {
-			approversHandler.RemoveApprover(c.Author)
+		if c.ReviewState == github.ReviewStateChangesRequested {
+			approversHandler.AddChangeRequested(c.Author)
+			if reviewActsAsApprove {
+				approversHandler.RemoveApprover(c.Author)
+			}
 		}
 
 		for _, match := range commandRegex.FindAllStringSubmatch(c.Body, -1) {

pkg/plugins/approve/approve_test.go

@@ -1054,11 +1054,14 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 <!-- META={"approvers":["alice","cjwagner"]} -->`,
 		},
 		{
-			name:                "approve command supersedes simultaneous changes requested review",
-			hasLabel:            false,
-			files:               []string{"a/a.go"},
-			comments:            []github.IssueComment{},
-			reviews:             []github.Review{newTestReview("Alice", "/approve", github.ReviewStateChangesRequested)},
+			name:     "approve command supersedes prior changes requested review",
+			hasLabel: false,
+			files:    []string{"a/a.go"},
+			comments: []github.IssueComment{},
+			reviews: []github.Review{
+				newTestReview("Alice", "/approve", github.ReviewStateChangesRequested),
+				newTestReview("Alice", "/approve", github.ReviewStateApproved),
+			},
 			selfApprove:         false,
 			needsIssue:          false,
 			lgtmActsAsApprove:   false,
@@ -1081,6 +1084,41 @@ Needs approval from an approver in each of these files:
 
 - ~~[a/OWNERS](https://github.com/org/repo/blob/master/a/OWNERS)~~ [Alice]
 
+Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
+Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
+</details>
+<!-- META={"approvers":[]} -->`,
+		},
+		{
+			name:     "changes requested supersedes approve command",
+			hasLabel: false,
+			files:    []string{"a/a.go"},
+			comments: []github.IssueComment{},
+			reviews: []github.Review{
+				newTestReview("Alice", "/approve", github.ReviewStateChangesRequested),
+			},
+			selfApprove:         false,
+			needsIssue:          false,
+			lgtmActsAsApprove:   false,
+			reviewActsAsApprove: true,
+			githubLinkURL:       &url.URL{Scheme: "https", Host: "github.com"},
+
+			expectDelete:  false,
+			expectToggle:  false,
+			expectComment: true,
+			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
+
+This pull-request has been approved by: *<a href="" title="Approved">Alice</a>*
+
+The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=org%2Frepo).
+
+The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)
+
+<details >
+Needs approval from an approver in each of these files:
+
+- ~~[a/OWNERS](https://github.com/org/repo/blob/master/a/OWNERS)~~ [Alice]
+
 Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
 Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
 </details>

pkg/plugins/approve/approvers/owners.go

@@ -289,6 +289,7 @@ func (a Approval) String() string {
 type Approvers struct {
 	owners          Owners
 	approvers       map[string]Approval // The keys of this map are normalized to lowercase.
+	changeRequested map[string]bool     // CHANGE_REQUESTED from any reviewer prevents submission
 	assignees       sets.Set[string]
 	AssociatedIssue int
 	RequireIssue    bool
@@ -316,9 +317,10 @@ func CaseInsensitiveIntersection(one, other sets.Set[string]) sets.Set[string] {
 // NewApprovers create a new "Approvers" with no approval.
 func NewApprovers(owners Owners) Approvers {
 	return Approvers{
-		owners:    owners,
-		approvers: map[string]Approval{},
-		assignees: sets.New[string](),
+		owners:          owners,
+		approvers:       map[string]Approval{},
+		changeRequested: map[string]bool{},
+		assignees:       sets.New[string](),
 
 		ManuallyApproved: func() bool {
 			return false
@@ -364,6 +366,11 @@ func (ap *Approvers) AddApprover(login, reference string, noIssue bool) {
 	}
 }
 
+// AddChangeRequested adds a new changeRequested
+func (ap *Approvers) AddChangeRequested(login string) {
+	ap.changeRequested[strings.ToLower(login)] = true
+}
+
 // AddAuthorSelfApprover adds the author self approval
 func (ap *Approvers) AddAuthorSelfApprover(login, reference string, noIssue bool) {
 	if ap.shouldNotOverrideApproval(login, noIssue) {
@@ -382,6 +389,11 @@ func (ap *Approvers) RemoveApprover(login string) {
 	delete(ap.approvers, strings.ToLower(login))
 }
 
+// RemoveChangeRequested removes a changeRequested from the map.
+func (ap *Approvers) RemoveChangeRequested(login string) {
+	delete(ap.changeRequested, strings.ToLower(login))
+}
+
 // AddAssignees adds assignees to the list
 func (ap *Approvers) AddAssignees(logins ...string) {
 	for _, login := range logins {
@@ -542,6 +554,12 @@ func (ap Approvers) GetCCs() []string {
 	return sets.List(suggested.Union(keepAssignees))
 }
 
+// AreChangesRequested returns a bool indicating whether changes are requested on the PR.
+// A PR cannot be submitted if there is an outstanding CHANGES_REQUESTED state.
+func (ap Approvers) AreChangesRequested() bool {
+	return len(ap.changeRequested) > 0
+}
+
 // AreFilesApproved returns a bool indicating whether or not OWNERS files associated with
 // the PR are approved.  A PR with no OWNERS files is not considered approved. If this
 // returns true, the PR may still not be fully approved depending on the associated issue
@@ -557,7 +575,7 @@ func (ap Approvers) AreFilesApproved() bool {
 //   - that there is an associated issue with the PR
 //   - an OWNER has indicated that the PR is trivial enough that an issue need not be associated with the PR
 func (ap Approvers) RequirementsMet() bool {
-	return ap.AreFilesApproved() && (!ap.RequireIssue || ap.AssociatedIssue != 0 || len(ap.NoIssueApprovers()) != 0)
+	return !ap.AreChangesRequested() && ap.AreFilesApproved() && (!ap.RequireIssue || ap.AssociatedIssue != 0 || len(ap.NoIssueApprovers()) != 0)
 }
 
 // IsApproved returns a bool indicating whether the PR is fully approved.