From 6b11fff033cc16ed4d2f11caf74e8c18307abc3c Mon Sep 17 00:00:00 2001 From: Hendrik de Graaf Date: Fri, 5 Jul 2024 12:06:35 +0200 Subject: [PATCH] fix(alerts): ensure hiding works correctly and alerts are not re-added [DHIS2-15438] (#859) * chore: add bug reproduction test case * fix: ensure hiding works correctly and alerts are not re-added * chore: fix typos * chore: import useAlerts from app-runtime --------- Co-authored-by: Kai Vandivier <49666798+KaiVandivier@users.noreply.github.com> --- adapter/src/components/Alerts.js | 140 +++++----- .../src/components/__tests__/Alerts.test.js | 262 +++++++++--------- 2 files changed, 198 insertions(+), 204 deletions(-) diff --git a/adapter/src/components/Alerts.js b/adapter/src/components/Alerts.js index 5679c4bc5..7221293fe 100644 --- a/adapter/src/components/Alerts.js +++ b/adapter/src/components/Alerts.js @@ -1,53 +1,89 @@ import { useAlerts } from '@dhis2/app-runtime' -import { AlertBar, AlertStack } from '@dhis2/ui' -import React, { useState, useEffect } from 'react' +import { AlertStack, AlertBar } from '@dhis2/ui' +import React, { useCallback, useState } from 'react' -/* - * The alert-manager which populates the `useAlerts` hook from `@dhis2/app-service-alerts` - * hook with alerts only supports simply adding and removing alerts. However, the - * `AlertBar` from `@dhis2/ui` should leave the screen with a hide-animation, so this - * requires an additional state. The `alertStackAlerts` state in the Alerts component - * provides this addional state: - * - It contains all alerts from the alert-manager, with `options.hidden` set to `false` - * - And also alerts which have been removed from the alert-manager, but still have their - * leave animation in progress, whtih `options.hidden` set to `true`) - * Alerts are removed from the `alertStackAlerts` state once the `onHidden` callback fires - */ +/* The alerts-manager which populates the `useAlerts` hook from + * `@dhis2/app-service-alerts` hook with alerts only supports + * simply adding and removing alerts. However, the `AlertBar` + * from `@dhis2/ui` should leave the screen with a hide-animation. + * This works well, for alerts that hide "naturally" (after the + * timeout expires or when the close icon is clicked). In these + * cases the component will request to be removed from the alerts- + * manager after the animation completes. However, when + * programatically hiding an alert this is the other way around: + * the alert is removed from the alerts-manager straight away and + * if we were to render the alerts from the `useAlerts` hook, these + * alerts would be removed from the DOM abruptly without an animation. + * To prevent this from happening, we have implemented the + * `useAlertsWithHideCache` hook: + * - It contains all alerts from the alert-manager, with + * `options.hidden` set to `false` + * - And also alerts which have been removed from the alert-manager, + * but still have their leave animation in progress, with + * `options.hidden` set to `true` + * - Alerts are removed once the `onHidden` callback fires */ -const Alerts = () => { +const useAlertsWithHideCache = () => { + const [alertsMap] = useState(new Map()) + /* We don't use this state value, it is used to trigger + * a rerender to remove the hidden alert from the DOM */ + const [, setLastRemovedId] = useState(null) const alertManagerAlerts = useAlerts() - const [alertStackAlerts, setAlertStackAlerts] = useState(alertManagerAlerts) - const removeAlertStackAlert = (id) => - setAlertStackAlerts( - alertStackAlerts.filter( - (alertStackAlert) => alertStackAlert.id !== id - ) - ) + const updateAlertsFromManager = useCallback( + (newAlerts = []) => { + const newAlertsIdLookup = new Set() + newAlerts.forEach((alert) => { + newAlertsIdLookup.add(alert.id) + if (!alertsMap.has(alert.id)) { + // new alerts, these are not hiding + alertsMap.set(alert.id, { + ...alert, + options: { + ...alert.options, + hidden: alert.options.hidden || false, + }, + }) + } + }) + // alerts in alertsMap but not in newAlerts are hiding + alertsMap.forEach((alert) => { + if (!newAlertsIdLookup.has(alert.id)) { + alert.options.hidden = true + } + }) + }, + [alertsMap] + ) + const removeAlert = useCallback( + (id) => { + alertsMap.delete(id) + setLastRemovedId(id) + }, + [alertsMap] + ) - useEffect(() => { - if (alertManagerAlerts.length > 0) { - setAlertStackAlerts((currentAlertStackAlerts) => - mergeAlertStackAlerts( - currentAlertStackAlerts, - alertManagerAlerts - ) - ) - } - }, [alertManagerAlerts]) + updateAlertsFromManager(alertManagerAlerts) + + return { + alerts: Array.from(alertsMap.values()).sort((a, b) => a.id - b.id), + removeAlert, + } +} + +const Alerts = () => { + const { alerts, removeAlert } = useAlertsWithHideCache() return ( - {alertStackAlerts.map( + {alerts.map( ({ message, remove, id, options: { onHidden, ...props } }) => ( { onHidden && onHidden() - removeAlertStackAlert(id) - if (alertManagerAlerts.some((a) => a.id === id)) { - remove() - } + removeAlert(id) + remove() }} > {message} @@ -58,34 +94,4 @@ const Alerts = () => { ) } -function mergeAlertStackAlerts(alertStackAlerts, alertManagerAlerts) { - return Object.values({ - /* - * Assume that all alerts in the alertStackAlerts array are hiding. - * After the object merge only the alerts not in the alertManagerAlerts - * array will have `options.hidden === true`. - */ - ...toIdBasedObjectWithHiddenOption(alertStackAlerts, true), - /* - * All alertManagerAlerts should be showing. This object merge will - * overwrite any alertStackAlert by the alertManagerAlert with - * the same `id`, thus ensuring the alert is visible. - */ - ...toIdBasedObjectWithHiddenOption(alertManagerAlerts, false), - }) -} - -function toIdBasedObjectWithHiddenOption(arr, hidden) { - return arr.reduce((obj, item) => { - obj[item.id] = { - ...item, - options: { - ...item.options, - hidden, - }, - } - return obj - }, {}) -} - -export { Alerts, mergeAlertStackAlerts } +export { Alerts } diff --git a/adapter/src/components/__tests__/Alerts.test.js b/adapter/src/components/__tests__/Alerts.test.js index 845dfc0b2..3a89ee958 100644 --- a/adapter/src/components/__tests__/Alerts.test.js +++ b/adapter/src/components/__tests__/Alerts.test.js @@ -2,7 +2,7 @@ import { useAlert } from '@dhis2/app-runtime' import { AlertsProvider } from '@dhis2/app-service-alerts' import { act, render, fireEvent, waitFor, screen } from '@testing-library/react' import React from 'react' -import { Alerts, mergeAlertStackAlerts } from '../Alerts.js' +import { Alerts } from '../Alerts.js' describe('Alerts', () => { beforeEach(() => { @@ -16,16 +16,16 @@ describe('Alerts', () => { ) - const AlertButtons = ({ message, options }) => { + const AlertButtons = ({ message, options, label }) => { const { show, hide } = useAlert(message, options) return ( <> ) @@ -102,140 +102,128 @@ describe('Alerts', () => { // But eventually it is gone expect(screen.queryByText(msg)).toBeNull() }) -}) + it('removes multiple alerts that hide simultaniously correctly', async () => { + /* This test case was added to reproduce and fix a bug that + * would cause the second alert to be re-added during the + * removal of the first alert. */ + const duration = 1000 + const options = { duration } + const message1 = 'message 1' + const message2 = 'message 2' -describe('mergeAlertStackAlerts', () => { - it('add alerts from the alert manager and adds `hidden: false` to the options', () => { - const alertStackAlerts = [] - const alertManagerAlerts = [ - { - id: 1, - message: 'test1', - options: { permanent: true }, - }, - { - id: 2, - message: 'test2', - options: { permanent: true }, - }, - ] - expect( - mergeAlertStackAlerts(alertStackAlerts, alertManagerAlerts) - ).toEqual([ - { - id: 1, - message: 'test1', - options: { hidden: false, permanent: true }, - }, - { - id: 2, - message: 'test2', - options: { hidden: false, permanent: true }, - }, - ]) - }) - it('keeps alerts unchanged if the alert-manager and alert-stack contain equivalent items', () => { - const alertStackAlerts = [ - { - id: 1, - message: 'test1', - options: { permanent: true, hidden: false }, - }, - { - id: 2, - message: 'test2', - options: { permanent: true, hidden: false }, - }, - ] - const alertManagerAlerts = [ - { - id: 1, - message: 'test1', - options: { permanent: true }, - }, - { - id: 2, - message: 'test2', - options: { permanent: true }, - }, - ] - expect( - mergeAlertStackAlerts(alertStackAlerts, alertManagerAlerts) - ).toEqual(alertStackAlerts) - }) - it('keeps alerts in the alert-stack and sets `hidden` to `true` if they are no longer in the alert-manager', () => { - const alertStackAlerts = [ - { - id: 1, - message: 'test1', - options: { permanent: true, hidden: false }, - }, - { - id: 2, - message: 'test2', - options: { permanent: true, hidden: false }, - }, - ] - const alertManagerAlerts = [ - { - id: 2, - message: 'test2', - options: { permanent: true }, - }, - ] - expect( - mergeAlertStackAlerts(alertStackAlerts, alertManagerAlerts) - ).toEqual([ - { - id: 1, - message: 'test1', - options: { permanent: true, hidden: true }, - }, - { - id: 2, - message: 'test2', - options: { permanent: true, hidden: false }, - }, - ]) + render( + + + + + ) + + act(() => { + fireEvent.click(screen.getByText(`Show ${message1}`)) + /* A small delay between hides is required to reproduce the bug, + * because if the hiding is done at the same time, the alerts + * would both be removed correctly */ + jest.advanceTimersByTime(10) + fireEvent.click(screen.getByText(`Show ${message2}`)) + }) + + // Both message should show + await waitFor(() => screen.getByText(message1)) + await waitFor(() => screen.getByText(message2)) + expect(screen.getAllByText(message1)).toHaveLength(1) + expect(screen.getAllByText(message2)).toHaveLength(1) + + act(() => { + jest.advanceTimersByTime(duration) + }) + + // Both should still be there while the hide animation runs + expect(screen.getAllByText(message1)).toHaveLength(1) + expect(screen.getAllByText(message2)).toHaveLength(1) + + act(() => { + // Now we advance the time until the hide animation completes + jest.advanceTimersByTime(700) + }) + + /* Now both should be gone. Prior to the bugfix, + * the alert with message2 would be showing */ + expect(screen.queryByText(message1)).toBeNull() + expect(screen.queryByText(message2)).toBeNull() }) - it('updates alerts in the alert-stack with the properties of the alerts in the alert-manager', () => { - const alertStackAlerts = [ - { - id: 1, - message: 'test1', - options: { permanent: true, hidden: false }, - }, - { - id: 2, - message: 'test2', - options: { permanent: true, hidden: false }, - }, - ] - const alertManagerAlerts = [ - { - id: 1, - message: 'test1 EDITED', - options: { success: true }, - }, - { - id: 2, - message: 'test2 EDITED', - options: { success: true }, - }, - ] - expect( - mergeAlertStackAlerts(alertStackAlerts, alertManagerAlerts) - ).toEqual([ - { - id: 1, - message: 'test1 EDITED', - options: { success: true, hidden: false }, - }, - { - id: 2, - message: 'test2 EDITED', - options: { success: true, hidden: false }, - }, - ]) + it('keeps alerts that have been removed programatically around until the animation is done', async () => { + const options = { permanent: true } + const message1 = 'message 1' + const message2 = 'message 2' + + render( + + + + + ) + + act(() => { + fireEvent.click(screen.getByText(`Show ${message1}`)) + fireEvent.click(screen.getByText(`Show ${message2}`)) + }) + + // Both message should show + await waitFor(() => screen.getByText(message1)) + await waitFor(() => screen.getByText(message2)) + expect(screen.getAllByText(message1)).toHaveLength(1) + expect(screen.getAllByText(message2)).toHaveLength(1) + + act(() => { + fireEvent.click(screen.getByText(`Hide ${message1}`)) + jest.advanceTimersByTime(50) + }) + + // Both should still be there while the hide animation runs + expect(screen.getAllByText(message1)).toHaveLength(1) + expect(screen.getAllByText(message2)).toHaveLength(1) + + act(() => { + // Now we advance the time until the hide animation completes + jest.advanceTimersByTime(1000) + }) + + // The alert that was hidden should now be gone + expect(screen.queryByText(message1)).toBeNull() + expect(screen.getAllByText(message2)).toHaveLength(1) + + act(() => { + fireEvent.click(screen.getByText(`Hide ${message2}`)) + jest.advanceTimersByTime(50) + }) + + // The second alert should still be there while its hide animation runs + expect(screen.queryByText(message1)).toBeNull() + expect(screen.getAllByText(message2)).toHaveLength(1) + + act(() => { + fireEvent.click(screen.getByText(`Hide ${message2}`)) + jest.advanceTimersByTime(700) + }) + + // Now both should be gone + expect(screen.queryByText(message1)).toBeNull() + expect(screen.queryByText(message2)).toBeNull() }) })