From f5101ebb5edf9ce35338a5e774357faeee69580f Mon Sep 17 00:00:00 2001 From: Artem Savchenko Date: Wed, 19 Nov 2025 12:24:54 +0700 Subject: [PATCH 1/3] Fix null error in getReviewers Signed-off-by: Artem Savchenko --- .../src/sync/__tests__/pullrequests.test.ts | 230 ++++++++++++++++++ .../pod-github/src/sync/pullrequests.ts | 8 +- 2 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 services/github/pod-github/src/sync/__tests__/pullrequests.test.ts diff --git a/services/github/pod-github/src/sync/__tests__/pullrequests.test.ts b/services/github/pod-github/src/sync/__tests__/pullrequests.test.ts new file mode 100644 index 00000000000..ddcbf95203d --- /dev/null +++ b/services/github/pod-github/src/sync/__tests__/pullrequests.test.ts @@ -0,0 +1,230 @@ +/* eslint-disable import/first */ +import { PersonId } from '@hcengineering/core' +import { PullRequestExternalData } from '../githubTypes' +import type { UserInfo } from '../../types' + +// Mock dependencies before imports +jest.mock('@octokit/webhooks-types', () => ({}), { virtual: true }) +jest.mock('octokit', () => ({}), { virtual: true }) +jest.mock('../../config', () => ({ + default: { + AccountsURL: 'http://localhost:3000', + ServerSecret: 'test-secret', + AppId: 'test-app-id', + ClientId: 'test-client-id', + ClientSecret: 'test-client-secret', + PrivateKey: 'test-private-key', + CollaboratorURL: 'http://localhost:3078', + BotName: 'test-bot' + } +})) + +import { PullRequestSyncManager } from '../pullrequests' +/* eslint-enable import/first */ + +describe('PullRequestSyncManager', () => { + describe('getReviewers', () => { + let manager: PullRequestSyncManager + let mockProvider: any + + beforeEach(async () => { + mockProvider = { + getAccount: jest.fn() + } + manager = new PullRequestSyncManager(null as any, null as any, null as any) + await manager.init(mockProvider) + }) + + it('should handle null reviewRequests gracefully', async () => { + const prData: Partial = { + reviewRequests: null as any, + latestReviews: { nodes: [] } + } + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual([]) + expect(mockProvider.getAccount).not.toHaveBeenCalled() + }) + + it('should handle undefined reviewRequests gracefully', async () => { + const prData: Partial = { + reviewRequests: undefined as any, + latestReviews: { nodes: [] } + } + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual([]) + expect(mockProvider.getAccount).not.toHaveBeenCalled() + }) + + it('should handle null items in reviewRequests.nodes', async () => { + const user1: UserInfo = { id: 'user1', login: 'user1' } + const user2: UserInfo = { id: 'user2', login: 'user2' } + const prData: Partial = { + reviewRequests: { + nodes: [null, { requestedReviewer: user1 }, null, { requestedReviewer: user2 }] as any + }, + latestReviews: { nodes: [] } + } + + mockProvider.getAccount.mockImplementation((user: UserInfo) => { + return Promise.resolve(user.id as PersonId) + }) + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual(['user1', 'user2']) + expect(mockProvider.getAccount).toHaveBeenCalledTimes(2) + }) + + it('should handle null requestedReviewer in nodes', async () => { + const user1: UserInfo = { id: 'user1', login: 'user1' } + const prData: Partial = { + reviewRequests: { + nodes: [{ requestedReviewer: null }, { requestedReviewer: user1 }, { requestedReviewer: null }] as any + }, + latestReviews: { nodes: [] } + } + + mockProvider.getAccount.mockImplementation((user: UserInfo) => { + return Promise.resolve(user.id as PersonId) + }) + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual(['user1']) + expect(mockProvider.getAccount).toHaveBeenCalledTimes(1) + }) + + it('should filter out reviewers when getAccount returns undefined', async () => { + const user1: UserInfo = { id: 'user1', login: 'user1' } + const user2: UserInfo = { id: 'user2', login: 'user2' } + const user3: UserInfo = { id: 'user3', login: 'user3' } + const prData: Partial = { + reviewRequests: { + nodes: [{ requestedReviewer: user1 }, { requestedReviewer: user2 }, { requestedReviewer: user3 }] as any + }, + latestReviews: { nodes: [] } + } + + mockProvider.getAccount.mockImplementation((user: UserInfo) => { + if (user.id === 'user2') { + return Promise.resolve(undefined) + } + return Promise.resolve(user.id as PersonId) + }) + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual(['user1', 'user3']) + expect(mockProvider.getAccount).toHaveBeenCalledTimes(3) + }) + + it('should handle null latestReviews', async () => { + const prData: Partial = { + reviewRequests: { nodes: [] }, + latestReviews: null as any + } + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual([]) + expect(mockProvider.getAccount).not.toHaveBeenCalled() + }) + + it('should handle undefined latestReviews', async () => { + const prData: Partial = { + reviewRequests: { nodes: [] }, + latestReviews: undefined as any + } + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual([]) + expect(mockProvider.getAccount).not.toHaveBeenCalled() + }) + + it('should skip reviews with null author', async () => { + const user1: UserInfo = { id: 'user1', login: 'user1' } + const prData: Partial = { + reviewRequests: { nodes: [] }, + latestReviews: { + nodes: [ + { author: null, state: 'APPROVED' } as any, + { author: user1, state: 'APPROVED' } as any, + { author: null, state: 'CHANGES_REQUESTED' } as any + ] + } + } + + mockProvider.getAccount.mockImplementation((user: UserInfo) => { + return Promise.resolve(user.id as PersonId) + }) + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual(['user1']) + expect(mockProvider.getAccount).toHaveBeenCalledTimes(1) + }) + + it('should combine reviewRequests and latestReviews', async () => { + const user1: UserInfo = { id: 'user1', login: 'user1' } + const user2: UserInfo = { id: 'user2', login: 'user2' } + const user3: UserInfo = { id: 'user3', login: 'user3' } + const user4: UserInfo = { id: 'user4', login: 'user4' } + const prData: Partial = { + reviewRequests: { + nodes: [{ requestedReviewer: user1 }, { requestedReviewer: user2 }] as any + }, + latestReviews: { + nodes: [{ author: user3, state: 'APPROVED' } as any, { author: user4, state: 'CHANGES_REQUESTED' } as any] + } + } + + mockProvider.getAccount.mockImplementation((user: UserInfo) => { + return Promise.resolve(user.id as PersonId) + }) + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual(['user1', 'user2', 'user3', 'user4']) + expect(mockProvider.getAccount).toHaveBeenCalledTimes(4) + }) + + it('should handle complex null cases', async () => { + const user1: UserInfo = { id: 'user1', login: 'user1' } + const user2: UserInfo = { id: 'user2', login: 'user2' } + const prData: Partial = { + reviewRequests: { + nodes: [null, { requestedReviewer: null }, { requestedReviewer: user1 }, null] as any + }, + latestReviews: { + nodes: [null as any, { author: null, state: 'APPROVED' } as any, { author: user2, state: 'APPROVED' } as any] + } + } + + mockProvider.getAccount.mockImplementation((user: UserInfo) => { + return Promise.resolve(user.id as PersonId) + }) + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual(['user1', 'user2']) + expect(mockProvider.getAccount).toHaveBeenCalledTimes(2) + }) + + it('should handle empty arrays', async () => { + const prData: Partial = { + reviewRequests: { nodes: [] }, + latestReviews: { nodes: [] } + } + + const result = await manager.getReviewers(prData as PullRequestExternalData) + + expect(result).toEqual([]) + expect(mockProvider.getAccount).not.toHaveBeenCalled() + }) + }) +}) diff --git a/services/github/pod-github/src/sync/pullrequests.ts b/services/github/pod-github/src/sync/pullrequests.ts index e4e22cf05bf..432777a0f82 100644 --- a/services/github/pod-github/src/sync/pullrequests.ts +++ b/services/github/pod-github/src/sync/pullrequests.ts @@ -317,7 +317,10 @@ export class PullRequestSyncManager extends IssueSyncManagerBase implements DocS async getReviewers (issue: PullRequestExternalData): Promise { // Find Assignees and reviewers - const ids: UserInfo[] = (issue.reviewRequests.nodes ?? []).map((it: any) => it.requestedReviewer) + const ids: UserInfo[] = (issue.reviewRequests?.nodes ?? []) + .filter((it: any) => it != null) + .map((it: any) => it.requestedReviewer) + .filter((id: any) => id != null) const values: PersonId[] = [] @@ -328,7 +331,8 @@ export class PullRequestSyncManager extends IssueSyncManagerBase implements DocS } } - for (const n of issue.latestReviews.nodes ?? []) { + for (const n of issue.latestReviews?.nodes ?? []) { + if (n?.author == null) continue const acc = await this.provider.getAccount(n.author) if (acc !== undefined) { values.push(acc) From e43342a16358cae704093c5efd682f15fc92e440 Mon Sep 17 00:00:00 2001 From: Artem Savchenko Date: Wed, 19 Nov 2025 13:20:17 +0700 Subject: [PATCH 2/3] Fix validate Signed-off-by: Artem Savchenko --- .../src/sync/__tests__/pullrequests.test.ts | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/services/github/pod-github/src/sync/__tests__/pullrequests.test.ts b/services/github/pod-github/src/sync/__tests__/pullrequests.test.ts index ddcbf95203d..25f5ccf3c03 100644 --- a/services/github/pod-github/src/sync/__tests__/pullrequests.test.ts +++ b/services/github/pod-github/src/sync/__tests__/pullrequests.test.ts @@ -38,7 +38,7 @@ describe('PullRequestSyncManager', () => { it('should handle null reviewRequests gracefully', async () => { const prData: Partial = { reviewRequests: null as any, - latestReviews: { nodes: [] } + latestReviews: { nodes: [], totalCount: 0 } } const result = await manager.getReviewers(prData as PullRequestExternalData) @@ -50,7 +50,7 @@ describe('PullRequestSyncManager', () => { it('should handle undefined reviewRequests gracefully', async () => { const prData: Partial = { reviewRequests: undefined as any, - latestReviews: { nodes: [] } + latestReviews: { nodes: [], totalCount: 0 } } const result = await manager.getReviewers(prData as PullRequestExternalData) @@ -64,9 +64,10 @@ describe('PullRequestSyncManager', () => { const user2: UserInfo = { id: 'user2', login: 'user2' } const prData: Partial = { reviewRequests: { - nodes: [null, { requestedReviewer: user1 }, null, { requestedReviewer: user2 }] as any + nodes: [null, { requestedReviewer: user1 }, null, { requestedReviewer: user2 }] as any, + totalCount: 4 }, - latestReviews: { nodes: [] } + latestReviews: { nodes: [], totalCount: 0 } } mockProvider.getAccount.mockImplementation((user: UserInfo) => { @@ -83,9 +84,10 @@ describe('PullRequestSyncManager', () => { const user1: UserInfo = { id: 'user1', login: 'user1' } const prData: Partial = { reviewRequests: { - nodes: [{ requestedReviewer: null }, { requestedReviewer: user1 }, { requestedReviewer: null }] as any + nodes: [{ requestedReviewer: null }, { requestedReviewer: user1 }, { requestedReviewer: null }] as any, + totalCount: 3 }, - latestReviews: { nodes: [] } + latestReviews: { nodes: [], totalCount: 0 } } mockProvider.getAccount.mockImplementation((user: UserInfo) => { @@ -104,9 +106,10 @@ describe('PullRequestSyncManager', () => { const user3: UserInfo = { id: 'user3', login: 'user3' } const prData: Partial = { reviewRequests: { - nodes: [{ requestedReviewer: user1 }, { requestedReviewer: user2 }, { requestedReviewer: user3 }] as any + nodes: [{ requestedReviewer: user1 }, { requestedReviewer: user2 }, { requestedReviewer: user3 }] as any, + totalCount: 3 }, - latestReviews: { nodes: [] } + latestReviews: { nodes: [], totalCount: 0 } } mockProvider.getAccount.mockImplementation((user: UserInfo) => { @@ -124,7 +127,7 @@ describe('PullRequestSyncManager', () => { it('should handle null latestReviews', async () => { const prData: Partial = { - reviewRequests: { nodes: [] }, + reviewRequests: { nodes: [], totalCount: 0 }, latestReviews: null as any } @@ -136,7 +139,7 @@ describe('PullRequestSyncManager', () => { it('should handle undefined latestReviews', async () => { const prData: Partial = { - reviewRequests: { nodes: [] }, + reviewRequests: { nodes: [], totalCount: 0 }, latestReviews: undefined as any } @@ -149,13 +152,14 @@ describe('PullRequestSyncManager', () => { it('should skip reviews with null author', async () => { const user1: UserInfo = { id: 'user1', login: 'user1' } const prData: Partial = { - reviewRequests: { nodes: [] }, + reviewRequests: { nodes: [], totalCount: 0 }, latestReviews: { nodes: [ { author: null, state: 'APPROVED' } as any, { author: user1, state: 'APPROVED' } as any, { author: null, state: 'CHANGES_REQUESTED' } as any - ] + ], + totalCount: 3 } } @@ -176,10 +180,12 @@ describe('PullRequestSyncManager', () => { const user4: UserInfo = { id: 'user4', login: 'user4' } const prData: Partial = { reviewRequests: { - nodes: [{ requestedReviewer: user1 }, { requestedReviewer: user2 }] as any + nodes: [{ requestedReviewer: user1 }, { requestedReviewer: user2 }] as any, + totalCount: 2 }, latestReviews: { - nodes: [{ author: user3, state: 'APPROVED' } as any, { author: user4, state: 'CHANGES_REQUESTED' } as any] + nodes: [{ author: user3, state: 'APPROVED' } as any, { author: user4, state: 'CHANGES_REQUESTED' } as any], + totalCount: 2 } } @@ -198,10 +204,12 @@ describe('PullRequestSyncManager', () => { const user2: UserInfo = { id: 'user2', login: 'user2' } const prData: Partial = { reviewRequests: { - nodes: [null, { requestedReviewer: null }, { requestedReviewer: user1 }, null] as any + nodes: [null, { requestedReviewer: null }, { requestedReviewer: user1 }, null] as any, + totalCount: 4 }, latestReviews: { - nodes: [null as any, { author: null, state: 'APPROVED' } as any, { author: user2, state: 'APPROVED' } as any] + nodes: [null as any, { author: null, state: 'APPROVED' } as any, { author: user2, state: 'APPROVED' } as any], + totalCount: 3 } } @@ -217,8 +225,8 @@ describe('PullRequestSyncManager', () => { it('should handle empty arrays', async () => { const prData: Partial = { - reviewRequests: { nodes: [] }, - latestReviews: { nodes: [] } + reviewRequests: { nodes: [], totalCount: 0 }, + latestReviews: { nodes: [], totalCount: 0 } } const result = await manager.getReviewers(prData as PullRequestExternalData) From b5218afd1ac4af3b793136008855721a5d7d0f14 Mon Sep 17 00:00:00 2001 From: Artem Savchenko Date: Wed, 19 Nov 2025 14:45:32 +0700 Subject: [PATCH 3/3] Fix code style Signed-off-by: Artem Savchenko --- services/github/pod-github/src/sync/pullrequests.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/github/pod-github/src/sync/pullrequests.ts b/services/github/pod-github/src/sync/pullrequests.ts index 432777a0f82..11bff84de4e 100644 --- a/services/github/pod-github/src/sync/pullrequests.ts +++ b/services/github/pod-github/src/sync/pullrequests.ts @@ -332,7 +332,9 @@ export class PullRequestSyncManager extends IssueSyncManagerBase implements DocS } for (const n of issue.latestReviews?.nodes ?? []) { - if (n?.author == null) continue + if (n?.author == null) { + continue + } const acc = await this.provider.getAccount(n.author) if (acc !== undefined) { values.push(acc)