Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add a "mark notification as done" button #706

Merged
merged 7 commits into from Feb 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/components/NotificationRow.test.tsx
Expand Up @@ -79,6 +79,31 @@ describe('components/Notification.js', () => {
expect(markNotification).toHaveBeenCalledTimes(1);
});

it('should mark a notification as done', () => {
const markNotificationDone = jest.fn();

const props = {
notification: mockedSingleNotification,
hostname: 'github.com',
};

const { getByTitle } = render(
<AppContext.Provider
value={{
settings: { ...mockSettings },
accounts: mockAccounts,
}}
>
<AppContext.Provider value={{ markNotificationDone }}>
<NotificationRow {...props} />
</AppContext.Provider>
</AppContext.Provider>,
);

fireEvent.click(getByTitle('Mark as Done'));
expect(markNotificationDone).toHaveBeenCalledTimes(1);
});

it('should unsubscribe from a notification thread', () => {
const unsubscribeNotification = jest.fn();

Expand Down
29 changes: 23 additions & 6 deletions src/components/NotificationRow.tsx
@@ -1,6 +1,6 @@
import React, { useCallback, useContext } from 'react';
import { formatDistanceToNow, parseISO } from 'date-fns';
import { CheckIcon, MuteIcon } from '@primer/octicons-react';
import { CheckIcon, MuteIcon, ReadIcon } from '@primer/octicons-react';

import {
formatReason,
Expand All @@ -20,8 +20,13 @@ export const NotificationRow: React.FC<IProps> = ({
notification,
hostname,
}) => {
const { settings, accounts, markNotification, unsubscribeNotification } =
useContext(AppContext);
const {
settings,
accounts,
markNotification,
markNotificationDone,
unsubscribeNotification,
} = useContext(AppContext);

const pressTitle = useCallback(() => {
openBrowser();
Expand Down Expand Up @@ -81,18 +86,30 @@ export const NotificationRow: React.FC<IProps> = ({
</div>
</div>

<div className="flex justify-center items-center w-8">
<div className="flex justify-center items-center gap-2">
<button
className="focus:outline-none"
title="Mark as Read"
onClick={() => markNotification(notification.id, hostname)}
>
<CheckIcon
<ReadIcon
className="hover:text-green-500"
size={20}
size={14}
aria-label="Mark as Read"
/>
</button>

<button
className="focus:outline-none"
title="Mark as Done"
onClick={() => markNotificationDone(notification.id, hostname)}
>
<CheckIcon
className="hover:text-green-500"
size={16}
aria-label="Mark as Done"
/>
</button>
</div>
</div>
);
Expand Down
6 changes: 3 additions & 3 deletions src/components/Repository.tsx
@@ -1,7 +1,7 @@
const { shell } = require('electron');

import React, { useCallback, useContext } from 'react';
import { CheckIcon } from '@primer/octicons-react';
import { ReadIcon } from '@primer/octicons-react';
import { CSSTransition, TransitionGroup } from 'react-transition-group';

import { AppContext } from '../context/App';
Expand Down Expand Up @@ -43,9 +43,9 @@ export const RepositoryNotifications: React.FC<IProps> = ({

<div className="flex w-8 justify-center items-center">
<button className="focus:outline-none" onClick={markRepoAsRead}>
<CheckIcon
<ReadIcon
className="hover:text-green-500"
size={20}
size={16}
aria-label="Mark Repository as Read"
/>
</button>
Expand Down
35 changes: 32 additions & 3 deletions src/components/__snapshots__/NotificationRow.test.tsx.snap
Expand Up @@ -87,7 +87,7 @@ exports[`components/Notification.js should render itself & its children 1`] = `
</div>
</div>
<div
className="flex justify-center items-center w-8"
className="flex justify-center items-center gap-2"
>
<button
className="focus:outline-none"
Expand All @@ -100,7 +100,7 @@ exports[`components/Notification.js should render itself & its children 1`] = `
className="hover:text-green-500"
fill="currentColor"
focusable="false"
height={20}
height={14}
role="img"
style={
{
Expand All @@ -111,7 +111,36 @@ exports[`components/Notification.js should render itself & its children 1`] = `
}
}
viewBox="0 0 16 16"
width={20}
width={14}
>
<path
d="M7.115.65a1.752 1.752 0 0 1 1.77 0l6.25 3.663c.536.314.865.889.865 1.51v6.427A1.75 1.75 0 0 1 14.25 14H1.75A1.75 1.75 0 0 1 0 12.25V5.823c0-.621.33-1.196.865-1.51Zm1.011 1.293a.252.252 0 0 0-.252 0l-5.72 3.353L6.468 7.76a2.748 2.748 0 0 1 3.066 0l4.312-2.464-5.719-3.353ZM13.15 12.5 8.772 9.06a1.25 1.25 0 0 0-1.544 0L2.85 12.5Zm1.35-5.85-3.687 2.106 3.687 2.897ZM5.187 8.756 1.5 6.65v5.003Z"
/>
</svg>
</button>
<button
className="focus:outline-none"
onClick={[Function]}
title="Mark as Done"
>
<svg
aria-hidden="false"
aria-label="Mark as Done"
className="hover:text-green-500"
fill="currentColor"
focusable="false"
height={16}
role="img"
style={
{
"display": "inline-block",
"overflow": "visible",
"userSelect": "none",
"verticalAlign": "text-bottom",
}
}
viewBox="0 0 16 16"
width={16}
>
<path
d="M13.78 4.22a.75.75 0 0 1 0 1.06l-7.25 7.25a.75.75 0 0 1-1.06 0L2.22 9.28a.751.751 0 0 1 .018-1.042.751.751 0 0 1 1.042-.018L6 10.94l6.72-6.72a.75.75 0 0 1 1.06 0Z"
Expand Down
6 changes: 3 additions & 3 deletions src/components/__snapshots__/Repository.test.tsx.snap
Expand Up @@ -31,7 +31,7 @@ exports[`components/Repository.tsx should render itself & its children 1`] = `
className="hover:text-green-500"
fill="currentColor"
focusable="false"
height={20}
height={16}
role="img"
style={
{
Expand All @@ -42,10 +42,10 @@ exports[`components/Repository.tsx should render itself & its children 1`] = `
}
}
viewBox="0 0 16 16"
width={20}
width={16}
>
<path
d="M13.78 4.22a.75.75 0 0 1 0 1.06l-7.25 7.25a.75.75 0 0 1-1.06 0L2.22 9.28a.751.751 0 0 1 .018-1.042.751.751 0 0 1 1.042-.018L6 10.94l6.72-6.72a.75.75 0 0 1 1.06 0Z"
d="M7.115.65a1.752 1.752 0 0 1 1.77 0l6.25 3.663c.536.314.865.889.865 1.51v6.427A1.75 1.75 0 0 1 14.25 14H1.75A1.75 1.75 0 0 1 0 12.25V5.823c0-.621.33-1.196.865-1.51Zm1.011 1.293a.252.252 0 0 0-.252 0l-5.72 3.353L6.468 7.76a2.748 2.748 0 0 1 3.066 0l4.312-2.464-5.719-3.353ZM13.15 12.5 8.772 9.06a1.25 1.25 0 0 0-1.544 0L2.85 12.5Zm1.35-5.85-3.687 2.106 3.687 2.897ZM5.187 8.756 1.5 6.65v5.003Z"
/>
</svg>
</button>
Expand Down
27 changes: 27 additions & 0 deletions src/context/App.test.tsx
Expand Up @@ -38,13 +38,15 @@ describe('context/App.tsx', () => {

const fetchNotificationsMock = jest.fn();
const markNotificationMock = jest.fn();
const markNotificationDoneMock = jest.fn();
const unsubscribeNotificationMock = jest.fn();
const markRepoNotificationsMock = jest.fn();

beforeEach(() => {
(useNotifications as jest.Mock).mockReturnValue({
fetchNotifications: fetchNotificationsMock,
markNotification: markNotificationMock,
markNotificationDone: markNotificationDoneMock,
unsubscribeNotification: unsubscribeNotificationMock,
markRepoNotifications: markRepoNotificationsMock,
});
Expand Down Expand Up @@ -121,6 +123,31 @@ describe('context/App.tsx', () => {
);
});

it('should call markNotificationDone', async () => {
const TestComponent = () => {
const { markNotificationDone } = useContext(AppContext);

return (
<button onClick={() => markNotificationDone('123-456', 'github.com')}>
Test Case
</button>
);
};

const { getByText } = customRender(<TestComponent />);

markNotificationDoneMock.mockReset();

fireEvent.click(getByText('Test Case'));

expect(markNotificationDoneMock).toHaveBeenCalledTimes(1);
expect(markNotificationDoneMock).toHaveBeenCalledWith(
{ enterpriseAccounts: [], token: null, user: null },
'123-456',
'github.com',
);
});

it('should call unsubscribeNotification', async () => {
const TestComponent = () => {
const { unsubscribeNotification } = useContext(AppContext);
Expand Down
9 changes: 9 additions & 0 deletions src/context/App.tsx
Expand Up @@ -52,6 +52,7 @@ interface AppContextState {
requestFailed: boolean;
fetchNotifications: () => Promise<void>;
markNotification: (id: string, hostname: string) => Promise<void>;
markNotificationDone: (id: string, hostname: string) => Promise<void>;
unsubscribeNotification: (id: string, hostname: string) => Promise<void>;
markRepoNotifications: (id: string, hostname: string) => Promise<void>;

Expand All @@ -70,6 +71,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => {
requestFailed,
isFetching,
markNotification,
markNotificationDone,
unsubscribeNotification,
markRepoNotifications,
} = useNotifications(settings.colors);
Expand Down Expand Up @@ -176,6 +178,12 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => {
[accounts, notifications],
);

const markNotificationDoneWithAccounts = useCallback(
async (id: string, hostname: string) =>
await markNotificationDone(accounts, id, hostname),
[accounts, notifications],
);

const unsubscribeNotificationWithAccounts = useCallback(
async (id: string, hostname: string) =>
await unsubscribeNotification(accounts, id, hostname),
Expand Down Expand Up @@ -203,6 +211,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => {
requestFailed,
fetchNotifications: fetchNotificationsWithAccounts,
markNotification: markNotificationWithAccounts,
markNotificationDone: markNotificationDoneWithAccounts,
unsubscribeNotification: unsubscribeNotificationWithAccounts,
markRepoNotifications: markRepoNotificationsWithAccounts,

Expand Down
86 changes: 86 additions & 0 deletions src/hooks/useNotifications.test.ts
Expand Up @@ -355,6 +355,92 @@ describe('hooks/useNotifications.ts', () => {
});
});

describe('markNotificationDone', () => {
const id = 'notification-123';

describe('github.com', () => {
const accounts = { ...mockAccounts, enterpriseAccounts: [] };
const hostname = 'github.com';

it('should mark a notification as done with success - github.com', async () => {
nock('https://api.github.com/')
.delete(`/notifications/threads/${id}`)
.reply(200);

const { result } = renderHook(() => useNotifications(false));

act(() => {
result.current.markNotificationDone(accounts, id, hostname);
});

await waitFor(() => {
expect(result.current.isFetching).toBe(false);
});

expect(result.current.notifications.length).toBe(0);
});

it('should mark a notification as done with failure - github.com', async () => {
nock('https://api.github.com/')
.delete(`/notifications/threads/${id}`)
.reply(400);

const { result } = renderHook(() => useNotifications(false));

act(() => {
result.current.markNotificationDone(accounts, id, hostname);
});

await waitFor(() => {
expect(result.current.isFetching).toBe(false);
});

expect(result.current.notifications.length).toBe(0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems odd. If the request failed, shouldn't the notification list remain untouched, length 1? My read of the code under test is that it doesn't change the notifications list in the failure condition, but maybe I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, tbh I just copy pasted the markNotification tests, so they were already broken before my PR 馃槵

I'm trying to figure out how to initialize a non-empty notifications array with mocked data, but without any success for now...

});
});

describe('enterprise', () => {
const accounts = { ...mockAccounts, token: null };
const hostname = 'github.gitify.io';

it('should mark a notification as done with success - enterprise', async () => {
nock('https://github.gitify.io/')
.delete(`/notifications/threads/${id}`)
.reply(200);

const { result } = renderHook(() => useNotifications(false));

act(() => {
result.current.markNotificationDone(accounts, id, hostname);
});

await waitFor(() => {
expect(result.current.isFetching).toBe(false);
});

expect(result.current.notifications.length).toBe(0);
});

it('should mark a notification as done with failure - enterprise', async () => {
nock('https://github.gitify.io/')
.delete(`/notifications/threads/${id}`)
.reply(400);

const { result } = renderHook(() => useNotifications(false));

act(() => {
result.current.markNotificationDone(accounts, id, hostname);
});

await waitFor(() => {
expect(result.current.isFetching).toBe(false);
});

expect(result.current.notifications.length).toBe(0);
});
});
});

describe('unsubscribeNotification', () => {
const id = 'notification-123';

Expand Down