Eliraz Levi has posted comments on this change. Change subject: webadmin: allowing prefix as mask staticIP conf ......................................................................
Patch Set 12: (6 comments) http://gerrit.ovirt.org/#/c/35145/12/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidation.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidation.java: Line 21: return isPrefixAllowed ? getBadPrefixFormatMessage() : getBadNetmaskFormatMessage(); Line 22: Line 23: } Line 24: Line 25: protected String getBadPrefixFormatMessage() { > Naming isn't great, as this is the error for bad format of either prefix or Done Line 26: return ConstantsManager.getInstance() Line 27: .getConstants() Line 28: .thisFieldMustContainValidPrefix(); Line 29: } Line 24: Line 25: protected String getBadPrefixFormatMessage() { Line 26: return ConstantsManager.getInstance() Line 27: .getConstants() Line 28: .thisFieldMustContainValidPrefix(); > Same comment. Done Line 29: } Line 30: Line 31: protected String getBadNetmaskFormatMessage() { Line 32: return ConstantsManager.getInstance() Line 42: public ValidationResult validate(Object value) { Line 43: assert value == null || value instanceof String : "This validation must be run on a String!";//$NON-NLS-1$ Line 44: String mask = (String) value; Line 45: Line 46: if (isPrefixAllowed) { > This flow can be simplified: Done Line 47: if (!(MaskValidator.getInstance().isPrefixValid(mask) || (MaskValidator.getInstance() Line 48: .isValidNetmaskFormat(mask) && MaskValidator.getInstance().isNetmaskValid( Line 49: mask)))) { Line 50: return failWith(getBadPrefixFormatMessage()); http://gerrit.ovirt.org/#/c/35145/12/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidationTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/SubnetMaskValidationTest.java: Line 18: Line 19: private SubnetMaskValidation validation; Line 20: Line 21: private final String mask; Line 22: private final boolean isMaskValidFormat; > When testing SubnetMaskValidation, you shouldn't differentiate between diff Done Line 23: private final boolean isMaskValid; Line 24: private final boolean isPrefixAllowed; Line 25: Line 26: public SubnetMaskValidationTest(String mask, boolean isMaskValidFormat, boolean isMaskValid, boolean isPrefixAllowed) { Line 43: public void checkValidMask() Line 44: { Line 45: assertEquals("Failed to validate mask: " + mask, isMaskValid, validation.validate(mask).getSuccess());//$NON-NLS-1$ Line 46: if (!isMaskValid) { Line 47: setupErrorMessage(); > See above comment, error mapping validation should be a different test. Done Line 48: } Line 49: Line 50: } Line 51: http://gerrit.ovirt.org/#/c/35145/12/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java: Line 2558: Line 2559: @DefaultStringValue("IP") Line 2560: String ipHostPopup(); Line 2561: Line 2562: @DefaultStringValue("Mask\\Prefix") > 1. Mask and prefix are too general terms. no space Netmask/Routing Prefix Line 2563: String subnetMaskHostPopup(); Line 2564: Line 2565: @DefaultStringValue("Gateway") Line 2566: String gwHostPopup(); -- To view, visit http://gerrit.ovirt.org/35145 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If909eda53a61bda5030c342156a01e53576e77cb Gerrit-PatchSet: 12 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
