Eliraz Levi has posted comments on this change. Change subject: webadmin: integrate CidrValidator to frontend ......................................................................
Patch Set 8: (6 comments) http://gerrit.ovirt.org/#/c/32541/8/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 18: String cidr = (String) value; Line 19: ValidationResult result = new ValidationResult(); Line 20: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 21: failWith(result, getThisFieldMustContainCidrInFormatMsg()); Line 22: } > Sorry I missed this earlier, it's customary to put else if statements in th Done Line 23: Line 24: else if (!CidrValidator.isCidrNetworkAddressValid(cidr)) { Line 25: failWith(result, getCidrNotNetworkAddress()); Line 26: } http://gerrit.ovirt.org/#/c/32541/8/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 73: { ".................", false}, //$NON-NLS-1$ Line 74: { "././././", false}, //$NON-NLS-1$ Line 75: { "?/?/?/?/", false}, //$NON-NLS-1$ Line 76: Line 77: // Not A Network address > The following tests look good, could you just sort them somehow? Maybe by d Done Line 78: { "255.255.255.250/16", false}, //$NON-NLS-1$ Line 79: { "192.0.0.0/1", false}, //$NON-NLS-1$ Line 80: { "224.0.0.0/2", false}, //$NON-NLS-1$ Line 81: { "240.255.255.247/3", false}, //$NON-NLS-1$ Line 97: { "255.252.0.0/14", true}, //$NON-NLS-1$ Line 98: { "255.0.0.0/8", true}, //$NON-NLS-1$ Line 99: { "248.0.0.0/5", true}, //$NON-NLS-1$ Line 100: { "128.0.0.0/1", true}, //$NON-NLS-1$ Line 101: { "255.0.254.0/23", true}, //$NON-NLS-1$ > I would put this next to the other one with 23 prefix, to have tests in ord Done Line 102: { "255.255.255.254/31", true}, //$NON-NLS-1$ Line 103: { "255.255.255.248/29", true}, //$NON-NLS-1$ Line 104: { "255.255.0.0/16", true} //$NON-NLS-1$ Line 105: }); Line 98: { "255.0.0.0/8", true}, //$NON-NLS-1$ Line 99: { "248.0.0.0/5", true}, //$NON-NLS-1$ Line 100: { "128.0.0.0/1", true}, //$NON-NLS-1$ Line 101: { "255.0.254.0/23", true}, //$NON-NLS-1$ Line 102: { "255.255.255.254/31", true}, //$NON-NLS-1$ > This is a duplicate, it already appears above. Done Line 103: { "255.255.255.248/29", true}, //$NON-NLS-1$ Line 104: { "255.255.0.0/16", true} //$NON-NLS-1$ Line 105: }); Line 106: } Line 99: { "248.0.0.0/5", true}, //$NON-NLS-1$ Line 100: { "128.0.0.0/1", true}, //$NON-NLS-1$ Line 101: { "255.0.254.0/23", true}, //$NON-NLS-1$ Line 102: { "255.255.255.254/31", true}, //$NON-NLS-1$ Line 103: { "255.255.255.248/29", true}, //$NON-NLS-1$ > I would put this higher up, between prefix 31 and 24, see other comment con Done Line 104: { "255.255.0.0/16", true} //$NON-NLS-1$ Line 105: }); Line 106: } Line 107: Line 100: { "128.0.0.0/1", true}, //$NON-NLS-1$ Line 101: { "255.0.254.0/23", true}, //$NON-NLS-1$ Line 102: { "255.255.255.254/31", true}, //$NON-NLS-1$ Line 103: { "255.255.255.248/29", true}, //$NON-NLS-1$ Line 104: { "255.255.0.0/16", true} //$NON-NLS-1$ > This is another duplicate. Done Line 105: }); Line 106: } Line 107: -- 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: 8 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
