Daniel Erez has posted comments on this change.
Change subject: Import VM dialog bug fix and cleanup (WIP)
......................................................................
Patch Set 1: (14 inline comments)
Some issues I've encountered while verifying:
* When importing a VM based on a template that is missing in export storage
domain 'Constants->errorTemplateCannotBeFoundMessage' message should be
displayed.
* When importing a VM based on a template that is missing in the system
'Constants->importMissingStorages' message should be displayed.
* Clone checkbox should be disabled for VM that already exists in the system
(currently it seems to work sporadically...).
* The 'problematic' VMs alert image (that was removed from ImportVmPopupView)
should probably be re-added on VM level, i.e. add it left to "Collapse
Snapshots" column.
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/VmBackupModel.java
Line 212:
Line 213: objectsToClone = new ArrayList<Object>();
Line 214: for (Object object : (ArrayList<Object>)
importModel.getItems()) {
Line 215: ImportEntityData item = (ImportEntityData) object;
Line 216: if ((Boolean) item.getClone().getEntity()) {
What about 'isObjectInSetup' check?
(i.e. only items that already exists in the system can be cloned)
Line 217: objectsToClone.add(object);
Line 218: }
Line 219: }
Line 220: executeImportClone();
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportEntityData.java
Line 3: import org.ovirt.engine.ui.uicommonweb.models.EntityModel;
Line 4:
Line 5: public abstract class ImportEntityData {
Line 6: protected Object entity;
Line 7: boolean isExistsInSystem;
consider modifying..:
'isExistsInSystem' -> 'existsInSystem'
Line 8: private EntityModel collapseSnapshots;
Line 9: private EntityModel clone;
Line 10:
Line 11: public ImportEntityData() {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmModel.java
Line 300: ((ImportVmData)
item).getCollapseSnapshots().setIsChangable(false);
Line 301: setMessage(ConstantsManager.getInstance()
Line 302: .getConstants()
Line 303:
.useSeparateImportOperationForMarkedVMsMsg());
Line 304: OnPropertyChanged(new
PropertyChangedEventArgs(ON_DISK_LOAD));
You can break the loop when finding a problematic VM
(and add some comment that a problematic item has been found...)
Line 305: }
Line 306: }
Line 307: }
Line 308: }
Line 319: }
Line 320:
templateDiskMap.get(vm.getVmtGuid()).addAll(vm.getDiskMap().values());
Line 321:
Line 322: if (!requiredTemplateIds.contains(vm.getVmtGuid())) {
Line 323: requiredTemplateIds.add(vm.getVmtGuid());
'requiredTemplateIds' is not used anywhere?
Line 324: }
Line 325: }
Line 326: for (Disk disk : vm.getDiskMap().values()) {
Line 327: DiskImage diskImage = (DiskImage) disk;
Line 347
Line 348
Line 349
Line 350
Line 351
Why removing the 'else'?
Line 353:
Line 354: for (Guid templateId : templateDiskMap.keySet()) {
Line 355: for (Disk disk :
templateDiskMap.get(templateId)) {
Line 356: DiskImage diskImage = (DiskImage) disk;
Line 357: if (diskImage.getParentId() != null &&
NGuid.Empty.equals(diskImage.getParentId())) {
Nice catch :)
Line 358: ArrayList<storage_domains>
storageDomains =
Line 359:
templateDisksStorageDomains.get(diskImage.getParentId());
Line 360: if (storageDomains == null) {
Line 361:
missingTemplateDiskMap.put(templateId, templateDiskMap.get(templateId));
Line 381
Line 382
Line 383
Line 384
Line 385
Why is it no longer needed?
Line 391
Line 392
Line 393
Line 394
Line 395
same here
Line 392: tempMap.put(entry.getKey().getId(), null);
Line 393: }
Line 394: for (Guid templateId :
missingTemplateDiskMap.keySet()) {
Line 395: if (tempMap.containsKey(templateId)) {
Line 396: for (Disk disk :
missingTemplateDiskMap.get(templateId)) { // REmove.... ???
Why removing? We want to show these disks..
Line 397: addDiskImportData(disk.getId(),
Line 398: filteredStorageDomains,
Line 399: ((DiskImage)
disk).getvolume_type(),
Line 400: new EntityModel(true));
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmImportInterfaceListModel.java
Line 2:
Line 3: import org.ovirt.engine.core.common.businessentities.VM;
Line 4: import org.ovirt.engine.ui.uicommonweb.models.SearchableListModel;
Line 5:
Line 6: public class VmImportInterfaceListModel extends SearchableListModel
The class no longer extends VmInterfaceListModel in order to set the entity
with ImportVmData?
Line 7: {
Line 8: public VmImportInterfaceListModel() {
Line 9: setIsTimerDisabled(true);
Line 10: }
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmPopupView.java
Line 335: ((SingleSelectionModel<ImportVmData>)
event.getSource()).getSelectedObject();
Line 336:
customSelectionCellFormatType.setEnabledWithToolTip((Boolean)
selectedObject.getCollapseSnapshots()
Line 337: .getEntity(),
Line 338: constants.importAllocationModifiedCollapse());
Line 339: //
diskTable.edit(importVmModel.getImportDiskListModel());
Remove redundant code.
Isn't the 'edit' needed for updating disk table on selection?
Line 340: }
Line 341: });
Line 342:
Line 343: ScrollPanel sp = new ScrollPanel();
Line 616: // constants.importAllocationModifiedCollapse());
Line 617: // }
Line 618: // if (object.getDisksToConvert() != null &&
object.getDisksToConvert().size() > 0) {
Line 619: // image.setVisible(true);
Line 620: // }
Redundant code?
Why removing the warning mark for problematic VMs?
Line 621: } else if (args instanceof PropertyChangedEventArgs
Line 622: && ((PropertyChangedEventArgs)
args).PropertyName.equals("Message")) { ////$NON-NLS-1$
Line 623: message.setText(object.getMessage());
Line 624: }
Line 641: ScrollPanel sp = new ScrollPanel();
Line 642: sp.add(table);
Line 643: splitLayoutPanel.add(sp);
Line 644:
table.getElement().getStyle().setPosition(Position.RELATIVE);
Line 645: // subTabLayoutPanel.selectTab(0);
Remove redundant code.
It indeed makes sense to keep the selected tab on VM selection change...
Line 646: }
Line 647:
Line 648: });
Line 649: nicTable.edit((SearchableListModel)
object.getDetailModels().get(1));
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmPopupView.ui.xml
Line 84
Line 85
Line 86
Line 87
Line 88
Why removing the problematic VMs alert image?
--
To view, visit http://gerrit.ovirt.org/10828
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8fb5139f43370705081675ab5ffa1552c16c3082
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tal Nisan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
