Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 79 additions & 0 deletions web/src/components/overview/StorageSection.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { screen } from "@testing-library/react";
import { plainRender } from "~/test-utils";
import { StorageSection } from "~/components/overview";
import * as ConfigModel from "~/api/storage/types/config-model";
import { Issue } from "~/types/issues";

const sdaDrive: ConfigModel.Drive = {
name: "/dev/sda",
Expand All @@ -43,18 +44,34 @@ const mockDevices = [
{ name: "/dev/sdb", size: 697932185600 },
];

const systemError: Issue = {
description: "System error",
kind: "storage",
details: "",
source: 1,
severity: 1,
};

const mockUseConfigModelFn = jest.fn();
const mockUseAvailableDevicesFn = jest.fn();
const mockUseSystemErrorsFn = jest.fn();

jest.mock("~/queries/storage", () => ({
...jest.requireActual("~/queries/storage"),
useDevices: () => mockDevices,
useAvailableDevices: () => mockUseAvailableDevicesFn(),
}));

jest.mock("~/queries/storage/config-model", () => ({
...jest.requireActual("~/queries/storage/config-model"),
useConfigModel: () => mockUseConfigModelFn(),
}));

jest.mock("~/queries/issues", () => ({
...jest.requireActual("~/queries/issues"),
useSystemErrors: () => mockUseSystemErrorsFn(),
}));

describe("when the configuration does not include any device", () => {
beforeEach(() => {
mockUseConfigModelFn.mockReturnValue({ drives: [] });
Expand Down Expand Up @@ -155,3 +172,65 @@ describe("when the configuration contains several drives", () => {
});
});
});

describe("when there is no configuration model (unsupported features)", () => {
beforeEach(() => {
mockUseConfigModelFn.mockReturnValue(undefined);
});

describe("if the storage proposal succeeded", () => {
beforeEach(() => {
mockUseSystemErrorsFn.mockReturnValue([]);
});

describe("and there are no available disks", () => {
beforeEach(() => {
mockUseAvailableDevicesFn.mockReturnValue([]);
});

it("indicates that an unhandled configuration was used", async () => {
plainRender(<StorageSection />);
await screen.findByText(/advanced configuration/);
});
});

describe("and there are available disks", () => {
beforeEach(() => {
mockUseAvailableDevicesFn.mockReturnValue(mockDevices);
});

it("indicates that an unhandled configuration was used", async () => {
plainRender(<StorageSection />);
await screen.findByText(/advanced configuration/);
});
});
});

describe("if the storage proposal was not possible", () => {
beforeEach(() => {
mockUseSystemErrorsFn.mockReturnValue([systemError]);
});

describe("and there are no available disks", () => {
beforeEach(() => {
mockUseAvailableDevicesFn.mockReturnValue([]);
});

it("indicates that there are no available disks", async () => {
plainRender(<StorageSection />);
await screen.findByText(/no disks available/);
});
});

describe("and there are available disks", () => {
beforeEach(() => {
mockUseAvailableDevicesFn.mockReturnValue(mockDevices);
});

it("indicates that an unhandled configuration was used", async () => {
plainRender(<StorageSection />);
await screen.findByText(/advanced configuration/);
});
});
});
});
39 changes: 27 additions & 12 deletions web/src/components/overview/StorageSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@
import React from "react";
import { Content } from "@patternfly/react-core";
import { deviceLabel } from "~/components/storage/utils";
import { useDevices } from "~/queries/storage";
import { useDevices, useAvailableDevices } from "~/queries/storage";
import { useConfigModel } from "~/queries/storage/config-model";
import { useSystemErrors } from "~/queries/issues";
import { StorageDevice } from "~/types/storage";
import * as ConfigModel from "~/api/storage/types/config-model";
import { _ } from "~/i18n";
Expand Down Expand Up @@ -78,26 +79,40 @@ const MultipleDisksSummary = ({ drives }: { drives: ConfigModel.Drive[] }): stri
return options[drives[0].spacePolicy];
};

const ModelSummary = ({ model }: { model: ConfigModel.Config }): React.ReactNode => {
const devices = useDevices("system", { suspense: true });
const drives = model?.drives || [];
const existDevice = (name: string) => devices.some((d) => d.name === name);
const noDrive = drives.length === 0 || drives.some((d) => !existDevice(d.name));

if (noDrive) return <NoDeviceSummary />;
if (drives.length > 1) return <MultipleDisksSummary drives={drives} />;
return <SingleDiskSummary drive={drives[0]} />;
};

const NoModelSummary = (): React.ReactNode => {
const availableDevices = useAvailableDevices();
const systemErrors = useSystemErrors("storage");
const hasDisks = !!availableDevices.length;
const hasResult = !systemErrors.length;

if (!hasResult && !hasDisks) return _("There are no disks available for the installation.");
return _("Install using an advanced configuration.");
};

/**
* Text explaining the storage proposal
*
* TODO: The current implementation assumes there are only drives and no other kind of devices like
* LVM volume groups or MD raids. Support for more cases (like LVM installation) will be added as
* the rest of the interface is also adapted.
*/
export default function StorageSection() {
const configModel = useConfigModel();
const devices = useDevices("system", { suspense: true });
const drives = configModel?.drives || [];
const existDevice = (name: string) => devices.some((d) => d.name === name);
const noDrive = drives.length === 0 || drives.some((d) => !existDevice(d.name));

return (
<Content>
<Content component="h3">{_("Storage")}</Content>
{noDrive && <NoDeviceSummary />}
{drives.length === 1 && <SingleDiskSummary drive={drives[0]} />}
{drives.length > 1 && <MultipleDisksSummary drives={drives} />}
<Content>
{configModel && <ModelSummary model={configModel} />}

@joseivanlopez joseivanlopez Feb 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: do we need to pass the model as prop? Just saying because after some discussions with @dgdavid, we decided that the components should recover all the info they need from hooks. But maybe in this case we want a component which is able to render different models?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. Somehow we realize that it's better having components able to query the information they need when possible instead of depending on the props of some parent. Not written in stone, of course, but thanks to the React Query cache most of times it works fine.

But maybe in this case we want a component which is able to render different models?

Good point.

{!configModel && <NoModelSummary />}
</Content>
</Content>
);
}