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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add components for different states of the API response #1001

Open
wants to merge 8 commits into
base: vb-ld/add-saved-article-routing-behind-switch
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 110 additions & 0 deletions client/__tests__/components/savedarticles/savedArticlesPage.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import { render, screen, waitFor } from '@testing-library/react';
import '@testing-library/jest-dom';
import { SavedArticlesPage } from '../../../components/mma/savedArticles/SavedArticles';
// Importing this in order to create the Response object, as per the advice of this
// https://stackoverflow.com/questions/52420318/referenceerror-response-is-not-defined
import 'isomorphic-fetch';

describe('SavedArticlesPage', () => {
const successfulAPIResponse: (body: string) => Response = (body) => {
return new Response(body, {
status: 200,
statusText: 'Ok',
headers: {
'Content-type': 'application/json',
},
});
};
it('should show an error page if the S4L API call fails', async () => {
render(
<SavedArticlesPage
saveForLaterAPICall={() => {
return Promise.reject();
}}
/>,
);

await waitFor(() => {
expect(screen.queryByText('Oops!')).toBeInTheDocument();
});
});

it('should display a list of articles if the S4L API call succeeds and user has saved articles', async () => {
const articles = {
version: '1',
articles: [
{
id: 'world/2018/mar/08/donald-trump-north-korea-kim-jong-un-meeting-may-letter-invite-talks-nuclear-weapons',
shortUrl: '/p/88btx',
date: '2018-03-09T14:08:02Z',
read: false,
},
{
id: 'football/2018/mar/12/jamie-carragher-dropped-by-danish-broadcaster-after-spitting-incident',
shortUrl: '/p/88qhj',
date: '2018-03-12T16:53:32Z',
read: false,
},
],
};

const apiResponse = successfulAPIResponse(JSON.stringify(articles));

render(
<SavedArticlesPage
saveForLaterAPICall={() => {
return Promise.resolve(apiResponse);
}}
/>,
);
await waitFor(() => {
expect(
screen.queryByText(
'world/2018/mar/08/donald-trump-north-korea-kim-jong-un-meeting-may-letter-invite-talks-nuclear-weapons',
),
).toBeInTheDocument();
});
});

// eslint-disable-next-line jest/no-disabled-tests -- Add this test in when we have validated the response type
it.skip('should display an error message if the API call is successful but the json response is not expected', async () => {
const invalidJson = {
foo: 'bar',
};

const apiResponse = successfulAPIResponse(JSON.stringify(invalidJson));

render(
<SavedArticlesPage
saveForLaterAPICall={() => {
return Promise.resolve(apiResponse);
}}
/>,
);

await waitFor(() => {
expect(screen.queryByText('Oops!')).toBeInTheDocument();
});
});

it('should display the empty articles page if user has no saved articles', async () => {
const noArticles = {
version: 1,
articles: [],
};

const apiResponse = successfulAPIResponse(JSON.stringify(noArticles));
render(
<SavedArticlesPage
saveForLaterAPICall={() => {
return Promise.resolve(apiResponse);
}}
/>,
);
await waitFor(() => {
expect(
screen.queryByText('You have no saved articles'),
).toBeInTheDocument();
});
});
});
6 changes: 3 additions & 3 deletions client/components/mma/MMAPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { ReactNode } from 'react';
import { lazy, Suspense, useEffect, useState } from 'react';
import { BrowserRouter, Navigate, Route, Routes } from 'react-router-dom';
import {
featureSwitches,
getFeatureSwitches,
initFeatureSwitchUrlParamOverride,
} from '../../../shared/featureSwitches';
import type {
Expand Down Expand Up @@ -325,7 +325,7 @@ const MMARouter = () => {
path="/email-prefs"
element={<EmailAndMarketing />}
/>
{featureSwitches.savedArticles && (
{getFeatureSwitches().savedArticles && (
<Route
path="/saved-articles"
element={<SavedArticles />}
Expand All @@ -339,7 +339,7 @@ const MMARouter = () => {
path="/account-settings"
element={<Settings />}
/>
{featureSwitches.productSwitching &&
{getFeatureSwitches().productSwitching &&
[
{ path: '/switch', fromApp: false },
{ path: '/app/switch', fromApp: true },
Expand Down
50 changes: 49 additions & 1 deletion client/components/mma/savedArticles/SavedArticles.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,61 @@
import { typeCheckObject } from '../../../../shared/typeCheckObject';
import { fetchWithDefaultParameters } from '../../../utilities/fetch';
import {
LoadingState,
useAsyncLoader,
} from '../../../utilities/hooks/useAsyncLoader';
import { GenericErrorScreen } from '../../shared/GenericErrorScreen';
import { NAV_LINKS } from '../../shared/nav/NavConfig';
import { PageContainer } from '../Page';
import { JsonResponseHandler } from '../shared/asyncComponents/DefaultApiResponseHandler';
import { isSavedArticlesResponse } from './models/SavedArticle';
import type { SavedArticlesResponse } from './models/SavedArticle';
import { SavedArticlesDisplay } from './SavedArticlesDisplay';
import { SavedArticlesEmpty } from './SavedArticlesEmpty';
import { SavedArticlesLoading } from './SavedArticlesLoading';

export const SavedArticles = () => {
return (
<PageContainer
selectedNavItem={NAV_LINKS.savedArticles}
pageTitle={NAV_LINKS.savedArticles.title}
>
<p>There is nothing to see here yet</p>
<SavedArticlesPage
saveForLaterAPICall={() =>
fetchWithDefaultParameters('/add/url')
}
/>
</PageContainer>
);
};

interface SavedArticlesPageProps {
saveForLaterAPICall: () => Promise<unknown>;
}

export function SavedArticlesPage(props: SavedArticlesPageProps) {
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if it's worth looking at the AsyncLoader utility used by the AccountOverview? There seem to be two async loaders in use in this repo - useAsyncLoader hook and the AsyncLoader class compononent. I would check with the rest of the MMA team which component is preferred and see if we can align our work with that.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah good shout, I think from our discussion yesterday we think hooks is the right approach?

const {
data,
loadingState,
}: {
data: unknown;
loadingState: LoadingState;
} = useAsyncLoader(props.saveForLaterAPICall, JsonResponseHandler);

if (loadingState === LoadingState.IsLoading) {
return <SavedArticlesLoading />;
}
if (loadingState === LoadingState.HasError) {
return <GenericErrorScreen />;
}
const saveForLaterData = typeCheckObject<SavedArticlesResponse>(
isSavedArticlesResponse,
)(data);
if (saveForLaterData === undefined) {
return <GenericErrorScreen />;
}
if (saveForLaterData.articles.length === 0) {
return <SavedArticlesEmpty />;
}
return <SavedArticlesDisplay savedArticles={saveForLaterData.articles} />;
}
15 changes: 15 additions & 0 deletions client/components/mma/savedArticles/SavedArticlesDisplay.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import type { SavedArticle } from './models/SavedArticle';

interface SavedArticlesProps {
savedArticles: SavedArticle[];
}

export const SavedArticlesDisplay = ({ savedArticles }: SavedArticlesProps) => {
return (
<>
{savedArticles.map((article) => {
return <li key={article.id}>{article.id}</li>;
})}
</>
);
};
3 changes: 3 additions & 0 deletions client/components/mma/savedArticles/SavedArticlesEmpty.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export const SavedArticlesEmpty = () => {
return <p>You have no saved articles</p>;
};
8 changes: 8 additions & 0 deletions client/components/mma/savedArticles/SavedArticlesLoading.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { Spinner } from '../../shared/Spinner';
import { WithStandardTopMargin } from '../../shared/WithStandardTopMargin';

export const SavedArticlesLoading = () => (
<WithStandardTopMargin>
<Spinner loadingMessage={'Fetching saved articles'} />
</WithStandardTopMargin>
);
18 changes: 18 additions & 0 deletions client/components/mma/savedArticles/models/SavedArticle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { isObject } from '@guardian/libs';

export interface SavedArticle {
id: string;
shortUrl: string;
date: Date;
read: boolean;
}
export interface SavedArticlesResponse {
version: number;
articles: SavedArticle[];
}
export function isSavedArticlesResponse(
data: unknown,
): data is SavedArticlesResponse {
// TODO - validate this properly when we have the schema
return isObject(data);
}
6 changes: 5 additions & 1 deletion client/components/shared/nav/NavConfig.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { conf } from '../../../../server/config';
import {
featureSwitches,
getFeatureSwitches,
initFeatureSwitchUrlParamOverride,
} from '../../../../shared/featureSwitches';
import { AccountOverviewIcon } from '../../mma/shared/assets/AccountOverviewIcon';
Expand Down Expand Up @@ -40,13 +41,16 @@ interface NavLinks {
}

let domain: string;
let featureSwitchState: Record<string, boolean>;
if (typeof window !== 'undefined' && window.guardian) {
// Window need to be defined in order to call this method.
// This method is necessary to called so that when we read
// the feature switch, it will respect the URL override
initFeatureSwitchUrlParamOverride();
featureSwitchState = getFeatureSwitches();
domain = window.guardian.domain;
} else {
featureSwitchState = featureSwitches;
domain = conf.DOMAIN;
}
export const PROFILE_HOST_NAME = `https://profile.${domain}`;
Expand Down Expand Up @@ -74,7 +78,7 @@ export const NAV_LINKS: NavLinks = {
title: 'Saved articles',
link: '/saved-articles',
local: true,
isExcludedByFeatureSwitch: !featureSwitches.savedArticles,
isExcludedByFeatureSwitch: !featureSwitchState.savedArticles,
},
emailPrefs: {
title: 'Emails & marketing',
Expand Down
3 changes: 2 additions & 1 deletion cypress/integration/parallel-6/savedArticles.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ describe('Saved Articles', () => {
});
});

// TODO - add an explicit mechanism for overriding the switch to false, for when we turn this on by default?
// NOTE: Switch state for Cypress environment is colocated in shared/featureSwitches
describe('Feature Switch OFF', () => {
it('redirects to account overview homepage from /saved-articles route ', () => {
cy.visit('/saved-articles');
Expand All @@ -20,6 +20,7 @@ describe('Saved Articles', () => {
cy.findAllByText('Saved articles').should('have.length', 0);
});
});

describe('Feature Switch ON:', () => {
it('displays Saved Article page', () => {
cy.visit('/saved-articles?withFeature=savedArticles');
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@
"fetch-mock": "^9.11.0",
"git-revision-webpack-plugin": "^3.0.6",
"husky": "^1.3.1",
"isomorphic-fetch": "^3.0.0",
"jest": "^27.3.1",
"jest-teamcity-reporter": "^0.9.0",
"lint-staged": "^13.0.3",
Expand Down Expand Up @@ -145,7 +146,7 @@
"@guardian/ab-core": "^2.0.0",
"@guardian/ab-react": "^2.0.1",
"@guardian/consent-management-platform": "^6.11.3",
"@guardian/libs": "^1.7.1",
"@guardian/libs": "^13.1.0",
"@guardian/source-foundations": "^5.0.0",
"@guardian/source-react-components": "^5.0.0",
"@sentry/browser": "^5.22.3",
Expand Down
28 changes: 28 additions & 0 deletions shared/featureSwitches.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ export const initFeatureSwitchUrlParamOverride = () => {
featureSwitches[key] = param === key;
}
}
// Required to use query params to force true states in Cypress tests
for (const [key, value] of Object.entries(cypressSwitchState)) {
if (!value) {
cypressSwitchState[key] = param === key;
}
}
}
};

Expand All @@ -29,3 +35,25 @@ export const featureSwitches: Record<string, boolean> = {
productSwitching: true,
savedArticles: false,
};

// Cypress Testing:
// We want to be able to write tests independent of switch state that test both states.
// Use getFeatureSwitches to retrieve switches dependant on clientside environment.
// For example you can set a switch to false
// and opt in in Cypress via query search params in cy.visit(url).
declare global {
interface Window {
Cypress: unknown;
}
}

const cypressSwitchState: Record<string, boolean> = {
...featureSwitches,
savedArticles: false,
};
export const getFeatureSwitches = (): Record<string, boolean> => {
// If running within cypress, return the forced switches to keep tests consistent regardless of production state
if (window !== undefined && window.Cypress) return cypressSwitchState;
// Otherwise just return empty object
return featureSwitches;
};
5 changes: 5 additions & 0 deletions shared/typeCheckObject.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const typeCheckObject =
<T>(guard: (o: unknown) => o is T) =>
(object: unknown): T | undefined => {
return guard(object) ? object : undefined;
};
Loading