Fix review approval check ordering

auto_submit/lib/validations/approval.dart

@@ -72,6 +72,7 @@ class Approver {
   int _remainingReviews = 2;
   final Set<String?> _approvers = <String?>{};
   final Set<String?> _changeRequestAuthors = <String?>{};
+  final Set<String?> _reviewAuthors = <String?>{};
 
   bool get approved => _approved;
 
@@ -104,7 +105,12 @@ class Approver {
 
     final int targetReviewCount = _remainingReviews;
 
-    for (ReviewNode review in reviews) {
+    for (ReviewNode review in reviews.reversed) {
+      if (review.author!.login == author) {
+        log.info('Author cannot review own pull request.');
+        continue;
+      }
+
       // Ignore reviews from non-members/owners.
       if (!allowedReviewers.contains(review.authorAssociation)) {
         continue;
@@ -117,18 +123,19 @@ class Approver {
       // reviews and will keep them all so the same person can provide two
       // reviews and bypass the two review rule. We make an _approvers
       // contains check to make sure this does not happen.
-      if (state == APPROVED_STATE && !_approvers.contains(authorLogin)) {
+      if (state == APPROVED_STATE && !_reviewAuthors.contains(authorLogin)) {
         _approvers.add(authorLogin);
         if (_remainingReviews > 0) {
           _remainingReviews--;
         }
-        _changeRequestAuthors.remove(authorLogin);
-      } else if (state == CHANGES_REQUESTED_STATE) {
+      } else if (state == CHANGES_REQUESTED_STATE && !_reviewAuthors.contains(authorLogin)) {
+        _changeRequestAuthors.add(authorLogin);
         if (_remainingReviews < targetReviewCount) {
           _remainingReviews++;
         }
-        _changeRequestAuthors.add(authorLogin);
       }
+
+      _reviewAuthors.add(authorLogin);
     }
 
     _approved = (_approvers.length > 1) && _changeRequestAuthors.isEmpty;

auto_submit/test/validations/approval_test.dart

@@ -35,7 +35,7 @@ void main() {
       final Map<String, dynamic> queryResultJsonDecode = jsonDecode(review) as Map<String, dynamic>;
       final QueryResult queryResult = QueryResult.fromJson(queryResultJsonDecode);
       final gh.PullRequest pullRequest = generatePullRequest();
-      return await approval.validate(queryResult, pullRequest);
+      return approval.validate(queryResult, pullRequest);
     }
 
     test('Author is member and reviewer is a member, pr approved', () async {
@@ -253,5 +253,30 @@ void main() {
               'This PR has not met approval requirements for merging. You have project association CONTRIBUTOR and need 1 more review(s) in order to merge this PR.'),
           isTrue);
     });
+
+    test('Successful review overwrites previous changes requested.', () async {
+      final ValidationResult result = await computeValidationResult(multipleReviewsSameAuthor);
+
+      expect(result.result, isTrue);
+      expect(result.action, Action.REMOVE_LABEL);
+      expect(result.message.contains('This PR has met approval requirements for merging.'), isTrue);
+    });
+
+    test('Author cannot review own pr', () async {
+      final String review = constructTwoReviewerReview(
+        authorAuthorAssociation: 'MEMBER',
+        reviewerAuthorAssociation: 'MEMBER',
+        secondReviewerAuthorAssociation: 'NON_MEMBER',
+        reviewState: 'APPROVED',
+        secondReviewState: 'APPROVED',
+        author: 'author1',
+      );
+
+      final ValidationResult result = await computeValidationResult(review);
+
+      expect(result.result, isFalse);
+      expect(result.action, Action.REMOVE_LABEL);
+      expect(result.message.contains('This PR has not met approval requirements for merging. You have'), isTrue);
+    });
   });
 }

auto_submit/test/validations/approval_test_data.dart

@@ -61,8 +61,8 @@ String constructTwoReviewerReview({
   required String secondReviewerAuthorAssociation,
   required String reviewState,
   required String secondReviewState,
-  String author = 'author1',
-  String secondAuthor = 'author2',
+  String author = 'author2',
+  String secondAuthor = 'author3',
 }) {
   return '''
   {
@@ -189,3 +189,79 @@ String constructMultipleReviewerReview({
   }
   ''';
 }
+
+const String multipleReviewsSameAuthor = '''
+
+{
+    "repository": {
+      "pullRequest": {
+        "author": {
+          "login": "author1"
+        },
+        "authorAssociation": "MEMBER",
+        "id": "PR_kwDOA8VHis43rs4_",
+        "title": "[dependabot] Remove human reviewers",
+        "commits": {
+          "nodes":[
+            {
+              "commit": {
+                "abbreviatedOid": "4009ecc",
+                "oid": "4009ecc0b6dbf5cb19cb97472147063e7368ec10",
+                "committedDate": "2022-05-11T22:35:02Z",
+                "pushedDate": "2022-05-11T22:35:03Z",
+                "status": {
+                  "contexts":[
+                    {
+                      "context":"luci-flutter",
+                      "state":"SUCCESS",
+                      "targetUrl":"https://ci.example.com/1000/output"
+                    }
+                  ]
+                }
+              }
+            }
+          ]
+        },
+        "reviews": {
+          "nodes": [
+            {
+              "author": {
+                "login": "jmagman"
+              },
+              "state": "COMMENTED",
+              "authorAssociation": "MEMBER"
+            },
+            {
+              "author": {
+                "login": "keyonghan"
+              },
+              "state": "COMMENTED",
+              "authorAssociation": "MEMBER"
+            },
+            {
+              "author": {
+                "login": "jmagman"
+              },
+              "state": "APPROVED",
+              "authorAssociation": "MEMBER"
+            },
+            {
+              "author": {
+                "login": "jmagman"
+              },
+              "state": "CHANGES_REQUESTED",
+              "authorAssociation": "MEMBER"
+            },
+            {
+              "author": {
+                "login": "jmagman"
+              },
+              "state": "APPROVED",
+              "authorAssociation": "MEMBER"
+            }
+          ]
+        }
+      }
+    }
+  }
+''';