Ayal Baron has posted comments on this change.
Change subject: webadmin: separate storage & cluster quota
......................................................................
Patch Set 8: (17 inline comments)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/quota/QuotaListModel.java
Line 178: qModel.getSpecificStorageQuota().setEntity(true);
shouldn't this be getSpecificClusterQuota(true)?
Line 179: qModel.setClusterQuota(true);
Why do we need 3 booleans to indicate if it's a storage quota or a cluster
quota?
In the least, setting of specific cluster could negate storage and vice versa.
Line 181: qModel.getSpecificClusterQuota().setEntity(true);
likewise - getSpecificStorageQuota..true ?
Line 218: if (isQuotaForCluster) {
code should be split into 2 methods. eventRaised should call the correct one
according to isQuotaForCluster
Line 225:
qModel.getAllDataCenterClusters().setItems(new ArrayList<QuotaVdsGroup>());
no qModel.StopProgress() ?
Line 260: &&
!storage.getstorage_domain_type().equals(StorageDomainType.Data)) {
only data domains can be master domains so this extra test seems redundant.
Line 315: } else if (model.getAllDataCenterClusters().getItems() !=
null) {
getGlobalClusterQuota should never be null to begin with. If you hit a null
pointer exception here then the fix is not to ignore but rather to make sure we
don't reach this state to begin with.
I'd say the correct solution would be either to add a ctor to ListModel which
accepts an Iterator and initializes items to it (and QuotaModel ctor would pass
a new empty list) or just in QuotaModel ctor setItems right after
initialization of AllDataCenterClusters.
Line 335: } else if (model.getAllDataCenterStorages().getItems() !=
null) {
same
Line 414: qModel.setClusterQuota(false);
no StorageQuota here?
Line 643: } else if (command.equals(getCreateQuotaClusterCommand())) {
need line break here.
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/quota/QuotaModel.java
Line 409: getSpecificClusterQuota().setIsValid(true);
a 'validate' method should not change properties
Line 412: empty = false;
just return
Line 415: if (getAllDataCenterClusters().getItems() != null) {
this should never happen
Line 419: break;
just return
Line 424: if (empty && getAllDataCenterStorages().getItems() !=
null) {
how would this have changed since previous call on line 415?
if you do not get rid of the if getAllData... then in the least this section
should go up into the previous if
Line 428: break;
just return
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/Constants.java
Line 1381: @DefaultStringValue("Quota must contain limitations")
I have no idea what this message means
--
To view, visit http://gerrit.ovirt.org/6434
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id8d23a8232d6b2cd8dcf4dfbde972dd9bfdd6420
Gerrit-PatchSet: 8
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]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
