diff --git a/auto_submit/lib/validations/approval.dart b/auto_submit/lib/validations/approval.dart index ba415f36da..4ad31605a3 100644 --- a/auto_submit/lib/validations/approval.dart +++ b/auto_submit/lib/validations/approval.dart @@ -72,6 +72,7 @@ class Approver { int _remainingReviews = 2; final Set _approvers = {}; final Set _changeRequestAuthors = {}; + final Set _reviewAuthors = {}; 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; diff --git a/auto_submit/test/validations/approval_test.dart b/auto_submit/test/validations/approval_test.dart index eade5dc5af..21464305d6 100644 --- a/auto_submit/test/validations/approval_test.dart +++ b/auto_submit/test/validations/approval_test.dart @@ -35,7 +35,7 @@ void main() { final Map queryResultJsonDecode = jsonDecode(review) as Map; 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); + }); }); } diff --git a/auto_submit/test/validations/approval_test_data.dart b/auto_submit/test/validations/approval_test_data.dart index dc48fea09d..b39ee629bf 100644 --- a/auto_submit/test/validations/approval_test_data.dart +++ b/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" + } + ] + } + } + } + } +''';