From 93bf0d4991e8135bb6b45b2e940e3b1240c32f19 Mon Sep 17 00:00:00 2001 From: gigigimay <51327193+gigigimay@users.noreply.github.com> Date: Thu, 24 Oct 2024 20:42:46 +0700 Subject: [PATCH] feat: Improve Cold Start requests by adding timeout and blocking user input | [#261] * feat: handle loading and server timeout in StartMenu | [#261] * feat: handle register loading and timeout in RegisterMenu | [#261] * feat: cancel requests when timeout for login + autologin + register | [#261] * fix: revert mocked token | [#261] * feat: move loading circle into the button | [#261] * test: add unit tests for login + auto login + register request timeout | [#261] * refactor: remove unused and refactor css | [#261] --- .../src/__tests__/pages/RegisterMenu.test.tsx | 392 ++++++++++-------- .../src/__tests__/pages/StartMenu.test.tsx | 60 ++- .../LoadingCircle/LoadingCircleSmall.tsx | 17 + .../LoadingCircleSmallNoPadding.tsx | 26 -- .../Accordion/AddSongPlayListAccordion.tsx | 2 +- Electron/src/global/global.ts | 2 + Electron/src/pages/StartMenu/RegisterMenu.tsx | 29 +- Electron/src/pages/StartMenu/StartMenu.tsx | 71 +++- .../src/pages/StartMenu/startMenu.module.css | 16 +- 9 files changed, 396 insertions(+), 219 deletions(-) create mode 100644 Electron/src/components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmall.tsx delete mode 100644 Electron/src/components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmallNoPadding.tsx diff --git a/Electron/src/__tests__/pages/RegisterMenu.test.tsx b/Electron/src/__tests__/pages/RegisterMenu.test.tsx index 407508ca..4cc87079 100644 --- a/Electron/src/__tests__/pages/RegisterMenu.test.tsx +++ b/Electron/src/__tests__/pages/RegisterMenu.test.tsx @@ -1,207 +1,263 @@ import '@testing-library/jest-dom'; -import { act, fireEvent, render } from '@testing-library/react'; +import { act, fireEvent, render, screen } from '@testing-library/react'; import '@testing-library/jest-dom/extend-expect'; import RegisterMenu from 'pages/StartMenu/RegisterMenu'; import Global from 'global/global'; +import timeout from 'utils/timeout'; +import { CancelablePromise } from 'swagger/api'; +import { UsersService } from '../../swagger/api/services/UsersService'; -afterEach(() => { - jest.clearAllMocks(); -}); - -test('render RegisterMenu', () => { - expect(render( {}} />)).toBeTruthy(); -}); - -test('RegisterMenu correct text', () => { - const component = render(); - - expect(component.container).toHaveTextContent( - 'Regístrate en Spotify Electron', - ); -}); - -test('Register failed not all required inputs filled', () => { - const mockSetIsSigningUp = jest.fn(); - - const component = render( - , - ); +jest.mock('../../swagger/api/services/UsersService'); +jest.mock('utils/timeout'); - const registerButton = component.getByText('Registrar'); - fireEvent.click(registerButton); +describe('RegisterMenu Component', () => { + afterEach(() => { + jest.clearAllMocks(); + jest.resetAllMocks(); + }); - expect( - component.queryByText( - 'No se han introducido todos los datos de registro obligatorios', - ), - ).toBeInTheDocument(); + test('render RegisterMenu', () => { + expect(render( {}} />)).toBeTruthy(); + }); - const popover = document.getElementById('button-info-popover'); - expect(popover).toBeVisible(); - if (popover) { - fireEvent.click(popover); - } + test('RegisterMenu correct text', () => { + const component = render(); - fireEvent.click(document.body); + expect(component.container).toHaveTextContent( + 'Regístrate en Spotify Electron', + ); + }); - expect( - component.queryByText( - 'No se han introducido todos los datos de registro obligatorios', - ), - ).toBeNull(); - expect(popover).not.toBeVisible(); -}); + test('Register failed not all required inputs filled', () => { + const mockSetIsSigningUp = jest.fn(); -test('Register failed bad input', () => { - const mockSetIsSigningUp = jest.fn(); + const component = render( + , + ); - const component = render( - , - ); + const registerButton = component.getByText('Registrar'); + fireEvent.click(registerButton); - const registerButton = component.getByText('Registrar'); - fireEvent.click(registerButton); + expect( + component.queryByText( + 'No se han introducido todos los datos de registro obligatorios', + ), + ).toBeInTheDocument(); - expect( - component.queryByText( - 'No se han introducido todos los datos de registro obligatorios', - ), - ).toBeInTheDocument(); + const popover = document.getElementById('button-info-popover'); + expect(popover).toBeVisible(); + if (popover) { + fireEvent.click(popover); + } - const popover = document.getElementById('button-info-popover'); - expect(popover).toBeVisible(); - if (popover) { - fireEvent.click(popover); - } + fireEvent.click(document.body); - fireEvent.click(document.body); + expect( + component.queryByText( + 'No se han introducido todos los datos de registro obligatorios', + ), + ).toBeNull(); + expect(popover).not.toBeVisible(); + }); - expect( - component.queryByText( - 'No se han introducido todos los datos de registro obligatorios', - ), - ).toBeNull(); - expect(popover).not.toBeVisible(); -}); + test('Register failed bad input', () => { + const mockSetIsSigningUp = jest.fn(); -test('Register failed different password inputs', () => { - const mockSetIsSigningUp = jest.fn(); + const component = render( + , + ); - global.fetch = jest.fn(() => - Promise.resolve({ - json: () => Promise.resolve({}), - status: 200, - ok: true, - }), - ) as jest.Mock; + const registerButton = component.getByText('Registrar'); + fireEvent.click(registerButton); - const component = render( - , - ); + expect( + component.queryByText( + 'No se han introducido todos los datos de registro obligatorios', + ), + ).toBeInTheDocument(); - // Simulate user input + const popover = document.getElementById('button-info-popover'); + expect(popover).toBeVisible(); + if (popover) { + fireEvent.click(popover); + } - const inputName = component.getByPlaceholderText('Nombre de usuario'); - const inputPhoto = component.getByPlaceholderText('Foto de perfil'); - const inputPassword = component.getByPlaceholderText('Contraseña'); - const inputConfirmPassword = component.getByPlaceholderText( - 'Confirma tu contraseña', - ); + fireEvent.click(document.body); - fireEvent.change(inputName, { - target: { value: 'testuser' }, - }); - fireEvent.change(inputPassword, { - target: { value: 'testpassword' }, - }); - fireEvent.change(inputPhoto, { - target: { value: 'testfoto' }, + expect( + component.queryByText( + 'No se han introducido todos los datos de registro obligatorios', + ), + ).toBeNull(); + expect(popover).not.toBeVisible(); }); - fireEvent.change(inputConfirmPassword, { - target: { value: 'testpassworddiferente' }, - }); - - const registerButton = component.getByText('Registrar'); - fireEvent.click(registerButton); - expect( - component.queryByText('Las contraseñas no coinciden'), - ).toBeInTheDocument(); -}); + test('Register failed different password inputs', () => { + const mockSetIsSigningUp = jest.fn(); -test('Register success', async () => { - const mockSetIsSigningUp = jest.fn(); - - global.fetch = jest.fn((url: string) => { - if ( - url === - `${Global.backendBaseUrl}/users/?name=testuser&photo=testphoto&password=testpassword` - ) { - return Promise.resolve({ + global.fetch = jest.fn(() => + Promise.resolve({ json: () => Promise.resolve({}), - status: 201, + status: 200, ok: true, - }).catch((error) => { - console.log(error); - }); - } - - // In case the URL doesn't match, return a rejected promise - return Promise.reject(new Error(`Unhandled URL in fetch mock: ${url}`)); - }) as jest.Mock; - - const component = render( - , - ); - - // Simulate user input - - const inputName = component.getByPlaceholderText('Nombre de usuario'); - const inputPhoto = component.getByPlaceholderText('Foto de perfil'); - const inputPassword = component.getByPlaceholderText('Contraseña'); - const inputConfirmPassword = component.getByPlaceholderText( - 'Confirma tu contraseña', - ); + }), + ) as jest.Mock; + + const component = render( + , + ); + + // Simulate user input + + const inputName = component.getByPlaceholderText('Nombre de usuario'); + const inputPhoto = component.getByPlaceholderText('Foto de perfil'); + const inputPassword = component.getByPlaceholderText('Contraseña'); + const inputConfirmPassword = component.getByPlaceholderText( + 'Confirma tu contraseña', + ); + + fireEvent.change(inputName, { + target: { value: 'testuser' }, + }); + fireEvent.change(inputPassword, { + target: { value: 'testpassword' }, + }); + fireEvent.change(inputPhoto, { + target: { value: 'testfoto' }, + }); + fireEvent.change(inputConfirmPassword, { + target: { value: 'testpassworddiferente' }, + }); + + const registerButton = component.getByText('Registrar'); + fireEvent.click(registerButton); - fireEvent.change(inputName, { - target: { value: 'testuser' }, - }); - fireEvent.change(inputPassword, { - target: { value: 'testpassword' }, - }); - fireEvent.change(inputPhoto, { - target: { value: 'testphoto' }, - }); - fireEvent.change(inputConfirmPassword, { - target: { value: 'testpassword' }, + expect( + component.queryByText('Las contraseñas no coinciden'), + ).toBeInTheDocument(); }); - const registerButton = component.getByText('Registrar'); - - await act(async () => { - fireEvent.click(registerButton); + test('Register failed request timeout', async () => { + const mockSetIsSigningUp = jest.fn(); + + const mockRegisterPromise = new CancelablePromise(() => {}); + (UsersService.createUserUsersPost as jest.Mock).mockImplementationOnce( + () => mockRegisterPromise, + ); + (timeout as jest.Mock).mockRejectedValueOnce(new Error('Timeout')); + + const component = render( + , + ); + + // Simulate user input + + const inputName = component.getByPlaceholderText('Nombre de usuario'); + const inputPhoto = component.getByPlaceholderText('Foto de perfil'); + const inputPassword = component.getByPlaceholderText('Contraseña'); + const inputConfirmPassword = component.getByPlaceholderText( + 'Confirma tu contraseña', + ); + + fireEvent.change(inputName, { + target: { value: 'testuser' }, + }); + fireEvent.change(inputPassword, { + target: { value: 'testpassword' }, + }); + fireEvent.change(inputPhoto, { + target: { value: 'testphoto' }, + }); + fireEvent.change(inputConfirmPassword, { + target: { value: 'testpassword' }, + }); + + const registerButton = component.getByText('Registrar'); + + await act(async () => { + fireEvent.click(registerButton); + }); + + expect(mockRegisterPromise.isCancelled).toBe(true); + expect( + screen.getByText('El servidor esta iniciándose'), + ).toBeInTheDocument(); }); - const popover = document.getElementById('button-info-popover'); - expect(popover).toBeVisible(); - if (popover) { - fireEvent.click(popover); - } + test('Register success', async () => { + const mockSetIsSigningUp = jest.fn(); + + global.fetch = jest.fn((url: string) => { + if ( + url === + `${Global.backendBaseUrl}/users/?name=testuser&photo=testphoto&password=testpassword` + ) { + return Promise.resolve({ + json: () => Promise.resolve({}), + status: 201, + ok: true, + }).catch((error) => { + console.log(error); + }); + } + + // In case the URL doesn't match, return a rejected promise + return Promise.reject(new Error(`Unhandled URL in fetch mock: ${url}`)); + }) as jest.Mock; + + const component = render( + , + ); + + // Simulate user input + + const inputName = component.getByPlaceholderText('Nombre de usuario'); + const inputPhoto = component.getByPlaceholderText('Foto de perfil'); + const inputPassword = component.getByPlaceholderText('Contraseña'); + const inputConfirmPassword = component.getByPlaceholderText( + 'Confirma tu contraseña', + ); + + fireEvent.change(inputName, { + target: { value: 'testuser' }, + }); + fireEvent.change(inputPassword, { + target: { value: 'testpassword' }, + }); + fireEvent.change(inputPhoto, { + target: { value: 'testphoto' }, + }); + fireEvent.change(inputConfirmPassword, { + target: { value: 'testpassword' }, + }); + + const registerButton = component.getByText('Registrar'); + + await act(async () => { + fireEvent.click(registerButton); + }); + + const popover = document.getElementById('button-info-popover'); + expect(popover).toBeVisible(); + if (popover) { + fireEvent.click(popover); + } - expect(mockSetIsSigningUp).toHaveBeenCalledWith(false); -}); + expect(mockSetIsSigningUp).toHaveBeenCalledWith(false); + }); -test('Change to login menu', () => { - const mockSetIsSigningUp = jest.fn(); + test('Change to login menu', () => { + const mockSetIsSigningUp = jest.fn(); - const component = render( - , - ); + const component = render( + , + ); - const regiserButton = component.getByText( - 'Inicia sesión en Spotify Electron', - ); - fireEvent.click(regiserButton); + const regiserButton = component.getByText( + 'Inicia sesión en Spotify Electron', + ); + fireEvent.click(regiserButton); - expect(mockSetIsSigningUp).toHaveBeenCalledWith(false); + expect(mockSetIsSigningUp).toHaveBeenCalledWith(false); + }); }); diff --git a/Electron/src/__tests__/pages/StartMenu.test.tsx b/Electron/src/__tests__/pages/StartMenu.test.tsx index f3cb829a..a4d56526 100644 --- a/Electron/src/__tests__/pages/StartMenu.test.tsx +++ b/Electron/src/__tests__/pages/StartMenu.test.tsx @@ -8,10 +8,13 @@ import { import '@testing-library/jest-dom'; import StartMenu from 'pages/StartMenu/StartMenu'; import { getToken } from 'utils/token'; +import timeout from 'utils/timeout'; +import { CancelablePromise } from 'swagger/api'; import { LoginService } from '../../swagger/api/services/LoginService'; jest.mock('../../swagger/api/services/LoginService'); jest.mock('utils/token'); +jest.mock('utils/timeout'); describe('StartMenu Component', () => { const setIsLoggedMock = jest.fn(); @@ -19,6 +22,7 @@ describe('StartMenu Component', () => { beforeEach(() => { jest.clearAllMocks(); + jest.resetAllMocks(); }); test('renders the component and displays title and form', async () => { @@ -80,7 +84,25 @@ describe('StartMenu Component', () => { ).mockResolvedValue(undefined); (getToken as jest.Mock).mockReturnValue('mock-jwt-token'); + await act(async () => { + render( + , + ); + }); + + expect(setIsLoggedMock).toHaveBeenCalledWith(true); + }); + + test('handles auto-login timeout on component mount and displays error popover', async () => { (getToken as jest.Mock).mockReturnValue('mock-jwt-token'); + const mockLoginPromise = new CancelablePromise(() => {}); + ( + LoginService.loginUserWithJwtLoginTokenTokenPost as jest.Mock + ).mockImplementationOnce(() => mockLoginPromise); + (timeout as jest.Mock).mockRejectedValueOnce(new Error('Timeout')); await act(async () => { render( @@ -91,7 +113,10 @@ describe('StartMenu Component', () => { ); }); - expect(setIsLoggedMock).toHaveBeenCalledWith(true); + expect(mockLoginPromise.isCancelled).toBe(true); + expect( + screen.getByText('El servidor esta iniciándose'), + ).toBeInTheDocument(); }); test('handles login error and displays error popover', async () => { @@ -124,6 +149,39 @@ describe('StartMenu Component', () => { ).toBeInTheDocument(); }); + test('handles login timeout and displays error popover', async () => { + const mockLoginPromise = new CancelablePromise(() => {}); + (LoginService.loginUserLoginPost as jest.Mock).mockImplementationOnce( + () => mockLoginPromise, + ); + (timeout as jest.Mock).mockRejectedValueOnce(new Error('Timeout')); + + await act(async () => { + render( + , + ); + }); + + fireEvent.change(screen.getByPlaceholderText('Nombre de usuario'), { + target: { value: 'user123' }, + }); + fireEvent.change(screen.getByPlaceholderText('Contraseña'), { + target: { value: 'password123' }, + }); + await act(async () => { + fireEvent.click(screen.getByText('Iniciar sesión')); + }); + + expect(mockLoginPromise.isCancelled).toBe(true); + expect(setIsLoggedMock).toHaveBeenCalledWith(false); + expect( + screen.getByText('El servidor esta iniciándose'), + ).toBeInTheDocument(); + }); + test('handles register button click', async () => { await act(async () => { render( diff --git a/Electron/src/components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmall.tsx b/Electron/src/components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmall.tsx new file mode 100644 index 00000000..72830475 --- /dev/null +++ b/Electron/src/components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmall.tsx @@ -0,0 +1,17 @@ +import CircularProgress from '@mui/material/CircularProgress/'; + +export default function LoadingCircleSmall() { + return ( + + ); +} diff --git a/Electron/src/components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmallNoPadding.tsx b/Electron/src/components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmallNoPadding.tsx deleted file mode 100644 index d30e7801..00000000 --- a/Electron/src/components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmallNoPadding.tsx +++ /dev/null @@ -1,26 +0,0 @@ -import CircularProgress from '@mui/material/CircularProgress/'; - -export default function LoadingCircleSmallNoPadding() { - return ( -
- -
- ); -} diff --git a/Electron/src/components/Sidebar/ModalAddSongPlaylist/Accordion/AddSongPlayListAccordion.tsx b/Electron/src/components/Sidebar/ModalAddSongPlaylist/Accordion/AddSongPlayListAccordion.tsx index 3c5809da..df5cc9b6 100644 --- a/Electron/src/components/Sidebar/ModalAddSongPlaylist/Accordion/AddSongPlayListAccordion.tsx +++ b/Electron/src/components/Sidebar/ModalAddSongPlaylist/Accordion/AddSongPlayListAccordion.tsx @@ -10,7 +10,7 @@ import { getTokenRole } from 'utils/token'; import { InfoPopoverType } from 'components/AdvancedUIComponents/InfoPopOver/types/InfoPopover'; import ConfirmationModal from 'components/AdvancedUIComponents/InfoPopOver/InfoPopover'; import UserType from 'utils/role'; -import LoadingCircleSmall from 'components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmallNoPadding'; +import LoadingCircleSmall from 'components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmall'; import { getGenreFromString } from 'utils/genre'; import GenreOption from './GenreOption/GenreOption'; import styles from './addSongPlayListAccordion.module.css'; diff --git a/Electron/src/global/global.ts b/Electron/src/global/global.ts index 2dc5ddcf..65c1f761 100644 --- a/Electron/src/global/global.ts +++ b/Electron/src/global/global.ts @@ -10,6 +10,8 @@ namespace Global { export const songArchitecture: SongArchitecture = SongArchitecture.BLOB_ARCHITECTURE; + export const coldStartRequestTimeout = 5000; + export interface HandleUrlChangeResponse { canGoBack: boolean | undefined; canGoForward: boolean | undefined; diff --git a/Electron/src/pages/StartMenu/RegisterMenu.tsx b/Electron/src/pages/StartMenu/RegisterMenu.tsx index b7ad91d0..6c2ddaee 100644 --- a/Electron/src/pages/StartMenu/RegisterMenu.tsx +++ b/Electron/src/pages/StartMenu/RegisterMenu.tsx @@ -5,6 +5,9 @@ import { PropsInfoPopover, } from 'components/AdvancedUIComponents/InfoPopOver/types/InfoPopover'; import timeout from 'utils/timeout'; +import Global from 'global/global'; +import LoadingCircleSmall from 'components/AdvancedUIComponents/LoadingCircle/LoadingCircleSmall'; +import { CancelablePromise } from 'swagger/api'; import styles from './startMenu.module.css'; import SpotifyElectronLogo from '../../assets/imgs/SpotifyElectronLogo.png'; import { UsersService } from '../../swagger/api/services/UsersService'; @@ -39,6 +42,9 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) { null, ); + /* Loading state */ + const [loading, setLoading] = useState(false); + const handleRegister = async (e: MouseEvent) => { e.preventDefault(); if ( @@ -76,16 +82,22 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) { return; } + let createUserPromise: CancelablePromise | null = null; try { - const createUserPromise = UsersService.createUserUsersPost( + setLoading(true); + + createUserPromise = UsersService.createUserUsersPost( formData.name, formData.photo, formData.password, ); - await Promise.race([createUserPromise, timeout(3000)]); - setisOpenPopover(true); + await Promise.race([ + createUserPromise, + timeout(Global.coldStartRequestTimeout), + ]); + setisOpenPopover(true); setPropsPopOver({ title: 'Usuario registrado', description: 'El usuario ha sido registrado con éxito', @@ -97,12 +109,15 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) { }, }); } catch (error: unknown) { + setLoading(false); console.log('Unable to register'); let title: string; let description: string; if (error instanceof Error && error.message === 'Timeout') { + createUserPromise?.cancel(); + title = 'El servidor esta iniciándose'; description = 'El servidor esta iniciándose (cold-start), inténtelo de nuevo en 1 minuto'; @@ -155,6 +170,7 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) { id="name" placeholder="Nombre de usuario" onChange={handleChange} + disabled={loading} spellCheck={false} required /> @@ -170,6 +186,7 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) { id="photo" placeholder="Foto de perfil" onChange={handleChange} + disabled={loading} spellCheck={false} required /> @@ -187,6 +204,7 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) { id="password" placeholder="Contraseña" onChange={handleChange} + disabled={loading} spellCheck={false} required /> @@ -203,6 +221,7 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) { id="confirmpassword" placeholder="Confirma tu contraseña" onChange={handleChange} + disabled={loading} spellCheck={false} required /> @@ -215,9 +234,10 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) { backgroundColor: 'var(--app-logo-color)', }} className={`${styles.registerButton}`} + disabled={loading} onClick={handleRegister} > - Registrar + Registrar {loading && } @@ -231,6 +251,7 @@ export default function RegisterMenu({ setIsSigningUp }: PropsRegisterMenu) {

@@ -185,6 +221,7 @@ export default function StartMenu({