Skip to content

Commit 8ad4b37

Browse files
authored
fix: use case-insensitive string comparison for fast-track approvals (#658)
I noticed today that a PR that had sufficient fast-track approvals was being rejected as needing one more approval. This is because one of the approvers' had a GitHub handle where the case in the README (`linkgoron`) did not match the case returned by GitHub (`Linkgoron`). Since GitHub handles are case-insensitive, this change makes the comparison case-insensitive. One might be tempted to think that we need to do the same for the list of TSC members, but handles are never compared there so it is not necessary.
1 parent beda17c commit 8ad4b37

File tree

4 files changed

+68
-2
lines changed

4 files changed

+68
-2
lines changed

lib/pr_checker.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -177,11 +177,11 @@ export default class PRChecker {
177177
}
178178
const [, requester] = comment.bodyText.match(FAST_TRACK_RE);
179179
const collaborators = Array.from(this.data.collaborators.values(),
180-
(c) => c.login);
180+
(c) => c.login.toLowerCase());
181181
const approvals = comment.reactions.nodes.filter((r) =>
182182
r.user.login !== requester &&
183183
r.user.login !== pr.author.login &&
184-
collaborators.includes(r.user.login)).length;
184+
collaborators.includes(r.user.login.toLowerCase())).length;
185185

186186
if (requester === pr.author.login && approvals < 2) {
187187
cli.error('The fast-track request requires' +
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
[
2+
{
3+
"publishedAt": "2017-10-22T05:16:36.458Z",
4+
"bodyText": "Fast-track has been requested by @bar. Please 👍 to approve.",
5+
"author": {
6+
"login": "github-actions"
7+
},
8+
"reactions": {
9+
"nodes": [
10+
{ "user": { "login": "Bar" } },
11+
{ "user": { "login": "Foo" } },
12+
{ "user": { "login": "BAZ" } }
13+
]
14+
}
15+
}
16+
]

test/fixtures/data.js

+2
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ export const requestingChangesReviews =
3737
export const commentsWithFastTrack = readJSON('comments_with_fast_track.json');
3838
export const commentsWithTwoFastTrack =
3939
readJSON('comments_with_two_fast_track.json');
40+
export const commentsWithTwoFastTrackDifferentCase =
41+
readJSON('comments_with_two_fast_track_different_case.json');
4042
export const commentsWithFastTrackInsuffientApprovals =
4143
readJSON('comments_with_fast_track_insufficient_approvals.json');
4244
export const commentsWithCI = readJSON('comments_with_ci.json');

test/unit/pr_checker.test.js

+48
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
noReviewers,
1919
commentsWithFastTrack,
2020
commentsWithTwoFastTrack,
21+
commentsWithTwoFastTrackDifferentCase,
2122
commentsWithFastTrackInsuffientApprovals,
2223
commentsWithCI,
2324
commentsWithFailedCI,
@@ -582,6 +583,53 @@ describe('PRChecker', () => {
582583
cli.assertCalledWith(expectedLogs);
583584
});
584585

586+
it('should compare collaborator handles as case-insensitive', () => {
587+
const cli = new TestCLI();
588+
589+
const expectedLogs = {
590+
ok:
591+
[['Approvals: 4'],
592+
['- Foo User (@foo): https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'],
593+
['- Quux User (@Quux): LGTM'],
594+
['- Baz User (@Baz): https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236'],
595+
['- Bar User (@bar) (TSC): lgtm']],
596+
info:
597+
[['This PR was created on Fri, 30 Nov 2018 17:50:44 GMT'],
598+
['This PR is being fast-tracked']]
599+
};
600+
601+
const pr = Object.assign({}, firstTimerPR, {
602+
author: {
603+
login: 'bar'
604+
},
605+
createdAt: LT_48H,
606+
labels: {
607+
nodes: [
608+
{ name: 'fast-track' }
609+
]
610+
}
611+
});
612+
613+
const data = {
614+
pr,
615+
reviewers: allGreenReviewers,
616+
comments: commentsWithTwoFastTrackDifferentCase,
617+
reviews: approvingReviews,
618+
commits: [],
619+
collaborators,
620+
authorIsNew: () => true,
621+
getThread() {
622+
return PRData.prototype.getThread.call(this);
623+
}
624+
};
625+
const checker = new PRChecker(cli, data, {}, argv);
626+
627+
cli.clearCalls();
628+
const status = checker.checkReviewsAndWait(new Date(NOW));
629+
assert(status);
630+
cli.assertCalledWith(expectedLogs);
631+
});
632+
585633
it('should error with 1 fast-track approval from the pr author', () => {
586634
const cli = new TestCLI();
587635

0 commit comments

Comments
 (0)