Lior Vernia 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 validation package. 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 rather that the developer has made a mistake. Therefore displaying an error message is not the right approach here, in my opinion. The mechanism commonly used for this type of check is an assert statement - this signifies that you're making an assumption about the state at this point in the code, and terminates the flow of code if that isn't the case. Please read up and use it here, i.e. assert that (value instanceof String). 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 it shouldn't be displayed to users. 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 thisFieldMustContainCidrInFormatMsg(). 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. And just return result at the end. It'll save a few lines of code. 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. 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. 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 - usually when a bug is fixed it's best to add tests to make sure it doesn't return. 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. 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... 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. 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. 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: "This field must contain CIDR notation of an IP subnet, please ensure that the IP address matches the prefix." Then you may supply your example in following lines. 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 <[email protected]> Gerrit-Reviewer: Eliraz Levi <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
