Eliraz Levi has posted comments on this change. Change subject: webadmin: integrate CidrValidator to frontend ......................................................................
Patch Set 6: (11 comments) http://gerrit.ovirt.org/#/c/32541/6/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: @Override Line 15: public ValidationResult validate(Object value) { Line 16: assert value instanceof String; > 1. Shouldn't you check assert (value == null || value instanceof String)? Done Line 17: String cidr = (String) value; Line 18: ValidationResult result = new ValidationResult(); Line 19: Line 20: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 17: String cidr = (String) value; Line 18: ValidationResult result = new ValidationResult(); Line 19: Line 20: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 21: return failWith(result, getThisFieldMustContainCidrInFormatMsg()); > I would not return here. It's cleaner to only set the error and message her Done Line 22: } Line 23: Line 24: else if (!CidrValidator.isCidrNetworkAddressValid(cidr)) { Line 25: return failWith(result, getCidrNotNetworkAddress()); Line 21: return failWith(result, getThisFieldMustContainCidrInFormatMsg()); Line 22: } Line 23: Line 24: else if (!CidrValidator.isCidrNetworkAddressValid(cidr)) { Line 25: return failWith(result, getCidrNotNetworkAddress()); > ...and here as well... Done Line 26: } Line 27: else { Line 28: result.setSuccess(true); Line 29: return result; Line 24: else if (!CidrValidator.isCidrNetworkAddressValid(cidr)) { Line 25: return failWith(result, getCidrNotNetworkAddress()); Line 26: } Line 27: else { Line 28: result.setSuccess(true); > ...and this isn't needed because ValidationResult is by default a success.. Done Line 29: return result; Line 30: } Line 31: Line 32: } Line 25: return failWith(result, getCidrNotNetworkAddress()); Line 26: } Line 27: else { Line 28: result.setSuccess(true); Line 29: return result; > And this needs to be returned anyway, whether there was an error or not. So Done Line 30: } Line 31: Line 32: } Line 33: http://gerrit.ovirt.org/#/c/32541/6/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 23: private String cidr; Line 24: private boolean validCidrFormatExpectedResult; Line 25: private boolean validNetworkAddressExpectedResult; Line 26: Line 27: public CidrValidationTest(String cidr, boolean validCidrFormatExpectedResult, boolean validNetworkAddressExpectedResult) { > I doesn't make sense to have two boolean values when you set the two error Done Line 28: this.cidr = cidr; Line 29: this.validCidrFormatExpectedResult = validCidrFormatExpectedResult; Line 30: this.validNetworkAddressExpectedResult = validNetworkAddressExpectedResult; Line 31: } Line 39: Line 40: @Test Line 41: public void checkCidrFormatValidation(){ Line 42: if (!validNetworkAddressExpectedResult){ Line 43: assertEquals("Failed to validate CIDR's format: "+cidr, validNetworkAddressExpectedResult, validation.validate(cidr).getSuccess());//$NON-NLS-1$ > Please add spaces between the string, the '+' and the cidr variable, it's n Done Line 44: } Line 45: Line 46: else{ Line 47: assertEquals("Failed to validate CIDR's format: "+cidr, validCidrFormatExpectedResult, validation.validate(cidr).getSuccess());//$NON-NLS-1$ Line 86: { "255?23?1?0/8", false, false}, //$NON-NLS-1$ Line 87: { "23\22\22\22\22\\", false, false}, //$NON-NLS-1$ Line 88: { ".................", false, false}, //$NON-NLS-1$ Line 89: { "././././", false, false}, //$NON-NLS-1$ Line 90: { "?/?/?/?/", false, false}, //$NON-NLS-1$ > Please add a blank line here, to visibly separate the section. Done Line 91: // Not A Network address Line 92: { "255.255.255.250/16", true, false}, //$NON-NLS-1$ Line 93: { "255.255.255.255/16", true, false}, //$NON-NLS-1$ Line 94: { "255.255.255.255/1", true, false}, //$NON-NLS-1$ Line 88: { ".................", false, false}, //$NON-NLS-1$ Line 89: { "././././", false, false}, //$NON-NLS-1$ Line 90: { "?/?/?/?/", false, false}, //$NON-NLS-1$ Line 91: // Not A Network address Line 92: { "255.255.255.250/16", true, false}, //$NON-NLS-1$ > How are the next ~30 tests different from this one? I would expect a few me Done Line 93: { "255.255.255.255/16", true, false}, //$NON-NLS-1$ Line 94: { "255.255.255.255/1", true, false}, //$NON-NLS-1$ Line 95: { "255.255.255.253/2", true, false}, //$NON-NLS-1$ Line 96: { "255.255.255.247/3", true, false}, //$NON-NLS-1$ Line 127: { "130.0.0.0/31", true, false}, //$NON-NLS-1$ Line 128: { "1.0.0.0/32", true, false}, //$NON-NLS-1$ Line 129: Line 130: // valid CIDR Line 131: { "255.255.255.255/0", true, true}, //$NON-NLS-1$ > Same comment here, in my opinion you don't have to test every prefix in the Done Line 132: { "255.255.255.254/1", true, true}, //$NON-NLS-1$ Line 133: { "255.255.255.252/2", true, true}, //$NON-NLS-1$ Line 134: { "255.255.255.248/3", true, true}, //$NON-NLS-1$ Line 135: { "255.255.255.240/4", true, true}, //$NON-NLS-1$ http://gerrit.ovirt.org/#/c/32541/6/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 2425: Line 2426: @DefaultStringValue("CPU Profile") Line 2427: String cpuProfileTitle(); Line 2428: Line 2429: @DefaultStringValue("This field must contain CIDR notation of an IP subnet, please ensure that the IP address matches the prefix.\nexample: valid network address: 2.2.0.0/16 \ninvalid: 2.2.0.1/16 ") > There's a spacebar character that isn't needed after 16. Done Line 2430: String CidrNotNetworkAddress(); Line 2431: } -- 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: 6 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
