Tomas Jelinek has posted comments on this change.
Change subject: webadmin: Add validation for create/edit pool (#849426)
......................................................................
Patch Set 2: (6 inline comments)
Hi Gilad,
I have replied to your comments, please have a look and let me know what do you
think.
Thanks
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingPoolModelBehavior.java
Line 61: return new IValidation[] { new
ExistingPoolNameLengthValidation(
Line 62: (String) getModel().getName().getEntity(),
Line 63: getModel().getAssignedVms().AsConvertible().Integer()
Line 64: +
getModel().getNumOfDesktops().AsConvertible().Integer(),
Line 65: (VmOsType) getModel().getOSType().getSelectedItem()
Good point, I will change it.
Line 66: ) };
Line 67: }
Line 68:
Line 69: @Override
Line 67: }
Line 68:
Line 69: @Override
Line 70: protected IValidation[] getPoolNameSpecificValidations() {
Line 71: return new IValidation[] { new DummyValidation() };
Well, I don't agree. Copy-pasting logic, even a dummy one, is not a good
practice...
Line 72: }
Line 73:
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/NewPoolModelBehavior.java
Line 40: }
Line 41:
Line 42: @Override
Line 43: protected IValidation[] getNumOfDesktopsSpecificValidations() {
Line 44: return new IValidation[] { new DummyValidation() };
same...
Line 45: }
Line 46:
Line 47: @Override
Line 48: protected IValidation[] getPoolNameSpecificValidations() {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/PoolModelBehaviorBase.java
Line 318: new LengthValidation(4),
Line 319: new IntegerValidation(isNew ? 1 : 0, isNew ?
maxAllowedVms : maxAllowedVms - assignedVms)
Line 320: },
Line 321:
Line 322: getNumOfDesktopsSpecificValidations()
The name validation is a different case - it is because of the former name
validation in UnitVmModel. While I still think that the union is a more clean
approach than an if statement, I understand your point that it is an overkill
for this simple case. I will change the getNumOfDesktopsSpecificValidations and
getPoolNameSpecificValidations to not return and array but just one validation.
Is this compromise acceptable for you?
Line 323: )
Line 324: );
Line 325:
Line 326: getModel().getPrestartedVms().ValidateEntity(
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/DummyValidation.java
Line 1: package org.ovirt.engine.ui.uicommonweb.validation;
Line 2:
Line 3: public class DummyValidation implements IValidation {
I think it is still better than copy pasting it's content as anonymous inner
class over and over again... At least it is a named logic.
Line 4:
Line 5: @Override
Line 6: public ValidationResult Validate(Object value) {
Line 7: ValidationResult res = new ValidationResult();
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/ExistingPoolNameLengthValidation.java
Line 18:
Line 19: private int generateMaxLength() {
Line 20: return doGenerateMaxLength(getMaxNameLength(),
getPoolName().length());
Line 21: }
Line 22:
I have left the default access - it is a common practice how to make the method
testable while not exposing it's functionality "too much". It is not perfect,
but it is the most common solution recommended in many sources...
Line 23: int doGenerateMaxLength(int maxNameLengt, int poolNameLength) {
Line 24: int allowedLength = maxNameLengt - poolNameLength - 1;
Line 25: StringBuilder sb = new StringBuilder("");
Line 26:
--
To view, visit http://gerrit.ovirt.org/7649
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I66f765ceb0567beb006d1ef4f45db3902ef95ea6
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
