Ayal Baron has posted comments on this change.
Change subject: webadmin: Import VM dialog redesign (#845947)
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(13 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java
Line 981: jobProperties = super.getJobMessageProperties();
Line 982: jobProperties.put(VdcObjectType.VM.name().toLowerCase(),
Line 983: (getVmName() == null) ? "" : getVmName());
Line 984:
jobProperties.put(VdcObjectType.VdsGroups.name().toLowerCase(),
getVdsGroupName());
Line 985: jobProperties.put(VdcObjectType.StoragePool.name(),
getStoragePoolName());
StorageHandlingCommandBase has:
jobProperties.put(VdcObjectType.StoragePool.name().toLowerCase(),
getStoragePoolName());
jobProperties.put(VdcObjectType.Storage.name().toLowerCase(),
getStorageDomainName());
jobProperties.put(VdcObjectType.VDS.name().toLowerCase(),
getVdsName());
So either rely on it or remove the super.getJobMessageProperties call above,
but right now it looks like you're doing something redundant.
Personally I prefer the latter (getting rid of super...)
Line 986: }
Line 987: return jobProperties;
Line 988: }
Line 989:
Line 1007: }
Line 1008:
Line 1009: private List<StorageQuotaValidationParameter>
getStorageQuotaListParameters() {
Line 1010: List<StorageQuotaValidationParameter> list = new
ArrayList<StorageQuotaValidationParameter>();
Line 1011: for (Disk disk :
getParameters().getVm().getDiskMap().values()) {
is this fixing anything? the constructor already sets the vm to
getParameters().getVm() and getVm either gets the one set in the constructor or
goes to the parameters as is done here (which seems redundant, but anyway).
So i don't understand why we need to go to the params here.
Line 1012: if(disk instanceof DiskImage){
Line 1013: DiskImage diskImage = (DiskImage)disk;
Line 1014: list.add(new
StorageQuotaValidationParameter(diskImage.getQuotaId(),
Line 1015:
imageToDestinationDomainMap.get(diskImage.getId()),
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/CommonApplicationConstants.java
Line 55:
Line 56: @DefaultStringValue("Storage")
Line 57: String singleDestinationStorage();
Line 58:
Line 59: @DefaultStringValue("Default Storage")
why isn't this defaultStorageDomain? That is actually what we're talking
about...
Line 60: String defaultStorage();
Line 61:
Line 62: @DefaultStringValue(" and Quota")
Line 63: String singleQuota();
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/VmBackupModel.java
Line 165: getEntity().getId());
Line 166: }
Line 167:
Line 168: @Override
Line 169: protected void Restore() {
Are you changing restore to import only in GUI? that could be quite confusing
for developers in the future.
Line 170: super.Restore();
Line 171:
Line 172: if (getWindow() != null) {
Line 173: return;
Line 210: }
Line 211: cloneVmMap = new HashMap<Guid, VM>();
Line 212:
Line 213: vmsInSetupMap = new HashMap<Guid, VM>();
Line 214: if (importModel.hasVmInSystem()) {
what do you need this 'if' for? There isn't supposed to be any indication in
the model that the vm is in the system before the user starts importing and
you're checking here in a loop for all VMs marked for import.
Line 215: for (VM vm : (ArrayList<VM>) importModel.getItems()) {
Line 216: if (importModel.isObjectInSetup(vm)) {
Line 217: vmsInSetupMap.put(vm.getId(), vm);
Line 218: }
Line 220: }
Line 221: excuteImportClone();
Line 222: }
Line 223:
Line 224: private void excuteImportClone() {
s/excute/execute/
Line 225: if (vmsInSetupMap.size() == 0) {
Line 226: excuteImport();
Line 227: return;
Line 228: }
Line 223:
Line 224: private void excuteImportClone() {
Line 225: if (vmsInSetupMap.size() == 0) {
Line 226: excuteImport();
Line 227: return;
unless I'm missing something, this 'if' clause should be in 'onRestore' the
code here should only be about clone as the name of the method indicates.
Line 228: }
Line 229: ImportCloneModel entity = new ImportCloneModel();
Line 230: VM vm = (VM) vmsInSetupMap.values().toArray()[0];
Line 231: entity.setEntity(vm);
Line 225: if (vmsInSetupMap.size() == 0) {
Line 226: excuteImport();
Line 227: return;
Line 228: }
Line 229: ImportCloneModel entity = new ImportCloneModel();
What causes the import clone model to be shown for each and every conflict?
Line 230: VM vm = (VM) vmsInSetupMap.values().toArray()[0];
Line 231: entity.setEntity(vm);
Line 232: entity.setTitle("Import Virtual Machine Conflict");
//$NON-NLS-1$
Line 233: entity.setHashName("import_virtual_machine_conflict");
//$NON-NLS-1$
Line 248: ImportCloneModel cloneModel = (ImportCloneModel)
getConfirmWindow();
Line 249: if ((Boolean) cloneModel.getApplyToAll().getEntity()) {
Line 250: if (!(Boolean) cloneModel.getNoClone().getEntity()) {
Line 251: String suffix = (String)
cloneModel.getSuffix().getEntity();
Line 252: if (!validateSuffix(suffix, cloneModel.getSuffix())) {
we need to support running numbers in suffix (probably in backend code
actually!)
Line 253: return;
Line 254: }
Line 255: for (VM vm : vmsInSetupMap.values()) {
Line 256: vm.setvm_name(vm.getvm_name() + suffix);
Line 261: } else {
Line 262: VM vm = (VM) cloneModel.getEntity();
Line 263: if (!(Boolean) cloneModel.getNoClone().getEntity()) {
Line 264: String vmName = (String)
cloneModel.getName().getEntity();
Line 265: if (!validateVmName(vmName, vm.getos(),
cloneModel.getName())) {
iiuc then each name conflict would fail the entire import? if so, we probably
need to a 'TODO' here to support 'Ignore' functionality (unless it's already
supported and I'm missing that of course)
Line 266: return;
Line 267: }
Line 268:
Line 269: vm.setvm_name(vmName);
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportData.java
Line 42: this.selectedQuota = selectedQuota;
Line 43: }
Line 44:
Line 45: public VolumeType getSelectedVolumeType() {
Line 46: if (!(Boolean) collapseSnapshots.getEntity() ||
selectedVolumeType == null) {
why is it ok for user to set the volume type if 'selectedVolumeType == null' ?
user should only be allowed to *change* the volume type if collapse is used.
Line 47: return getVolumeType();
Line 48: }
Line 49: return selectedVolumeType;
Line 50: }
Line 64: this.storageDomains = storageDomains;
Line 65: }
Line 66:
Line 67: public storage_domains getSelectedStorageDomain() {
Line 68: if (selectedStorageDomain == null &&
!storageDomains.isEmpty()) {
storageDomains cannot be null at this point?
if it can, why not just set it in constructor to be 'empty' and then remove all
null checks altogether?
Line 69: selectedStorageDomain = storageDomains.get(0);
Line 70: }
Line 71:
Line 72: return selectedStorageDomain;
Line 76: this.selectedStorageDomain = selectedStorageDomain;
Line 77: }
Line 78:
Line 79: public void setSelectedStorageDomainString(String value) {
Line 80: for (storage_domains storageDomain : getStorageDomains()) {
how do you know that getStorageDomains doesn't return null?
Line 81: if (storageDomain.getstorage_name().equals(value)) {
Line 82: setSelectedStorageDomain(storageDomain);
Line 83: break;
Line 84: }
--
To view, visit http://gerrit.ovirt.org/7313
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b6551b235c66e6a32db0f8842cd8e410d6e0318
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Haim Ateya <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
