From 4ca31fc162c586aa35ae21eb44afd0ebac67289a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 7 Nov 2022 18:24:26 +0100 Subject: [PATCH 1/3] Added flag on short URL creation which tells if the short URL was already saved --- src/container/types.ts | 2 +- src/reducers/index.ts | 2 +- src/short-urls/CreateShortUrl.tsx | 8 ++++---- src/short-urls/helpers/CreateShortUrlResult.tsx | 3 +-- src/short-urls/reducers/shortUrlCreation.ts | 14 +++++++++----- src/short-urls/reducers/shortUrlEdition.ts | 2 +- src/short-urls/services/provideServices.ts | 2 +- test/short-urls/CreateShortUrl.test.tsx | 2 +- test/short-urls/reducers/shortUrlCreation.test.ts | 7 ++++--- 9 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/container/types.ts b/src/container/types.ts index 84cb2df1..24b4cb73 100644 --- a/src/container/types.ts +++ b/src/container/types.ts @@ -21,7 +21,7 @@ export interface ShlinkState { servers: ServersMap; selectedServer: SelectedServer; shortUrlsList: ShortUrlsList; - shortUrlCreationResult: ShortUrlCreation; + shortUrlCreation: ShortUrlCreation; shortUrlDeletion: ShortUrlDeletion; shortUrlEdition: ShortUrlEdition; shortUrlVisits: ShortUrlVisits; diff --git a/src/reducers/index.ts b/src/reducers/index.ts index 9e473c05..9c412690 100644 --- a/src/reducers/index.ts +++ b/src/reducers/index.ts @@ -21,7 +21,7 @@ export default (container: IContainer) => combineReducers({ servers: serversReducer, selectedServer: selectedServerReducer, shortUrlsList: shortUrlsListReducer, - shortUrlCreationResult: container.shortUrlCreationReducer, + shortUrlCreation: container.shortUrlCreationReducer, shortUrlDeletion: container.shortUrlDeletionReducer, shortUrlEdition: container.shortUrlEditionReducer, shortUrlDetail: container.shortUrlDetailReducer, diff --git a/src/short-urls/CreateShortUrl.tsx b/src/short-urls/CreateShortUrl.tsx index 8da5a480..890f2af5 100644 --- a/src/short-urls/CreateShortUrl.tsx +++ b/src/short-urls/CreateShortUrl.tsx @@ -12,7 +12,7 @@ export interface CreateShortUrlProps { interface CreateShortUrlConnectProps extends CreateShortUrlProps { settings: Settings; - shortUrlCreationResult: ShortUrlCreation; + shortUrlCreation: ShortUrlCreation; selectedServer: SelectedServer; createShortUrl: (data: ShortUrlData) => Promise; resetCreateShortUrl: () => void; @@ -38,7 +38,7 @@ export const CreateShortUrl = ( CreateShortUrlResult: FC, ) => ({ createShortUrl, - shortUrlCreationResult, + shortUrlCreation, resetCreateShortUrl, selectedServer, basicMode = false, @@ -50,7 +50,7 @@ export const CreateShortUrl = ( <> { @@ -59,7 +59,7 @@ export const CreateShortUrl = ( }} /> diff --git a/src/short-urls/helpers/CreateShortUrlResult.tsx b/src/short-urls/helpers/CreateShortUrlResult.tsx index 218755b3..e89bab34 100644 --- a/src/short-urls/helpers/CreateShortUrlResult.tsx +++ b/src/short-urls/helpers/CreateShortUrlResult.tsx @@ -1,7 +1,6 @@ import { faCopy as copyIcon } from '@fortawesome/free-regular-svg-icons'; import { faTimes as closeIcon } from '@fortawesome/free-solid-svg-icons'; import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; -import { isNil } from 'ramda'; import { useEffect } from 'react'; import CopyToClipboard from 'react-copy-to-clipboard'; import { Tooltip } from 'reactstrap'; @@ -34,7 +33,7 @@ export const CreateShortUrlResult = (useTimeoutToggle: TimeoutToggle) => ( ); } - if (isNil(result)) { + if (!result) { return null; } diff --git a/src/short-urls/reducers/shortUrlCreation.ts b/src/short-urls/reducers/shortUrlCreation.ts index 7bdc91f8..5124701d 100644 --- a/src/short-urls/reducers/shortUrlCreation.ts +++ b/src/short-urls/reducers/shortUrlCreation.ts @@ -8,8 +8,9 @@ import { ProblemDetailsError } from '../../api/types/errors'; export const CREATE_SHORT_URL = 'shlink/createShortUrl/CREATE_SHORT_URL'; export interface ShortUrlCreation { - result: ShortUrl | null; + result?: ShortUrl; saving: boolean; + saved: boolean; error: boolean; errorData?: ProblemDetailsError; } @@ -17,8 +18,8 @@ export interface ShortUrlCreation { export type CreateShortUrlAction = PayloadAction; const initialState: ShortUrlCreation = { - result: null, saving: false, + saved: false, error: false, }; @@ -35,12 +36,15 @@ export const shortUrlCreationReducerCreator = (buildShlinkApiClient: ShlinkApiCl resetCreateShortUrl: () => initialState, }, extraReducers: (builder) => { - builder.addCase(createShortUrl.pending, (state) => ({ ...state, saving: true, error: false })); + builder.addCase(createShortUrl.pending, (state) => ({ ...state, saving: true, saved: false, error: false })); builder.addCase( createShortUrl.rejected, - (state, { error }) => ({ ...state, saving: false, error: true, errorData: parseApiError(error) }), + (state, { error }) => ({ ...state, saving: false, saved: false, error: true, errorData: parseApiError(error) }), + ); + builder.addCase( + createShortUrl.fulfilled, + (_, { payload: result }) => ({ result, saving: false, saved: true, error: false }), ); - builder.addCase(createShortUrl.fulfilled, (_, { payload: result }) => ({ result, saving: false, error: false })); }, }); diff --git a/src/short-urls/reducers/shortUrlEdition.ts b/src/short-urls/reducers/shortUrlEdition.ts index 267d9b23..fbdfac13 100644 --- a/src/short-urls/reducers/shortUrlEdition.ts +++ b/src/short-urls/reducers/shortUrlEdition.ts @@ -10,8 +10,8 @@ export const SHORT_URL_EDITED = 'shlink/shortUrlEdition/SHORT_URL_EDITED'; export interface ShortUrlEdition { shortUrl?: ShortUrl; saving: boolean; - error: boolean; saved: boolean; + error: boolean; errorData?: ProblemDetailsError; } diff --git a/src/short-urls/services/provideServices.ts b/src/short-urls/services/provideServices.ts index d2b865a8..8feb709b 100644 --- a/src/short-urls/services/provideServices.ts +++ b/src/short-urls/services/provideServices.ts @@ -36,7 +36,7 @@ const provideServices = (bottle: Bottle, connect: ConnectDecorator) => { bottle.serviceFactory('CreateShortUrl', CreateShortUrl, 'ShortUrlForm', 'CreateShortUrlResult'); bottle.decorator( 'CreateShortUrl', - connect(['shortUrlCreationResult', 'selectedServer', 'settings'], ['createShortUrl', 'resetCreateShortUrl']), + connect(['shortUrlCreation', 'selectedServer', 'settings'], ['createShortUrl', 'resetCreateShortUrl']), ); bottle.serviceFactory('EditShortUrl', EditShortUrl, 'ShortUrlForm'); diff --git a/test/short-urls/CreateShortUrl.test.tsx b/test/short-urls/CreateShortUrl.test.tsx index 553d1a65..2e53e8dc 100644 --- a/test/short-urls/CreateShortUrl.test.tsx +++ b/test/short-urls/CreateShortUrl.test.tsx @@ -13,7 +13,7 @@ describe('', () => { const CreateShortUrl = createShortUrlsCreator(ShortUrlForm, CreateShortUrlResult); const setUp = () => render( {}} diff --git a/test/short-urls/reducers/shortUrlCreation.test.ts b/test/short-urls/reducers/shortUrlCreation.test.ts index 94b5f02b..e99e0ea2 100644 --- a/test/short-urls/reducers/shortUrlCreation.test.ts +++ b/test/short-urls/reducers/shortUrlCreation.test.ts @@ -22,16 +22,16 @@ describe('shortUrlCreationReducer', () => { it('returns loading on CREATE_SHORT_URL_START', () => { expect(reducer(undefined, action(createShortUrl.pending.toString()))).toEqual({ - result: null, saving: true, + saved: false, error: false, }); }); it('returns error on CREATE_SHORT_URL_ERROR', () => { expect(reducer(undefined, action(createShortUrl.rejected.toString()))).toEqual({ - result: null, saving: false, + saved: false, error: true, }); }); @@ -40,14 +40,15 @@ describe('shortUrlCreationReducer', () => { expect(reducer(undefined, action(createShortUrl.fulfilled.toString(), { payload: shortUrl }))).toEqual({ result: shortUrl, saving: false, + saved: true, error: false, }); }); it('returns default state on RESET_CREATE_SHORT_URL', () => { expect(reducer(undefined, action(resetCreateShortUrl.toString()))).toEqual({ - result: null, saving: false, + saved: false, error: false, }); }); From 61b274bab9e2eb12b31f0cea538d6027db943944 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 7 Nov 2022 18:27:41 +0100 Subject: [PATCH 2/3] Replaced inheritance by composition on short URL creation interface --- src/short-urls/CreateShortUrl.tsx | 2 +- src/short-urls/helpers/CreateShortUrlResult.tsx | 6 ++++-- test/short-urls/helpers/CreateShortUrlResult.test.tsx | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/short-urls/CreateShortUrl.tsx b/src/short-urls/CreateShortUrl.tsx index 890f2af5..2f045023 100644 --- a/src/short-urls/CreateShortUrl.tsx +++ b/src/short-urls/CreateShortUrl.tsx @@ -59,7 +59,7 @@ export const CreateShortUrl = ( }} /> diff --git a/src/short-urls/helpers/CreateShortUrlResult.tsx b/src/short-urls/helpers/CreateShortUrlResult.tsx index e89bab34..d37b511c 100644 --- a/src/short-urls/helpers/CreateShortUrlResult.tsx +++ b/src/short-urls/helpers/CreateShortUrlResult.tsx @@ -10,15 +10,17 @@ import { Result } from '../../utils/Result'; import './CreateShortUrlResult.scss'; import { ShlinkApiError } from '../../api/ShlinkApiError'; -export interface CreateShortUrlResultProps extends ShortUrlCreation { +export interface CreateShortUrlResultProps { + creation: ShortUrlCreation; resetCreateShortUrl: () => void; canBeClosed?: boolean; } export const CreateShortUrlResult = (useTimeoutToggle: TimeoutToggle) => ( - { error, errorData, result, resetCreateShortUrl, canBeClosed = false }: CreateShortUrlResultProps, + { creation, resetCreateShortUrl, canBeClosed = false }: CreateShortUrlResultProps, ) => { const [showCopyTooltip, setShowCopyTooltip] = useTimeoutToggle(); + const { error, errorData, result } = creation; useEffect(() => { resetCreateShortUrl(); diff --git a/test/short-urls/helpers/CreateShortUrlResult.test.tsx b/test/short-urls/helpers/CreateShortUrlResult.test.tsx index 03082a93..55b46e13 100644 --- a/test/short-urls/helpers/CreateShortUrlResult.test.tsx +++ b/test/short-urls/helpers/CreateShortUrlResult.test.tsx @@ -9,8 +9,8 @@ describe('', () => { const copyToClipboard = jest.fn(); const useTimeoutToggle = jest.fn(() => [false, copyToClipboard]) as TimeoutToggle; const CreateShortUrlResult = createResult(useTimeoutToggle); - const setUp = (result: ShortUrl | null = null, error = false) => renderWithEvents( - {}} result={result} error={error} saving={false} />, + const setUp = (result?: ShortUrl, error = false) => renderWithEvents( + {}} creation={{ result, saving: false, error, saved: false }} />, ); afterEach(jest.clearAllMocks); From 085ab521c38705c10efc4d12705befa5ce678c17 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 7 Nov 2022 18:55:12 +0100 Subject: [PATCH 3/3] Implemented state machine for short URL creation --- .../helpers/CreateShortUrlResult.tsx | 8 ++--- src/short-urls/reducers/shortUrlCreation.ts | 30 +++++++++++++------ .../helpers/CreateShortUrlResult.test.tsx | 19 +++++++----- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/src/short-urls/helpers/CreateShortUrlResult.tsx b/src/short-urls/helpers/CreateShortUrlResult.tsx index d37b511c..220eb42f 100644 --- a/src/short-urls/helpers/CreateShortUrlResult.tsx +++ b/src/short-urls/helpers/CreateShortUrlResult.tsx @@ -20,7 +20,7 @@ export const CreateShortUrlResult = (useTimeoutToggle: TimeoutToggle) => ( { creation, resetCreateShortUrl, canBeClosed = false }: CreateShortUrlResultProps, ) => { const [showCopyTooltip, setShowCopyTooltip] = useTimeoutToggle(); - const { error, errorData, result } = creation; + const { error, saved } = creation; useEffect(() => { resetCreateShortUrl(); @@ -30,16 +30,16 @@ export const CreateShortUrlResult = (useTimeoutToggle: TimeoutToggle) => ( return ( {canBeClosed && } - + ); } - if (!result) { + if (!saved) { return null; } - const { shortUrl } = result; + const { shortUrl } = creation.result; return ( diff --git a/src/short-urls/reducers/shortUrlCreation.ts b/src/short-urls/reducers/shortUrlCreation.ts index 5124701d..f30cdcd3 100644 --- a/src/short-urls/reducers/shortUrlCreation.ts +++ b/src/short-urls/reducers/shortUrlCreation.ts @@ -7,13 +7,25 @@ import { ProblemDetailsError } from '../../api/types/errors'; export const CREATE_SHORT_URL = 'shlink/createShortUrl/CREATE_SHORT_URL'; -export interface ShortUrlCreation { - result?: ShortUrl; - saving: boolean; - saved: boolean; - error: boolean; +export type ShortUrlCreation = { + saving: false; + saved: false; + error: false; +} | { + saving: true; + saved: false; + error: false; +} | { + saving: false; + saved: false; + error: true; errorData?: ProblemDetailsError; -} +} | { + result: ShortUrl; + saving: false; + saved: true; + error: false; +}; export type CreateShortUrlAction = PayloadAction; @@ -31,15 +43,15 @@ export const shortUrlCreationReducerCreator = (buildShlinkApiClient: ShlinkApiCl const { reducer, actions } = createSlice({ name: 'shortUrlCreationReducer', - initialState, + initialState: initialState as ShortUrlCreation, // Without this casting it infers type ShortUrlCreationWaiting reducers: { resetCreateShortUrl: () => initialState, }, extraReducers: (builder) => { - builder.addCase(createShortUrl.pending, (state) => ({ ...state, saving: true, saved: false, error: false })); + builder.addCase(createShortUrl.pending, () => ({ saving: true, saved: false, error: false })); builder.addCase( createShortUrl.rejected, - (state, { error }) => ({ ...state, saving: false, saved: false, error: true, errorData: parseApiError(error) }), + (_, { error }) => ({ saving: false, saved: false, error: true, errorData: parseApiError(error) }), ); builder.addCase( createShortUrl.fulfilled, diff --git a/test/short-urls/helpers/CreateShortUrlResult.test.tsx b/test/short-urls/helpers/CreateShortUrlResult.test.tsx index 55b46e13..d9cbba96 100644 --- a/test/short-urls/helpers/CreateShortUrlResult.test.tsx +++ b/test/short-urls/helpers/CreateShortUrlResult.test.tsx @@ -4,34 +4,39 @@ import { CreateShortUrlResult as createResult } from '../../../src/short-urls/he import { ShortUrl } from '../../../src/short-urls/data'; import { TimeoutToggle } from '../../../src/utils/helpers/hooks'; import { renderWithEvents } from '../../__helpers__/setUpTest'; +import { ShortUrlCreation } from '../../../src/short-urls/reducers/shortUrlCreation'; describe('', () => { const copyToClipboard = jest.fn(); const useTimeoutToggle = jest.fn(() => [false, copyToClipboard]) as TimeoutToggle; const CreateShortUrlResult = createResult(useTimeoutToggle); - const setUp = (result?: ShortUrl, error = false) => renderWithEvents( - {}} creation={{ result, saving: false, error, saved: false }} />, + const setUp = (creation: ShortUrlCreation) => renderWithEvents( + {}} creation={creation} />, ); afterEach(jest.clearAllMocks); it('renders an error when error is true', () => { - setUp(Mock.all(), true); + setUp({ error: true, saved: false, saving: false }); expect(screen.getByText('An error occurred while creating the URL :(')).toBeInTheDocument(); }); - it('renders nothing when no result is provided', () => { - const { container } = setUp(); + it.each([[true], [false]])('renders nothing when not saved yet', (saving) => { + const { container } = setUp({ error: false, saved: false, saving }); expect(container.firstChild).toBeNull(); }); it('renders a result message when result is provided', () => { - setUp(Mock.of({ shortUrl: 'https://doma.in/abc123' })); + setUp( + { result: Mock.of({ shortUrl: 'https://doma.in/abc123' }), saving: false, saved: true, error: false }, + ); expect(screen.getByText(/The short URL is/)).toHaveTextContent('Great! The short URL is https://doma.in/abc123'); }); it('Invokes tooltip timeout when copy to clipboard button is clicked', async () => { - const { user } = setUp(Mock.of({ shortUrl: 'https://doma.in/abc123' })); + const { user } = setUp( + { result: Mock.of({ shortUrl: 'https://doma.in/abc123' }), saving: false, saved: true, error: false }, + ); expect(copyToClipboard).not.toHaveBeenCalled(); await user.click(screen.getByRole('button'));