Vered Volansky has posted comments on this change. Change subject: core: change storage_domain_static update policy ......................................................................
Patch Set 7: (7 comments) http://gerrit.ovirt.org/#/c/36151/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageDomainCommand.java: Line 39: return validateStoragePropertiesUpdate(); Line 40: } Line 41: Line 42: private boolean validateStoragePropertiesUpdate() { Line 43: if (!checkStorageDomainStatusNotEqual(StorageDomainStatus.Locked) || !validateStorageNameUpdate()) { > If the storage is locked, we can't do anything - this should be the first c If the storage is locked, the above greatly named method will add a message and return false, and so will this statement. Line 44: return false; Line 45: } Line 46: Line 47: // Collect changed fields to update in a list. Line 49: .getStorageStaticData()); Line 50: Line 51: if (props.isEmpty()) { Line 52: return true; Line 53: } > should be done after all the removals, no? Looks like. Line 54: // Allow change only to name & description field Line 55: props.remove("storageName"); Line 56: props.remove("description"); Line 57: props.remove("comment"); Line 73: } Line 74: Line 75: // if domain is part of pool, and name changed, check that pool is up in Line 76: // order to change description in spm Line 77: if (storageDomainNameChanged && !validate(new StoragePoolValidator(getStoragePool()).isUp())) { > storageDomainNameChanged is known to be true at this point - you shouldn't Well, *I* didn't, but reformatted the whole thing now. Line 78: return false; Line 79: } Line 80: Line 81: if (storageDomainNameChanged && isStorageWithSameNameExists()) { http://gerrit.ovirt.org/#/c/36151/7/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java: Line 703: Line 704: String name1 = storageModel.getName().getEntity(); Line 705: String tempVar = storageModel.getOriginalName(); Line 706: String originalName = (tempVar != null) ? tempVar : ""; //$NON-NLS-1$ Line 707: boolean isNameUnique = (Boolean) returnValue; > Why remove the black line here? Now returned Line 708: if (!isNameUnique && name1.compareToIgnoreCase(originalName) != 0) { Line 709: storageModel.getName() Line 710: .getInvalidityReasons() Line 711: .add(ConstantsManager.getInstance().getConstants().nameMustBeUniqueInvalidReason()); Line 881: }, Line 882: model); Line 883: } Line 884: Line 885: private void onSave() { > unrelated to the patch, and seems to be counter the class' conventions Automatically done by the formatter, amended manually, see if it'll hold. Line 886: storageNameValidation(); Line 887: } Line 888: Line 889: private void onSavePostNameValidation() Line 1131: getDestroyCommand().setIsAvailable(isAvailable); Line 1132: } Line 1133: Line 1134: private boolean isEditAvailable(StorageDomain storageDomain) { Line 1135: if (storageDomain == null || storageDomain.getStorageDomainSharedStatus() == StorageDomainSharedStatus.Locked) { > shouldn't this too be a call to isStorageNotLocked? Why? The whole point is not to be able to edit when it's locked. Line 1136: return false; Line 1137: } Line 1138: Line 1139: boolean isEditAvailable; Line 1364: storageListModel.saveNewGlusterStorage(); Line 1365: } Line 1366: } Line 1367: }), null, path); Line 1368: } else { > Why remove the black line here? I'll remove because it's not related to the patch, but seriously? I disagree, it's sooo ugly. Line 1369: updateStorageDomain(); Line 1370: } Line 1371: } Line 1372: -- To view, visit http://gerrit.ovirt.org/36151 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2dfa97b1dbe047d98f9f1e7f7ec2d53ae5c8a16b Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
