diff --git a/src/renderer/utils/api/__mocks__/response-mocks.ts b/src/renderer/utils/api/__mocks__/response-mocks.ts index d1db2fb7b..8130de5a3 100644 --- a/src/renderer/utils/api/__mocks__/response-mocks.ts +++ b/src/renderer/utils/api/__mocks__/response-mocks.ts @@ -418,27 +418,26 @@ export const mockDiscussionLabels: DiscussionLabels = { ], }; -export const mockGraphQLResponse: GraphQLSearch = { +export const mockDiscussion: Discussion = { + number: 123, + title: '1.16.0', + isAnswered: false, + stateReason: 'OPEN', + url: 'https://github.com/gitify-app/notifications-test/discussions/612' as Link, + author: { + login: 'discussion-creator', + url: 'https://github.com/discussion-creator' as Link, + avatar_url: 'https://avatars.githubusercontent.com/u/123456789?v=4' as Link, + type: 'User', + }, + comments: mockDiscussionComments, + labels: mockDiscussionLabels, +}; + +export const mockSearchDiscussionsGraphQLResponse: GraphQLSearch = { data: { search: { - nodes: [ - { - number: 123, - title: '1.16.0', - isAnswered: false, - stateReason: 'OPEN', - url: 'https://github.com/gitify-app/notifications-test/discussions/612' as Link, - author: { - login: 'discussion-creator', - url: 'https://github.com/discussion-creator' as Link, - avatar_url: - 'https://avatars.githubusercontent.com/u/123456789?v=4' as Link, - type: 'User', - }, - comments: mockDiscussionComments, - labels: mockDiscussionLabels, - }, - ], + nodes: [mockDiscussion], }, }, }; diff --git a/src/renderer/utils/api/client.ts b/src/renderer/utils/api/client.ts index 0fdfe4920..54985a1ca 100644 --- a/src/renderer/utils/api/client.ts +++ b/src/renderer/utils/api/client.ts @@ -284,4 +284,6 @@ export async function getLatestDiscussion( notification, ); } + + return null; } diff --git a/src/renderer/utils/helpers.test.ts b/src/renderer/utils/helpers.test.ts index bf8825bcd..91206608d 100644 --- a/src/renderer/utils/helpers.test.ts +++ b/src/renderer/utils/helpers.test.ts @@ -4,21 +4,19 @@ import { ChevronRightIcon, } from '@primer/octicons-react'; -import type { AxiosResponse } from 'axios'; - -import { mockPersonalAccessTokenAccount } from '../__mocks__/account-mocks'; import type { Hostname, Link } from '../types'; import type { SubjectType } from '../typesGitHub'; import * as logger from '../utils/logger'; import { - mockGraphQLResponse, + mockDiscussion, mockSingleNotification, } from './api/__mocks__/response-mocks'; -import * as apiRequests from './api/request'; +import * as apiClient from './api/client'; import { generateGitHubWebUrl, generateNotificationReferrerId, getChevronDetails, + getDefaultURLForType, getPlatformFromHostname, isEnterpriseServerHost, } from './helpers'; @@ -74,7 +72,9 @@ describe('renderer/utils/helpers.ts', () => { 'https://github.com/gitify-app/notifications-test/issues/785'; const mockNotificationReferrer = 'notification_referrer_id=MDE4Ok5vdGlmaWNhdGlvblRocmVhZDEzODY2MTA5NjoxMjM0NTY3ODk%3D'; - const apiRequestAuthSpy = jest.spyOn(apiRequests, 'apiRequestAuth'); + + const getHtmlUrlSpy = jest.spyOn(apiClient, 'getHtmlUrl'); + const getLatestDiscussionSpy = jest.spyOn(apiClient, 'getLatestDiscussion'); afterEach(() => { jest.clearAllMocks(); @@ -89,23 +89,14 @@ describe('renderer/utils/helpers.ts', () => { type: 'Issue' as SubjectType, }; - apiRequestAuthSpy.mockResolvedValue({ - data: { - html_url: mockHtmlUrl, - }, - } as AxiosResponse); + getHtmlUrlSpy.mockResolvedValue(mockHtmlUrl); const result = await generateGitHubWebUrl({ ...mockSingleNotification, subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1); - expect(apiRequestAuthSpy).toHaveBeenCalledWith( - subject.latest_comment_url, - 'GET', - mockPersonalAccessTokenAccount.token, - ); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(1); expect(result).toBe(`${mockHtmlUrl}?${mockNotificationReferrer}`); }); @@ -117,23 +108,14 @@ describe('renderer/utils/helpers.ts', () => { type: 'Issue' as SubjectType, }; - apiRequestAuthSpy.mockResolvedValue({ - data: { - html_url: mockHtmlUrl, - }, - } as AxiosResponse); + getHtmlUrlSpy.mockResolvedValue(mockHtmlUrl); const result = await generateGitHubWebUrl({ ...mockSingleNotification, subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1); - expect(apiRequestAuthSpy).toHaveBeenCalledWith( - subject.url, - 'GET', - mockPersonalAccessTokenAccount.token, - ); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(1); expect(result).toBe(`${mockHtmlUrl}?${mockNotificationReferrer}`); }); @@ -151,7 +133,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?query=workflow%3A%22Demo%22+is%3Asuccess+branch%3Amain&${mockNotificationReferrer}`, ); @@ -170,7 +152,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?query=workflow%3A%22Demo%22+is%3Afailure+branch%3Amain&${mockNotificationReferrer}`, ); @@ -189,7 +171,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?query=workflow%3A%22Demo%22+is%3Afailure+branch%3Amain&${mockNotificationReferrer}`, ); @@ -208,7 +190,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?query=workflow%3A%22Demo%22+is%3Askipped+branch%3Amain&${mockNotificationReferrer}`, ); @@ -227,7 +209,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?${mockNotificationReferrer}`, ); @@ -246,7 +228,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?query=workflow%3A%22Demo%22+branch%3Amain&${mockNotificationReferrer}`, ); @@ -265,7 +247,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?${mockNotificationReferrer}`, ); @@ -273,7 +255,7 @@ describe('renderer/utils/helpers.ts', () => { }); describe('Discussions URLs', () => { - it('when no subject urls and no discussions found via query, default to linking to repository discussions', async () => { + it('when no discussion found via graphql api, default to base repository discussion url', async () => { const subject = { title: 'generate github web url unit tests', url: null, @@ -281,22 +263,20 @@ describe('renderer/utils/helpers.ts', () => { type: 'Discussion' as SubjectType, }; - apiRequestAuthSpy.mockResolvedValue({ - data: { data: { search: { nodes: [] } } }, - } as AxiosResponse); + getLatestDiscussionSpy.mockResolvedValue(null); const result = await generateGitHubWebUrl({ ...mockSingleNotification, subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1); + expect(getLatestDiscussionSpy).toHaveBeenCalledTimes(1); expect(result).toBe( `${mockSingleNotification.repository.html_url}/discussions?${mockNotificationReferrer}`, ); }); - it('link to matching discussion and comment hash', async () => { + it('when discussion found via graphql api, link to matching discussion and comment hash', async () => { const subject = { title: '1.16.0', url: null, @@ -304,48 +284,18 @@ describe('renderer/utils/helpers.ts', () => { type: 'Discussion' as SubjectType, }; - apiRequestAuthSpy.mockResolvedValue({ - data: { - ...mockGraphQLResponse, - }, - } as AxiosResponse); + getLatestDiscussionSpy.mockResolvedValue(mockDiscussion); const result = await generateGitHubWebUrl({ ...mockSingleNotification, subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1); + expect(getLatestDiscussionSpy).toHaveBeenCalledTimes(1); expect(result).toBe( `https://github.com/gitify-app/notifications-test/discussions/612?${mockNotificationReferrer}#discussioncomment-2300902`, ); }); - - it('default to base discussions url when graphql query fails', async () => { - const rendererLogErrorSpy = jest - .spyOn(logger, 'rendererLogError') - .mockImplementation(); - - const subject = { - title: '1.16.0', - url: null, - latest_comment_url: null, - type: 'Discussion' as SubjectType, - }; - - apiRequestAuthSpy.mockResolvedValue(null as AxiosResponse); - - const result = await generateGitHubWebUrl({ - ...mockSingleNotification, - subject: subject, - }); - - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1); - expect(result).toBe( - `https://github.com/gitify-app/notifications-test/discussions?${mockNotificationReferrer}`, - ); - expect(rendererLogErrorSpy).toHaveBeenCalledTimes(1); - }); }); it('Repository Invitation url', async () => { @@ -362,7 +312,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/invitations?${mockNotificationReferrer}`, ); @@ -381,7 +331,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/security/dependabot?${mockNotificationReferrer}`, ); @@ -401,7 +351,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?query=is%3Awaiting&${mockNotificationReferrer}`, ); @@ -421,7 +371,7 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?${mockNotificationReferrer}`, ); @@ -440,15 +390,15 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( `https://github.com/gitify-app/notifications-test/actions?${mockNotificationReferrer}`, ); }); }); - describe('defaults to repository url', () => { - it('defaults when no urls present in notification', async () => { + describe('defaults web urls', () => { + it('issues - defaults when no urls present in notification', async () => { const subject = { title: 'generate github web url unit tests', url: null, @@ -461,9 +411,47 @@ describe('renderer/utils/helpers.ts', () => { subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); expect(result).toBe( - `${mockSingleNotification.repository.html_url}?${mockNotificationReferrer}`, + `https://github.com/gitify-app/notifications-test/issues?${mockNotificationReferrer}`, + ); + }); + + it('pull requests - defaults when no urls present in notification', async () => { + const subject = { + title: 'generate github web url unit tests', + url: null, + latest_comment_url: null, + type: 'PullRequest' as SubjectType, + }; + + const result = await generateGitHubWebUrl({ + ...mockSingleNotification, + subject: subject, + }); + + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); + expect(result).toBe( + `https://github.com/gitify-app/notifications-test/pulls?${mockNotificationReferrer}`, + ); + }); + + it('other - defaults when no urls present in notification', async () => { + const subject = { + title: 'generate github web url unit tests', + url: null, + latest_comment_url: null, + type: 'Commit' as SubjectType, + }; + + const result = await generateGitHubWebUrl({ + ...mockSingleNotification, + subject: subject, + }); + + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(0); + expect(result).toBe( + `https://github.com/gitify-app/notifications-test?${mockNotificationReferrer}`, ); }); @@ -475,34 +463,58 @@ describe('renderer/utils/helpers.ts', () => { const subject = { title: 'generate github web url unit tests', url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1' as Link, - latest_comment_url: - 'https://api.github.com/repos/gitify-app/notifications-test/issues/comments/302888448' as Link, + latest_comment_url: null as Link, type: 'Issue' as SubjectType, }; - const mockError = new Error('Test error'); - - apiRequestAuthSpy.mockRejectedValue(mockError); + getHtmlUrlSpy.mockRejectedValue(new Error('Test error')); const result = await generateGitHubWebUrl({ ...mockSingleNotification, subject: subject, }); - expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1); - expect(apiRequestAuthSpy).toHaveBeenCalledWith( - subject.latest_comment_url, - 'GET', - mockPersonalAccessTokenAccount.token, - ); + expect(getHtmlUrlSpy).toHaveBeenCalledTimes(1); expect(result).toBe( - `https://github.com/gitify-app/notifications-test?${mockNotificationReferrer}`, + `https://github.com/gitify-app/notifications-test/issues?${mockNotificationReferrer}`, ); - expect(rendererLogErrorSpy).toHaveBeenCalledTimes(2); + expect(rendererLogErrorSpy).toHaveBeenCalledTimes(1); }); }); }); + describe('getDefaultURLForType', () => { + let mockUrl: URL; + + beforeEach(() => { + mockUrl = new URL('https://github.com/gitify-app/notifications-test'); + }); + + it('discussions', () => { + expect(getDefaultURLForType(mockUrl, 'Discussion')).toEqual( + 'https://github.com/gitify-app/notifications-test/discussions', + ); + }); + + it('issues', () => { + expect(getDefaultURLForType(mockUrl, 'Issue')).toEqual( + 'https://github.com/gitify-app/notifications-test/issues', + ); + }); + + it('pull requests', () => { + expect(getDefaultURLForType(mockUrl, 'PullRequest')).toEqual( + 'https://github.com/gitify-app/notifications-test/pulls', + ); + }); + + it('default', () => { + expect(getDefaultURLForType(mockUrl, 'RepositoryInvitation')).toEqual( + 'https://github.com/gitify-app/notifications-test', + ); + }); + }); + describe('getChevronDetails', () => { it('should return correct chevron details', () => { expect(getChevronDetails(true, true, 'account')).toEqual({ diff --git a/src/renderer/utils/helpers.ts b/src/renderer/utils/helpers.ts index 25c87e753..9071901d4 100644 --- a/src/renderer/utils/helpers.ts +++ b/src/renderer/utils/helpers.ts @@ -6,7 +6,7 @@ import { import { Constants } from '../constants'; import type { Chevron, Hostname, Link } from '../types'; -import type { Notification } from '../typesGitHub'; +import type { Notification, SubjectType } from '../typesGitHub'; import { getHtmlUrl, getLatestDiscussion } from './api/client'; import type { PlatformType } from './auth/types'; import { rendererLogError } from './logger'; @@ -106,48 +106,38 @@ export async function generateGitHubWebUrl( ): Promise { const url = new URL(notification.repository.html_url); - /** - * Discussions - override generic response values for subject and comment urls, - * as we will fetch more specific html urls ourselves below - * See issue #1583: - */ - if (notification.subject.type === 'Discussion') { - notification.subject.url = null; - notification.subject.latest_comment_url = null; - } - try { - if (notification.subject.latest_comment_url) { - url.href = await getHtmlUrl( - notification.subject.latest_comment_url, - notification.account.token, - ); - } else if (notification.subject.url) { - url.href = await getHtmlUrl( - notification.subject.url, - notification.account.token, - ); - } else { - // Perform any specific notification type handling (only required for a few special notification scenarios) - switch (notification.subject.type) { - case 'CheckSuite': - url.href = getCheckSuiteUrl(notification); - break; - case 'Discussion': - url.href = await getDiscussionUrl(notification); - break; - case 'RepositoryInvitation': - url.pathname += '/invitations'; - break; - case 'RepositoryDependabotAlertsThread': - url.pathname += '/security/dependabot'; - break; - case 'WorkflowRun': - url.href = getWorkflowRunUrl(notification); - break; - default: - break; - } + // Perform any specific notification type handling (only required for a few special notification scenarios) + switch (notification.subject.type) { + case 'CheckSuite': + url.href = getCheckSuiteUrl(notification); + break; + case 'Discussion': + url.href = await getDiscussionUrl(notification); + break; + case 'RepositoryInvitation': + url.pathname += '/invitations'; + break; + case 'RepositoryDependabotAlertsThread': + url.pathname += '/security/dependabot'; + break; + case 'WorkflowRun': + url.href = getWorkflowRunUrl(notification); + break; + default: + if (notification.subject.latest_comment_url) { + url.href = await getHtmlUrl( + notification.subject.latest_comment_url, + notification.account.token, + ); + } else if (notification.subject.url) { + url.href = await getHtmlUrl( + notification.subject.url, + notification.account.token, + ); + } else { + url.href = getDefaultURLForType(url, notification.subject.type); + } } } catch (err) { rendererLogError( @@ -156,6 +146,8 @@ export async function generateGitHubWebUrl( err, notification, ); + + url.href = getDefaultURLForType(url, notification.subject.type); } url.searchParams.set( @@ -166,6 +158,24 @@ export async function generateGitHubWebUrl( return url.toString() as Link; } +export function getDefaultURLForType(url: URL, type: SubjectType) { + switch (type) { + case 'Discussion': + url.pathname += '/discussions'; + break; + case 'Issue': + url.pathname += '/issues'; + break; + case 'PullRequest': + url.pathname += '/pulls'; + break; + default: + break; + } + + return url.href; +} + export function getChevronDetails( hasNotifications: boolean, isVisible: boolean,