From 97ac475511e38885ddcbd0d433c3841d251f3446 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Fri, 21 Jun 2024 15:21:29 +0100 Subject: [PATCH 1/3] fix(web): do not render App on protected routes --- web/src/App.jsx | 34 +-------- web/src/App.test.jsx | 71 ++++--------------- web/src/Protected.jsx | 2 + .../overview/SoftwareSection.test.jsx | 2 +- web/src/context/installer.jsx | 33 ++++++++- web/src/context/installer.test.jsx | 16 ++++- web/src/context/installerL10n.test.jsx | 6 ++ web/src/test-utils.js | 12 ++++ 8 files changed, 83 insertions(+), 93 deletions(-) diff --git a/web/src/App.jsx b/web/src/App.jsx index 30316590f1..9cbad27b3e 100644 --- a/web/src/App.jsx +++ b/web/src/App.jsx @@ -19,13 +19,13 @@ * find current contact information at www.suse.com. */ -import React, { useEffect, useState } from "react"; +import React from "react"; import { Navigate, Outlet, useLocation } from "react-router-dom"; import { Loading } from "./components/layout"; import { Questions } from "~/components/questions"; import { ServerError, Installation } from "~/components/core"; import { useInstallerL10n } from "./context/installerL10n"; -import { useInstallerClient, useInstallerClientStatus } from "~/context/installer"; +import { useInstallerClientStatus } from "~/context/installer"; import { useProduct } from "./context/product"; import { CONFIG, INSTALL, STARTUP } from "~/client/phase"; import { BUSY } from "~/client/status"; @@ -38,38 +38,10 @@ import { BUSY } from "~/client/status"; * error (3 by default). The component will keep trying to connect. */ function App() { - const client = useInstallerClient(); const location = useLocation(); - const { connected, error } = useInstallerClientStatus(); + const { connected, error, phase, status } = useInstallerClientStatus(); const { selectedProduct, products } = useProduct(); const { language } = useInstallerL10n(); - const [status, setStatus] = useState(undefined); - const [phase, setPhase] = useState(undefined); - - useEffect(() => { - if (client) { - return client.manager.onPhaseChange(setPhase); - } - }, [client, setPhase]); - - useEffect(() => { - if (client) { - return client.manager.onStatusChange(setStatus); - } - }, [client, setStatus]); - - useEffect(() => { - const loadPhase = async () => { - const phase = await client.manager.getPhase(); - const status = await client.manager.getStatus(); - setPhase(phase); - setStatus(status); - }; - - if (client) { - loadPhase().catch(console.error); - } - }, [client, setPhase, setStatus]); const Content = () => { if (error) return ; diff --git a/web/src/App.test.jsx b/web/src/App.test.jsx index 760cdc2e5b..303891708f 100644 --- a/web/src/App.test.jsx +++ b/web/src/App.test.jsx @@ -44,14 +44,16 @@ jest.mock("~/context/product", () => ({ } })); +const mockClientStatus = { + connected: true, + error: false, + phase: STARTUP, + status: BUSY +}; + jest.mock("~/context/installer", () => ({ ...jest.requireActual("~/context/installer"), - useInstallerClientStatus: () => { - return { - connected: true, - error: false - }; - } + useInstallerClientStatus: () => mockClientStatus })); // Mock some components, @@ -61,32 +63,12 @@ jest.mock("~/components/core/Installation", () => () =>
Installation Mock () =>
Loading Mock
); jest.mock("~/components/product/ProductSelectionProgress", () => () =>
Product progress
); -// this object holds the mocked callbacks -const callbacks = {}; -const getStatusFn = jest.fn(); -const getPhaseFn = jest.fn(); - -// capture the latest subscription to the manager#onPhaseChange for triggering it manually -const onPhaseChangeFn = cb => { - callbacks.onPhaseChange = cb; -}; -const onStatusChangeFn = cb => { - callbacks.onStatusChange = cb; -}; -const changePhaseTo = phase => act(() => callbacks.onPhaseChange(phase)); - describe("App", () => { beforeEach(() => { // setting the language through a cookie document.cookie = "agamaLang=en-us; path=/;"; createClient.mockImplementation(() => { return { - manager: { - getStatus: getStatusFn, - getPhase: getPhaseFn, - onPhaseChange: onPhaseChangeFn, - onStatusChange: onStatusChangeFn - }, l10n: { locales: jest.fn().mockResolvedValue([["en_us", "English", "United States"]]), getLocales: jest.fn().mockResolvedValue(["en_us"]), @@ -126,22 +108,10 @@ describe("App", () => { }); }); - describe("on the startup phase", () => { - beforeEach(() => { - getPhaseFn.mockResolvedValue(STARTUP); - getStatusFn.mockResolvedValue(BUSY); - }); - - it("renders the Loading screen", async () => { - installerRender(, { withL10n: true }); - await screen.findByText("Loading Mock"); - }); - }); - describe("when the service is busy during startup", () => { beforeEach(() => { - getPhaseFn.mockResolvedValue(STARTUP); - getStatusFn.mockResolvedValue(BUSY); + mockClientStatus.phase = STARTUP; + mockClientStatus.status = BUSY; }); it("renders the Loading screen", async () => { @@ -152,12 +122,12 @@ describe("App", () => { describe("on the CONFIG phase", () => { beforeEach(() => { - getPhaseFn.mockResolvedValue(CONFIG); + mockClientStatus.phase = CONFIG; }); describe("if the service is busy", () => { beforeEach(() => { - getStatusFn.mockResolvedValue(BUSY); + mockClientStatus.status = BUSY; }); it("redirects to product selection progress", async () => { @@ -168,7 +138,7 @@ describe("App", () => { describe("if the service is not busy", () => { beforeEach(() => { - getStatusFn.mockResolvedValue(IDLE); + mockClientStatus.status = IDLE; }); it("renders the application content", async () => { @@ -180,7 +150,7 @@ describe("App", () => { describe("on the INSTALL phase", () => { beforeEach(() => { - getPhaseFn.mockResolvedValue(INSTALL); + mockClientStatus.phase = INSTALL; mockSelectedProduct = { id: "Fake product" }; }); @@ -189,17 +159,4 @@ describe("App", () => { await screen.findByText("Installation Mock"); }); }); - - describe("when service phase changes", () => { - beforeEach(() => { - getPhaseFn.mockResolvedValue(CONFIG); - }); - - it("renders the Installation component on the INSTALL phase", async () => { - installerRender(, { withL10n: true }); - await screen.findByText(/Outlet Content/); - changePhaseTo(INSTALL); - await screen.findByText("Installation Mock"); - }); - }); }); diff --git a/web/src/Protected.jsx b/web/src/Protected.jsx index 2f83011247..508005cfd6 100644 --- a/web/src/Protected.jsx +++ b/web/src/Protected.jsx @@ -27,6 +27,8 @@ import { AppProviders } from "./context/app"; export default function Protected() { const { isLoggedIn } = useAuth(); + if (isLoggedIn === undefined) return; + if (isLoggedIn === false) { return ; } diff --git a/web/src/components/overview/SoftwareSection.test.jsx b/web/src/components/overview/SoftwareSection.test.jsx index f5edbf84c0..7c32f146ea 100644 --- a/web/src/components/overview/SoftwareSection.test.jsx +++ b/web/src/components/overview/SoftwareSection.test.jsx @@ -56,7 +56,7 @@ beforeEach(() => { }); }); -it("renders the required space and the selected patterns", async () => { +it.only("renders the required space and the selected patterns", async () => { installerRender(); await screen.findByText("500 MiB"); await screen.findByText(kdePattern.summary); diff --git a/web/src/context/installer.jsx b/web/src/context/installer.jsx index dccf73d0f5..fd4f063a3b 100644 --- a/web/src/context/installer.jsx +++ b/web/src/context/installer.jsx @@ -27,7 +27,9 @@ import { createDefaultClient } from "~/client"; const InstallerClientContext = React.createContext(null); // TODO: we use a separate context to avoid changing all the codes to // `useInstallerClient`. We should merge them in the future. -const InstallerClientStatusContext = React.createContext({ connected: false, error: false }); +const InstallerClientStatusContext = React.createContext({ + connected: false, error: false, phase: undefined, status: undefined +}); /** * Returns the D-Bus installer client @@ -77,6 +79,8 @@ function InstallerClientProvider({ const [value, setValue] = useState(client); const [connected, setConnected] = useState(false); const [error, setError] = useState(false); + const [status, setStatus] = useState(undefined); + const [phase, setPhase] = useState(undefined); useEffect(() => { const connectClient = async () => { @@ -99,6 +103,31 @@ function InstallerClientProvider({ if (!value) connectClient(); }, [setValue, value]); + useEffect(() => { + if (value) { + return value.manager.onPhaseChange(setPhase); + } + }, [value, setPhase]); + + useEffect(() => { + if (value) { + return value.manager.onStatusChange(setStatus); + } + }, [value, setStatus]); + + useEffect(() => { + const loadPhase = async () => { + const initialPhase = await value.manager.getPhase(); + const initialStatus = await value.manager.getStatus(); + setPhase(initialPhase); + setStatus(initialStatus); + }; + + if (value) { + loadPhase().catch(console.error); + } + }, [value, setPhase, setStatus]); + useEffect(() => { if (!value) return; @@ -115,7 +144,7 @@ function InstallerClientProvider({ return ( - + {children} diff --git a/web/src/context/installer.test.jsx b/web/src/context/installer.test.jsx index 25354eaf5e..ef84f406c6 100644 --- a/web/src/context/installer.test.jsx +++ b/web/src/context/installer.test.jsx @@ -24,16 +24,20 @@ import { act, screen } from "@testing-library/react"; import { createDefaultClient } from "~/client"; import { plainRender, createCallbackMock } from "~/test-utils"; import { InstallerClientProvider, useInstallerClientStatus } from "./installer"; +import { STARTUP } from "~/client/phase"; +import { BUSY } from "~/client/status"; jest.mock("~/client"); // Helper component to check the client status. const ClientStatus = () => { - const { connected } = useInstallerClientStatus(); + const { connected, phase, status } = useInstallerClientStatus(); return (
  • {`connected: ${connected}`}
    • +
  • {`phase: ${phase}`}
    • +
  • {`status: ${status}`}
); }; @@ -43,7 +47,13 @@ describe("installer context", () => { createDefaultClient.mockImplementation(() => { return { onConnect: jest.fn(), - onDisconnect: jest.fn() + onDisconnect: jest.fn(), + manager: { + getPhase: jest.fn().mockResolvedValue(STARTUP), + getStatus: jest.fn().mockResolvedValue(BUSY), + onPhaseChange: jest.fn(), + onStatusChange: jest.fn() + } }; }); }); @@ -55,5 +65,7 @@ describe("installer context", () => { ); await screen.findByText("connected: false"); + await screen.findByText("phase: 0"); + await screen.findByText("status: 1"); }); }); diff --git a/web/src/context/installerL10n.test.jsx b/web/src/context/installerL10n.test.jsx index 50af75fa4b..1808b398a6 100644 --- a/web/src/context/installerL10n.test.jsx +++ b/web/src/context/installerL10n.test.jsx @@ -35,6 +35,12 @@ const setUILocaleFn = jest.fn().mockResolvedValue(); const client = { onConnect: jest.fn(), onDisconnect: jest.fn(), + manager: { + getPhase: jest.fn(), + getStatus: jest.fn(), + onPhaseChange: jest.fn(), + onStatusChange: jest.fn() + }, l10n: { getUILocale: getUILocaleFn, setUILocale: setUILocaleFn, diff --git a/web/src/test-utils.js b/web/src/test-utils.js index 77b00d9a67..7f64aa3602 100644 --- a/web/src/test-utils.js +++ b/web/src/test-utils.js @@ -84,6 +84,18 @@ const Providers = ({ children, withL10n }) => { client.onDisconnect = noop; } + if (!client.manager) { + client.manager = {}; + } + + client.manager = { + getPhase: noop, + getStatus: noop, + onPhaseChange: noop, + onStatusChange: noop, + ...client.manager + }; + if (withL10n) { return ( From 94fa7badd63a260a2abd84bc6241e74ffcbc00c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Fri, 21 Jun 2024 15:37:29 +0100 Subject: [PATCH 2/3] doc(web): update the changes file --- web/package/agama-web-ui.changes | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/web/package/agama-web-ui.changes b/web/package/agama-web-ui.changes index a862bfa63a..c581ffde82 100644 --- a/web/package/agama-web-ui.changes +++ b/web/package/agama-web-ui.changes @@ -1,3 +1,9 @@ +------------------------------------------------------------------- +Fri Jun 21 14:32:11 UTC 2024 - Imobach Gonzalez Sosa + +- Avoid connection attempts when the user is not logged in + (gh#openSUSE/agama#1366). + ------------------------------------------------------------------- Thu Jun 20 05:33:29 UTC 2024 - Imobach Gonzalez Sosa From 1a9f3746729c5587d0e8fc4d6f1c51f87c894d7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Imobach=20Gonz=C3=A1lez=20Sosa?= Date: Fri, 21 Jun 2024 20:36:09 +0100 Subject: [PATCH 3/3] update from code review --- web/src/Protected.jsx | 1 + 1 file changed, 1 insertion(+) diff --git a/web/src/Protected.jsx b/web/src/Protected.jsx index 508005cfd6..821dfec13d 100644 --- a/web/src/Protected.jsx +++ b/web/src/Protected.jsx @@ -27,6 +27,7 @@ import { AppProviders } from "./context/app"; export default function Protected() { const { isLoggedIn } = useAuth(); + // It is not known yet whether the user is logged or not. if (isLoggedIn === undefined) return; if (isLoggedIn === false) {