From f56f7c75ef00121193151c6df6507a5a423cb13d Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Wed, 1 May 2024 09:31:00 -1000 Subject: [PATCH 01/14] feat: add icon for previously approved prs --- src/__mocks__/partial-mocks.ts | 2 + src/components/NotificationRow.tsx | 13 ++++ src/hooks/useNotifications.ts | 7 +- src/typesGitHub.ts | 31 +++++++++ src/utils/api/client.ts | 13 ++++ src/utils/helpers.test.ts | 13 ++++ src/utils/helpers.ts | 10 +++ src/utils/subject.test.ts | 102 ++++++++++++++--------------- src/utils/subject.ts | 49 +++++++++++++- 9 files changed, 180 insertions(+), 60 deletions(-) diff --git a/src/__mocks__/partial-mocks.ts b/src/__mocks__/partial-mocks.ts index 2c8b52b72..f0d2c0ef6 100644 --- a/src/__mocks__/partial-mocks.ts +++ b/src/__mocks__/partial-mocks.ts @@ -1,9 +1,11 @@ import type { Notification, Subject, User } from '../typesGitHub'; +import Constants from '../utils/constants'; export function partialMockNotification( subject: Partial, ): Notification { const mockNotification: Partial = { + hostname: Constants.GITHUB_API_BASE_URL, subject: subject as Subject, }; diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index 410115059..dc27bbc35 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -1,5 +1,6 @@ import { BellSlashIcon, + CheckCircleFillIcon, CheckIcon, FeedPersonIcon, ReadIcon, @@ -136,6 +137,18 @@ export const NotificationRow: FC = ({ notification, hostname }) => { + {notification.subject.isApprovedByUser && ( +
+ + + +
+ )} +
- {notification.subject.isApprovedByUser && ( + {prApprovalIcon && (
- - + +
)} diff --git a/src/types.ts b/src/types.ts index 70adfd7a2..75395fa44 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,3 +1,5 @@ +import type { OcticonProps } from '@primer/octicons-react'; +import type { FC } from 'react'; import type { Notification } from './typesGitHub'; export interface AuthState { @@ -102,3 +104,9 @@ export enum IconColor { PURPLE = 'text-purple-500', GRAY = 'text-gray-500 dark:text-gray-300', } + +export type PullRequestApprovalIcon = { + type: FC; + color: IconColor; + description: string; +}; diff --git a/src/typesGitHub.ts b/src/typesGitHub.ts index c6b427c3c..2f684b2c9 100644 --- a/src/typesGitHub.ts +++ b/src/typesGitHub.ts @@ -251,7 +251,7 @@ interface GitHubSubject { export interface GitifySubject { state?: StateType; user?: SubjectUser; - isApprovedByUser?: boolean; + approvalState?: PullRequestReviewState; } export interface PullRequest { diff --git a/src/utils/icons.test.ts b/src/utils/icons.test.ts index 15fb35651..4ebfcbe02 100644 --- a/src/utils/icons.test.ts +++ b/src/utils/icons.test.ts @@ -1,5 +1,11 @@ +import { CheckIcon, CommentIcon, FileDiffIcon } from '@primer/octicons-react'; +import { IconColor } from '../types'; import type { StateType, Subject, SubjectType } from '../typesGitHub'; -import { getNotificationTypeIcon, getNotificationTypeIconColor } from './icons'; +import { + getNotificationTypeIcon, + getNotificationTypeIconColor, + getPullRequestReviewIcon, +} from './icons'; describe('utils/icons.ts', () => { describe('getNotificationTypeIcon - should get the notification type icon', () => { @@ -248,6 +254,31 @@ describe('utils/icons.ts', () => { ).toMatchSnapshot(); }); }); + + describe('getPullRequestReviewIcon - should get the approval icon', () => { + expect(getPullRequestReviewIcon('APPROVED')).toEqual({ + type: CheckIcon, + color: IconColor.GREEN, + description: 'You approved these changes', + }); + expect(getPullRequestReviewIcon('CHANGES_REQUESTED')).toEqual({ + type: FileDiffIcon, + color: IconColor.RED, + description: 'You requested changes', + }); + expect(getPullRequestReviewIcon('COMMENTED')).toEqual({ + type: CommentIcon, + color: IconColor.GRAY, + description: 'You left review comments', + }); + expect(getPullRequestReviewIcon('DISMISSED')).toEqual({ + type: CommentIcon, + color: IconColor.GRAY, + description: 'Your review has been dismissed', + }); + expect(getPullRequestReviewIcon('PENDING')).toBeNull(); + expect(getPullRequestReviewIcon(null)).toBeNull(); + }); }); function createSubjectMock(mocks: { diff --git a/src/utils/icons.ts b/src/utils/icons.ts index 609421457..d9f54c33d 100644 --- a/src/utils/icons.ts +++ b/src/utils/icons.ts @@ -2,9 +2,11 @@ import { AlertIcon, CheckIcon, CommentDiscussionIcon, + CommentIcon, DiscussionClosedIcon, DiscussionDuplicateIcon, DiscussionOutdatedIcon, + FileDiffIcon, GitCommitIcon, GitMergeIcon, GitPullRequestClosedIcon, @@ -24,8 +26,8 @@ import { XIcon, } from '@primer/octicons-react'; import type { FC } from 'react'; -import { IconColor } from '../types'; -import type { Subject } from '../typesGitHub'; +import { IconColor, type PullRequestApprovalIcon } from '../types'; +import type { PullRequestReviewState, Subject } from '../typesGitHub'; export function getNotificationTypeIcon(subject: Subject): FC { switch (subject.type) { @@ -111,3 +113,36 @@ export function getNotificationTypeIconColor(subject: Subject): IconColor { return IconColor.GRAY; } } + +export function getPullRequestReviewIcon( + reviewState: PullRequestReviewState, +): PullRequestApprovalIcon | null { + switch (reviewState) { + case 'APPROVED': + return { + type: CheckIcon, + color: IconColor.GREEN, + description: 'You approved these changes', + }; + case 'CHANGES_REQUESTED': + return { + type: FileDiffIcon, + color: IconColor.RED, + description: 'You requested changes', + }; + case 'COMMENTED': + return { + type: CommentIcon, + color: IconColor.GRAY, + description: 'You left review comments', + }; + case 'DISMISSED': + return { + type: CommentIcon, + color: IconColor.GRAY, + description: 'Your review has been dismissed', + }; + default: + return null; + } +} diff --git a/src/utils/subject.ts b/src/utils/subject.ts index 34cb0ecf0..fd746cf5a 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -5,6 +5,7 @@ import type { DiscussionStateType, GitifySubject, Notification, + PullRequestReviewState, PullRequestStateType, SubjectUser, User, @@ -250,7 +251,7 @@ async function getGitifySubjectForPullRequest( prCommentUser = prComment.user; } - const userApprovedPR = await checkIfUserApprovedPR(notification, accounts); + const approvalState = await getUserApprovalState(notification, accounts); return { state: prState, @@ -260,19 +261,19 @@ async function getGitifySubjectForPullRequest( avatar_url: prCommentUser?.avatar_url ?? pr.user.avatar_url, type: prCommentUser?.type ?? pr.user.type, }, - isApprovedByUser: userApprovedPR, + approvalState: approvalState, }; } catch (err) { console.error('Pull Request subject retrieval failed'); } } -async function checkIfUserApprovedPR( +async function getUserApprovalState( notification: Notification, accounts: AuthState, -) { +): Promise | null { if (notification.subject.type !== 'PullRequest') { - return false; + return; } const token = getTokenForHost(notification.hostname, accounts); @@ -284,13 +285,13 @@ async function checkIfUserApprovedPR( ); if (!prReviews) { - return false; + return; } - return prReviews.data.some( - (prReview) => - prReview.state === 'APPROVED' && prReview.user.login === login, - ); + // Find the last occurrence of the user's review, as this is the most recent + return prReviews.data + .reverse() + .find((prReview) => prReview.user.login === login)?.state; } async function getGitifySubjectForRelease( From aedcbb9ca0664d3be42ccc2aba0263670e616f5b Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Wed, 1 May 2024 15:54:42 -1000 Subject: [PATCH 03/14] feat: add pr review icons (approved, requested changes, commented, dismissed) --- src/components/NotificationRow.tsx | 15 ++-- src/typesGitHub.ts | 2 +- src/utils/subject.test.ts | 110 +++++++++++++++++++++++++---- src/utils/subject.ts | 13 ++-- 4 files changed, 114 insertions(+), 26 deletions(-) diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index 7e08aa8a6..b43d54465 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -71,10 +71,10 @@ export const NotificationRow: FC = ({ notification, hostname }) => { const NotificationIcon = getNotificationTypeIcon(notification.subject); const iconColor = getNotificationTypeIconColor(notification.subject); - const prApprovalIcon = getPullRequestReviewIcon( - notification.subject.approvalState, + const latestSelfReviewIcon = getPullRequestReviewIcon( + notification.subject.latestSelfReviewState, ); - const PrApprovalIcon = prApprovalIcon?.type; + const PrSelfApprovalIcon = latestSelfReviewIcon?.type; const updatedAt = formatDistanceToNow(parseISO(notification.updated_at), { addSuffix: true, @@ -143,10 +143,13 @@ export const NotificationRow: FC = ({ notification, hostname }) => { - {prApprovalIcon && ( + {latestSelfReviewIcon && (
- - + +
)} diff --git a/src/typesGitHub.ts b/src/typesGitHub.ts index 2f684b2c9..02d16575c 100644 --- a/src/typesGitHub.ts +++ b/src/typesGitHub.ts @@ -251,7 +251,7 @@ interface GitHubSubject { export interface GitifySubject { state?: StateType; user?: SubjectUser; - approvalState?: PullRequestReviewState; + latestSelfReviewState?: PullRequestReviewState; } export interface PullRequest { diff --git a/src/utils/subject.test.ts b/src/utils/subject.test.ts index c394ba29c..3d7ea0b7c 100644 --- a/src/utils/subject.test.ts +++ b/src/utils/subject.test.ts @@ -612,7 +612,7 @@ describe('utils/subject.ts', () => { type: 'PullRequest', url: 'https://api.github.com/repos/gitify-app/notifications-test/pulls/1', latest_comment_url: - 'https://api.github.com/repos/gitify-app/notifications-test/pull/1', + 'https://api.github.com/repos/gitify-app/notifications-test/issues/comments/302888448', }); }); @@ -626,9 +626,9 @@ describe('utils/subject.ts', () => { user: mockAuthor, }); - // nock('https://api.github.com') - // .get('/repos/gitify-app/notifications-test/pull/1/reviews') - // .reply(200, { user: mockReviewer }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/issues/comments/302888448') + .reply(200, { user: mockCommenter }); const result = await getGitifySubjectDetails( mockNotification, @@ -656,9 +656,9 @@ describe('utils/subject.ts', () => { user: mockAuthor, }); - // nock('https://api.github.com') - // .get('/repos/gitify-app/notifications-test/pull/1/reviews') - // .reply(200, { user: mockCommenter }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/issues/comments/302888448') + .reply(200, { user: mockCommenter }); const result = await getGitifySubjectDetails( mockNotification, @@ -686,9 +686,9 @@ describe('utils/subject.ts', () => { user: mockAuthor, }); - // nock('https://api.github.com') - // .get('/repos/gitify-app/notifications-test/issues/comments/302888448') - // .reply(200, { user: mockCommenter }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/issues/comments/302888448') + .reply(200, { user: mockCommenter }); const result = await getGitifySubjectDetails( mockNotification, @@ -716,9 +716,9 @@ describe('utils/subject.ts', () => { user: mockAuthor, }); - // nock('https://api.github.com') - // .get('/repos/gitify-app/notifications-test/issues/comments/302888448') - // .reply(200, { user: mockCommenter }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/issues/comments/302888448') + .reply(200, { user: mockCommenter }); const result = await getGitifySubjectDetails( mockNotification, @@ -763,6 +763,90 @@ describe('utils/subject.ts', () => { }, }); }); + + describe('Pull Request Reviews', () => { + beforeEach(() => { + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1') + .reply(200, { + state: 'open', + draft: false, + merged: false, + user: mockAuthor, + }); + + nock('https://api.github.com') + .get( + '/repos/gitify-app/notifications-test/issues/comments/302888448', + ) + .reply(200, { user: mockCommenter }); + }); + + it('fetches latest self review state - none found for self', async () => { + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1/reviews') + .reply(200, [ + { + user: { + login: 'some-other-user', + }, + state: 'APPROVED', + }, + ]); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts, + ); + + expect(result.latestSelfReviewState).toBeNull(); + }); + + it('fetches latest self review state - return latest result', async () => { + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1/reviews') + .reply(200, [ + { + user: { + login: mockAccounts.user.login, + }, + state: 'REQUESTED_CHANGES', + }, + { + user: { + login: 'some-other-user', + }, + state: 'COMMENTED', + }, + { + user: { + login: mockAccounts.user.login, + }, + state: 'APPROVED', + }, + ]); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts, + ); + + expect(result.latestSelfReviewState).toEqual('APPROVED'); + }); + + it('fetches latest self review state - no reviews', async () => { + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1/reviews') + .reply(200, []); + + const result = await getGitifySubjectDetails( + mockNotification, + mockAccounts, + ); + + expect(result.latestSelfReviewState).toBeNull(); + }); + }); }); describe('Releases', () => { diff --git a/src/utils/subject.ts b/src/utils/subject.ts index fd746cf5a..882550cf5 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -251,7 +251,7 @@ async function getGitifySubjectForPullRequest( prCommentUser = prComment.user; } - const approvalState = await getUserApprovalState(notification, accounts); + const selfApproval = await getLatestSelfApproval(notification, accounts); return { state: prState, @@ -261,14 +261,14 @@ async function getGitifySubjectForPullRequest( avatar_url: prCommentUser?.avatar_url ?? pr.user.avatar_url, type: prCommentUser?.type ?? pr.user.type, }, - approvalState: approvalState, + latestSelfReviewState: selfApproval, }; } catch (err) { console.error('Pull Request subject retrieval failed'); } } -async function getUserApprovalState( +async function getLatestSelfApproval( notification: Notification, accounts: AuthState, ): Promise | null { @@ -289,9 +289,10 @@ async function getUserApprovalState( } // Find the last occurrence of the user's review, as this is the most recent - return prReviews.data - .reverse() - .find((prReview) => prReview.user.login === login)?.state; + return ( + prReviews.data.reverse().find((prReview) => prReview.user.login === login) + ?.state ?? null + ); } async function getGitifySubjectForRelease( From 57a7e885d3162c0fc5f27d1c6a731b95ee288808 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Wed, 1 May 2024 16:03:27 -1000 Subject: [PATCH 04/14] fix tests --- src/hooks/useNotifications.test.ts | 12 ++++++++---- src/utils/subject.test.ts | 25 +++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index 0ac0ae070..9e48c1dcd 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -403,7 +403,7 @@ describe('hooks/useNotifications.ts', () => { subject: { title: 'This is an Issue.', type: 'Issue', - url: 'https://api.github.com/1', + url: 'https://api.github.com/repos/gitify-app/notifications-test/issues/1', latest_comment_url: null, }, repository: { @@ -415,7 +415,7 @@ describe('hooks/useNotifications.ts', () => { subject: { title: 'This is a Pull Request.', type: 'PullRequest', - url: 'https://api.github.com/2', + url: 'https://api.github.com/repos/gitify-app/notifications-test/pulls/2', latest_comment_url: null, }, repository: { @@ -427,8 +427,9 @@ describe('hooks/useNotifications.ts', () => { nock('https://api.github.com') .get('/notifications?participating=false') .reply(200, notifications); + nock('https://api.github.com') - .get('/1') + .get('/repos/gitify-app/notifications-test/issues/1') .reply(200, { state: 'closed', merged: true, @@ -438,7 +439,7 @@ describe('hooks/useNotifications.ts', () => { }, }); nock('https://api.github.com') - .get('/2') + .get('/repos/gitify-app/notifications-test/pulls/2') .reply(200, { state: 'closed', merged: false, @@ -447,6 +448,9 @@ describe('hooks/useNotifications.ts', () => { type: 'Bot', }, }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/2/reviews') + .reply(200, []); const { result } = renderHook(() => useNotifications()); diff --git a/src/utils/subject.test.ts b/src/utils/subject.test.ts index 3d7ea0b7c..42be987c7 100644 --- a/src/utils/subject.test.ts +++ b/src/utils/subject.test.ts @@ -630,6 +630,10 @@ describe('utils/subject.ts', () => { .get('/repos/gitify-app/notifications-test/issues/comments/302888448') .reply(200, { user: mockCommenter }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1/reviews') + .reply(200, []); + const result = await getGitifySubjectDetails( mockNotification, mockAccounts, @@ -643,6 +647,7 @@ describe('utils/subject.ts', () => { avatar_url: mockCommenter.avatar_url, type: mockCommenter.type, }, + latestSelfReviewState: null, }); }); @@ -660,6 +665,10 @@ describe('utils/subject.ts', () => { .get('/repos/gitify-app/notifications-test/issues/comments/302888448') .reply(200, { user: mockCommenter }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1/reviews') + .reply(200, []); + const result = await getGitifySubjectDetails( mockNotification, mockAccounts, @@ -673,6 +682,7 @@ describe('utils/subject.ts', () => { avatar_url: mockCommenter.avatar_url, type: mockCommenter.type, }, + latestSelfReviewState: null, }); }); @@ -690,6 +700,10 @@ describe('utils/subject.ts', () => { .get('/repos/gitify-app/notifications-test/issues/comments/302888448') .reply(200, { user: mockCommenter }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1/reviews') + .reply(200, []); + const result = await getGitifySubjectDetails( mockNotification, mockAccounts, @@ -703,6 +717,7 @@ describe('utils/subject.ts', () => { avatar_url: mockCommenter.avatar_url, type: mockCommenter.type, }, + latestSelfReviewState: null, }); }); @@ -720,6 +735,10 @@ describe('utils/subject.ts', () => { .get('/repos/gitify-app/notifications-test/issues/comments/302888448') .reply(200, { user: mockCommenter }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1/reviews') + .reply(200, []); + const result = await getGitifySubjectDetails( mockNotification, mockAccounts, @@ -733,6 +752,7 @@ describe('utils/subject.ts', () => { avatar_url: mockCommenter.avatar_url, type: mockCommenter.type, }, + latestSelfReviewState: null, }); }); @@ -748,6 +768,10 @@ describe('utils/subject.ts', () => { user: mockAuthor, }); + nock('https://api.github.com') + .get('/repos/gitify-app/notifications-test/pulls/1/reviews') + .reply(200, []); + const result = await getGitifySubjectDetails( mockNotification, mockAccounts, @@ -761,6 +785,7 @@ describe('utils/subject.ts', () => { avatar_url: mockAuthor.avatar_url, type: mockAuthor.type, }, + latestSelfReviewState: null, }); }); From 2a3aef8fdb7cddcb0ef031c842c55aa8afff9faa Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Wed, 1 May 2024 16:24:53 -1000 Subject: [PATCH 05/14] update coverage --- src/utils/subject.test.ts | 49 ++++++++++++++++++--------------------- src/utils/subject.ts | 9 +++---- 2 files changed, 25 insertions(+), 33 deletions(-) diff --git a/src/utils/subject.test.ts b/src/utils/subject.test.ts index 42be987c7..228b7f429 100644 --- a/src/utils/subject.test.ts +++ b/src/utils/subject.test.ts @@ -17,6 +17,7 @@ import type { import { getCheckSuiteAttributes, getGitifySubjectDetails, + getLatestSelfApproval, getWorkflowRunAttributes, } from './subject'; @@ -789,25 +790,8 @@ describe('utils/subject.ts', () => { }); }); - describe('Pull Request Reviews', () => { - beforeEach(() => { - nock('https://api.github.com') - .get('/repos/gitify-app/notifications-test/pulls/1') - .reply(200, { - state: 'open', - draft: false, - merged: false, - user: mockAuthor, - }); - - nock('https://api.github.com') - .get( - '/repos/gitify-app/notifications-test/issues/comments/302888448', - ) - .reply(200, { user: mockCommenter }); - }); - - it('fetches latest self review state - none found for self', async () => { + describe('Pull Request Reviews - Latest Self Review', () => { + it('returns null when no review found for matching user', async () => { nock('https://api.github.com') .get('/repos/gitify-app/notifications-test/pulls/1/reviews') .reply(200, [ @@ -819,15 +803,15 @@ describe('utils/subject.ts', () => { }, ]); - const result = await getGitifySubjectDetails( + const result = await getLatestSelfApproval( mockNotification, mockAccounts, ); - expect(result.latestSelfReviewState).toBeNull(); + expect(result).toBeNull(); }); - it('fetches latest self review state - return latest result', async () => { + it('returns latest review state for matching user', async () => { nock('https://api.github.com') .get('/repos/gitify-app/notifications-test/pulls/1/reviews') .reply(200, [ @@ -851,25 +835,36 @@ describe('utils/subject.ts', () => { }, ]); - const result = await getGitifySubjectDetails( + const result = await getLatestSelfApproval( mockNotification, mockAccounts, ); - expect(result.latestSelfReviewState).toEqual('APPROVED'); + expect(result).toEqual('APPROVED'); }); - it('fetches latest self review state - no reviews', async () => { + it('returns null when no reviews on PR yet', async () => { nock('https://api.github.com') .get('/repos/gitify-app/notifications-test/pulls/1/reviews') .reply(200, []); - const result = await getGitifySubjectDetails( + const result = await getLatestSelfApproval( + mockNotification, + mockAccounts, + ); + + expect(result).toBeNull(); + }); + + it('returns null when not a PR notification', async () => { + mockNotification.subject.type = 'Issue'; + + const result = await getLatestSelfApproval( mockNotification, mockAccounts, ); - expect(result.latestSelfReviewState).toBeNull(); + expect(result).toBeNull(); }); }); }); diff --git a/src/utils/subject.ts b/src/utils/subject.ts index 882550cf5..28ae9eca8 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -268,12 +268,13 @@ async function getGitifySubjectForPullRequest( } } -async function getLatestSelfApproval( +export async function getLatestSelfApproval( notification: Notification, accounts: AuthState, ): Promise | null { + console.log('ADAM ', notification.subject.type); if (notification.subject.type !== 'PullRequest') { - return; + return null; } const token = getTokenForHost(notification.hostname, accounts); @@ -284,10 +285,6 @@ async function getLatestSelfApproval( token, ); - if (!prReviews) { - return; - } - // Find the last occurrence of the user's review, as this is the most recent return ( prReviews.data.reverse().find((prReview) => prReview.user.login === login) From 2c1de507a732160175918f2ae8c1f81126616605 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 3 May 2024 05:57:04 -1000 Subject: [PATCH 06/14] feat: move icon to notification row footer. include all reviewers --- src/components/NotificationRow.tsx | 34 ++++++++-------- src/typesGitHub.ts | 7 +++- src/utils/helpers.test.ts | 13 ------ src/utils/helpers.ts | 10 ----- src/utils/icons.ts | 18 +++++---- src/utils/notifications.ts | 10 +---- src/utils/subject.test.ts | 60 ++++++++++++--------------- src/utils/subject.ts | 65 ++++++++++++++++++++---------- 8 files changed, 106 insertions(+), 111 deletions(-) diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index b43d54465..1333622cf 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -71,11 +71,6 @@ export const NotificationRow: FC = ({ notification, hostname }) => { const NotificationIcon = getNotificationTypeIcon(notification.subject); const iconColor = getNotificationTypeIconColor(notification.subject); - const latestSelfReviewIcon = getPullRequestReviewIcon( - notification.subject.latestSelfReviewState, - ); - const PrSelfApprovalIcon = latestSelfReviewIcon?.type; - const updatedAt = formatDistanceToNow(parseISO(notification.updated_at), { addSuffix: true, }); @@ -138,22 +133,29 @@ export const NotificationRow: FC = ({ notification, hostname }) => { {reason.title} {updatedAt} + {notification.subject?.reviews + ? notification.subject.reviews.map((review) => { + const icon = getPullRequestReviewIcon(review); + return ( + + + + ); + }) + : null} - {latestSelfReviewIcon && ( -
- - - -
- )} -
@@ -267,6 +321,60 @@ exports[`components/NotificationRow.tsx should render itself & its children with > over 6 years ago + + + + + + + + + + diff --git a/src/routes/__snapshots__/Notifications.test.tsx.snap b/src/routes/__snapshots__/Notifications.test.tsx.snap index 8495c7836..0442f4937 100644 --- a/src/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/routes/__snapshots__/Notifications.test.tsx.snap @@ -172,7 +172,20 @@ exports[`routes/Notifications.tsx should render itself & its children (show acco }, "subject": { "latest_comment_url": "https://api.github.com/repos/gitify-app/notifications-test/issues/comments/302888448", - "reviews": null, + "reviews": [ + { + "state": "APPROVED", + "users": [ + "octocat", + ], + }, + { + "state": "CHANGES_REQUESTED", + "users": [ + "gitify-app", + ], + }, + ], "state": "open", "title": "I am a robot and this is a test!", "type": "Issue", @@ -323,7 +336,20 @@ exports[`routes/Notifications.tsx should render itself & its children (with noti }, "subject": { "latest_comment_url": "https://api.github.com/repos/gitify-app/notifications-test/issues/comments/302888448", - "reviews": null, + "reviews": [ + { + "state": "APPROVED", + "users": [ + "octocat", + ], + }, + { + "state": "CHANGES_REQUESTED", + "users": [ + "gitify-app", + ], + }, + ], "state": "open", "title": "I am a robot and this is a test!", "type": "Issue", From 0bdc0f4ec6e75ac38758509309e6af0d60fc51b1 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 3 May 2024 06:46:45 -1000 Subject: [PATCH 09/14] feat: move icon to notification row footer. include all reviewers --- src/__mocks__/mockedData.ts | 4 ++++ src/routes/__snapshots__/Notifications.test.tsx.snap | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/__mocks__/mockedData.ts b/src/__mocks__/mockedData.ts index 5b9fa0022..e4c916072 100644 --- a/src/__mocks__/mockedData.ts +++ b/src/__mocks__/mockedData.ts @@ -81,6 +81,10 @@ export const mockedGitHubNotifications: Notification[] = [ state: 'CHANGES_REQUESTED', users: ['gitify-app'], }, + { + state: 'PENDING', + users: ['gitify-user'], + }, ], }, repository: { diff --git a/src/routes/__snapshots__/Notifications.test.tsx.snap b/src/routes/__snapshots__/Notifications.test.tsx.snap index 0442f4937..887334d78 100644 --- a/src/routes/__snapshots__/Notifications.test.tsx.snap +++ b/src/routes/__snapshots__/Notifications.test.tsx.snap @@ -185,6 +185,12 @@ exports[`routes/Notifications.tsx should render itself & its children (show acco "gitify-app", ], }, + { + "state": "PENDING", + "users": [ + "gitify-user", + ], + }, ], "state": "open", "title": "I am a robot and this is a test!", @@ -349,6 +355,12 @@ exports[`routes/Notifications.tsx should render itself & its children (with noti "gitify-app", ], }, + { + "state": "PENDING", + "users": [ + "gitify-user", + ], + }, ], "state": "open", "title": "I am a robot and this is a test!", From 82cebff926bbaafba99ddd8e7ce7913497b08cfb Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 3 May 2024 08:24:58 -1000 Subject: [PATCH 10/14] update commented icon color to green. ensure ordering is consistent --- src/utils/icons.test.ts | 4 ++-- src/utils/icons.ts | 2 +- src/utils/subject.ts | 8 +++++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/utils/icons.test.ts b/src/utils/icons.test.ts index 9c727d73a..d309dc506 100644 --- a/src/utils/icons.test.ts +++ b/src/utils/icons.test.ts @@ -314,13 +314,13 @@ describe('utils/icons.ts', () => { expect(getPullRequestReviewIcon(mockReviewSingleReviewer)).toEqual({ type: CommentIcon, - color: IconColor.GRAY, + color: IconColor.GREEN, description: 'user1 left review comments', }); expect(getPullRequestReviewIcon(mockReviewMultipleReviewer)).toEqual({ type: CommentIcon, - color: IconColor.GRAY, + color: IconColor.GREEN, description: 'user1, user2 left review comments', }); }); diff --git a/src/utils/icons.ts b/src/utils/icons.ts index 951310ad4..a56495608 100644 --- a/src/utils/icons.ts +++ b/src/utils/icons.ts @@ -135,7 +135,7 @@ export function getPullRequestReviewIcon( case 'COMMENTED': return { type: CommentIcon, - color: IconColor.GRAY, + color: IconColor.GREEN, description: `${descriptionPrefix} left review comments`, }; case 'DISMISSED': diff --git a/src/utils/subject.ts b/src/utils/subject.ts index 87c0f4216..50100d24d 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -308,7 +308,13 @@ export async function getLatestReviewForReviewers( reviewerFound.users.push(prReview.user.login); } - return reviewers; + // Sort the reviews by state for consistent draw order + return [ + ...reviewers.filter((review) => review.state === 'APPROVED'), + ...reviewers.filter((review) => review.state === 'CHANGES_REQUESTED'), + ...reviewers.filter((review) => review.state === 'COMMENTED'), + ...reviewers.filter((review) => review.state === 'DISMISSED'), + ]; } async function getGitifySubjectForRelease( From cec83fb7594d738716facb00814f051428f4f195 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 3 May 2024 08:37:21 -1000 Subject: [PATCH 11/14] update commented icon color to yellow. ensure ordering is consistent --- src/types.ts | 1 + src/utils/icons.test.ts | 4 ++-- src/utils/icons.ts | 2 +- src/utils/subject.ts | 1 + 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/types.ts b/src/types.ts index 75395fa44..5c0593a62 100644 --- a/src/types.ts +++ b/src/types.ts @@ -103,6 +103,7 @@ export enum IconColor { RED = 'text-red-500', PURPLE = 'text-purple-500', GRAY = 'text-gray-500 dark:text-gray-300', + YELLOW = 'text-yellow-500 dark:text-yellow-300', } export type PullRequestApprovalIcon = { diff --git a/src/utils/icons.test.ts b/src/utils/icons.test.ts index d309dc506..7f1f7747f 100644 --- a/src/utils/icons.test.ts +++ b/src/utils/icons.test.ts @@ -314,13 +314,13 @@ describe('utils/icons.ts', () => { expect(getPullRequestReviewIcon(mockReviewSingleReviewer)).toEqual({ type: CommentIcon, - color: IconColor.GREEN, + color: IconColor.YELLOW, description: 'user1 left review comments', }); expect(getPullRequestReviewIcon(mockReviewMultipleReviewer)).toEqual({ type: CommentIcon, - color: IconColor.GREEN, + color: IconColor.YELLOW, description: 'user1, user2 left review comments', }); }); diff --git a/src/utils/icons.ts b/src/utils/icons.ts index a56495608..b85864e2c 100644 --- a/src/utils/icons.ts +++ b/src/utils/icons.ts @@ -135,7 +135,7 @@ export function getPullRequestReviewIcon( case 'COMMENTED': return { type: CommentIcon, - color: IconColor.GREEN, + color: IconColor.YELLOW, description: `${descriptionPrefix} left review comments`, }; case 'DISMISSED': diff --git a/src/utils/subject.ts b/src/utils/subject.ts index 50100d24d..81e30e4a7 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -314,6 +314,7 @@ export async function getLatestReviewForReviewers( ...reviewers.filter((review) => review.state === 'CHANGES_REQUESTED'), ...reviewers.filter((review) => review.state === 'COMMENTED'), ...reviewers.filter((review) => review.state === 'DISMISSED'), + ...reviewers.filter((review) => review.state === 'PENDING'), ]; } From b3c7f2ee09e06628f412972b1b5c21ccc5fece20 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 3 May 2024 17:54:28 -1000 Subject: [PATCH 12/14] refactor: alphabetic order for icon color enum --- src/utils/subject.ts | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/utils/subject.ts b/src/utils/subject.ts index 81e30e4a7..f18fdd0fb 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -302,20 +302,15 @@ export async function getLatestReviewForReviewers( state: prReview.state, users: [prReview.user.login], }); - continue; + } else { + reviewerFound.users.push(prReview.user.login); } - - reviewerFound.users.push(prReview.user.login); } - // Sort the reviews by state for consistent draw order - return [ - ...reviewers.filter((review) => review.state === 'APPROVED'), - ...reviewers.filter((review) => review.state === 'CHANGES_REQUESTED'), - ...reviewers.filter((review) => review.state === 'COMMENTED'), - ...reviewers.filter((review) => review.state === 'DISMISSED'), - ...reviewers.filter((review) => review.state === 'PENDING'), - ]; + // Sort reviews by state for consistent order when rendering + return reviewers.sort((a, b) => { + return a.state.localeCompare(b.state); + }); } async function getGitifySubjectForRelease( From 286536678a25fb673ef473784a25aba1508192ca Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 5 May 2024 06:54:07 -1000 Subject: [PATCH 13/14] revert token --- src/utils/notifications.ts | 10 ++++-- src/utils/subject.test.ts | 68 +++++++++++++++++++------------------- src/utils/subject.ts | 11 ++---- 3 files changed, 44 insertions(+), 45 deletions(-) diff --git a/src/utils/notifications.ts b/src/utils/notifications.ts index 8cb282ad5..142ec3598 100644 --- a/src/utils/notifications.ts +++ b/src/utils/notifications.ts @@ -1,6 +1,10 @@ import { ipcRenderer } from 'electron'; import { Notification } from '../typesGitHub'; -import { isGitHubLoggedIn, openInBrowser } from '../utils/helpers'; +import { + getTokenForHost, + isGitHubLoggedIn, + openInBrowser, +} from '../utils/helpers'; import { updateTrayIcon } from './comms'; import type { AccountNotifications, AuthState, SettingsState } from '../types'; @@ -181,9 +185,11 @@ export async function enrichNotifications( const enrichedNotifications = await Promise.all( notifications.map(async (notification: Notification) => { + const token = getTokenForHost(notification.hostname, accounts); + const additionalSubjectDetails = await getGitifySubjectDetails( notification, - accounts, + token, ); return { diff --git a/src/utils/subject.test.ts b/src/utils/subject.test.ts index cfc8a7696..560252ba3 100644 --- a/src/utils/subject.test.ts +++ b/src/utils/subject.test.ts @@ -47,7 +47,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -64,7 +64,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -81,7 +81,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -98,7 +98,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -115,7 +115,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -132,7 +132,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toBeNull(); @@ -146,7 +146,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toBeNull(); @@ -174,7 +174,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -204,7 +204,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -246,7 +246,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -273,7 +273,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -300,7 +300,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -327,7 +327,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -354,7 +354,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -381,7 +381,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -411,7 +411,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -449,7 +449,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -474,7 +474,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -503,7 +503,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -532,7 +532,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -561,7 +561,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -589,7 +589,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -637,7 +637,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -672,7 +672,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -707,7 +707,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -742,7 +742,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -776,7 +776,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -809,7 +809,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -908,7 +908,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -932,7 +932,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toEqual({ @@ -950,7 +950,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toBeNull(); @@ -964,7 +964,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toBeNull(); @@ -981,7 +981,7 @@ describe('utils/subject.ts', () => { const result = await getGitifySubjectDetails( mockNotification, - mockAccounts, + mockAccounts.token, ); expect(result).toBeNull(); @@ -1005,7 +1005,7 @@ describe('utils/subject.ts', () => { .get('/repos/gitify-app/notifications-test/issues/1') .replyWithError(mockError); - await getGitifySubjectDetails(mockNotification, mockAccounts); + await getGitifySubjectDetails(mockNotification, mockAccounts.token); expect(consoleErrorSpy).toHaveBeenCalledWith( 'Error occurred while fetching details for Issue notification: This issue will throw an error', diff --git a/src/utils/subject.ts b/src/utils/subject.ts index f18fdd0fb..3a94b9574 100644 --- a/src/utils/subject.ts +++ b/src/utils/subject.ts @@ -1,4 +1,3 @@ -import type { AuthState } from '../types'; import type { CheckSuiteAttributes, CheckSuiteStatus, @@ -21,19 +20,13 @@ import { getPullRequestReviews, getRelease, } from './api/client'; -import { - fetchDiscussion, - getLatestDiscussionComment, - getTokenForHost, -} from './helpers'; +import { fetchDiscussion, getLatestDiscussionComment } from './helpers'; export async function getGitifySubjectDetails( notification: Notification, - accounts: AuthState, + token: string, ): Promise { try { - const token = getTokenForHost(notification.hostname, accounts); - switch (notification.subject.type) { case 'CheckSuite': return getGitifySubjectForCheckSuite(notification); From cfa7ea7ca1e9a7e3549eca943c3a665ee8fa92db Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sun, 5 May 2024 06:56:00 -1000 Subject: [PATCH 14/14] revert partial mock --- src/__mocks__/partial-mocks.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/__mocks__/partial-mocks.ts b/src/__mocks__/partial-mocks.ts index f0d2c0ef6..2c8b52b72 100644 --- a/src/__mocks__/partial-mocks.ts +++ b/src/__mocks__/partial-mocks.ts @@ -1,11 +1,9 @@ import type { Notification, Subject, User } from '../typesGitHub'; -import Constants from '../utils/constants'; export function partialMockNotification( subject: Partial, ): Notification { const mockNotification: Partial = { - hostname: Constants.GITHUB_API_BASE_URL, subject: subject as Subject, };