Skip to content

Commit 6491833

Browse files
setchysetchy
committed
feat(url): improve fallback url handling
Signed-off-by: Adam Setch <[email protected]>
1 parent df35341 commit 6491833

File tree

2 files changed

+120
-71
lines changed

2 files changed

+120
-71
lines changed

‎src/renderer/utils/helpers.test.ts‎

Lines changed: 68 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ describe('renderer/utils/helpers.ts', () => {
273273
});
274274

275275
describe('Discussions URLs', () => {
276-
it('when no subject urls and no discussions found via query, default to linking to repository discussions', async () => {
276+
it('when no discussion found via graphql api, default to base repository discussion url', async () => {
277277
const subject = {
278278
title: 'generate github web url unit tests',
279279
url: null,
@@ -296,7 +296,11 @@ describe('renderer/utils/helpers.ts', () => {
296296
);
297297
});
298298

299-
it('link to matching discussion and comment hash', async () => {
299+
it('when error fetching discussion via graphql api, default to base repository discussion url', async () => {
300+
const rendererLogErrorSpy = jest
301+
.spyOn(logger, 'rendererLogError')
302+
.mockImplementation();
303+
300304
const subject = {
301305
title: '1.16.0',
302306
url: null,
@@ -306,7 +310,12 @@ describe('renderer/utils/helpers.ts', () => {
306310

307311
apiRequestAuthSpy.mockResolvedValue({
308312
data: {
309-
...mockGraphQLResponse,
313+
data: null,
314+
errors: [
315+
{
316+
message: 'Something failed',
317+
},
318+
],
310319
},
311320
} as AxiosResponse);
312321

@@ -317,23 +326,24 @@ describe('renderer/utils/helpers.ts', () => {
317326

318327
expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1);
319328
expect(result).toBe(
320-
`https://github.com/gitify-app/notifications-test/discussions/612?${mockNotificationReferrer}#discussioncomment-2300902`,
329+
`https://github.com/gitify-app/notifications-test/discussions?${mockNotificationReferrer}`,
321330
);
331+
expect(rendererLogErrorSpy).toHaveBeenCalledTimes(1);
322332
});
323333

324-
it('default to base discussions url when graphql query fails', async () => {
325-
const rendererLogErrorSpy = jest
326-
.spyOn(logger, 'rendererLogError')
327-
.mockImplementation();
328-
334+
it('when discussion found via graphql api, link to matching discussion and comment hash', async () => {
329335
const subject = {
330336
title: '1.16.0',
331337
url: null,
332338
latest_comment_url: null,
333339
type: 'Discussion' as SubjectType,
334340
};
335341

336-
apiRequestAuthSpy.mockResolvedValue(null as AxiosResponse);
342+
apiRequestAuthSpy.mockResolvedValue({
343+
data: {
344+
...mockGraphQLResponse,
345+
},
346+
} as AxiosResponse);
337347

338348
const result = await generateGitHubWebUrl({
339349
...mockSingleNotification,
@@ -342,9 +352,8 @@ describe('renderer/utils/helpers.ts', () => {
342352

343353
expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1);
344354
expect(result).toBe(
345-
`https://github.com/gitify-app/notifications-test/discussions?${mockNotificationReferrer}`,
355+
`https://github.com/gitify-app/notifications-test/discussions/612?${mockNotificationReferrer}#discussioncomment-2300902`,
346356
);
347-
expect(rendererLogErrorSpy).toHaveBeenCalledTimes(1);
348357
});
349358
});
350359

@@ -447,8 +456,8 @@ describe('renderer/utils/helpers.ts', () => {
447456
});
448457
});
449458

450-
describe('defaults to repository url', () => {
451-
it('defaults when no urls present in notification', async () => {
459+
describe('defaults web urls', () => {
460+
it('issues - defaults when no urls present in notification', async () => {
452461
const subject = {
453462
title: 'generate github web url unit tests',
454463
url: null,
@@ -463,7 +472,45 @@ describe('renderer/utils/helpers.ts', () => {
463472

464473
expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0);
465474
expect(result).toBe(
466-
`${mockSingleNotification.repository.html_url}?${mockNotificationReferrer}`,
475+
`https://github.com/gitify-app/notifications-test/issues?${mockNotificationReferrer}`,
476+
);
477+
});
478+
479+
it('issues - defaults when no urls present in notification', async () => {
480+
const subject = {
481+
title: 'generate github web url unit tests',
482+
url: null,
483+
latest_comment_url: null,
484+
type: 'PullRequest' as SubjectType,
485+
};
486+
487+
const result = await generateGitHubWebUrl({
488+
...mockSingleNotification,
489+
subject: subject,
490+
});
491+
492+
expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0);
493+
expect(result).toBe(
494+
`https://github.com/gitify-app/notifications-test/pulls?${mockNotificationReferrer}`,
495+
);
496+
});
497+
498+
it('other - defaults when no urls present in notification', async () => {
499+
const subject = {
500+
title: 'generate github web url unit tests',
501+
url: null,
502+
latest_comment_url: null,
503+
type: 'Commit' as SubjectType,
504+
};
505+
506+
const result = await generateGitHubWebUrl({
507+
...mockSingleNotification,
508+
subject: subject,
509+
});
510+
511+
expect(apiRequestAuthSpy).toHaveBeenCalledTimes(0);
512+
expect(result).toBe(
513+
`https://github.com/gitify-app/notifications-test?${mockNotificationReferrer}`,
467514
);
468515
});
469516

@@ -474,31 +521,23 @@ describe('renderer/utils/helpers.ts', () => {
474521

475522
const subject = {
476523
title: 'generate github web url unit tests',
477-
url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1' as Link,
478-
latest_comment_url:
479-
'https://api.github.com/repos/gitify-app/notifications-test/issues/comments/302888448' as Link,
480-
type: 'Issue' as SubjectType,
524+
url: 'https://api.github.com/repos/gitify-app/notifications-test/discussion/1' as Link,
525+
latest_comment_url: null as Link,
526+
type: 'Discussion' as SubjectType,
481527
};
482528

483-
const mockError = new Error('Test error');
484-
485-
apiRequestAuthSpy.mockRejectedValue(mockError);
529+
apiRequestAuthSpy.mockRejectedValue(new Error('Test error'));
486530

487531
const result = await generateGitHubWebUrl({
488532
...mockSingleNotification,
489533
subject: subject,
490534
});
491535

492536
expect(apiRequestAuthSpy).toHaveBeenCalledTimes(1);
493-
expect(apiRequestAuthSpy).toHaveBeenCalledWith(
494-
subject.latest_comment_url,
495-
'GET',
496-
mockPersonalAccessTokenAccount.token,
497-
);
498537
expect(result).toBe(
499-
`https://github.com/gitify-app/notifications-test?${mockNotificationReferrer}`,
538+
`https://github.com/gitify-app/notifications-test/discussions?${mockNotificationReferrer}`,
500539
);
501-
expect(rendererLogErrorSpy).toHaveBeenCalledTimes(2);
540+
expect(rendererLogErrorSpy).toHaveBeenCalledTimes(1);
502541
});
503542
});
504543
});

‎src/renderer/utils/helpers.ts‎

Lines changed: 52 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66

77
import { Constants } from '../constants';
88
import type { Chevron, Hostname, Link } from '../types';
9-
import type { Notification } from '../typesGitHub';
9+
import type { Notification, SubjectType } from '../typesGitHub';
1010
import { getHtmlUrl, getLatestDiscussion } from './api/client';
1111
import type { PlatformType } from './auth/types';
1212
import { rendererLogError } from './logger';
@@ -106,48 +106,38 @@ export async function generateGitHubWebUrl(
106106
): Promise<Link> {
107107
const url = new URL(notification.repository.html_url);
108108

109-
/**
110-
* Discussions - override generic response values for subject and comment urls,
111-
* as we will fetch more specific html urls ourselves below
112-
* See issue #1583:
113-
*/
114-
if (notification.subject.type === 'Discussion') {
115-
notification.subject.url = null;
116-
notification.subject.latest_comment_url = null;
117-
}
118-
119109
try {
120-
if (notification.subject.latest_comment_url) {
121-
url.href = await getHtmlUrl(
122-
notification.subject.latest_comment_url,
123-
notification.account.token,
124-
);
125-
} else if (notification.subject.url) {
126-
url.href = await getHtmlUrl(
127-
notification.subject.url,
128-
notification.account.token,
129-
);
130-
} else {
131-
// Perform any specific notification type handling (only required for a few special notification scenarios)
132-
switch (notification.subject.type) {
133-
case 'CheckSuite':
134-
url.href = getCheckSuiteUrl(notification);
135-
break;
136-
case 'Discussion':
137-
url.href = await getDiscussionUrl(notification);
138-
break;
139-
case 'RepositoryInvitation':
140-
url.pathname += '/invitations';
141-
break;
142-
case 'RepositoryDependabotAlertsThread':
143-
url.pathname += '/security/dependabot';
144-
break;
145-
case 'WorkflowRun':
146-
url.href = getWorkflowRunUrl(notification);
147-
break;
148-
default:
149-
break;
150-
}
110+
// Perform any specific notification type handling (only required for a few special notification scenarios)
111+
switch (notification.subject.type) {
112+
case 'CheckSuite':
113+
url.href = getCheckSuiteUrl(notification);
114+
break;
115+
case 'Discussion':
116+
url.href = await getDiscussionUrl(notification);
117+
break;
118+
case 'RepositoryInvitation':
119+
url.pathname += '/invitations';
120+
break;
121+
case 'RepositoryDependabotAlertsThread':
122+
url.pathname += '/security/dependabot';
123+
break;
124+
case 'WorkflowRun':
125+
url.href = getWorkflowRunUrl(notification);
126+
break;
127+
default:
128+
if (notification.subject.latest_comment_url) {
129+
url.href = await getHtmlUrl(
130+
notification.subject.latest_comment_url,
131+
notification.account.token,
132+
);
133+
} else if (notification.subject.url) {
134+
url.href = await getHtmlUrl(
135+
notification.subject.url,
136+
notification.account.token,
137+
);
138+
} else {
139+
url.href = defaultGitHubWebUrl(url, notification.subject.type);
140+
}
151141
}
152142
} catch (err) {
153143
rendererLogError(
@@ -156,6 +146,8 @@ export async function generateGitHubWebUrl(
156146
err,
157147
notification,
158148
);
149+
150+
url.href = defaultGitHubWebUrl(url, notification.subject.type);
159151
}
160152

161153
url.searchParams.set(
@@ -166,6 +158,24 @@ export async function generateGitHubWebUrl(
166158
return url.toString() as Link;
167159
}
168160

161+
export function defaultGitHubWebUrl(url: URL, type: SubjectType) {
162+
switch (type) {
163+
case 'Issue':
164+
url.pathname += '/issues';
165+
break;
166+
case 'Discussion':
167+
url.pathname += '/discussions';
168+
break;
169+
case 'PullRequest':
170+
url.pathname += '/pulls';
171+
break;
172+
default:
173+
break;
174+
}
175+
176+
return url.href;
177+
}
178+
169179
export function getChevronDetails(
170180
hasNotifications: boolean,
171181
isVisible: boolean,

0 commit comments

Comments
 (0)