Skip to content

Commit 3a07014

Browse files
committed
feat: make reviewer-to-assignee bot dynamic with removal support
- Add pull_request_review: submitted trigger to on-review.yml so the workflow fires when a reviewer submits their feedback - Split the bot into addReviewersAsAssignees / removeReviewerFromAssignees functions, routing by eventName + action - Remove the MAX_ASSIGNEES=2 cap so all requested reviewers are assigned - Mirror the existing 403 guard on the new removal path - Update tests: drop the cap assertion, add removal flow coverage (happy path, no-op, 403, rethrow) and workflow_dispatch routing check Signed-off-by: Mounil Kanakhara <mounilkankhara@gmail.com>
1 parent 1b15756 commit 3a07014

3 files changed

Lines changed: 219 additions & 56 deletions

File tree

.github/scripts/__tests__/jest/bot-pr-add-reviewers-as-assignees.test.js

Lines changed: 134 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
1616

1717
const createTestState = () => ({
1818
addAssigneesCalls: [],
19+
removeAssigneesCalls: [],
1920
pullsGetCalls: 0,
2021
currentPrData: null,
2122
});
@@ -24,6 +25,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
2425
repo: { owner: 'hiero-ledger', repo: 'hiero-sdk-python' },
2526
eventName: 'pull_request_target',
2627
payload: {
28+
action: 'review_requested',
2729
pull_request: {
2830
number: 123,
2931
requested_reviewers: [],
@@ -34,6 +36,21 @@ describe('Bot: Add Reviewers as Assignees', () => {
3436
}
3537
});
3638

39+
const createMockReviewContext = ({ reviewerLogin, assignees = [] } = {}) => ({
40+
repo: { owner: 'hiero-ledger', repo: 'hiero-sdk-python' },
41+
eventName: 'pull_request_review',
42+
payload: {
43+
action: 'submitted',
44+
review: {
45+
user: { login: reviewerLogin }
46+
},
47+
pull_request: {
48+
number: 123,
49+
assignees
50+
}
51+
}
52+
});
53+
3754
const createMockGithub = (state) => ({
3855
rest: {
3956
pulls: {
@@ -53,11 +70,17 @@ describe('Bot: Add Reviewers as Assignees', () => {
5370
addAssignees: async (params) => {
5471
state.addAssigneesCalls.push(params);
5572
return { data: {} };
73+
},
74+
removeAssignees: async (params) => {
75+
state.removeAssigneesCalls.push(params);
76+
return { data: {} };
5677
}
5778
}
5879
}
5980
});
6081

82+
// ─── Add flow ────────────────────────────────────────────────────────────────
83+
6184
test('adds individual reviewers correctly as assignees', async () => {
6285
const state = createTestState();
6386
state.currentPrData = {
@@ -76,6 +99,23 @@ describe('Bot: Add Reviewers as Assignees', () => {
7699
expect(call.assignees.sort()).toEqual(['alice', 'bob']);
77100
});
78101

102+
test('adds all requested reviewers as assignees without cap', async () => {
103+
const state = createTestState();
104+
state.currentPrData = {
105+
requested_reviewers: [{ login: 'u1' }, { login: 'u2' }, { login: 'u3' }],
106+
assignees: []
107+
};
108+
109+
const ctx = createMockContext({
110+
requested_reviewers: [{ login: 'u1' }, { login: 'u2' }, { login: 'u3' }]
111+
});
112+
113+
await handler({ github: createMockGithub(state), context: ctx });
114+
115+
expect(state.addAssigneesCalls).toHaveLength(1);
116+
expect(state.addAssigneesCalls[0].assignees.sort()).toEqual(['u1', 'u2', 'u3']);
117+
});
118+
79119
test('ignores team reviewers', async () => {
80120
const state = createTestState();
81121
state.currentPrData = {
@@ -112,23 +152,6 @@ describe('Bot: Add Reviewers as Assignees', () => {
112152
expect(state.addAssigneesCalls).toHaveLength(0);
113153
});
114154

115-
test('respects MAX_ASSIGNEES = 2 cap', async () => {
116-
const state = createTestState();
117-
state.currentPrData = {
118-
requested_reviewers: [{ login: 'u1' }, { login: 'u2' }, { login: 'u3' }],
119-
assignees: []
120-
};
121-
122-
const ctx = createMockContext({
123-
requested_reviewers: [{ login: 'u1' }, { login: 'u2' }, { login: 'u3' }]
124-
});
125-
126-
await handler({ github: createMockGithub(state), context: ctx });
127-
128-
expect(state.addAssigneesCalls).toHaveLength(1);
129-
expect(state.addAssigneesCalls[0].assignees).toHaveLength(2);
130-
});
131-
132155
test('does nothing when no reviewers are requested', async () => {
133156
const state = createTestState();
134157
const ctx = createMockContext({ requested_reviewers: [] });
@@ -150,7 +173,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
150173
expect(state.addAssigneesCalls).toHaveLength(0);
151174
});
152175

153-
test('supports workflow_dispatch with pr_number input', async () => {
176+
test('supports workflow_dispatch with pr_number input and routes to add flow', async () => {
154177
const state = createTestState();
155178
state.currentPrData = { requested_reviewers: [{ login: 'eve' }], assignees: [] };
156179

@@ -164,6 +187,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
164187

165188
expect(state.addAssigneesCalls).toHaveLength(1);
166189
expect(state.addAssigneesCalls[0].issue_number).toBe(128);
190+
expect(state.removeAssigneesCalls).toHaveLength(0);
167191
});
168192

169193
test('handles invalid pr_number in workflow_dispatch', async () => {
@@ -183,7 +207,7 @@ describe('Bot: Add Reviewers as Assignees', () => {
183207
}
184208
});
185209

186-
test('gracefully handles 403 permission errors', async () => {
210+
test('gracefully handles 403 permission errors on add', async () => {
187211
const ctx = createMockContext({ requested_reviewers: [{ login: 'x' }] });
188212

189213
const errorMock = {
@@ -194,15 +218,16 @@ describe('Bot: Add Reviewers as Assignees', () => {
194218
const err = new Error('Forbidden');
195219
err.status = 403;
196220
throw err;
197-
}
221+
},
222+
removeAssignees: async () => {}
198223
}
199224
}
200225
};
201226

202227
await expect(handler({ github: errorMock, context: ctx })).resolves.not.toThrow();
203228
});
204229

205-
test('rethrows non-403 errors', async () => {
230+
test('rethrows non-403 errors on add', async () => {
206231
const ctx = createMockContext({ requested_reviewers: [{ login: 'x' }] });
207232

208233
const errorMock = {
@@ -213,6 +238,94 @@ describe('Bot: Add Reviewers as Assignees', () => {
213238
const err = new Error('Internal Server Error');
214239
err.status = 500;
215240
throw err;
241+
},
242+
removeAssignees: async () => {}
243+
}
244+
}
245+
};
246+
247+
await expect(handler({ github: errorMock, context: ctx })).rejects.toHaveProperty('status', 500);
248+
});
249+
250+
// ─── Remove flow ─────────────────────────────────────────────────────────────
251+
252+
test('removes reviewer from assignees when they submit a review', async () => {
253+
const state = createTestState();
254+
const ctx = createMockReviewContext({
255+
reviewerLogin: 'alice',
256+
assignees: [{ login: 'alice' }, { login: 'bob' }]
257+
});
258+
259+
await handler({ github: createMockGithub(state), context: ctx });
260+
261+
expect(state.removeAssigneesCalls).toHaveLength(1);
262+
expect(state.removeAssigneesCalls[0].assignees).toEqual(['alice']);
263+
expect(state.addAssigneesCalls).toHaveLength(0);
264+
});
265+
266+
test('does not call removeAssignees when reviewer is not an assignee', async () => {
267+
const state = createTestState();
268+
const ctx = createMockReviewContext({
269+
reviewerLogin: 'carol',
270+
assignees: [{ login: 'alice' }]
271+
});
272+
273+
await handler({ github: createMockGithub(state), context: ctx });
274+
275+
expect(state.removeAssigneesCalls).toHaveLength(0);
276+
expect(state.addAssigneesCalls).toHaveLength(0);
277+
});
278+
279+
test('review submitted by someone who was never a reviewer is a no-op', async () => {
280+
const state = createTestState();
281+
const ctx = createMockReviewContext({
282+
reviewerLogin: 'outsider',
283+
assignees: []
284+
});
285+
286+
await handler({ github: createMockGithub(state), context: ctx });
287+
288+
expect(state.removeAssigneesCalls).toHaveLength(0);
289+
});
290+
291+
test('gracefully handles 403 permission errors on remove', async () => {
292+
const ctx = createMockReviewContext({
293+
reviewerLogin: 'alice',
294+
assignees: [{ login: 'alice' }]
295+
});
296+
297+
const errorMock = {
298+
rest: {
299+
pulls: { get: async () => ({}) },
300+
issues: {
301+
addAssignees: async () => {},
302+
removeAssignees: async () => {
303+
const err = new Error('Forbidden');
304+
err.status = 403;
305+
throw err;
306+
}
307+
}
308+
}
309+
};
310+
311+
await expect(handler({ github: errorMock, context: ctx })).resolves.not.toThrow();
312+
});
313+
314+
test('rethrows non-403 errors on remove', async () => {
315+
const ctx = createMockReviewContext({
316+
reviewerLogin: 'alice',
317+
assignees: [{ login: 'alice' }]
318+
});
319+
320+
const errorMock = {
321+
rest: {
322+
pulls: { get: async () => ({}) },
323+
issues: {
324+
addAssignees: async () => {},
325+
removeAssignees: async () => {
326+
const err = new Error('Internal Server Error');
327+
err.status = 500;
328+
throw err;
216329
}
217330
}
218331
}

0 commit comments

Comments
 (0)