Skip to content

Commit 400e9e9

Browse files
ArtyomSavchenkoArtyomSavchenko
authored
Fix null error in getReviewers (#10229)
* Fix null error in getReviewers Signed-off-by: Artem Savchenko <[email protected]> * Fix validate Signed-off-by: Artem Savchenko <[email protected]> * Fix code style Signed-off-by: Artem Savchenko <[email protected]> --------- Signed-off-by: Artem Savchenko <[email protected]>
1 parent 1150f0c commit 400e9e9

File tree

2 files changed

+246
-2
lines changed

2 files changed

+246
-2
lines changed

‎services/github/pod-github/src/sync/__tests__/pullrequests.test.ts‎

Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,238 @@
1+
/* eslint-disable import/first */
2+
import { PersonId } from '@hcengineering/core'
3+
import { PullRequestExternalData } from '../githubTypes'
4+
import type { UserInfo } from '../../types'
5+
6+
// Mock dependencies before imports
7+
jest.mock('@octokit/webhooks-types', () => ({}), { virtual: true })
8+
jest.mock('octokit', () => ({}), { virtual: true })
9+
jest.mock('../../config', () => ({
10+
default: {
11+
AccountsURL: 'http://localhost:3000',
12+
ServerSecret: 'test-secret',
13+
AppId: 'test-app-id',
14+
ClientId: 'test-client-id',
15+
ClientSecret: 'test-client-secret',
16+
PrivateKey: 'test-private-key',
17+
CollaboratorURL: 'http://localhost:3078',
18+
BotName: 'test-bot'
19+
}
20+
}))
21+
22+
import { PullRequestSyncManager } from '../pullrequests'
23+
/* eslint-enable import/first */
24+
25+
describe('PullRequestSyncManager', () => {
26+
describe('getReviewers', () => {
27+
let manager: PullRequestSyncManager
28+
let mockProvider: any
29+
30+
beforeEach(async () => {
31+
mockProvider = {
32+
getAccount: jest.fn()
33+
}
34+
manager = new PullRequestSyncManager(null as any, null as any, null as any)
35+
await manager.init(mockProvider)
36+
})
37+
38+
it('should handle null reviewRequests gracefully', async () => {
39+
const prData: Partial<PullRequestExternalData> = {
40+
reviewRequests: null as any,
41+
latestReviews: { nodes: [], totalCount: 0 }
42+
}
43+
44+
const result = await manager.getReviewers(prData as PullRequestExternalData)
45+
46+
expect(result).toEqual([])
47+
expect(mockProvider.getAccount).not.toHaveBeenCalled()
48+
})
49+
50+
it('should handle undefined reviewRequests gracefully', async () => {
51+
const prData: Partial<PullRequestExternalData> = {
52+
reviewRequests: undefined as any,
53+
latestReviews: { nodes: [], totalCount: 0 }
54+
}
55+
56+
const result = await manager.getReviewers(prData as PullRequestExternalData)
57+
58+
expect(result).toEqual([])
59+
expect(mockProvider.getAccount).not.toHaveBeenCalled()
60+
})
61+
62+
it('should handle null items in reviewRequests.nodes', async () => {
63+
const user1: UserInfo = { id: 'user1', login: 'user1' }
64+
const user2: UserInfo = { id: 'user2', login: 'user2' }
65+
const prData: Partial<PullRequestExternalData> = {
66+
reviewRequests: {
67+
nodes: [null, { requestedReviewer: user1 }, null, { requestedReviewer: user2 }] as any,
68+
totalCount: 4
69+
},
70+
latestReviews: { nodes: [], totalCount: 0 }
71+
}
72+
73+
mockProvider.getAccount.mockImplementation((user: UserInfo) => {
74+
return Promise.resolve(user.id as PersonId)
75+
})
76+
77+
const result = await manager.getReviewers(prData as PullRequestExternalData)
78+
79+
expect(result).toEqual(['user1', 'user2'])
80+
expect(mockProvider.getAccount).toHaveBeenCalledTimes(2)
81+
})
82+
83+
it('should handle null requestedReviewer in nodes', async () => {
84+
const user1: UserInfo = { id: 'user1', login: 'user1' }
85+
const prData: Partial<PullRequestExternalData> = {
86+
reviewRequests: {
87+
nodes: [{ requestedReviewer: null }, { requestedReviewer: user1 }, { requestedReviewer: null }] as any,
88+
totalCount: 3
89+
},
90+
latestReviews: { nodes: [], totalCount: 0 }
91+
}
92+
93+
mockProvider.getAccount.mockImplementation((user: UserInfo) => {
94+
return Promise.resolve(user.id as PersonId)
95+
})
96+
97+
const result = await manager.getReviewers(prData as PullRequestExternalData)
98+
99+
expect(result).toEqual(['user1'])
100+
expect(mockProvider.getAccount).toHaveBeenCalledTimes(1)
101+
})
102+
103+
it('should filter out reviewers when getAccount returns undefined', async () => {
104+
const user1: UserInfo = { id: 'user1', login: 'user1' }
105+
const user2: UserInfo = { id: 'user2', login: 'user2' }
106+
const user3: UserInfo = { id: 'user3', login: 'user3' }
107+
const prData: Partial<PullRequestExternalData> = {
108+
reviewRequests: {
109+
nodes: [{ requestedReviewer: user1 }, { requestedReviewer: user2 }, { requestedReviewer: user3 }] as any,
110+
totalCount: 3
111+
},
112+
latestReviews: { nodes: [], totalCount: 0 }
113+
}
114+
115+
mockProvider.getAccount.mockImplementation((user: UserInfo) => {
116+
if (user.id === 'user2') {
117+
return Promise.resolve(undefined)
118+
}
119+
return Promise.resolve(user.id as PersonId)
120+
})
121+
122+
const result = await manager.getReviewers(prData as PullRequestExternalData)
123+
124+
expect(result).toEqual(['user1', 'user3'])
125+
expect(mockProvider.getAccount).toHaveBeenCalledTimes(3)
126+
})
127+
128+
it('should handle null latestReviews', async () => {
129+
const prData: Partial<PullRequestExternalData> = {
130+
reviewRequests: { nodes: [], totalCount: 0 },
131+
latestReviews: null as any
132+
}
133+
134+
const result = await manager.getReviewers(prData as PullRequestExternalData)
135+
136+
expect(result).toEqual([])
137+
expect(mockProvider.getAccount).not.toHaveBeenCalled()
138+
})
139+
140+
it('should handle undefined latestReviews', async () => {
141+
const prData: Partial<PullRequestExternalData> = {
142+
reviewRequests: { nodes: [], totalCount: 0 },
143+
latestReviews: undefined as any
144+
}
145+
146+
const result = await manager.getReviewers(prData as PullRequestExternalData)
147+
148+
expect(result).toEqual([])
149+
expect(mockProvider.getAccount).not.toHaveBeenCalled()
150+
})
151+
152+
it('should skip reviews with null author', async () => {
153+
const user1: UserInfo = { id: 'user1', login: 'user1' }
154+
const prData: Partial<PullRequestExternalData> = {
155+
reviewRequests: { nodes: [], totalCount: 0 },
156+
latestReviews: {
157+
nodes: [
158+
{ author: null, state: 'APPROVED' } as any,
159+
{ author: user1, state: 'APPROVED' } as any,
160+
{ author: null, state: 'CHANGES_REQUESTED' } as any
161+
],
162+
totalCount: 3
163+
}
164+
}
165+
166+
mockProvider.getAccount.mockImplementation((user: UserInfo) => {
167+
return Promise.resolve(user.id as PersonId)
168+
})
169+
170+
const result = await manager.getReviewers(prData as PullRequestExternalData)
171+
172+
expect(result).toEqual(['user1'])
173+
expect(mockProvider.getAccount).toHaveBeenCalledTimes(1)
174+
})
175+
176+
it('should combine reviewRequests and latestReviews', async () => {
177+
const user1: UserInfo = { id: 'user1', login: 'user1' }
178+
const user2: UserInfo = { id: 'user2', login: 'user2' }
179+
const user3: UserInfo = { id: 'user3', login: 'user3' }
180+
const user4: UserInfo = { id: 'user4', login: 'user4' }
181+
const prData: Partial<PullRequestExternalData> = {
182+
reviewRequests: {
183+
nodes: [{ requestedReviewer: user1 }, { requestedReviewer: user2 }] as any,
184+
totalCount: 2
185+
},
186+
latestReviews: {
187+
nodes: [{ author: user3, state: 'APPROVED' } as any, { author: user4, state: 'CHANGES_REQUESTED' } as any],
188+
totalCount: 2
189+
}
190+
}
191+
192+
mockProvider.getAccount.mockImplementation((user: UserInfo) => {
193+
return Promise.resolve(user.id as PersonId)
194+
})
195+
196+
const result = await manager.getReviewers(prData as PullRequestExternalData)
197+
198+
expect(result).toEqual(['user1', 'user2', 'user3', 'user4'])
199+
expect(mockProvider.getAccount).toHaveBeenCalledTimes(4)
200+
})
201+
202+
it('should handle complex null cases', async () => {
203+
const user1: UserInfo = { id: 'user1', login: 'user1' }
204+
const user2: UserInfo = { id: 'user2', login: 'user2' }
205+
const prData: Partial<PullRequestExternalData> = {
206+
reviewRequests: {
207+
nodes: [null, { requestedReviewer: null }, { requestedReviewer: user1 }, null] as any,
208+
totalCount: 4
209+
},
210+
latestReviews: {
211+
nodes: [null as any, { author: null, state: 'APPROVED' } as any, { author: user2, state: 'APPROVED' } as any],
212+
totalCount: 3
213+
}
214+
}
215+
216+
mockProvider.getAccount.mockImplementation((user: UserInfo) => {
217+
return Promise.resolve(user.id as PersonId)
218+
})
219+
220+
const result = await manager.getReviewers(prData as PullRequestExternalData)
221+
222+
expect(result).toEqual(['user1', 'user2'])
223+
expect(mockProvider.getAccount).toHaveBeenCalledTimes(2)
224+
})
225+
226+
it('should handle empty arrays', async () => {
227+
const prData: Partial<PullRequestExternalData> = {
228+
reviewRequests: { nodes: [], totalCount: 0 },
229+
latestReviews: { nodes: [], totalCount: 0 }
230+
}
231+
232+
const result = await manager.getReviewers(prData as PullRequestExternalData)
233+
234+
expect(result).toEqual([])
235+
expect(mockProvider.getAccount).not.toHaveBeenCalled()
236+
})
237+
})
238+
})

‎services/github/pod-github/src/sync/pullrequests.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,10 @@ export class PullRequestSyncManager extends IssueSyncManagerBase implements DocS
317317

318318
async getReviewers (issue: PullRequestExternalData): Promise<PersonId[]> {
319319
// Find Assignees and reviewers
320-
const ids: UserInfo[] = (issue.reviewRequests.nodes ?? []).map((it: any) => it.requestedReviewer)
320+
const ids: UserInfo[] = (issue.reviewRequests?.nodes ?? [])
321+
.filter((it: any) => it != null)
322+
.map((it: any) => it.requestedReviewer)
323+
.filter((id: any) => id != null)
321324

322325
const values: PersonId[] = []
323326

@@ -328,7 +331,10 @@ export class PullRequestSyncManager extends IssueSyncManagerBase implements DocS
328331
}
329332
}
330333

331-
for (const n of issue.latestReviews.nodes ?? []) {
334+
for (const n of issue.latestReviews?.nodes ?? []) {
335+
if (n?.author == null) {
336+
continue
337+
}
332338
const acc = await this.provider.getAccount(n.author)
333339
if (acc !== undefined) {
334340
values.push(acc)

0 commit comments

Comments
 (0)