Arik Hadas has uploaded a new change for review. Change subject: webadmin: improve error checking on import dialog ......................................................................
webadmin: improve error checking on import dialog Consider the following scenario: 1. VM with id=1 and name='A' already exists in the system 2. The user is about to import a VM with id=2 and name='A' We would expect the UI to tell the user that the VM already exists so he'll choose a different name. However, the actual result is that the UI pass the request to the backend, and the backend generates the error. To overcome this problem this patch does: 1. Instead of querying only VMs with IDs of those VMs that are going to be imported, we are now querying all the VMs in the system (using a query instead of search) 2. We validate that the name of the VM to be imported is unique using the result of that query 3. In case a VM with the given name already exists in the system, we show an error next to the VM that cannot be imported (the user can go back and deselect this VM) Change-Id: Id3f7e8d1780dc500f1c400559b6e4a40ebcd32ef Signed-off-by: Arik Hadas <[email protected]> --- M frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmData.java M frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmFromExportDomainModel.java M frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java M frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmFromExportDomainPopupView.java 4 files changed, 68 insertions(+), 32 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/10/40310/1 diff --git a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmData.java b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmData.java index a795158..5f8276e 100644 --- a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmData.java +++ b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmData.java @@ -13,6 +13,7 @@ private boolean templateExistsInSetup = true; private EntityModel<Boolean> collapseSnapshots; private String warning; + private String error; public ImportVmData(VM vm) { setCollapseSnapshots(new EntityModel<>(true)); @@ -80,4 +81,12 @@ public void setWarning(String warning) { this.warning = warning; } + + public String getError() { + return error; + } + + public void setError(String error) { + this.error = error; + } } diff --git a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmFromExportDomainModel.java b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmFromExportDomainModel.java index 474f038..a9eee28 100644 --- a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmFromExportDomainModel.java +++ b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmFromExportDomainModel.java @@ -25,10 +25,8 @@ import org.ovirt.engine.core.common.businessentities.storage.DiskImage; import org.ovirt.engine.core.common.businessentities.storage.VolumeFormat; import org.ovirt.engine.core.common.businessentities.storage.VolumeType; -import org.ovirt.engine.core.common.interfaces.SearchType; import org.ovirt.engine.core.common.queries.GetAllFromExportDomainQueryParameters; import org.ovirt.engine.core.common.queries.IdQueryParameters; -import org.ovirt.engine.core.common.queries.SearchParameters; import org.ovirt.engine.core.common.queries.VdcQueryParametersBase; import org.ovirt.engine.core.common.queries.VdcQueryReturnValue; import org.ovirt.engine.core.common.queries.VdcQueryType; @@ -72,6 +70,7 @@ private Map<Guid, ArrayList<Quota>> storageQuotaMap; private final Map<Guid, List<Disk>> templateDiskMap = new HashMap<>(); private final Map<Guid, ImportDiskData> diskImportDataMap = new HashMap<>(); + private List<VM> vms; public StoragePool getStoragePool() { return storagePool; @@ -577,6 +576,37 @@ super.activeDetailModelChanged(); } + private boolean validateNames() { + boolean valid = true; + for (ImportVmData importVmData : (Iterable<ImportVmData>) getItems()) { + if (!validateName(importVmData)) { + valid = false; + } else { + importVmData.setError(null); + } + } + + if (!valid) { + onPropertyChanged(new PropertyChangedEventArgs("InvalidVm")); //$NON-NLS-1$ + } + return valid; + } + + private boolean validateName(ImportVmData data) { + if (data.getClone().getEntity()) { + return true; + } + for (VM vm : vms) { + if (vm.getName().equals(data.getVm().getName())) { + if (!vm.getId().equals(data.getVm().getId())) { + data.setError(ConstantsManager.getInstance().getConstants().vmWithSameNameExists()); + return false; + } + } + } + return true; + } + public boolean validate() { if (QuotaEnforcementTypeEnum.HARD_ENFORCEMENT.equals(storagePool.getQuotaEnforcementType())) { getClusterQuota().validateSelectedItem( @@ -599,50 +629,34 @@ getCluster().validateSelectedItem( new IValidation[] { new NotEmptyValidation() }); - return getStorage().getIsValid() + return validateNames() + && getStorage().getIsValid() && getCluster().getIsValid() && getClusterQuota().getIsValid(); } - public void setItems(final Collection value, final Guid storageDomainId) - { - String vm_guidKey = "ID ="; //$NON-NLS-1$ - String orKey = " or "; //$NON-NLS-1$ - StringBuilder searchPattern = new StringBuilder(); - searchPattern.append("VM: "); //$NON-NLS-1$ - - final List<VM> list = (List<VM>) value; - for (int i = 0; i < list.size(); i++) { - VM vm = list.get(i); - - searchPattern.append(vm_guidKey); - searchPattern.append(vm.getId().toString()); - if (i < list.size() - 1) { - searchPattern.append(orKey); - } - } - - Frontend.getInstance().runQuery(VdcQueryType.Search, - new SearchParameters(searchPattern.toString(), SearchType.VM), + public void setItems(final Collection value, final Guid storageDomainId) { + Frontend.getInstance().runQuery(VdcQueryType.GetAllVms, + new VdcQueryParametersBase(), new AsyncQuery(this, new INewAsyncCallback() { @Override public void onSuccess(Object model, Object returnValue) { - List<VM> vmList = - ((VdcQueryReturnValue) returnValue).getReturnValue(); + vms = ((VdcQueryReturnValue) returnValue).getReturnValue(); List<ImportVmData> vmDataList = new ArrayList<>(); - for (VM vm : (Iterable<VM>) value) { ImportVmData vmData = new ImportVmData(vm); - boolean vmExistsInSystem = vmList.contains(vm); - vmData.setExistsInSystem(vmExistsInSystem); - if (vmExistsInSystem) { + if (vms.contains(vm)) { + vmData.setExistsInSystem(true); vmData.getClone().setEntity(true); vmData.getClone().setChangeProhibitionReason(ConstantsManager.getInstance() .getConstants() .importVMThatExistsInSystemMustClone()); vmData.getClone().setIsChangable(false); + } + else { + vms.add(vm); } vmDataList.add(vmData); } @@ -650,7 +664,6 @@ doInit(storageDomainId); } })); - } public void setSuperItems(Collection value) { diff --git a/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java b/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java index e4f6631..60a6c51 100644 --- a/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java +++ b/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java @@ -2703,4 +2703,7 @@ @DefaultStringValue("Warning : Recommendations for geo-replication not met -") String geoReplicationRecommendedConfigViolation(); + + @DefaultStringValue("VM with the same name already exists") + String vmWithSameNameExists(); } diff --git a/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmFromExportDomainPopupView.java b/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmFromExportDomainPopupView.java index 976e879..96473ee 100644 --- a/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmFromExportDomainPopupView.java +++ b/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmFromExportDomainPopupView.java @@ -240,12 +240,20 @@ AbstractImageResourceColumn<Object> isProblematicImportVmColumn = new AbstractImageResourceColumn<Object>() { @Override public ImageResource getValue(Object object) { - return ((ImportVmData) object).getWarning() != null ? resources.alertImage() : null; + ImportVmData importVmData = ((ImportVmData) object); + if (importVmData.getError() != null) { + return resources.errorImage(); + } + if (importVmData.getWarning() != null) { + return resources.alertImage(); + } + return null; } @Override public SafeHtml getTooltip(Object object) { - String problem = ((ImportVmData) object).getWarning(); + ImportVmData importVmData = ((ImportVmData) object); + String problem = importVmData.getError() != null ? importVmData.getError() : importVmData.getWarning(); return problem != null ? SafeHtmlUtils.fromSafeConstant(problem) : null; } }; @@ -678,6 +686,9 @@ } else if (args.propertyName.equals("Message")) { ////$NON-NLS-1$ message.setText(object.getMessage()); } + else if (args.propertyName.equals("InvalidVm")) { ////$NON-NLS-1$ + table.redraw(); + } } }); -- To view, visit https://gerrit.ovirt.org/40310 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Id3f7e8d1780dc500f1c400559b6e4a40ebcd32ef Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
