Skip to content

Commit

Permalink
Re-write disk add dialog to functional component
Browse files Browse the repository at this point in the history
Split disk add dialog footer to new component.
The main component was already too large, so splitting to increase readbility.
This now allowed us to cleanup the logic for followup state updates,
which are not done in useEffect hooks.

Two small behavior changes were introduced:
First, on the validation of the input form. Previously the 'custom path'
error was introduced also before 'add' was clicked. Now, this behaves like all other form fields,
and validates on user's first 'Add' click.
Second the error messages were unified, so 'Disk failed to be attached'
now writes to 'Disk failed to be added'

This fixes the recursive setState (Fixes #1275)
  • Loading branch information
KKoukiou authored and martinpitt committed Nov 20, 2023
1 parent fa1632e commit 9b49f32
Show file tree
Hide file tree
Showing 7 changed files with 440 additions and 465 deletions.
2 changes: 1 addition & 1 deletion src/components/vm/deleteDialog.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export class DeleteDialog extends React.Component {
const Dialogs = this.context;
const storage = this.state.disks.filter(d => d.checked);
const { vm, onAddErrorNotification } = this.props;
const storagePools = getVmStoragePools(vm);
const storagePools = getVmStoragePools(vm.connectionName);

Promise.all(
(Array.isArray(vm.snapshots) ? vm.snapshots : [])
Expand Down
885 changes: 433 additions & 452 deletions src/components/vm/disks/diskAdd.jsx

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/components/vm/disks/vmDiskColumns.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export const DiskActions = ({ vm, vms, disk, supportedDiskBusTypes, idPrefixRow,
const Dialogs = useDialogs();

function openMediaInsertionDialog() {
Dialogs.show(<AddDiskModalBody idPrefix={idPrefixRow + "-insert-dialog"}
Dialogs.show(<AddDiskModalBody idPrefix={idPrefixRow + "-insert-dialog-adddisk"}
vm={vm} vms={vms}
disk={disk}
supportedDiskBusTypes={supportedDiskBusTypes}
Expand Down
2 changes: 1 addition & 1 deletion src/components/vm/disks/vmDisksCard.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export const VmDisksActions = ({ vm, vms, supportedDiskBusTypes }) => {
const idPrefix = `${vmId(vm.name)}-disks`;

function open() {
Dialogs.show(<AddDiskModalBody idPrefix={idPrefix}
Dialogs.show(<AddDiskModalBody idPrefix={idPrefix + "-adddisk"}
vm={vm} vms={vms}
supportedDiskBusTypes={supportedDiskBusTypes} />);
}
Expand Down
4 changes: 2 additions & 2 deletions src/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -847,9 +847,9 @@ export function getHostDevSourceObject(dev) {
return source;
}

export function getVmStoragePools(vm) {
export function getVmStoragePools(connectionName) {
const { storagePools } = store.getState();
return storagePools.filter(sp => sp && sp.name && sp.connectionName == vm.connectionName && sp.active);
return storagePools.filter(sp => sp && sp.name && sp.connectionName == connectionName && sp.active);
}

export function nicLookupByMAC(interfacesList, mac) {
Expand Down
8 changes: 2 additions & 6 deletions test/check-machines-disks
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,6 @@ class TestMachinesDisks(VirtualMachinesCase):
xfail_error_message=None, xfail_error_title=None,
xwarning_object=None, xwarning_message=None,
pixel_test_ignore=None,
skip_add=False,
):
self.test_obj = test_obj
self.vm_name = vm_name
Expand All @@ -518,7 +517,6 @@ class TestMachinesDisks(VirtualMachinesCase):
self.expected_volume_format = expected_volume_format
self.persistent_vm = persistent_vm
self.expected_access = expected_access
self.skip_add = skip_add

self.pixel_test_tag = pixel_test_tag
self.pixel_test_ignore = pixel_test_ignore
Expand Down Expand Up @@ -553,8 +551,7 @@ class TestMachinesDisks(VirtualMachinesCase):
ignore=[self.pixel_test_ignore] if self.pixel_test_ignore else [],
skip_layouts=["rtl"])

if not self.skip_add:
self.add_disk()
self.add_disk()
if not self.xfail:
self.verify_disk_added()
else:
Expand Down Expand Up @@ -1124,7 +1121,7 @@ class TestMachinesDisks(VirtualMachinesCase):
device='disk',
file_path='/tmp/',
mode='custom-path',
xfail=True, xfail_error_title="Disk failed to be attached",
xfail=True, xfail_error_title="Disk failed to be added",
).execute()

# check disk device type
Expand Down Expand Up @@ -1158,7 +1155,6 @@ class TestMachinesDisks(VirtualMachinesCase):
device='disk',
file_path='/tmp/base.qcow2',
mode='custom-path',
skip_add=True,
xfail=True, xfail_object='file-autocomplete',
xfail_error_message='Importing an image with a backing file is unsupported',
).execute()
Expand Down
2 changes: 0 additions & 2 deletions test/machineslib.py
Original file line number Diff line number Diff line change
Expand Up @@ -436,8 +436,6 @@ def setUp(self):
"Warning: Failed.*type:.*The prop `format` is marked as required in `VolumeCreateBody`, but its value is `undefined`",
# FIXME: https://github.com/cockpit-project/cockpit-machines/issues/1273
" Warning: Failed.*type:.* Invalid prop `size` of type `string` supplied to `VolumeCreateBody`, expected `number`",
# FIXME: https://github.com/cockpit-project/cockpit-machines/issues/1275
"Warning: An update .* was scheduled from inside an update function",
# deprecated PF SelectGroup has invalid properties
r"Warning: React does not recognize the .* prop.*(inputId|isSelected|sendRef|keyHandler)",
)
Expand Down

0 comments on commit 9b49f32

Please sign in to comment.