Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage proposal UI #321

Merged
merged 42 commits into from
Dec 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
4e09c7b
[web] Add storage proposal page
joseivanlopez Nov 25, 2022
841db4e
[service] Do not encrypt with empty password
joseivanlopez Nov 25, 2022
c93d8e0
[web] Use Section component
joseivanlopez Nov 25, 2022
320be64
[web] Fix section component
joseivanlopez Nov 25, 2022
3df4c30
[web] Add overview namespace
joseivanlopez Nov 25, 2022
8b7adb6
[web] Fix path in storage client
joseivanlopez Nov 29, 2022
2d1beae
[web] Improve storage section
joseivanlopez Nov 29, 2022
1725b46
[web] Show errors in actions section
joseivanlopez Nov 29, 2022
43788a9
[web] Wording and presentation improvements
joseivanlopez Nov 29, 2022
fda586a
[web] Remove proposal volumes table
joseivanlopez Nov 29, 2022
9a60009
[web] Improvements for storage page
joseivanlopez Nov 29, 2022
39cfff1
[web] Add proposal summary component
joseivanlopez Nov 30, 2022
0907234
[web] Add link for editing storage settings
joseivanlopez Nov 30, 2022
1be8e7d
[web] Fixes for events in storage section
joseivanlopez Nov 30, 2022
a1cc635
[web] Improve wording
joseivanlopez Nov 30, 2022
2b706da
[service] Modify error messages
joseivanlopez Nov 30, 2022
3bd9fb9
[web] Add unit tests for StorageSection
joseivanlopez Nov 30, 2022
75969b2
[web] Add unit test for DeviceSelector
dgdavid Dec 1, 2022
def0e12
[web] Fix typo
dgdavid Dec 1, 2022
d61e25b
[web] Add unit test for ProposalActions
dgdavid Dec 1, 2022
9564309
[web] Add simple test for ProposalActionsSection
dgdavid Dec 1, 2022
88085f4
[web] Small ProposalSettingsForm adjustments
dgdavid Dec 1, 2022
9376ae0
[web] Add test for ProposalSettingsForm
dgdavid Dec 1, 2022
ebb6eaf
[web] Add test for ProposalTargetForm
dgdavid Dec 1, 2022
a6f8055
[web] Remove not needed calls to "act"
dgdavid Dec 1, 2022
8cb670d
[web] Improvements for ProposalPage
joseivanlopez Dec 1, 2022
d571301
[web] Tests for ProposalPage
joseivanlopez Dec 1, 2022
c030ac5
[web] Rename fake component
joseivanlopez Dec 2, 2022
744915f
[web] Close popup when accepting
joseivanlopez Dec 2, 2022
494e2bd
[web] Add tests for ProposalTargetSection
joseivanlopez Dec 2, 2022
af7a602
[web] Improve doc of storage client
joseivanlopez Dec 2, 2022
e3664d2
[web] Rename callback
joseivanlopez Dec 2, 2022
c282e8e
[web] Fix storage client types
joseivanlopez Dec 2, 2022
a3959e8
[web] Remove old tests
joseivanlopez Dec 2, 2022
427a9e6
[web] Fix overview tests
joseivanlopez Dec 2, 2022
5096232
[web] Fix storage client tests
joseivanlopez Dec 2, 2022
13da855
[web] Small improvements
joseivanlopez Dec 2, 2022
ca8ef9f
[web] Add encryption password confirmation
joseivanlopez Dec 2, 2022
35416c4
[web] Add tests for ProposalSettingsSection
joseivanlopez Dec 2, 2022
7509837
[web] Use initial value for confirmation password
joseivanlopez Dec 2, 2022
9344647
[web] Update changelog
joseivanlopez Dec 2, 2022
88334f1
[service] Update changelog
joseivanlopez Dec 2, 2022
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
6 changes: 2 additions & 4 deletions service/lib/dinstaller/dbus/storage/proposal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,7 @@ def lvm
#
# @return [String]
def encryption_password
return "" unless backend.calculated_settings

backend.calculated_settings.encryption_password
backend.calculated_settings&.encryption_password || ""
end

# Volumes used as template for creating a new volume
Expand Down Expand Up @@ -172,7 +170,7 @@ def register_callbacks
SETTINGS_CONVERSIONS = {
"CandidateDevices" => ["candidate_devices=", proc { |v| v }],
"LVM" => ["lvm=", proc { |v| v }],
"EncryptionPassword" => ["encryption_password=", proc { |v| v }],
"EncryptionPassword" => ["encryption_password=", proc { |v| v.empty? ? nil : v }],
"Volumes" => ["volumes=", proc { |v, o| o.send(:to_proposal_volumes, v) }]
}.freeze
private_constant :SETTINGS_CONVERSIONS
Expand Down
8 changes: 2 additions & 6 deletions service/lib/dinstaller/storage/proposal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -255,17 +255,13 @@ def storage_manager
def validate_proposal
return if candidate_devices.empty? || !proposal.failed?

message = format(
"Could not create a storage proposal using %{devices}",
devices: candidate_devices.join(", ")
)
ValidationError.new(message)
ValidationError.new("Cannot accommodate the required file systems for installation")
end

def validate_available_devices
return if available_devices.any?

ValidationError.new("Could not find a suitable device for installation")
ValidationError.new("There is no suitable device for installation")
end

def validate_candidate_devices
Expand Down
7 changes: 7 additions & 0 deletions service/package/rubygem-d-installer.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
-------------------------------------------------------------------
Fri Dec 2 14:52:36 UTC 2022 - José Iván López González <[email protected]>

- Improve messages of storage validation errors.
- Do not encrypt devices when receiving an empty password from
D-Bus (gh#yast/d-installer#321).

-------------------------------------------------------------------
Thu Dec 1 16:22:58 UTC 2022 - Josef Reidinger <[email protected]>

Expand Down
4 changes: 2 additions & 2 deletions service/test/dinstaller/storage/proposal_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@
it "returns an error" do
errors = subject.validate
expect(errors.size).to eq(1)
expect(errors.first.message).to include("not find a suitable device")
expect(errors.first.message).to include("There is no suitable device")
end
end

Expand All @@ -381,7 +381,7 @@
it "returns an error" do
errors = subject.validate
expect(errors.size).to eq(1)
expect(errors.first.message).to include("not create a storage proposal using /dev/sda")
expect(errors.first.message).to include("Cannot accommodate")
end
end

Expand Down
6 changes: 6 additions & 0 deletions web/package/cockpit-d-installer.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Fri Dec 2 13:41:41 UTC 2022 - José Iván López González <[email protected]>

- Add new UI for storage proposal offering LVM and encyption
options (gh#yast/d-installer#321).

-------------------------------------------------------------------
Fri Nov 18 16:27:26 UTC 2022 - Imobach Gonzalez Sosa <[email protected]>

Expand Down
6 changes: 6 additions & 0 deletions web/src/app.scss
Original file line number Diff line number Diff line change
Expand Up @@ -306,3 +306,9 @@ button.hidden-popover-button {
}
}
}

.volumes-list {
.pf-c-label {
margin-inline-end: 5px;
}
}
116 changes: 82 additions & 34 deletions web/src/client/storage.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,69 +44,117 @@ class StorageBaseClient {
}

/**
* Returns the actions for the current proposal
* Returns storage proposal values
*
* @return {Promise<object[]>}
* @return {Promise<object>}
*/
async getStorageActions() {
async getProposal() {
const proxy = await this.client.proxy(STORAGE_PROPOSAL_IFACE);
return proxy.Actions.map(action => {
const { Text: { v: textVar }, Subvol: { v: subvolVar }, Delete: { v: deleteVar } } = action;

const volume = dbusVolume => {
const valueFrom = dbusValue => dbusValue?.v;

const valuesFrom = (dbusValues) => {
if (dbusValues === undefined) return [];
return dbusValues.v.map(valueFrom);
};

return {
mountPoint: valueFrom(dbusVolume.MountPoint),
optional: valueFrom(dbusVolume.Optional),
deviceType: valueFrom(dbusVolume.DeviceType),
encrypted: valueFrom(dbusVolume.Encrypted),
fsTypes: valuesFrom(dbusVolume.FsTypes),
fsType: valueFrom(dbusVolume.FsType),
minSize: valueFrom(dbusVolume.MinSize),
maxSize: valueFrom(dbusVolume.MaxSize),
fixedSizeLimits: valueFrom(dbusVolume.FixedSizeLimits),
adaptiveSizes: valueFrom(dbusVolume.AdaptiveSizes),
snapshots: valueFrom(dbusVolume.Snapshots),
snapshotsConfigurable: valueFrom(dbusVolume.SnapshotsConfigurable),
snapshotsAffectSizes: valueFrom(dbusVolume.SnapshotsAffectSizes),
sizeRelevantVolumes: valueFrom(dbusVolume.SizeRelevantVolumes)
};
};

const action = dbusAction => {
const { Text: { v: textVar }, Subvol: { v: subvolVar }, Delete: { v: deleteVar } } = dbusAction;
return { text: textVar, subvol: subvolVar, delete: deleteVar };
});
};

return {
availableDevices: proxy.AvailableDevices.map(([id, label]) => ({ id, label })),
candidateDevices: proxy.CandidateDevices,
lvm: proxy.LVM,
encryptionPassword: proxy.EncryptionPassword,
volumes: proxy.Volumes.map(volume),
actions: proxy.Actions.map(action)
};
}

/**
* Returns storage proposal settings
* Calculates a new proposal
*
* @return {Promise<object>}
* @todo Do not send undefined values
*
* @param {object} settings - proposal settings
* @param {?string[]} [settings.candidateDevices] - Devices to use for the proposal
* @param {?string} [settings.encryptionPassword] - Password for encrypting devices
* @param {?boolean} [settings.lvm] - Whether to calculate the proposal with LVM volumes
* @param {?object[]} [settings.volumes] - Volumes to create
* @return {Promise<number>} - 0 success, other for failure
*/
async getStorageProposal() {
async calculateProposal({ candidateDevices, encryptionPassword, lvm, volumes }) {
const proxy = await this.client.proxy(STORAGE_PROPOSAL_IFACE);
return {
availableDevices: proxy.AvailableDevices.map(([id, label]) => {
return { id, label };
}),
candidateDevices: proxy.CandidateDevices,
lvm: proxy.LVM

const dbusVolume = volume => {
return {
MountPoint: cockpit.variant("s", volume.mountPoint),
Encrypted: cockpit.variant("b", volume.encrypted),
FsType: cockpit.variant("s", volume.fsType),
MinSize: cockpit.variant("x", volume.minSize),
MaxSize: cockpit.variant("x", volume.maxSize),
FixedSizeLimits: cockpit.variant("b", volume.fixedSizeLimits),
Snapshots: cockpit.variant("b", volume.snapshots)
};
};
}

async calculateStorageProposal({ candidateDevices }) {
const proxy = await this.client.proxy(STORAGE_PROPOSAL_IFACE);
return proxy.Calculate({
CandidateDevices: cockpit.variant("as", candidateDevices)
CandidateDevices: cockpit.variant("as", candidateDevices),
EncryptionPassword: cockpit.variant("s", encryptionPassword),
LVM: cockpit.variant("b", lvm),
Volumes: cockpit.variant("aa{sv}", volumes.map(dbusVolume))
});
}

/**
* Registers a callback to run when properties in the Actions object change
* Registers a callback to run when properties in the Storage Proposal object change
*
* @param {function} handler - callback function
*/
onActionsChange(handler) {
onProposalChange(handler) {
return this.client.onObjectChanged(STORAGE_PROPOSAL_PATH, STORAGE_PROPOSAL_IFACE, changes => {
const { Actions: actions } = changes;
if (actions !== undefined && Array.isArray(actions.v)) {
const newActions = actions.v.map(action => {
const { Text: textVar, Subvol: subvolVar, Delete: deleteVar } = action;
return { text: textVar.v, subvol: subvolVar.v, delete: deleteVar.v };
});
handler(newActions);
if (Array.isArray(changes.CandidateDevices.v)) {
// FIXME return the proposal object (see getProposal)
handler({ candidateDevices: changes.CandidateDevices.v });
}
});
}

/**
* Registers a callback to run when properties in the Storage Proposal object change
* Registers a callback to run when properties in the Actions object change
*
* @param {function} handler - callback function
*/
onStorageProposalChange(handler) {
onActionsChange(handler) {
return this.client.onObjectChanged(STORAGE_PROPOSAL_PATH, STORAGE_PROPOSAL_IFACE, changes => {
if (Array.isArray(changes.CandidateDevices.v)) {
const [selected] = changes.CandidateDevices.v;
handler(selected);
const { Actions: actions } = changes;
if (actions !== undefined && Array.isArray(actions.v)) {
const newActions = actions.v.map(action => {
const { Text: textVar, Subvol: subvolVar, Delete: deleteVar } = action;
return { text: textVar.v, subvol: subvolVar.v, delete: deleteVar.v };
});
handler(newActions);
}
});
}
Expand All @@ -116,7 +164,7 @@ class StorageBaseClient {
* Allows interacting with the storage settings
*/
class StorageClient extends WithValidation(
WithStatus(StorageBaseClient, STORAGE_PROPOSAL_PATH), STORAGE_PATH
WithStatus(StorageBaseClient, STORAGE_PATH), STORAGE_PATH
) {}

export { StorageClient };
36 changes: 18 additions & 18 deletions web/src/client/storage.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ const storageProposalProxy = {
],
CandidateDevices: ["/dev/sda"],
LVM: true,
Volumes: [
{ MountPoint: { t: "s", v: "/test1" } },
{ MountPoint: { t: "s", v: "/test2" } }
],
Actions: [
{
Text: { t: "s", v: "Mount /dev/sdb1 as root" },
Expand All @@ -49,25 +53,21 @@ beforeEach(() => {
});
});

describe("#getStorageProposal", () => {
it("returns the storage proposal settings", async () => {
describe("#getProposal", () => {
it("returns the storage proposal settings and actions", async () => {
const client = new StorageClient(dbusClient);
const proposal = await client.getStorageProposal();
expect(proposal).toEqual({
availableDevices: [
{ id: "/dev/sda", label: "/dev/sda, 950 GiB, Windows" },
{ id: "/dev/sdb", label: "/dev/sdb, 500 GiB" }
],
candidateDevices: ["/dev/sda"],
lvm: true
});
});
});
const proposal = await client.getProposal();
expect(proposal.availableDevices).toEqual([
{ id: "/dev/sda", label: "/dev/sda, 950 GiB, Windows" },
{ id: "/dev/sdb", label: "/dev/sdb, 500 GiB" }
]);
expect(proposal.candidateDevices).toEqual(["/dev/sda"]);
expect(proposal.lvm).toBeTruthy();
expect(proposal.actions).toEqual([
{ text: "Mount /dev/sdb1 as root", subvol: false, delete: false }
]);

describe("#getStorageActions", () => {
it("returns the storage actions", async () => {
const client = new StorageClient(dbusClient);
const actions = await client.getStorageActions();
expect(actions).toEqual([{ text: "Mount /dev/sdb1 as root", subvol: false, delete: false }]);
expect(proposal.volumes[0].mountPoint).toEqual("/test1");
expect(proposal.volumes[1].mountPoint).toEqual("/test2");
});
});
2 changes: 1 addition & 1 deletion web/src/components/core/PasswordAndConfirmationInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
} from "@patternfly/react-core";

const PasswordAndConfirmationInput = ({ value, onChange, onValidation }) => {
const [confirmation, setConfirmation] = useState("");
const [confirmation, setConfirmation] = useState(value || "");
const [error, setError] = useState("");

const validate = (password, passwordConfirmation) => {
Expand Down
12 changes: 12 additions & 0 deletions web/src/components/core/PasswordAndConfirmationInput.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,15 @@ describe("when the passwords do not match", () => {
await screen.findByText("Passwords do not match");
});
});

it("uses the given password value for confirmation too", async () => {
const { user } = plainRender(
<PasswordAndConfirmationInput value={"12345"} />
);

const passwordInput = screen.getByLabelText("Password");
const confirmationInput = screen.getByLabelText("Password confirmation");

expect(passwordInput.value).toEqual("12345");
expect(passwordInput.value).toEqual(confirmationInput.value);
});
2 changes: 1 addition & 1 deletion web/src/components/core/Section.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export default function Section({
</Text>
</TextContent>
</StackItem> }
{ errors &&
{ errors?.length > 0 &&
<StackItem>
<ValidationErrors errors={errors} title={`${title} errors`} />
</StackItem> }
Expand Down
5 changes: 3 additions & 2 deletions web/src/components/core/Section.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*/

import React, { useState } from "react";
import { act, screen } from "@testing-library/react";
import { screen } from "@testing-library/react";
import { installerRender } from "@/test-utils";
import { Section } from "@components/core";

Expand Down Expand Up @@ -84,7 +84,8 @@ describe("Section", () => {
expect(inputText).not.toBeInTheDocument();

const actionIcon = screen.getByRole("figure", { name: "Awesome settings section action icon" });
await act(async () => user.click(actionIcon));

await user.click(actionIcon);

inputText = screen.queryByRole("textbox", { name: "Awesome input" });
expect(inputText).toBeInTheDocument();
Expand Down
1 change: 0 additions & 1 deletion web/src/components/core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ export { default as InstallationProgress } from "./InstallationProgress";
export { default as InstallButton } from "./InstallButton";
export { default as InstallerSkeleton } from "./InstallerSkeleton";
export { default as KebabMenu } from "./KebabMenu";
export { default as Overview } from "./Overview";
export { default as PasswordAndConfirmationInput } from "./PasswordAndConfirmationInput";
export { default as Popup } from "./Popup";
export { default as ProgressReport } from "./ProgressReport";
Expand Down
Loading