diff --git a/src/renderer/context/App.test.tsx b/src/renderer/context/App.test.tsx index 4d68cfd5c..d833d246f 100644 --- a/src/renderer/context/App.test.tsx +++ b/src/renderer/context/App.test.tsx @@ -184,8 +184,6 @@ describe('renderer/context/App.tsx', () => { }); describe('authentication methods', () => { - const apiRequestAuthSpy = jest.spyOn(apiRequests, 'apiRequestAuth'); - it('should call loginWithGitHubApp', async () => { const { button } = renderContextButton('loginWithGitHubApp'); @@ -207,7 +205,9 @@ describe('renderer/context/App.tsx', () => { }); it('should call loginWithPersonalAccessToken', async () => { - apiRequestAuthSpy.mockResolvedValueOnce(null); + const performAuthenticatedRESTRequestSpy = jest + .spyOn(apiRequests, 'performAuthenticatedRESTRequest') + .mockResolvedValueOnce(null); const { button } = renderContextButton('loginWithPersonalAccessToken', { hostname: 'github.com' as Hostname, @@ -220,10 +220,10 @@ describe('renderer/context/App.tsx', () => { expect(mockFetchNotifications).toHaveBeenCalledTimes(1), ); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1); - expect(apiRequestAuthSpy).toHaveBeenCalledWith( - 'https://api.github.com/notifications', + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledTimes(1); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( 'HEAD', + 'https://api.github.com/notifications', 'encrypted', ); }); diff --git a/src/renderer/utils/api/client.test.ts b/src/renderer/utils/api/client.test.ts index 71b5f4677..f31399293 100644 --- a/src/renderer/utils/api/client.test.ts +++ b/src/renderer/utils/api/client.test.ts @@ -1,21 +1,15 @@ -import axios, { type AxiosPromise, type AxiosResponse } from 'axios'; - import { mockGitHubCloudAccount } from '../../__mocks__/account-mocks'; import { mockGitHubCloudGitifyNotifications, mockPartialGitifyNotification, } from '../../__mocks__/notifications-mocks'; import { mockToken } from '../../__mocks__/state-mocks'; -import { - mockAuthHeaders, - mockNonCachedAuthHeaders, -} from './__mocks__/request-mocks'; import { Constants } from '../../constants'; import type { Hostname, Link, SettingsState, Token } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; -import * as logger from '../../utils/logger'; import { fetchAuthenticatedUserDetails, fetchDiscussionByNumber, @@ -39,7 +33,6 @@ import { FetchPullRequestByNumberDocument, type FetchPullRequestByNumberQuery, } from './graphql/generated/graphql'; -import type { ExecutionResultWithHeaders } from './request'; import * as apiRequests from './request'; jest.mock('axios'); @@ -48,6 +41,11 @@ const mockGitHubHostname = 'github.com' as Hostname; const mockThreadId = '1234'; describe('renderer/utils/api/client.ts', () => { + const performAuthenticatedRESTRequestSpy = jest.spyOn( + apiRequests, + 'performAuthenticatedRESTRequest', + ); + afterEach(() => { jest.clearAllMocks(); }); @@ -55,12 +53,11 @@ describe('renderer/utils/api/client.ts', () => { it('headNotifications - should fetch notifications head', async () => { await headNotifications(mockGitHubHostname, mockToken); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications', - headers: mockNonCachedAuthHeaders, - method: 'HEAD', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'HEAD', + 'https://api.github.com/notifications', + mockToken, + ); }); describe('listNotificationsForAuthenticatedUser', () => { @@ -68,6 +65,7 @@ describe('renderer/utils/api/client.ts', () => { const mockSettings: Partial = { participating: true, fetchReadNotifications: false, + fetchAllNotifications: false, }; await listNotificationsForAuthenticatedUser( @@ -75,18 +73,20 @@ describe('renderer/utils/api/client.ts', () => { mockSettings as SettingsState, ); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications?participating=true&all=false', - headers: mockNonCachedAuthHeaders, - method: 'GET', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/notifications?participating=true&all=false', + mockGitHubCloudAccount.token, + {}, + false, + ); }); it('should list participating and watching notifications for user', async () => { const mockSettings: Partial = { participating: false, fetchReadNotifications: false, + fetchAllNotifications: false, }; await listNotificationsForAuthenticatedUser( @@ -94,18 +94,20 @@ describe('renderer/utils/api/client.ts', () => { mockSettings as SettingsState, ); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications?participating=false&all=false', - headers: mockNonCachedAuthHeaders, - method: 'GET', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/notifications?participating=false&all=false', + mockGitHubCloudAccount.token, + {}, + false, + ); }); - it('should include all=true when fetchReadNotifications is enabled', async () => { + it('should list read and done notifications for user', async () => { const mockSettings: Partial = { participating: false, fetchReadNotifications: true, + fetchAllNotifications: false, }; await listNotificationsForAuthenticatedUser( @@ -113,18 +115,22 @@ describe('renderer/utils/api/client.ts', () => { mockSettings as SettingsState, ); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications?participating=false&all=true', - headers: mockNonCachedAuthHeaders, - method: 'GET', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/notifications?participating=false&all=true', + mockGitHubCloudAccount.token, + {}, + false, + ); }); - it('should include all=false when fetchReadNotifications is disabled', async () => { + it('should unpaginate notifications list for user', async () => { + performAuthenticatedRESTRequestSpy.mockImplementation(); + const mockSettings: Partial = { participating: false, fetchReadNotifications: false, + fetchAllNotifications: true, }; await listNotificationsForAuthenticatedUser( @@ -132,12 +138,13 @@ describe('renderer/utils/api/client.ts', () => { mockSettings as SettingsState, ); - expect(axios).toHaveBeenCalledWith({ - url: 'https://api.github.com/notifications?participating=false&all=false', - headers: mockNonCachedAuthHeaders, - method: 'GET', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/notifications?participating=false&all=false', + mockGitHubCloudAccount.token, + {}, + true, + ); }); }); @@ -148,12 +155,12 @@ describe('renderer/utils/api/client.ts', () => { mockToken, ); - expect(axios).toHaveBeenCalledWith({ - url: `https://api.github.com/notifications/threads/${mockThreadId}`, - headers: mockAuthHeaders, - method: 'PATCH', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'PATCH', + `https://api.github.com/notifications/threads/${mockThreadId}`, + mockToken, + {}, + ); }); it('markNotificationThreadAsDone - should mark notification thread as done', async () => { @@ -163,12 +170,12 @@ describe('renderer/utils/api/client.ts', () => { mockToken, ); - expect(axios).toHaveBeenCalledWith({ - url: `https://api.github.com/notifications/threads/${mockThreadId}`, - headers: mockAuthHeaders, - method: 'DELETE', - data: {}, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'DELETE', + `https://api.github.com/notifications/threads/${mockThreadId}`, + mockToken, + {}, + ); }); it('ignoreNotificationThreadSubscription - should ignore notification thread subscription', async () => { @@ -178,54 +185,25 @@ describe('renderer/utils/api/client.ts', () => { mockToken, ); - expect(axios).toHaveBeenCalledWith({ - url: `https://api.github.com/notifications/threads/${mockThreadId}/subscription`, - headers: mockAuthHeaders, - method: 'PUT', - data: { ignored: true }, - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'PUT', + `https://api.github.com/notifications/threads/${mockThreadId}/subscription`, + mockToken, + { ignored: true }, + ); }); - describe('getHtmlUrl', () => { - it('should return the HTML URL', async () => { - const apiRequestAuthSpy = jest.spyOn(apiRequests, 'apiRequestAuth'); - - const requestPromise = Promise.resolve({ - data: { - html_url: - 'https://github.com/gitify-app/notifications-test/issues/785', - }, - } as AxiosResponse) as AxiosPromise; - - apiRequestAuthSpy.mockResolvedValue(requestPromise); - - const result = await getHtmlUrl( - 'https://api.github.com/repos/gitify-app/notifications-test/issues/785' as Link, - '123' as Token, - ); - expect(result).toBe( - 'https://github.com/gitify-app/notifications-test/issues/785', - ); - }); - - it('should handle error', async () => { - const rendererLogErrorSpy = jest - .spyOn(logger, 'rendererLogError') - .mockImplementation(); - - const apiRequestAuthSpy = jest.spyOn(apiRequests, 'apiRequestAuth'); - - const mockError = new Error('Test error'); - - apiRequestAuthSpy.mockRejectedValue(mockError); - - await getHtmlUrl( - 'https://api.github.com/repos/gitify-app/gitify/issues/785' as Link, - '123' as Token, - ); + it('getHtmlUrl - should return the HTML URL', async () => { + await getHtmlUrl( + 'https://api.github.com/repos/gitify-app/notifications-test/issues/785' as Link, + '123' as Token, + ); - expect(rendererLogErrorSpy).toHaveBeenCalledTimes(1); - }); + expect(performAuthenticatedRESTRequestSpy).toHaveBeenCalledWith( + 'GET', + 'https://api.github.com/repos/gitify-app/notifications-test/issues/785', + '123', + ); }); it('fetchAuthenticatedUserDetails calls performGraphQLRequest with correct args', async () => { @@ -237,7 +215,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchAuthenticatedUserDetails(mockGitHubHostname, mockToken); @@ -263,7 +241,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchDiscussionByNumber(mockNotification); @@ -298,7 +276,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchIssueByNumber(mockNotification); @@ -331,7 +309,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchPullByNumber(mockNotification); @@ -367,7 +345,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestStringSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchNotificationDetailsForList([mockNotification]); @@ -383,7 +361,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestStringSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchNotificationDetailsForList([]); @@ -399,7 +377,7 @@ describe('renderer/utils/api/client.ts', () => { performGraphQLRequestStringSpy.mockResolvedValue({ data: {}, headers: {}, - } as ExecutionResultWithHeaders); + } as GitHubGraphQLResponse); await fetchNotificationDetailsForList(mockGitHubCloudGitifyNotifications); diff --git a/src/renderer/utils/api/client.ts b/src/renderer/utils/api/client.ts index 4f7d392c6..02582f237 100644 --- a/src/renderer/utils/api/client.ts +++ b/src/renderer/utils/api/client.ts @@ -1,5 +1,4 @@ -import type { AxiosPromise } from 'axios'; -import type { ExecutionResult } from 'graphql'; +import type { AxiosResponse } from 'axios'; import { Constants } from '../../constants'; @@ -11,10 +10,12 @@ import type { SettingsState, Token, } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import type { GetCommitCommentResponse, GetCommitResponse, GetReleaseResponse, + GitHubHtmlUrlResponse, HeadNotificationsResponse, IgnoreNotificationThreadSubscriptionResponse, ListNotificationsForAuthenticatedUserResponse, @@ -23,7 +24,6 @@ import type { } from './types'; import { isAnsweredDiscussionFeatureSupported } from '../features'; -import { rendererLogError } from '../logger'; import { createNotificationHandler } from '../notifications/handlers'; import { FetchAuthenticatedUserDetailsDocument, @@ -38,8 +38,7 @@ import { } from './graphql/generated/graphql'; import { MergeQueryBuilder } from './graphql/MergeQueryBuilder'; import { - apiRequestAuth, - type ExecutionResultWithHeaders, + performAuthenticatedRESTRequest, performGraphQLRequest, performGraphQLRequestString, } from './request'; @@ -57,11 +56,15 @@ import { export function headNotifications( hostname: Hostname, token: Token, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(hostname); url.pathname += 'notifications'; - return apiRequestAuth(url.toString() as Link, 'HEAD', token); + return performAuthenticatedRESTRequest( + 'HEAD', + url.toString() as Link, + token, + ); } /** @@ -73,15 +76,15 @@ export function headNotifications( export function listNotificationsForAuthenticatedUser( account: Account, settings: SettingsState, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(account.hostname); url.pathname += 'notifications'; url.searchParams.append('participating', String(settings.participating)); url.searchParams.append('all', String(settings.fetchReadNotifications)); - return apiRequestAuth( - url.toString() as Link, + return performAuthenticatedRESTRequest( 'GET', + url.toString() as Link, account.token, {}, settings.fetchAllNotifications, @@ -99,11 +102,16 @@ export function markNotificationThreadAsRead( threadId: string, hostname: Hostname, token: Token, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(hostname); url.pathname += `notifications/threads/${threadId}`; - return apiRequestAuth(url.toString() as Link, 'PATCH', token, {}); + return performAuthenticatedRESTRequest( + 'PATCH', + url.toString() as Link, + token, + {}, + ); } /** @@ -118,11 +126,16 @@ export function markNotificationThreadAsDone( threadId: string, hostname: Hostname, token: Token, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(hostname); url.pathname += `notifications/threads/${threadId}`; - return apiRequestAuth(url.toString() as Link, 'DELETE', token, {}); + return performAuthenticatedRESTRequest( + 'DELETE', + url.toString() as Link, + token, + {}, + ); } /** @@ -134,13 +147,18 @@ export function ignoreNotificationThreadSubscription( threadId: string, hostname: Hostname, token: Token, -): AxiosPromise { +): Promise> { const url = getGitHubAPIBaseUrl(hostname); url.pathname += `notifications/threads/${threadId}/subscription`; - return apiRequestAuth(url.toString() as Link, 'PUT', token, { - ignored: true, - }); + return performAuthenticatedRESTRequest( + 'PUT', + url.toString() as Link, + token, + { + ignored: true, + }, + ); } /** @@ -151,8 +169,8 @@ export function ignoreNotificationThreadSubscription( export function getCommit( url: Link, token: Token, -): AxiosPromise { - return apiRequestAuth(url, 'GET', token); +): Promise> { + return performAuthenticatedRESTRequest('GET', url, token); } /** @@ -163,8 +181,12 @@ export function getCommit( export function getCommitComment( url: Link, token: Token, -): AxiosPromise { - return apiRequestAuth(url, 'GET', token); +): Promise> { + return performAuthenticatedRESTRequest( + 'GET', + url, + token, + ); } /** @@ -175,27 +197,23 @@ export function getCommitComment( export function getRelease( url: Link, token: Token, -): AxiosPromise { - return apiRequestAuth(url, 'GET', token); +): Promise> { + return performAuthenticatedRESTRequest('GET', url, token); } /** * Get the `html_url` from the GitHub response * */ -export async function getHtmlUrl(url: Link, token: Token): Promise { - try { - // TODO - Add explicit type for response shape - const response = (await apiRequestAuth(url, 'GET', token)).data; - - return response.html_url; - } catch (err) { - rendererLogError( - 'getHtmlUrl', - `error occurred while fetching html url for ${url}`, - err, - ); - } +export async function getHtmlUrl( + url: Link, + token: Token, +): Promise> { + return performAuthenticatedRESTRequest( + 'GET', + url, + token, + ); } /** @@ -204,7 +222,7 @@ export async function getHtmlUrl(url: Link, token: Token): Promise { export async function fetchAuthenticatedUserDetails( hostname: Hostname, token: Token, -): Promise> { +): Promise> { const url = getGitHubGraphQLUrl(hostname); return performGraphQLRequest( @@ -219,7 +237,7 @@ export async function fetchAuthenticatedUserDetails( */ export async function fetchDiscussionByNumber( notification: GitifyNotification, -): Promise> { +): Promise> { const url = getGitHubGraphQLUrl(notification.account.hostname); const number = getNumberFromUrl(notification.subject.url); @@ -246,7 +264,7 @@ export async function fetchDiscussionByNumber( */ export async function fetchIssueByNumber( notification: GitifyNotification, -): Promise> { +): Promise> { const url = getGitHubGraphQLUrl(notification.account.hostname); const number = getNumberFromUrl(notification.subject.url); @@ -269,7 +287,7 @@ export async function fetchIssueByNumber( */ export async function fetchPullByNumber( notification: GitifyNotification, -): Promise> { +): Promise> { const url = getGitHubGraphQLUrl(notification.account.hostname); const number = getNumberFromUrl(notification.subject.url); diff --git a/src/renderer/utils/api/errors.test.ts b/src/renderer/utils/api/errors.test.ts index 90a9f31ac..cd86b04f5 100644 --- a/src/renderer/utils/api/errors.test.ts +++ b/src/renderer/utils/api/errors.test.ts @@ -1,91 +1,124 @@ import { AxiosError, type AxiosResponse } from 'axios'; +import type { DeepPartial } from '../../__helpers__/test-utils'; + import { EVENTS } from '../../../shared/events'; import type { Link } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import type { GitHubRESTError } from './types'; import { Errors } from '../errors'; -import { determineFailureType } from './errors'; +import * as rendererLogger from '../logger'; +import { assertNoGraphQLErrors, determineFailureType } from './errors'; describe('renderer/utils/api/errors.ts', () => { - it('network error', async () => { - const mockError: Partial> = { - code: AxiosError.ERR_NETWORK, - }; - - const result = determineFailureType( - mockError as AxiosError, - ); - - expect(result).toBe(Errors.NETWORK); - }); - - describe('bad request errors', () => { - it('bad credentials', async () => { - const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 401, - response: createMockResponse(401, 'Bad credentials'), - }; - - const result = determineFailureType( - mockError as AxiosError, - ); - - expect(result).toBe(Errors.BAD_CREDENTIALS); - }); - - it('missing scopes', async () => { + describe('determineFailureType', () => { + it('network error', async () => { const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 403, - response: createMockResponse(403, "Missing the 'notifications' scope"), + code: AxiosError.ERR_NETWORK, }; const result = determineFailureType( mockError as AxiosError, ); - expect(result).toBe(Errors.MISSING_SCOPES); + expect(result).toBe(Errors.NETWORK); }); - it('rate limited - primary', async () => { - const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 403, - response: createMockResponse(403, 'API rate limit exceeded'), - }; - - const result = determineFailureType( - mockError as AxiosError, - ); - - expect(result).toBe(Errors.RATE_LIMITED); + describe('bad request errors', () => { + it('bad credentials', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 401, + response: createMockResponse(401, 'Bad credentials'), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.BAD_CREDENTIALS); + }); + + it('missing scopes', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 403, + response: createMockResponse( + 403, + "Missing the 'notifications' scope", + ), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.MISSING_SCOPES); + }); + + it('rate limited - primary', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 403, + response: createMockResponse(403, 'API rate limit exceeded'), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.RATE_LIMITED); + }); + + it('rate limited - secondary', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 403, + response: createMockResponse( + 403, + 'You have exceeded a secondary rate limit', + ), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.RATE_LIMITED); + }); + + it('unhandled bad request error', async () => { + const mockError: Partial> = { + code: AxiosError.ERR_BAD_REQUEST, + status: 400, + response: createMockResponse(403, 'Oops! Something went wrong.'), + }; + + const result = determineFailureType( + mockError as AxiosError, + ); + + expect(result).toBe(Errors.UNKNOWN); + }); }); - it('rate limited - secondary', async () => { + it('bad credentials - safe storage', async () => { const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 403, - response: createMockResponse( - 403, - 'You have exceeded a secondary rate limit', - ), + message: `Error invoking remote method '${EVENTS.SAFE_STORAGE_DECRYPT}': Error: Error while decrypting the ciphertext provided to safeStorage.decryptString. Ciphertext does not appear to be encrypted.`, }; const result = determineFailureType( mockError as AxiosError, ); - expect(result).toBe(Errors.RATE_LIMITED); + expect(result).toBe(Errors.BAD_CREDENTIALS); }); - it('unhandled bad request error', async () => { + it('unknown error', async () => { const mockError: Partial> = { - code: AxiosError.ERR_BAD_REQUEST, - status: 400, - response: createMockResponse(403, 'Oops! Something went wrong.'), + code: 'anything', }; const result = determineFailureType( @@ -95,29 +128,50 @@ describe('renderer/utils/api/errors.ts', () => { expect(result).toBe(Errors.UNKNOWN); }); }); +}); - it('bad credentials - safe storage', async () => { - const mockError: Partial> = { - message: `Error invoking remote method '${EVENTS.SAFE_STORAGE_DECRYPT}': Error: Error while decrypting the ciphertext provided to safeStorage.decryptString. Ciphertext does not appear to be encrypted.`, - }; - - const result = determineFailureType( - mockError as AxiosError, +describe('assertNoGraphQLErrors', () => { + it('throws and logs when GraphQL errors are present', () => { + const rendererLogErrorSpy = jest + .spyOn(rendererLogger, 'rendererLogError') + .mockImplementation(); + + const payload: GitHubGraphQLResponse = { + data: {}, + errors: [ + { + message: 'Something went wrong', + locations: [{ line: 1, column: 1 }], + }, + ], + } as DeepPartial< + GitHubGraphQLResponse + > as GitHubGraphQLResponse; + + expect(() => assertNoGraphQLErrors('test-context', payload)).toThrow( + 'GraphQL request returned errors', ); - expect(result).toBe(Errors.BAD_CREDENTIALS); + expect(rendererLogErrorSpy).toHaveBeenCalled(); }); - it('unknown error', async () => { - const mockError: Partial> = { - code: 'anything', + it('does not throw when errors array is empty or undefined', () => { + const payload: GitHubGraphQLResponse = { + data: {}, + errors: [], + headers: {}, }; - const result = determineFailureType( - mockError as AxiosError, - ); + expect(() => assertNoGraphQLErrors('test-context', payload)).not.toThrow(); + }); + + it('does not throw when errors array is undefined', () => { + const payload: GitHubGraphQLResponse = { + data: {}, + headers: {}, + }; - expect(result).toBe(Errors.UNKNOWN); + expect(() => assertNoGraphQLErrors('test-context', payload)).not.toThrow(); }); }); diff --git a/src/renderer/utils/api/errors.ts b/src/renderer/utils/api/errors.ts index a9508a4d7..23e13a4fe 100644 --- a/src/renderer/utils/api/errors.ts +++ b/src/renderer/utils/api/errors.ts @@ -1,10 +1,19 @@ import { AxiosError } from 'axios'; +import type { GraphQLError } from 'graphql'; import type { GitifyError } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import type { GitHubRESTError } from './types'; import { Errors } from '../errors'; +import { rendererLogError } from '../logger'; +/** + * Determine the failure type based on an Axios error. + * + * @param err The Axios error + * @returns The Gitify error type + */ export function determineFailureType( err: AxiosError, ): GitifyError { @@ -44,3 +53,31 @@ export function determineFailureType( return Errors.UNKNOWN; } + +/** + * Assert that a GraphQL response does not contain errors. + * Logs and throws if `errors` array is present and non-empty. + * + * @param context The context of the GraphQL request + * @param payload The GraphQL response payload + */ +export function assertNoGraphQLErrors( + context: string, + payload: GitHubGraphQLResponse, +) { + if (Array.isArray(payload.errors) && payload.errors.length > 0) { + const errorMessages = payload.errors + .map((graphQLError: GraphQLError) => graphQLError.message) + .join('; '); + + const err = new Error( + errorMessages + ? `GraphQL request returned errors: ${errorMessages}` + : 'GraphQL request returned errors', + ); + + rendererLogError(context, 'GraphQL errors present in response', err); + + throw err; + } +} diff --git a/src/renderer/utils/api/graphql/generated/graphql.ts b/src/renderer/utils/api/graphql/generated/graphql.ts index 57a1d9b18..e1294c5fb 100644 --- a/src/renderer/utils/api/graphql/generated/graphql.ts +++ b/src/renderer/utils/api/graphql/generated/graphql.ts @@ -12419,6 +12419,8 @@ export type Mutation = { reprioritizeSubIssue?: Maybe; /** Set review requests on a pull request. */ requestReviews?: Maybe; + /** Set review requests on a pull request using login strings instead of IDs. */ + requestReviewsByLogin?: Maybe; /** Rerequests an existing check suite. */ rerequestCheckSuite?: Maybe; /** Marks a review thread as resolved. */ @@ -13586,6 +13588,12 @@ export type MutationRequestReviewsArgs = { }; +/** The root query for implementing GraphQL mutations. */ +export type MutationRequestReviewsByLoginArgs = { + input: RequestReviewsByLoginInput; +}; + + /** The root query for implementing GraphQL mutations. */ export type MutationRerequestCheckSuiteArgs = { input: RerequestCheckSuiteInput; @@ -28217,6 +28225,35 @@ export type ReprioritizeSubIssuePayload = { issue?: Maybe; }; +/** Autogenerated input type of RequestReviewsByLogin */ +export type RequestReviewsByLoginInput = { + /** The logins of the bots to request reviews from, including the [bot] suffix (e.g., 'copilot-pull-request-reviewer[bot]'). */ + botLogins?: InputMaybe>; + /** A unique identifier for the client performing the mutation. */ + clientMutationId?: InputMaybe; + /** The Node ID of the pull request to modify. */ + pullRequestId: Scalars['ID']['input']; + /** The slugs of the teams to request reviews from (format: 'org/team-slug'). */ + teamSlugs?: InputMaybe>; + /** Add users to the set rather than replace. */ + union?: InputMaybe; + /** The login strings of the users to request reviews from. */ + userLogins?: InputMaybe>; +}; + +/** Autogenerated return type of RequestReviewsByLogin. */ +export type RequestReviewsByLoginPayload = { + __typename?: 'RequestReviewsByLoginPayload'; + /** Identifies the actor who performed the event. */ + actor?: Maybe; + /** A unique identifier for the client performing the mutation. */ + clientMutationId?: Maybe; + /** The pull request that is getting requests. */ + pullRequest?: Maybe; + /** The edge from the pull request to the requested reviewers. */ + requestedReviewersEdge?: Maybe; +}; + /** Autogenerated input type of RequestReviews */ export type RequestReviewsInput = { /** The Node IDs of the bot to request. */ diff --git a/src/renderer/utils/api/graphql/types.ts b/src/renderer/utils/api/graphql/types.ts index f5960c472..8149c6dbe 100644 --- a/src/renderer/utils/api/graphql/types.ts +++ b/src/renderer/utils/api/graphql/types.ts @@ -1,3 +1,12 @@ +import type { ExecutionResult } from 'graphql'; + +/** + * GitHub GraphQL API response type with HTTP response headers + */ +export type GitHubGraphQLResponse = ExecutionResult & { + headers: Record; +}; + export type FragmentInfo = { name: string; typeCondition: string; diff --git a/src/renderer/utils/api/request.test.ts b/src/renderer/utils/api/request.test.ts index de501988b..824ab1e47 100644 --- a/src/renderer/utils/api/request.test.ts +++ b/src/renderer/utils/api/request.test.ts @@ -11,8 +11,8 @@ import type { Link } from '../../types'; import { FetchAuthenticatedUserDetailsDocument } from './graphql/generated/graphql'; import { - apiRequestAuth, getHeaders, + performAuthenticatedRESTRequest, performGraphQLRequest, performGraphQLRequestString, shouldRequestWithNoCache, @@ -36,7 +36,7 @@ describe('renderer/utils/api/request.ts', () => { it('should make an authenticated request with the correct parameters', async () => { const data = { key: 'value' }; - await apiRequestAuth(url, method, mockToken, data); + await performAuthenticatedRESTRequest(method, url, mockToken, data); expect(axios).toHaveBeenCalledWith({ method, @@ -49,7 +49,7 @@ describe('renderer/utils/api/request.ts', () => { it('should make an authenticated request with the correct parameters and default data', async () => { const data = {}; - await apiRequestAuth(url, method, mockToken); + await performAuthenticatedRESTRequest(method, url, mockToken, data); expect(axios).toHaveBeenCalledWith({ method, @@ -112,21 +112,25 @@ describe('renderer/utils/api/request.ts', () => { describe('shouldRequestWithNoCache', () => { it('shouldRequestWithNoCache', () => { expect( - shouldRequestWithNoCache('https://example.com/api/v3/notifications'), + shouldRequestWithNoCache( + 'https://example.com/api/v3/notifications' as Link, + ), ).toBe(true); expect( shouldRequestWithNoCache( - 'https://example.com/login/oauth/access_token', + 'https://example.com/login/oauth/access_token' as Link, ), ).toBe(true); expect( - shouldRequestWithNoCache('https://example.com/notifications'), + shouldRequestWithNoCache('https://example.com/notifications' as Link), ).toBe(true); expect( - shouldRequestWithNoCache('https://example.com/some/other/endpoint'), + shouldRequestWithNoCache( + 'https://example.com/some/other/endpoint' as Link, + ), ).toBe(false); }); }); diff --git a/src/renderer/utils/api/request.ts b/src/renderer/utils/api/request.ts index b3971b206..4f4d54aa1 100644 --- a/src/renderer/utils/api/request.ts +++ b/src/renderer/utils/api/request.ts @@ -1,49 +1,39 @@ -import axios, { - type AxiosPromise, - type AxiosResponse, - type Method, -} from 'axios'; -import type { ExecutionResult } from 'graphql'; +import axios, { type AxiosResponse, type Method } from 'axios'; import type { Link, Token } from '../../types'; +import type { GitHubGraphQLResponse } from './graphql/types'; import { decryptValue } from '../comms'; import { rendererLogError } from '../logger'; +import { assertNoGraphQLErrors } from './errors'; import type { TypedDocumentString } from './graphql/generated/graphql'; import { getNextURLFromLinkHeader } from './utils'; /** - * ExecutionResult with HTTP response headers - */ -export type ExecutionResultWithHeaders = ExecutionResult & { - headers: Record; -}; - -/** - * Perform an authenticated API request + * Perform an authenticated REST API request * - * @param url - * @param method - * @param token - * @param data - * @param fetchAllRecords whether to fetch all records or just the first page - * @returns + * @param method The REST http method + * @param url The API url + * @param token A GitHub token (decrypted) + * @param data The API request body + * @param fetchAllRecords Whether to fetch all records or just the first page + * @returns Resolves to a GitHub REST response with the specified type */ -export async function apiRequestAuth( - url: Link, +export async function performAuthenticatedRESTRequest( method: Method, + url: Link, token: Token, - data = {}, + data: Record = {}, fetchAllRecords = false, -): AxiosPromise | null { +): Promise> { const headers = await getHeaders(url, token); if (!fetchAllRecords) { return axios({ method, url, data, headers }); } - let response: AxiosResponse | null = null; - let combinedData = []; + let response: AxiosResponse | null = null; + let combinedData: unknown[] = []; try { let nextUrl: string | null = url; @@ -56,36 +46,40 @@ export async function apiRequestAuth( break; } - combinedData = combinedData.concat(response.data); // Accumulate data + // Accumulate paginated array results + combinedData = combinedData.concat(response.data); nextUrl = getNextURLFromLinkHeader(response); } } catch (err) { - rendererLogError('apiRequestAuth', 'API request failed:', err); + rendererLogError( + 'performAuthenticatedRESTRequest', + 'API request failed:', + err, + ); throw err; } - return { - ...response, - data: combinedData, - } as AxiosResponse; + // Return a response object with the combined array of records as data + response.data = combinedData as TResult; + return response; } /** - * Perform a GraphQL API request for account + * Perform a GraphQL API request with typed operation document * - * @param account - * @param query - * @param variables - * @returns + * @param url The API url + * @param query The GraphQL operation/query statement + * @param variables The GraphQL operation variables + * @returns Resolves to a GitHub GraphQL response */ export async function performGraphQLRequest( url: Link, token: Token, query: TypedDocumentString, ...[variables]: TVariables extends Record ? [] : [TVariables] -) { +): Promise> { const headers = await getHeaders(url, token); return axios({ @@ -97,24 +91,32 @@ export async function performGraphQLRequest( }, headers: headers, }).then((response) => { + assertNoGraphQLErrors('performGraphQLRequest', response.data); + return { ...response.data, headers: response.headers, }; - }) as Promise>; + }) as Promise>; } /** * Perform a GraphQL API request using a raw query string instead of a TypedDocumentString. * - * Useful for dynamically composed queries (e.g., merged queries built at runtime). + * Useful for dynamically composed queries (e.g: merged queries built at runtime). + * + * @param url The API url + * @param token The GitHub token (decrypted) + * @param query The GraphQL operation/query statement + * @param variables The GraphQL operation variables + * @returns Resolves to a GitHub GraphQL response */ export async function performGraphQLRequestString( url: Link, token: Token, query: string, variables?: Record, -): Promise> { +): Promise> { const headers = await getHeaders(url, token); return axios({ @@ -126,20 +128,26 @@ export async function performGraphQLRequestString( }, headers: headers, }).then((response) => { + assertNoGraphQLErrors( + 'performGraphQLRequestString', + response.data, + ); + return { ...response.data, headers: response.headers, - } as ExecutionResultWithHeaders; + } as GitHubGraphQLResponse; }); } /** - * Return true if the request should be made with no-cache + * Determine if the API request should be made with no-cache header + * based on the API url path * - * @param url - * @returns boolean + * @param url The API url + * @returns Whether a cache heading should be set or not */ -export function shouldRequestWithNoCache(url: string) { +export function shouldRequestWithNoCache(url: Link): boolean { const parsedUrl = new URL(url); switch (parsedUrl.pathname) { @@ -155,11 +163,14 @@ export function shouldRequestWithNoCache(url: string) { /** * Construct headers for API requests * - * @param username - * @param token - * @returns + * @param username A GitHub account username + * @param token A GitHub token (decrypted) + * @returns A headers object to use with API requests */ -export async function getHeaders(url: Link, token?: Token) { +export async function getHeaders( + url: Link, + token?: Token, +): Promise> { const headers: Record = { Accept: 'application/json', 'Cache-Control': shouldRequestWithNoCache(url) ? 'no-cache' : '', diff --git a/src/renderer/utils/api/types.ts b/src/renderer/utils/api/types.ts index d09cdd5e1..fc327cab0 100644 --- a/src/renderer/utils/api/types.ts +++ b/src/renderer/utils/api/types.ts @@ -28,6 +28,15 @@ export type RawGitHubNotification = export type RawUser = components['schemas']['simple-user']; +/** + * Minimal response for endpoints where we're only interested in the `html_url`. + * + * Used when following a notification thread's subject URL or latest comment URL. + */ +export type GitHubHtmlUrlResponse = { + html_url: string; +}; + /** * These API endpoints don't return a response body: * - HEAD /notifications diff --git a/src/renderer/utils/helpers.test.ts b/src/renderer/utils/helpers.test.ts index 38ae0607b..32115c939 100644 --- a/src/renderer/utils/helpers.test.ts +++ b/src/renderer/utils/helpers.test.ts @@ -4,6 +4,8 @@ import { ChevronRightIcon, } from '@primer/octicons-react'; +import type { AxiosResponse } from 'axios'; + import { mockGitifyNotification } from '../__mocks__/notifications-mocks'; import { mockToken } from '../__mocks__/state-mocks'; @@ -67,7 +69,7 @@ describe('renderer/utils/helpers.ts', () => { describe('generateGitHubWebUrl', () => { const mockHtmlUrl = - 'https://github.com/gitify-app/notifications-test/issues/785'; + 'https://github.com/gitify-app/notifications-test/issues/785' as Link; const mockNotificationReferrer = 'notification_referrer_id=MDE4Ok5vdGlmaWNhdGlvblRocmVhZDEzODY2MTA5NjoxMjM0NTY3ODk%3D'; @@ -116,7 +118,13 @@ describe('renderer/utils/helpers.ts', () => { htmlUrl: mockSubjectHtmlUrl, } as GitifySubject; - getHtmlUrlSpy.mockResolvedValue(mockHtmlUrl); + getHtmlUrlSpy.mockResolvedValue( + Promise.resolve({ + data: { + html_url: mockHtmlUrl, + }, + } as AxiosResponse), + ); const result = await generateGitHubWebUrl({ ...mockGitifyNotification, @@ -145,7 +153,13 @@ describe('renderer/utils/helpers.ts', () => { htmlUrl: mockSubjectHtmlUrl, } as GitifySubject; - getHtmlUrlSpy.mockResolvedValue(mockHtmlUrl); + getHtmlUrlSpy.mockResolvedValue( + Promise.resolve({ + data: { + html_url: mockHtmlUrl, + }, + } as AxiosResponse), + ); const result = await generateGitHubWebUrl({ ...mockGitifyNotification, diff --git a/src/renderer/utils/helpers.ts b/src/renderer/utils/helpers.ts index 5054bba2b..f8b5e673b 100644 --- a/src/renderer/utils/helpers.ts +++ b/src/renderer/utils/helpers.ts @@ -62,15 +62,20 @@ export async function generateGitHubWebUrl( } else { try { if (notification.subject.latestCommentUrl) { - url.href = await getHtmlUrl( - notification.subject.latestCommentUrl, - notification.account.token, - ); + const response = ( + await getHtmlUrl( + notification.subject.latestCommentUrl, + notification.account.token, + ) + ).data; + + url.href = response.html_url; } else if (notification.subject.url) { - url.href = await getHtmlUrl( - notification.subject.url, - notification.account.token, - ); + const response = ( + await getHtmlUrl(notification.subject.url, notification.account.token) + ).data; + + url.href = response.html_url; } } catch (err) { rendererLogError(