From d21758c410b35a987ee1e30df04d89d29d418f03 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 22 Nov 2022 19:39:07 +0100 Subject: [PATCH 1/5] Fixed DeleteShortUrlModal being removed from the DOM before CSS transition finished --- .../helpers/DeleteShortUrlModal.tsx | 21 ++++++++++++------- src/short-urls/reducers/shortUrlDeletion.ts | 6 ++++-- src/short-urls/reducers/shortUrlsList.ts | 5 ++--- src/short-urls/services/provideServices.ts | 9 +++++--- .../helpers/DeleteShortUrlModal.test.tsx | 11 ++++++---- .../short-urls/reducers/shortUrlsList.test.ts | 7 +++---- 6 files changed, 36 insertions(+), 23 deletions(-) diff --git a/src/short-urls/helpers/DeleteShortUrlModal.tsx b/src/short-urls/helpers/DeleteShortUrlModal.tsx index d240279f..37582b95 100644 --- a/src/short-urls/helpers/DeleteShortUrlModal.tsx +++ b/src/short-urls/helpers/DeleteShortUrlModal.tsx @@ -10,23 +10,30 @@ import { ShlinkApiError } from '../../api/ShlinkApiError'; interface DeleteShortUrlModalConnectProps extends ShortUrlModalProps { shortUrlDeletion: ShortUrlDeletion; - deleteShortUrl: (shortUrl: ShortUrlIdentifier) => void; + deleteShortUrl: (shortUrl: ShortUrlIdentifier) => Promise; + shortUrlDeleted: (shortUrl: ShortUrlIdentifier) => void; resetDeleteShortUrl: () => void; } -export const DeleteShortUrlModal = ( - { shortUrl, toggle, isOpen, shortUrlDeletion, resetDeleteShortUrl, deleteShortUrl }: DeleteShortUrlModalConnectProps, -) => { +export const DeleteShortUrlModal = ({ + shortUrl, + toggle, + isOpen, + shortUrlDeletion, + resetDeleteShortUrl, + deleteShortUrl, + shortUrlDeleted, +}: DeleteShortUrlModalConnectProps) => { const [inputValue, setInputValue] = useState(''); useEffect(() => resetDeleteShortUrl, []); - const { loading, error, errorData } = shortUrlDeletion; + const { loading, error, deleted, errorData } = shortUrlDeletion; const close = pipe(resetDeleteShortUrl, toggle); - const handleDeleteUrl = handleEventPreventingDefault(() => deleteShortUrl(shortUrl)); + const handleDeleteUrl = handleEventPreventingDefault(() => deleteShortUrl(shortUrl).then(toggle)); return ( - + deleted && shortUrlDeleted(shortUrl)}>
Delete short URL diff --git a/src/short-urls/reducers/shortUrlDeletion.ts b/src/short-urls/reducers/shortUrlDeletion.ts index 67d86499..294e1986 100644 --- a/src/short-urls/reducers/shortUrlDeletion.ts +++ b/src/short-urls/reducers/shortUrlDeletion.ts @@ -1,9 +1,9 @@ -import { createSlice } from '@reduxjs/toolkit'; +import { createAction, createSlice } from '@reduxjs/toolkit'; import { createAsyncThunk } from '../../utils/helpers/redux'; import { ShlinkApiClientBuilder } from '../../api/services/ShlinkApiClientBuilder'; import { parseApiError } from '../../api/utils'; import { ProblemDetailsError } from '../../api/types/errors'; -import { ShortUrlIdentifier } from '../data'; +import { ShortUrl, ShortUrlIdentifier } from '../data'; const REDUCER_PREFIX = 'shlink/shortUrlDeletion'; @@ -31,6 +31,8 @@ export const deleteShortUrl = (buildShlinkApiClient: ShlinkApiClientBuilder) => }, ); +export const shortUrlDeleted = createAction(`${REDUCER_PREFIX}/shortUrlDeleted`); + export const shortUrlDeletionReducerCreator = (deleteShortUrlThunk: ReturnType) => { const { actions, reducer } = createSlice({ name: REDUCER_PREFIX, diff --git a/src/short-urls/reducers/shortUrlsList.ts b/src/short-urls/reducers/shortUrlsList.ts index c6885353..66ba341d 100644 --- a/src/short-urls/reducers/shortUrlsList.ts +++ b/src/short-urls/reducers/shortUrlsList.ts @@ -5,7 +5,7 @@ import { createNewVisits } from '../../visits/reducers/visitCreation'; import { createAsyncThunk } from '../../utils/helpers/redux'; import { ShlinkApiClientBuilder } from '../../api/services/ShlinkApiClientBuilder'; import { ShlinkShortUrlsListParams, ShlinkShortUrlsResponse } from '../../api/types'; -import { deleteShortUrl } from './shortUrlDeletion'; +import { shortUrlDeleted } from './shortUrlDeletion'; import { createShortUrl } from './shortUrlCreation'; import { editShortUrl } from './shortUrlEdition'; import { ShortUrl } from '../data'; @@ -36,7 +36,6 @@ export const shortUrlsListReducerCreator = ( listShortUrlsThunk: ReturnType, editShortUrlThunk: ReturnType, createShortUrlThunk: ReturnType, - deleteShortUrlThunk: ReturnType, ) => createSlice({ name: REDUCER_PREFIX, initialState, @@ -81,7 +80,7 @@ export const shortUrlsListReducerCreator = ( ); builder.addCase( - deleteShortUrlThunk.fulfilled, + shortUrlDeleted, pipe( (state, { payload }) => (!state.shortUrls ? state : assocPath( ['shortUrls', 'data'], diff --git a/src/short-urls/services/provideServices.ts b/src/short-urls/services/provideServices.ts index 4ddb4ea7..eeb10ce9 100644 --- a/src/short-urls/services/provideServices.ts +++ b/src/short-urls/services/provideServices.ts @@ -9,7 +9,7 @@ import { DeleteShortUrlModal } from '../helpers/DeleteShortUrlModal'; import { CreateShortUrlResult } from '../helpers/CreateShortUrlResult'; import { listShortUrls, shortUrlsListReducerCreator } from '../reducers/shortUrlsList'; import { shortUrlCreationReducerCreator, createShortUrl } from '../reducers/shortUrlCreation'; -import { shortUrlDeletionReducerCreator, deleteShortUrl } from '../reducers/shortUrlDeletion'; +import { shortUrlDeletionReducerCreator, deleteShortUrl, shortUrlDeleted } from '../reducers/shortUrlDeletion'; import { editShortUrl, shortUrlEditionReducerCreator } from '../reducers/shortUrlEdition'; import { shortUrlDetailReducerCreator } from '../reducers/shortUrlDetail'; import { ConnectDecorator } from '../../container/types'; @@ -46,7 +46,10 @@ const provideServices = (bottle: Bottle, connect: ConnectDecorator) => { )); bottle.serviceFactory('DeleteShortUrlModal', () => DeleteShortUrlModal); - bottle.decorator('DeleteShortUrlModal', connect(['shortUrlDeletion'], ['deleteShortUrl', 'resetDeleteShortUrl'])); + bottle.decorator('DeleteShortUrlModal', connect( + ['shortUrlDeletion'], + ['deleteShortUrl', 'shortUrlDeleted', 'resetDeleteShortUrl'], + )); bottle.serviceFactory('QrCodeModal', QrCodeModal, 'ImageDownloader'); bottle.decorator('QrCodeModal', connect(['selectedServer'])); @@ -63,7 +66,6 @@ const provideServices = (bottle: Bottle, connect: ConnectDecorator) => { 'listShortUrls', 'editShortUrl', 'createShortUrl', - 'deleteShortUrl', ); bottle.serviceFactory('shortUrlsListReducer', prop('reducer'), 'shortUrlsListReducerCreator'); @@ -87,6 +89,7 @@ const provideServices = (bottle: Bottle, connect: ConnectDecorator) => { bottle.serviceFactory('deleteShortUrl', deleteShortUrl, 'buildShlinkApiClient'); bottle.serviceFactory('resetDeleteShortUrl', prop('resetDeleteShortUrl'), 'shortUrlDeletionReducerCreator'); + bottle.serviceFactory('shortUrlDeleted', () => shortUrlDeleted); bottle.serviceFactory('getShortUrlDetail', prop('getShortUrlDetail'), 'shortUrlDetailReducerCreator'); diff --git a/test/short-urls/helpers/DeleteShortUrlModal.test.tsx b/test/short-urls/helpers/DeleteShortUrlModal.test.tsx index 282c4738..d4145296 100644 --- a/test/short-urls/helpers/DeleteShortUrlModal.test.tsx +++ b/test/short-urls/helpers/DeleteShortUrlModal.test.tsx @@ -1,4 +1,4 @@ -import { screen } from '@testing-library/react'; +import { screen, waitFor } from '@testing-library/react'; import { Mock } from 'ts-mockery'; import { DeleteShortUrlModal } from '../../../src/short-urls/helpers/DeleteShortUrlModal'; import { ShortUrl } from '../../../src/short-urls/data'; @@ -12,15 +12,17 @@ describe('', () => { shortCode: 'abc123', longUrl: 'https://long-domain.com/foo/bar', }); - const deleteShortUrl = jest.fn(); + const deleteShortUrl = jest.fn().mockResolvedValue(undefined); + const toggle = jest.fn(); const setUp = (shortUrlDeletion: Partial) => renderWithEvents( (shortUrlDeletion)} deleteShortUrl={deleteShortUrl} - toggle={() => {}} - resetDeleteShortUrl={() => {}} + shortUrlDeleted={jest.fn()} + toggle={toggle} + resetDeleteShortUrl={jest.fn()} />, ); @@ -81,5 +83,6 @@ describe('', () => { await user.type(screen.getByPlaceholderText(/^Insert the short code/), shortCode); await user.click(screen.getByRole('button', { name: 'Delete' })); expect(deleteShortUrl).toHaveBeenCalledTimes(1); + await waitFor(() => expect(toggle).toHaveBeenCalledTimes(1)); }); }); diff --git a/test/short-urls/reducers/shortUrlsList.test.ts b/test/short-urls/reducers/shortUrlsList.test.ts index cc9e748d..8e639603 100644 --- a/test/short-urls/reducers/shortUrlsList.test.ts +++ b/test/short-urls/reducers/shortUrlsList.test.ts @@ -3,7 +3,7 @@ import { listShortUrls as listShortUrlsCreator, shortUrlsListReducerCreator, } from '../../../src/short-urls/reducers/shortUrlsList'; -import { deleteShortUrl as deleteShortUrlCreator } from '../../../src/short-urls/reducers/shortUrlDeletion'; +import { shortUrlDeleted } from '../../../src/short-urls/reducers/shortUrlDeletion'; import { ShlinkPaginator, ShlinkShortUrlsResponse } from '../../../src/api/types'; import { createShortUrl as createShortUrlCreator } from '../../../src/short-urls/reducers/shortUrlCreation'; import { editShortUrl as editShortUrlCreator } from '../../../src/short-urls/reducers/shortUrlEdition'; @@ -18,8 +18,7 @@ describe('shortUrlsListReducer', () => { const listShortUrls = listShortUrlsCreator(buildShlinkApiClient); const editShortUrl = editShortUrlCreator(buildShlinkApiClient); const createShortUrl = createShortUrlCreator(buildShlinkApiClient); - const deleteShortUrl = deleteShortUrlCreator(buildShlinkApiClient); - const { reducer } = shortUrlsListReducerCreator(listShortUrls, editShortUrl, createShortUrl, deleteShortUrl); + const { reducer } = shortUrlsListReducerCreator(listShortUrls, editShortUrl, createShortUrl); afterEach(jest.clearAllMocks); @@ -59,7 +58,7 @@ describe('shortUrlsListReducer', () => { error: false, }; - expect(reducer(state, { type: deleteShortUrl.fulfilled.toString(), payload: { shortCode } })).toEqual({ + expect(reducer(state, { type: shortUrlDeleted.toString(), payload: { shortCode } })).toEqual({ shortUrls: { data: [{ shortCode, domain: 'example.com' }, { shortCode: 'foo' }], pagination: { totalItems: 9 }, From 9bdf55374cf970eeddcc00fcef9e434f69abf565 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 22 Nov 2022 20:08:08 +0100 Subject: [PATCH 2/5] Ensured DeleteServerModal is not removed from the DOM before close transition has finished --- src/servers/DeleteServerModal.tsx | 18 ++++++++++---- test/servers/DeleteServerModal.test.tsx | 32 +++++++++++++++---------- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/src/servers/DeleteServerModal.tsx b/src/servers/DeleteServerModal.tsx index 8fba141c..d22c93c4 100644 --- a/src/servers/DeleteServerModal.tsx +++ b/src/servers/DeleteServerModal.tsx @@ -1,4 +1,4 @@ -import { FC } from 'react'; +import { FC, useRef } from 'react'; import { Button, Modal, ModalBody, ModalFooter, ModalHeader } from 'reactstrap'; import { useNavigate } from 'react-router-dom'; import { ServerWithId } from './data'; @@ -18,14 +18,22 @@ export const DeleteServerModal: FC = ( { server, toggle, isOpen, deleteServer, redirectHome = true }, ) => { const navigate = useNavigate(); - const closeModal = () => { - deleteServer(server); + const doDelete = useRef(false); + const toggleAndDelete = () => { + doDelete.current = true; toggle(); + }; + const onClosed = () => { + if (!doDelete.current) { + return; + } + + deleteServer(server); redirectHome && navigate('/'); }; return ( - + Remove server

Are you sure you want to remove {server ? server.name : ''}?

@@ -38,7 +46,7 @@ export const DeleteServerModal: FC = ( - + ); diff --git a/test/servers/DeleteServerModal.test.tsx b/test/servers/DeleteServerModal.test.tsx index cf509b41..0e89afa9 100644 --- a/test/servers/DeleteServerModal.test.tsx +++ b/test/servers/DeleteServerModal.test.tsx @@ -1,25 +1,37 @@ -import { screen } from '@testing-library/react'; +import { FC } from 'react'; +import { screen, waitFor } from '@testing-library/react'; import { Mock } from 'ts-mockery'; import { useNavigate } from 'react-router-dom'; import { DeleteServerModal } from '../../src/servers/DeleteServerModal'; import { ServerWithId } from '../../src/servers/data'; import { renderWithEvents } from '../__helpers__/setUpTest'; +import { useToggle } from '../../src/utils/helpers/hooks'; jest.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), useNavigate: jest.fn() })); +const DeleteServerModalWrapper: FC<{ name: string; deleteServer: () => void }> = ({ name, deleteServer }) => { + const [isOpen, toggle] = useToggle(true); + + return ( + ({ name })} + toggle={toggle} + isOpen={isOpen} + deleteServer={deleteServer} + /> + ); +}; + describe('', () => { const deleteServerMock = jest.fn(); const navigate = jest.fn(); - const toggleMock = jest.fn(); const serverName = 'the_server_name'; const setUp = () => { (useNavigate as any).mockReturnValue(navigate); return renderWithEvents( - ({ name: serverName })} - toggle={toggleMock} - isOpen + , ); @@ -47,10 +59,8 @@ describe('', () => { ])('toggles when clicking cancel button', async (getButton) => { const { user } = setUp(); - expect(toggleMock).not.toHaveBeenCalled(); await user.click(getButton()); - expect(toggleMock).toHaveBeenCalledTimes(1); expect(deleteServerMock).not.toHaveBeenCalled(); expect(navigate).not.toHaveBeenCalled(); }); @@ -58,13 +68,11 @@ describe('', () => { it('deletes server when clicking accept button', async () => { const { user } = setUp(); - expect(toggleMock).not.toHaveBeenCalled(); expect(deleteServerMock).not.toHaveBeenCalled(); expect(navigate).not.toHaveBeenCalled(); await user.click(screen.getByRole('button', { name: 'Delete' })); - expect(toggleMock).toHaveBeenCalledTimes(1); - expect(deleteServerMock).toHaveBeenCalledTimes(1); - expect(navigate).toHaveBeenCalledTimes(1); + await waitFor(() => expect(deleteServerMock).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(navigate).toHaveBeenCalledTimes(1)); }); }); From cf9163784821b689f1d85ea23535940b1e72b266 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 22 Nov 2022 20:09:00 +0100 Subject: [PATCH 3/5] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ceac5c98..adabe5f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), * [#590](https://github.com/shlinkio/shlink-web-client/issues/590) Fixed position of the datepicker triangle. * [#729](https://github.com/shlinkio/shlink-web-client/issues/729) Fixed wrong stats displayed in tags after renaming. * [#737](https://github.com/shlinkio/shlink-web-client/issues/737) Fixed incorrect contrast in warning messages when using dark theme. +* [#726](https://github.com/shlinkio/shlink-web-client/issues/726) Fixed delete server and delete short URL modals getting removed from the DOM before finishing close transition. ## [3.7.3] - 2022-09-13 From f0a6420ba91d7defd0c5ec1983f8d5043779258b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 22 Nov 2022 20:18:11 +0100 Subject: [PATCH 4/5] Extracted helper to test modals --- test/__helpers__/TestModalWrapper.tsx | 14 +++++++++++++ test/servers/DeleteServerModal.test.tsx | 27 +++++++++---------------- 2 files changed, 23 insertions(+), 18 deletions(-) create mode 100644 test/__helpers__/TestModalWrapper.tsx diff --git a/test/__helpers__/TestModalWrapper.tsx b/test/__helpers__/TestModalWrapper.tsx new file mode 100644 index 00000000..e2364192 --- /dev/null +++ b/test/__helpers__/TestModalWrapper.tsx @@ -0,0 +1,14 @@ +import { FC, ReactElement } from 'react'; +import { useToggle } from '../../src/utils/helpers/hooks'; + +interface RenderModalArgs { + isOpen: boolean; + toggle: () => void; +} + +export const TestModalWrapper: FC<{ renderModal: (args: RenderModalArgs) => ReactElement }> = ( + { renderModal }, +) => { + const [isOpen, toggle] = useToggle(true); + return renderModal({ isOpen, toggle }); +}; diff --git a/test/servers/DeleteServerModal.test.tsx b/test/servers/DeleteServerModal.test.tsx index 0e89afa9..0a5affe5 100644 --- a/test/servers/DeleteServerModal.test.tsx +++ b/test/servers/DeleteServerModal.test.tsx @@ -1,27 +1,13 @@ -import { FC } from 'react'; import { screen, waitFor } from '@testing-library/react'; import { Mock } from 'ts-mockery'; import { useNavigate } from 'react-router-dom'; import { DeleteServerModal } from '../../src/servers/DeleteServerModal'; import { ServerWithId } from '../../src/servers/data'; import { renderWithEvents } from '../__helpers__/setUpTest'; -import { useToggle } from '../../src/utils/helpers/hooks'; +import { TestModalWrapper } from '../__helpers__/TestModalWrapper'; jest.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), useNavigate: jest.fn() })); -const DeleteServerModalWrapper: FC<{ name: string; deleteServer: () => void }> = ({ name, deleteServer }) => { - const [isOpen, toggle] = useToggle(true); - - return ( - ({ name })} - toggle={toggle} - isOpen={isOpen} - deleteServer={deleteServer} - /> - ); -}; - describe('', () => { const deleteServerMock = jest.fn(); const navigate = jest.fn(); @@ -30,9 +16,14 @@ describe('', () => { (useNavigate as any).mockReturnValue(navigate); return renderWithEvents( - ( + ({ name: serverName })} + deleteServer={deleteServerMock} + /> + )} />, ); }; From 32f29a84f7f35d53fd46efee9252b02784c35e5f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 22 Nov 2022 20:22:03 +0100 Subject: [PATCH 5/5] Used TestModalWrapper in DeleteShortUrlModal test --- .../helpers/DeleteShortUrlModal.test.tsx | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/test/short-urls/helpers/DeleteShortUrlModal.test.tsx b/test/short-urls/helpers/DeleteShortUrlModal.test.tsx index d4145296..448705ef 100644 --- a/test/short-urls/helpers/DeleteShortUrlModal.test.tsx +++ b/test/short-urls/helpers/DeleteShortUrlModal.test.tsx @@ -5,6 +5,7 @@ import { ShortUrl } from '../../../src/short-urls/data'; import { ShortUrlDeletion } from '../../../src/short-urls/reducers/shortUrlDeletion'; import { renderWithEvents } from '../../__helpers__/setUpTest'; import { ErrorTypeV2, ErrorTypeV3, InvalidShortUrlDeletion, ProblemDetailsError } from '../../../src/api/types/errors'; +import { TestModalWrapper } from '../../__helpers__/TestModalWrapper'; describe('', () => { const shortUrl = Mock.of({ @@ -13,16 +14,19 @@ describe('', () => { longUrl: 'https://long-domain.com/foo/bar', }); const deleteShortUrl = jest.fn().mockResolvedValue(undefined); - const toggle = jest.fn(); + const shortUrlDeleted = jest.fn(); const setUp = (shortUrlDeletion: Partial) => renderWithEvents( - (shortUrlDeletion)} - deleteShortUrl={deleteShortUrl} - shortUrlDeleted={jest.fn()} - toggle={toggle} - resetDeleteShortUrl={jest.fn()} + ( + (shortUrlDeletion)} + deleteShortUrl={deleteShortUrl} + shortUrlDeleted={shortUrlDeleted} + resetDeleteShortUrl={jest.fn()} + /> + )} />, ); @@ -76,6 +80,7 @@ describe('', () => { const { user } = setUp({ loading: false, error: false, + deleted: true, shortCode, }); @@ -83,6 +88,6 @@ describe('', () => { await user.type(screen.getByPlaceholderText(/^Insert the short code/), shortCode); await user.click(screen.getByRole('button', { name: 'Delete' })); expect(deleteShortUrl).toHaveBeenCalledTimes(1); - await waitFor(() => expect(toggle).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(shortUrlDeleted).toHaveBeenCalledTimes(1)); }); });