Eliraz Levi has posted comments on this change. Change subject: webadmin: integrate CidrValidator to frontend ......................................................................
Patch Set 1: (13 comments) http://gerrit.ovirt.org/#/c/32541/1/frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml File frontend/webadmin/modules/gwt-common/src/main/resources/org/ovirt/engine/core/Common.gwt.xml: Line 236: <include name="common/utils/SimpleDependecyInjector.java"/> Line 237: <include name="common/utils/VersionStorageFormatUtil.java"/> Line 238: <include name="common/utils/SizeConverter.java"/> Line 239: <include name="common/utils/CommonConstants.java"/> Line 240: <include name="common/validation/CidrValidator.java"/> > Maybe better to put this together with the other classes imported from the Done Line 241: Line 242: Line 243: <!-- Required by frontend --> Line 244: <include name="common/interfaces/SearchType.java" /> http://gerrit.ovirt.org/#/c/32541/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidation.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidation.java: Line 12: } Line 13: Line 14: public ValidationResult validate(Object value) { Line 15: ValidationResult result = new ValidationResult(); Line 16: if (!(value instanceof String)) { > Note that this case doesn't mean that the user has input a bad value, but r Done Line 17: result.setSuccess(false); Line 18: result.setReasons(Arrays.asList(ConstantsManager.getInstance().getConstants().StringArgExpected())); Line 19: return result; Line 20: } Line 14: public ValidationResult validate(Object value) { Line 15: ValidationResult result = new ValidationResult(); Line 16: if (!(value instanceof String)) { Line 17: result.setSuccess(false); Line 18: result.setReasons(Arrays.asList(ConstantsManager.getInstance().getConstants().StringArgExpected())); > In case of an assertion error, you won't need to externalize the string as Done Line 19: return result; Line 20: } Line 21: String cidr = (String) value; Line 22: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 20: } Line 21: String cidr = (String) value; Line 22: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 23: result.setSuccess(false); Line 24: result.setReasons(Arrays.asList(ConstantsManager.getInstance().getConstants().CidrBadFormat())); > You don't need this new constant, you can just use the existing thisFieldMu Done Line 25: return result; Line 26: } Line 27: if (!CidrValidator.isCidrValidNetworkAddress(cidr)) { Line 28: result.setSuccess(false); Line 21: String cidr = (String) value; Line 22: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 23: result.setSuccess(false); Line 24: result.setReasons(Arrays.asList(ConstantsManager.getInstance().getConstants().CidrBadFormat())); Line 25: return result; > I would get rid of the return statement here and use an if... else clause. Done Line 26: } Line 27: if (!CidrValidator.isCidrValidNetworkAddress(cidr)) { Line 28: result.setSuccess(false); Line 29: result.setReasons(Arrays.asList(ConstantsManager.getInstance().getConstants().CidrNotNetworkAddress())); Line 26: } Line 27: if (!CidrValidator.isCidrValidNetworkAddress(cidr)) { Line 28: result.setSuccess(false); Line 29: result.setReasons(Arrays.asList(ConstantsManager.getInstance().getConstants().CidrNotNetworkAddress())); Line 30: return result; > Same here. Done Line 31: } Line 32: result.setSuccess(true); Line 33: return result; Line 34: } Line 28: result.setSuccess(false); Line 29: result.setReasons(Arrays.asList(ConstantsManager.getInstance().getConstants().CidrNotNetworkAddress())); Line 30: return result; Line 31: } Line 32: result.setSuccess(true); > This should be in the else clause. Done Line 33: return result; Line 34: } http://gerrit.ovirt.org/#/c/32541/1/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidationTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidationTest.java: Line 40: { "1.1.1.1/", false }, //$NON-NLS-1$ Line 41: { "1.1.1.1/40", false }, //$NON-NLS-1$ Line 42: { "1000.1.1.1/24", false }, //$NON-NLS-1$ Line 43: { "1.1000.1.1/24", false }, //$NON-NLS-1$ Line 44: { "1.1.1000.1/24", false }, //$NON-NLS-1$ > Please extend this to include the new use cases that should return false - i took what missing in CidrValidatorTest in common validation added it there. the test here,now uses CidrValidatorTest's params collection Line 45: { "1.1.1.1000/24", false }, //$NON-NLS-1$ Line 46: Line 47: { "0.0.0.0/0", true }, //$NON-NLS-1$ Line 48: { "1.1.1.1/0", true }, //$NON-NLS-1$ Line 44: { "1.1.1000.1/24", false }, //$NON-NLS-1$ Line 45: { "1.1.1.1000/24", false }, //$NON-NLS-1$ Line 46: Line 47: { "0.0.0.0/0", true }, //$NON-NLS-1$ Line 48: { "1.1.1.1/0", true }, //$NON-NLS-1$ > This should probably return false now. Done Line 49: { "1.1.1.1/24", true }, //$NON-NLS-1$ Line 50: { "1.1.1.1/32", true }, //$NON-NLS-1$ Line 51: { "100.1.1.1/24", true }, //$NON-NLS-1$ Line 52: { "255.1.1.1/24", true }, //$NON-NLS-1$ Line 47: { "0.0.0.0/0", true }, //$NON-NLS-1$ Line 48: { "1.1.1.1/0", true }, //$NON-NLS-1$ Line 49: { "1.1.1.1/24", true }, //$NON-NLS-1$ Line 50: { "1.1.1.1/32", true }, //$NON-NLS-1$ Line 51: { "100.1.1.1/24", true }, //$NON-NLS-1$ > These two as well... Done Line 52: { "255.1.1.1/24", true }, //$NON-NLS-1$ Line 53: }); Line 54: } Line 55: http://gerrit.ovirt.org/#/c/32541/1/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 2426: @DefaultStringValue("CPU Profile") Line 2427: String cpuProfileTitle(); Line 2428: Line 2429: @DefaultStringValue("String was expected") Line 2430: String StringArgExpected(); > This should be removed if you use assert in CidrValidation. Done Line 2431: Line 2432: @DefaultStringValue("Cidr bad format, expected:\n x.x.x.x/y where:\n x belongs to [0,255] \n y belongs to [0,32] \n both inclusive") Line 2433: String CidrBadFormat(); Line 2434: Line 2429: @DefaultStringValue("String was expected") Line 2430: String StringArgExpected(); Line 2431: Line 2432: @DefaultStringValue("Cidr bad format, expected:\n x.x.x.x/y where:\n x belongs to [0,255] \n y belongs to [0,32] \n both inclusive") Line 2433: String CidrBadFormat(); > As mentioned, this isn't needed either. Done Line 2434: Line 2435: @DefaultStringValue("Cidr not represnting a network address.\nplease ensure ip and mask are matching to network ip address. \nexample: valid network address: 2.2.0.0/16 \ninvalid: 2.2.0.1/16 ") Line 2436: String CidrNotNetworkAddress(); Line 2437: } Line 2431: Line 2432: @DefaultStringValue("Cidr bad format, expected:\n x.x.x.x/y where:\n x belongs to [0,255] \n y belongs to [0,32] \n both inclusive") Line 2433: String CidrBadFormat(); Line 2434: Line 2435: @DefaultStringValue("Cidr not represnting a network address.\nplease ensure ip and mask are matching to network ip address. \nexample: valid network address: 2.2.0.0/16 \ninvalid: 2.2.0.1/16 ") > I would phrase it more consistently with other errors: Done Line 2436: String CidrNotNetworkAddress(); Line 2437: } -- To view, visit http://gerrit.ovirt.org/32541 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib09d8943b0265960a35f3dd328dae20247b69fd3 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@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
