Omer Frenkel has posted comments on this change.
Change subject: core: Introduce Change quota command - disks (#848310)
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(3 inline comments)
you cannot add new commands without tests...
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/ChangeQuotaCommand.java
Line 26:
Line 27: @Override
Line 28: protected boolean canDoAction() {
Line 29: // check if SP exist
Line 30: setStoragePoolId(getParameters().getStoragePoolId());
well it's not a must but i think the above line better be in the ctor
Line 31: if (getStoragePool() == null) {
Line 32:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST);
Line 33: return false;
Line 34: }
Line 28: protected boolean canDoAction() {
Line 29: // check if SP exist
Line 30: setStoragePoolId(getParameters().getStoragePoolId());
Line 31: if (getStoragePool() == null) {
Line 32:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST);
you are missing override for setActionMessageParameters method (or at least a
set for action here and type in any extending command),
the above message will not appear nicely to the user
Line 33: return false;
Line 34: }
Line 35: // Check if quota exist:
Line 36: if (getQuotaId() == null) {
Line 48: public List<PermissionSubject> getPermissionCheckSubjects() {
Line 49: List<PermissionSubject> permissionList = new
ArrayList<PermissionSubject>();
Line 50: permissionList.add(new
PermissionSubject(getParameters().getQuotaId(),
Line 51: VdcObjectType.Quota,
Line 52: ActionGroup.CONSUME_QUOTA));
well i am not sure as well, on one hand, i'd say, user has quota, if he want to
spend it by taking objects under his quota, let it be.
on the other hand, it sounds a little weird someone can change an object he has
no relation to.
we can raise it for discussion if we want more opinions...
Line 53: return permissionList;
Line 54: }
Line 55:
Line 56: @Override
--
To view, visit http://gerrit.ovirt.org/7357
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0017a18109faecd1acab52897bba5474b6600390
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
