From cfb78427cc771eba0c54b8cfc0ccf7493b8bca3a Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Fri, 17 Sep 2021 15:28:56 +0200 Subject: [PATCH 1/3] feat: use notification_referrer_id for better UX --- src/__mocks__/mock-state.ts | 2 + src/__mocks__/mockedData.ts | 8 +- src/components/NotificationRow.test.tsx | 9 +- src/components/NotificationRow.tsx | 8 +- src/context/App.test.tsx | 17 ++- src/context/App.tsx | 14 +- src/hooks/useNotifications.test.ts | 2 + src/hooks/useNotifications.ts | 7 +- src/routes/LoginEnterprise.test.tsx | 2 + src/routes/LoginWithToken.tsx | 2 +- src/types.ts | 3 +- src/typesGithub.ts | 6 + src/utils/auth.test.ts | 3 +- src/utils/auth.ts | 25 +++- src/utils/constants.ts | 2 +- src/utils/helpers.test.ts | 165 ++++++++++++++++-------- src/utils/helpers.ts | 23 +++- src/utils/notifications.test.ts | 28 +++- src/utils/notifications.ts | 15 ++- src/utils/storage.test.ts | 1 + 20 files changed, 254 insertions(+), 88 deletions(-) diff --git a/src/__mocks__/mock-state.ts b/src/__mocks__/mock-state.ts index 6ce8ee858..3b2f00ced 100644 --- a/src/__mocks__/mock-state.ts +++ b/src/__mocks__/mock-state.ts @@ -1,4 +1,5 @@ import { Appearance, AuthState, SettingsState } from '../types'; +import { mockedUser } from './mockedData'; export const mockAccounts: AuthState = { token: 'token-123-456', @@ -8,6 +9,7 @@ export const mockAccounts: AuthState = { hostname: 'github.gitify.io', }, ], + user: mockedUser, }; export const mockSettings: SettingsState = { diff --git a/src/__mocks__/mockedData.ts b/src/__mocks__/mockedData.ts index 8b37380b3..c18d0184a 100644 --- a/src/__mocks__/mockedData.ts +++ b/src/__mocks__/mockedData.ts @@ -1,5 +1,5 @@ import { AccountNotifications, EnterpriseAccount } from '../types'; -import { Notification, Repository } from '../typesGithub'; +import { Notification, Repository, User } from '../typesGithub'; export const mockedEnterpriseAccounts: EnterpriseAccount[] = [ { @@ -8,6 +8,12 @@ export const mockedEnterpriseAccounts: EnterpriseAccount[] = [ }, ]; +export const mockedUser: User = { + login: 'octocat', + name: 'Mona Lisa Octocat', + id: 123456789, +}; + // prettier-ignore export const mockedSingleNotification: Notification = { id: '138661096', diff --git a/src/components/NotificationRow.test.tsx b/src/components/NotificationRow.test.tsx index b38da7a8f..ad47fda8a 100644 --- a/src/components/NotificationRow.test.tsx +++ b/src/components/NotificationRow.test.tsx @@ -7,7 +7,7 @@ const { shell } = require('electron'); import { AppContext } from '../context/App'; import { mockedSingleNotification } from '../__mocks__/mockedData'; import { NotificationRow } from './NotificationRow'; -import { mockSettings } from '../__mocks__/mock-state'; +import { mockAccounts, mockSettings } from '../__mocks__/mock-state'; describe('components/Notification.js', () => { beforeEach(() => { @@ -39,6 +39,7 @@ describe('components/Notification.js', () => { value={{ settings: { ...mockSettings, markOnClick: true }, markNotification, + accounts: mockAccounts, }} > @@ -62,6 +63,7 @@ describe('components/Notification.js', () => { value={{ settings: { ...mockSettings, markOnClick: true }, markNotification, + accounts: mockAccounts, }} > @@ -83,7 +85,10 @@ describe('components/Notification.js', () => { const { getByTitle } = render( diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index 12d955dc4..fd2f821a8 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -18,7 +18,7 @@ export const NotificationRow: React.FC = ({ notification, hostname, }) => { - const { settings } = useContext(AppContext); + const { settings, accounts } = useContext(AppContext); const { markNotification, unsubscribeNotification } = useContext(AppContext); const pressTitle = useCallback(() => { @@ -32,7 +32,11 @@ export const NotificationRow: React.FC = ({ const openBrowser = useCallback(() => { // Some Notification types from GitHub are missing urls in their subjects. if (notification.subject.url) { - const url = generateGitHubWebUrl(notification.subject.url); + const url = generateGitHubWebUrl( + notification.subject.url, + notification.id, + accounts.user.id + ); shell.openExternal(url); } }, [notification]); diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx index c230dd1dc..158388cf8 100644 --- a/src/context/App.test.tsx +++ b/src/context/App.test.tsx @@ -105,7 +105,7 @@ describe('context/App.tsx', () => { expect(markNotificationMock).toHaveBeenCalledTimes(1); expect(markNotificationMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null }, + { enterpriseAccounts: [], token: null, user: null }, '123-456', 'github.com' ); @@ -132,7 +132,7 @@ describe('context/App.tsx', () => { expect(unsubscribeNotificationMock).toHaveBeenCalledTimes(1); expect(unsubscribeNotificationMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null }, + { enterpriseAccounts: [], token: null, user: null }, '123-456', 'github.com' ); @@ -161,7 +161,7 @@ describe('context/App.tsx', () => { expect(markRepoNotificationsMock).toHaveBeenCalledTimes(1); expect(markRepoNotificationsMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null }, + { enterpriseAccounts: [], token: null, user: null }, 'manosim/gitify', 'github.com' ); @@ -192,12 +192,17 @@ describe('context/App.tsx', () => { expect(fetchNotificationsMock).toHaveBeenCalledTimes(2) ); - expect(apiRequestAuthMock).toHaveBeenCalledTimes(1); + expect(apiRequestAuthMock).toHaveBeenCalledTimes(2); expect(apiRequestAuthMock).toHaveBeenCalledWith( 'https://api.github.com/notifications', 'HEAD', '123-456' ); + expect(apiRequestAuthMock).toHaveBeenCalledWith( + 'https://api.github.com/user', + 'GET', + '123-456' + ); }); }); @@ -240,7 +245,7 @@ describe('context/App.tsx', () => { expect(saveStateMock).toHaveBeenCalled(); expect(saveStateMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null }, + { enterpriseAccounts: [], token: null, user: null }, { appearance: 'SYSTEM', markOnClick: false, @@ -276,7 +281,7 @@ describe('context/App.tsx', () => { expect(setAutoLaunchMock).toHaveBeenCalledWith(true); expect(saveStateMock).toHaveBeenCalled(); expect(saveStateMock).toHaveBeenCalledWith( - { enterpriseAccounts: [], token: null }, + { enterpriseAccounts: [], token: null, user: null }, { appearance: 'SYSTEM', markOnClick: false, diff --git a/src/context/App.tsx b/src/context/App.tsx index 357a581ee..15edf9e4c 100644 --- a/src/context/App.tsx +++ b/src/context/App.tsx @@ -15,16 +15,18 @@ import { SettingsState, } from '../types'; import { apiRequestAuth } from '../utils/api-requests'; -import { addAccount, authGitHub, getToken } from '../utils/auth'; +import { addAccount, authGitHub, getToken, getUserData } from '../utils/auth'; import { clearState, loadState, saveState } from '../utils/storage'; import { setAppearance } from '../utils/appearance'; import { setAutoLaunch } from '../utils/comms'; import { useInterval } from '../hooks/useInterval'; import { useNotifications } from '../hooks/useNotifications'; +import Constants from '../utils/constants'; const defaultAccounts: AuthState = { token: null, enterpriseAccounts: [], + user: null, }; export const defaultSettings: SettingsState = { @@ -111,8 +113,11 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { const login = useCallback(async () => { const { authCode } = await authGitHub(); const { token } = await getToken(authCode); - setAccounts({ ...accounts, token }); - saveState({ ...accounts, token }, settings); + const user = await getUserData(token); + const hostname = Constants.DEFAULT_AUTH_OPTIONS.hostname; + const updatedAccounts = addAccount(accounts, token, hostname, user); + setAccounts(updatedAccounts); + saveState(updatedAccounts, settings); }, [accounts, settings]); const loginEnterprise = useCallback( @@ -133,7 +138,8 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { 'HEAD', token ); - const updatedAccounts = addAccount(accounts, token, hostname); + const user = await getUserData(token); + const updatedAccounts = addAccount(accounts, token, hostname, user); setAccounts(updatedAccounts); saveState(updatedAccounts, settings); }, diff --git a/src/hooks/useNotifications.test.ts b/src/hooks/useNotifications.test.ts index 9a11ed238..73f12c42e 100644 --- a/src/hooks/useNotifications.test.ts +++ b/src/hooks/useNotifications.test.ts @@ -5,6 +5,7 @@ import { act, renderHook } from '@testing-library/react-hooks'; import { mockAccounts, mockSettings } from '../__mocks__/mock-state'; import { useNotifications } from './useNotifications'; import { AuthState } from '../types'; +import { mockedUser } from '../__mocks__/mockedData'; describe('hooks/useNotifications.ts', () => { beforeEach(() => { @@ -135,6 +136,7 @@ describe('hooks/useNotifications.ts', () => { const accounts: AuthState = { ...mockAccounts, enterpriseAccounts: [], + user: mockedUser, }; const notifications = [ diff --git a/src/hooks/useNotifications.ts b/src/hooks/useNotifications.ts index 4165387cb..88e670cb2 100644 --- a/src/hooks/useNotifications.ts +++ b/src/hooks/useNotifications.ts @@ -96,7 +96,12 @@ export const useNotifications = (): NotificationsState => { ] : [...enterpriseNotifications]; - triggerNativeNotifications(notifications, data, settings); + triggerNativeNotifications( + notifications, + data, + settings, + accounts.user + ); setNotifications(data); setIsFetching(false); }) diff --git a/src/routes/LoginEnterprise.test.tsx b/src/routes/LoginEnterprise.test.tsx index f093ca9ae..e36efdaed 100644 --- a/src/routes/LoginEnterprise.test.tsx +++ b/src/routes/LoginEnterprise.test.tsx @@ -18,6 +18,7 @@ describe('routes/LoginEnterprise.js', () => { const mockAccounts: AuthState = { enterpriseAccounts: [], + user: null, }; beforeEach(function () { @@ -93,6 +94,7 @@ describe('routes/LoginEnterprise.js', () => { value={{ accounts: { enterpriseAccounts: mockedEnterpriseAccounts, + user: null, }, }} > diff --git a/src/routes/LoginWithToken.tsx b/src/routes/LoginWithToken.tsx index 6be9caba5..967b483dc 100644 --- a/src/routes/LoginWithToken.tsx +++ b/src/routes/LoginWithToken.tsx @@ -81,7 +81,7 @@ export const LoginWithToken: React.FC = () => { {!isValidToken && (
- This token could not get validated with {values.hostname}. + This token could not be validated with {values.hostname}.
)} diff --git a/src/types.ts b/src/types.ts index 984af3edf..31b2607cd 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,8 +1,9 @@ -import { Notification } from './typesGithub'; +import { Notification, User } from './typesGithub'; export interface AuthState { token?: string; enterpriseAccounts: EnterpriseAccount[]; + user: User; } export interface SettingsState { diff --git a/src/typesGithub.ts b/src/typesGithub.ts index fbfd2078a..1ff4e7810 100644 --- a/src/typesGithub.ts +++ b/src/typesGithub.ts @@ -33,6 +33,12 @@ export interface Notification { subscription_url: string; } +export interface User { + login: string; + name: string; + id: number; +} + export interface Repository { id: number; node_id: string; diff --git a/src/utils/auth.test.ts b/src/utils/auth.test.ts index 8b7bf68a5..9085e164a 100644 --- a/src/utils/auth.test.ts +++ b/src/utils/auth.test.ts @@ -35,7 +35,7 @@ describe('utils/auth.tsx', () => { expect(loadURLMock).toHaveBeenCalledTimes(1); expect(loadURLMock).toHaveBeenCalledWith( - 'https://github.com/login/oauth/authorize?client_id=FAKE_CLIENT_ID_123&scope=user:email,notifications' + 'https://github.com/login/oauth/authorize?client_id=FAKE_CLIENT_ID_123&scope=read:user,notifications' ); expect(new BrowserWindow().destroy).toHaveBeenCalledTimes(1); @@ -103,6 +103,7 @@ describe('utils/auth.tsx', () => { const accounts: AuthState = { token: null, enterpriseAccounts: [], + user: null, }; it('should add a github.com accont', async () => { diff --git a/src/utils/auth.ts b/src/utils/auth.ts index bf7141674..db94c2b3c 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -1,9 +1,10 @@ const { remote } = require('electron'); const BrowserWindow = remote.BrowserWindow; -import { apiRequest } from '../utils/api-requests'; +import { apiRequest, apiRequestAuth } from '../utils/api-requests'; import { AuthResponse, AuthState, AuthTokenResponse } from '../types'; import { Constants } from '../utils/constants'; +import { User } from '../typesGithub'; export const authGitHub = ( authOptions = Constants.DEFAULT_AUTH_OPTIONS @@ -67,6 +68,20 @@ export const authGitHub = ( }); }; +export const getUserData = async (token: string): Promise => { + const response = await apiRequestAuth( + `https://api.${Constants.DEFAULT_AUTH_OPTIONS.hostname}/user`, + 'GET', + token + ); + + return { + id: response.data.id, + login: response.data.login, + name: response.data.name, + }; +}; + export const getToken = async ( authCode: string, authOptions = Constants.DEFAULT_AUTH_OPTIONS @@ -85,11 +100,17 @@ export const getToken = async ( }; }; -export const addAccount = (accounts: AuthState, token, hostname): AuthState => { +export const addAccount = ( + accounts: AuthState, + token, + hostname, + user?: User +): AuthState => { if (hostname === Constants.DEFAULT_AUTH_OPTIONS.hostname) { return { ...accounts, token, + user: user ?? null, }; } diff --git a/src/utils/constants.ts b/src/utils/constants.ts index 8f9d2f55e..26a2eddb9 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -1,6 +1,6 @@ export const Constants = { // GitHub OAuth - AUTH_SCOPE: ['user:email', 'notifications'], + AUTH_SCOPE: ['read:user', 'notifications'], DEFAULT_AUTH_OPTIONS: { hostname: 'github.com', diff --git a/src/utils/helpers.test.ts b/src/utils/helpers.test.ts index 05951fed2..079373b0f 100644 --- a/src/utils/helpers.test.ts +++ b/src/utils/helpers.test.ts @@ -1,65 +1,126 @@ -import { generateGitHubWebUrl, generateGitHubAPIUrl } from './helpers'; +import { + generateGitHubWebUrl, + generateGitHubAPIUrl, + generateNotificationReferrerId, +} from './helpers'; +import { mockedSingleNotification, mockedUser } from '../__mocks__/mockedData'; describe('utils/helpers.ts', () => { - it('should generate the GitHub url - non enterprise - (issue)', () => { - const apiUrl = - 'https://api.github.com/repos/ekonstantinidis/notifications-test/issues/3'; - const newUrl = generateGitHubWebUrl(apiUrl); - expect(newUrl).toBe( - 'https://github.com/ekonstantinidis/notifications-test/issues/3' - ); + describe('generateNotificationReferrerId', () => { + it('should generate the notification_referrer_id', () => { + const referrerId = generateNotificationReferrerId( + mockedSingleNotification.id, + mockedUser.id + ); + expect(referrerId).toBe( + 'notification_referrer_id=MDE4Ok5vdGlmaWNhdGlvblRocmVhZDEzODY2MTA5NjoxMjM0NTY3ODk=' + ); + }); }); - it('should generate the GitHub url - non enterprise - (pull request)', () => { - const apiUrl = - 'https://api.github.com/repos/ekonstantinidis/notifications-test/pulls/123'; - const newUrl = generateGitHubWebUrl(apiUrl); - expect(newUrl).toBe( - 'https://github.com/ekonstantinidis/notifications-test/pull/123' - ); - }); + describe('generateGitHubWebUrl', () => { + let notificationReferrerId; + beforeAll(() => { + notificationReferrerId = generateNotificationReferrerId( + mockedSingleNotification.id, + mockedUser.id + ); + }); - it('should generate the GitHub url - non enterprise - (release)', () => { - const apiUrl = - 'https://api.github.com/repos/myorg/notifications-test/releases/3988077'; - const newUrl = generateGitHubWebUrl(apiUrl); - expect(newUrl).toBe('https://github.com/myorg/notifications-test/releases'); - }); + it('should generate the GitHub url - non enterprise - (issue)', () => { + const apiUrl = + 'https://api.github.com/repos/ekonstantinidis/notifications-test/issues/3'; + const notif = { ...mockedSingleNotification, subject: { url: apiUrl } }; + const newUrl = generateGitHubWebUrl( + notif.subject.url, + notif.id, + mockedUser.id + ); + expect(newUrl).toBe( + `https://github.com/ekonstantinidis/notifications-test/issues/3?${notificationReferrerId}` + ); + }); - it('should generate the GitHub url - enterprise - (issue)', () => { - const apiUrl = - 'https://github.gitify.io/api/v3/repos/myorg/notifications-test/issues/123'; - const newUrl = generateGitHubWebUrl(apiUrl); - expect(newUrl).toBe( - 'https://github.gitify.io/myorg/notifications-test/issues/123' - ); - }); + it('should generate the GitHub url - non enterprise - (pull request)', () => { + const apiUrl = + 'https://api.github.com/repos/ekonstantinidis/notifications-test/pulls/123'; + const notif = { ...mockedSingleNotification, subject: { url: apiUrl } }; + const newUrl = generateGitHubWebUrl( + notif.subject.url, + notif.id, + mockedUser.id + ); + expect(newUrl).toBe( + `https://github.com/ekonstantinidis/notifications-test/pull/123?${notificationReferrerId}` + ); + }); - it('should generate the GitHub url - enterprise - (pull request)', () => { - const apiUrl = - 'https://github.gitify.io/api/v3/repos/myorg/notifications-test/pulls/3'; - const newUrl = generateGitHubWebUrl(apiUrl); - expect(newUrl).toBe( - 'https://github.gitify.io/myorg/notifications-test/pull/3' - ); - }); + it('should generate the GitHub url - non enterprise - (release)', () => { + const apiUrl = + 'https://api.github.com/repos/myorg/notifications-test/releases/3988077'; + const notif = { ...mockedSingleNotification, subject: { url: apiUrl } }; + const newUrl = generateGitHubWebUrl( + notif.subject.url, + notif.id, + mockedUser.id + ); + expect(newUrl).toBe( + `https://github.com/myorg/notifications-test/releases?${notificationReferrerId}` + ); + }); - it('should generate the GitHub url - enterprise - (release)', () => { - const apiUrl = - 'https://github.gitify.io/api/v3/repos/myorg/notifications-test/releases/1'; - const newUrl = generateGitHubWebUrl(apiUrl); - expect(newUrl).toBe( - 'https://github.gitify.io/myorg/notifications-test/releases' - ); - }); + it('should generate the GitHub url - enterprise - (issue)', () => { + const apiUrl = + 'https://github.gitify.io/api/v3/repos/myorg/notifications-test/issues/123'; + const notif = { ...mockedSingleNotification, subject: { url: apiUrl } }; + const newUrl = generateGitHubWebUrl( + notif.subject.url, + notif.id, + mockedUser.id + ); + expect(newUrl).toBe( + `https://github.gitify.io/myorg/notifications-test/issues/123?${notificationReferrerId}` + ); + }); + + it('should generate the GitHub url - enterprise - (pull request)', () => { + const apiUrl = + 'https://github.gitify.io/api/v3/repos/myorg/notifications-test/pulls/3'; + const notif = { ...mockedSingleNotification, subject: { url: apiUrl } }; + const newUrl = generateGitHubWebUrl( + notif.subject.url, + notif.id, + mockedUser.id + ); + expect(newUrl).toBe( + `https://github.gitify.io/myorg/notifications-test/pull/3?${notificationReferrerId}` + ); + }); - it('should generate a GitHub API url - non enterprise', () => { - const result = generateGitHubAPIUrl('github.com'); - expect(result).toBe('https://api.github.com/'); + it('should generate the GitHub url - enterprise - (release)', () => { + const apiUrl = + 'https://github.gitify.io/api/v3/repos/myorg/notifications-test/releases/1'; + const notif = { ...mockedSingleNotification, subject: { url: apiUrl } }; + const newUrl = generateGitHubWebUrl( + notif.subject.url, + notif.id, + mockedUser.id + ); + expect(newUrl).toBe( + `https://github.gitify.io/myorg/notifications-test/releases?${notificationReferrerId}` + ); + }); }); - it('should generate a GitHub API url - enterprise', () => { - const result = generateGitHubAPIUrl('github.manos.im'); - expect(result).toBe('https://github.manos.im/api/v3/'); + describe('generateGitHubAPIUrl', () => { + it('should generate a GitHub API url - non enterprise', () => { + const result = generateGitHubAPIUrl('github.com'); + expect(result).toBe('https://api.github.com/'); + }); + + it('should generate a GitHub API url - enterprise', () => { + const result = generateGitHubAPIUrl('github.manos.im'); + expect(result).toBe('https://github.manos.im/api/v3/'); + }); }); }); diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index 515a598e9..a20c2d1c2 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -17,7 +17,21 @@ export function generateGitHubAPIUrl(hostname) { : `https://api.${hostname}/`; } -export function generateGitHubWebUrl(url: string) { +export function generateNotificationReferrerId( + notificationId: string, + userId: number +) { + const buffer = Buffer.from( + `018:NotificationThread${notificationId}:${userId}` + ); + return `notification_referrer_id=${buffer.toString('base64')}`; +} + +export function generateGitHubWebUrl( + url: string, + notificationId: string, + userId: number +) { const { hostname } = parse(url); const isEnterprise = hostname !== `api.${Constants.DEFAULT_AUTH_OPTIONS.hostname}`; @@ -35,5 +49,10 @@ export function generateGitHubWebUrl(url: string) { newUrl = newUrl.substr(0, newUrl.lastIndexOf('/')); } - return newUrl; + const notificationReferrerId = generateNotificationReferrerId( + notificationId, + userId + ); + + return `${newUrl}?${notificationReferrerId}`; } diff --git a/src/utils/notifications.test.ts b/src/utils/notifications.test.ts index f11e4910c..86bea2f3a 100644 --- a/src/utils/notifications.test.ts +++ b/src/utils/notifications.test.ts @@ -5,6 +5,7 @@ import { mockedAccountNotifications, mockedGithubNotifications, mockedSingleAccountNotifications, + mockedUser, } from '../__mocks__/mockedData'; import * as comms from './comms'; import * as notificationsHelpers from './notifications'; @@ -25,7 +26,8 @@ describe('utils/notifications.ts', () => { notificationsHelpers.triggerNativeNotifications( [], mockedAccountNotifications, - settings + settings, + mockedUser ); expect(notificationsHelpers.raiseNativeNotification).toHaveBeenCalledTimes( @@ -49,7 +51,8 @@ describe('utils/notifications.ts', () => { notificationsHelpers.triggerNativeNotifications( [], mockedAccountNotifications, - settings + settings, + mockedUser ); expect(notificationsHelpers.raiseNativeNotification).not.toHaveBeenCalled(); @@ -69,7 +72,8 @@ describe('utils/notifications.ts', () => { notificationsHelpers.triggerNativeNotifications( mockedSingleAccountNotifications, mockedSingleAccountNotifications, - settings + settings, + mockedUser ); expect(notificationsHelpers.raiseNativeNotification).not.toHaveBeenCalled(); @@ -86,7 +90,12 @@ describe('utils/notifications.ts', () => { spyOn(notificationsHelpers, 'raiseNativeNotification'); spyOn(notificationsHelpers, 'raiseSoundNotification'); - notificationsHelpers.triggerNativeNotifications([], [], settings); + notificationsHelpers.triggerNativeNotifications( + [], + [], + settings, + mockedUser + ); expect(notificationsHelpers.raiseNativeNotification).not.toHaveBeenCalled(); expect(notificationsHelpers.raiseSoundNotification).not.toHaveBeenCalled(); @@ -96,12 +105,16 @@ describe('utils/notifications.ts', () => { spyOn(comms, 'openExternalLink'); const nativeNotification: Notification = notificationsHelpers.raiseNativeNotification( - [mockedGithubNotifications[0]] + [mockedGithubNotifications[0]], + mockedUser.id ); nativeNotification.onclick(null); + const notif = mockedGithubNotifications[0]; const newUrl = generateGitHubWebUrl( - mockedGithubNotifications[0].subject.url + notif.subject.url, + notif.id, + mockedUser.id ); expect(comms.openExternalLink).toHaveBeenCalledTimes(1); expect(comms.openExternalLink).toHaveBeenCalledWith(newUrl); @@ -111,7 +124,8 @@ describe('utils/notifications.ts', () => { spyOn(comms, 'reOpenWindow'); const nativeNotification = notificationsHelpers.raiseNativeNotification( - mockedGithubNotifications + mockedGithubNotifications, + mockedUser.id ); nativeNotification.onclick(null); diff --git a/src/utils/notifications.ts b/src/utils/notifications.ts index 911c604e8..1cff85207 100644 --- a/src/utils/notifications.ts +++ b/src/utils/notifications.ts @@ -2,7 +2,7 @@ const { remote } = require('electron'); import { generateGitHubWebUrl } from './helpers'; import { reOpenWindow, openExternalLink, updateTrayIcon } from './comms'; -import { Notification } from '../typesGithub'; +import { Notification, User } from '../typesGithub'; import { AccountNotifications, SettingsState } from '../types'; @@ -18,7 +18,8 @@ export const setTrayIconColor = (notifications: AccountNotifications[]) => { export const triggerNativeNotifications = ( previousNotifications: AccountNotifications[], newNotifications: AccountNotifications[], - settings: SettingsState + settings: SettingsState, + user: User ) => { const diffNotifications = newNotifications .map((account) => { @@ -54,11 +55,14 @@ export const triggerNativeNotifications = ( } if (settings.showNotifications) { - raiseNativeNotification(diffNotifications); + raiseNativeNotification(diffNotifications, user.id); } }; -export const raiseNativeNotification = (notifications: Notification[]) => { +export const raiseNativeNotification = ( + notifications: Notification[], + userId: number +) => { let title: string; let body: string; let notificationUrl: string | null; @@ -85,7 +89,8 @@ export const raiseNativeNotification = (notifications: Notification[]) => { // Some Notification types from GitHub are missing urls in their subjects. if (notificationUrl) { - const url = generateGitHubWebUrl(notificationUrl); + const { subject, id } = notifications[0]; + const url = generateGitHubWebUrl(subject.url, id, userId); openExternalLink(url); } } else { diff --git a/src/utils/storage.test.ts b/src/utils/storage.test.ts index 175a1fa5b..6a0acae0a 100644 --- a/src/utils/storage.test.ts +++ b/src/utils/storage.test.ts @@ -29,6 +29,7 @@ describe('utils/storage.ts', () => { { token: '123-456', enterpriseAccounts: [], + user: null, }, mockSettings ); From a4d6d6558b40e4cf70cd04e8861228693f195a5f Mon Sep 17 00:00:00 2001 From: Manos Konstantinidis Date: Sun, 3 Oct 2021 15:52:09 +0100 Subject: [PATCH 2/3] Prevent crash when there is no user id (legacy login) --- src/components/NotificationRow.tsx | 2 +- src/types.ts | 2 +- src/utils/helpers.ts | 19 +++++++++++-------- src/utils/notifications.ts | 4 ++-- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index fd2f821a8..eb23e75d5 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -35,7 +35,7 @@ export const NotificationRow: React.FC = ({ const url = generateGitHubWebUrl( notification.subject.url, notification.id, - accounts.user.id + accounts.user?.id ); shell.openExternal(url); } diff --git a/src/types.ts b/src/types.ts index 31b2607cd..1d472051d 100644 --- a/src/types.ts +++ b/src/types.ts @@ -3,7 +3,7 @@ import { Notification, User } from './typesGithub'; export interface AuthState { token?: string; enterpriseAccounts: EnterpriseAccount[]; - user: User; + user: User | null; } export interface SettingsState { diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index a20c2d1c2..dbeb33a48 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -1,4 +1,3 @@ -import { parse } from 'url'; import { EnterpriseAccount } from '../types'; import { Constants } from './constants'; @@ -30,9 +29,9 @@ export function generateNotificationReferrerId( export function generateGitHubWebUrl( url: string, notificationId: string, - userId: number + userId?: number ) { - const { hostname } = parse(url); + const { hostname } = new URL(url); const isEnterprise = hostname !== `api.${Constants.DEFAULT_AUTH_OPTIONS.hostname}`; @@ -49,10 +48,14 @@ export function generateGitHubWebUrl( newUrl = newUrl.substr(0, newUrl.lastIndexOf('/')); } - const notificationReferrerId = generateNotificationReferrerId( - notificationId, - userId - ); + if (userId) { + const notificationReferrerId = generateNotificationReferrerId( + notificationId, + userId + ); + + return `${newUrl}?${notificationReferrerId}`; + } - return `${newUrl}?${notificationReferrerId}`; + return newUrl; } diff --git a/src/utils/notifications.ts b/src/utils/notifications.ts index 1cff85207..5e9a4e12c 100644 --- a/src/utils/notifications.ts +++ b/src/utils/notifications.ts @@ -55,13 +55,13 @@ export const triggerNativeNotifications = ( } if (settings.showNotifications) { - raiseNativeNotification(diffNotifications, user.id); + raiseNativeNotification(diffNotifications, user?.id); } }; export const raiseNativeNotification = ( notifications: Notification[], - userId: number + userId?: number ) => { let title: string; let body: string; From e33627b86da1666bdb65f04ed95ea65cb1ffe5d2 Mon Sep 17 00:00:00 2001 From: Manos Konstantinidis Date: Sun, 3 Oct 2021 16:24:32 +0100 Subject: [PATCH 3/3] List required scope when logging in with token --- src/routes/LoginWithToken.tsx | 6 +++++- .../__snapshots__/LoginWithToken.test.tsx.snap | 14 +++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/routes/LoginWithToken.tsx b/src/routes/LoginWithToken.tsx index 967b483dc..89db81fe7 100644 --- a/src/routes/LoginWithToken.tsx +++ b/src/routes/LoginWithToken.tsx @@ -67,7 +67,11 @@ export const LoginWithToken: React.FC = () => { > personal access tokens {' '} - and create one with the notifications scope. + and create one with the{' '} + + {Constants.AUTH_SCOPE.join(', ')}{' '} + + scopes. } /> diff --git a/src/routes/__snapshots__/LoginWithToken.test.tsx.snap b/src/routes/__snapshots__/LoginWithToken.test.tsx.snap index 1b29e7f82..518cd5d65 100644 --- a/src/routes/__snapshots__/LoginWithToken.test.tsx.snap +++ b/src/routes/__snapshots__/LoginWithToken.test.tsx.snap @@ -79,11 +79,15 @@ exports[`routes/LoginWithToken.js renders correctly 1`] = ` personal access tokens - and create one with the - - notifications - - scope. + and create one with the + + + read:user, notifications + + + scopes.