Lior Vernia has posted comments on this change. Change subject: webadmin: allowing prefix as mask staticIP conf ......................................................................
Patch Set 4: (4 comments) http://gerrit.ovirt.org/#/c/35145/4/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 isPrefixSupported ? ConstantsManager.getInstance() Line 22: .getConstants() Line 23: .thisFieldMustContainSubnetFormatAsNetmaskOrPrefixMsg() : ConstantsManager.getInstance() Line 24: .getConstants() Line 25: .thisFieldMustContainSubnetFormatAsNetmaskOrPrefixFreeMsg(); Again with the "PrefixFree", why not just keep it as "thisFieldMustContainSubnetFormatAsNetmask"? Line 26: Line 27: } Line 28: Line 29: protected String getInvalidNetmaskMessage() { Line 36: String mask = (String) value; Line 37: boolean isValidFormat = Line 38: isPrefixSupported ? MaskValidator.getInstance().isMaskFormatValid(mask) : MaskValidator.getInstance() Line 39: .isMaskValidFormatPrefixFree(mask); Line 40: boolean isValidValue = You should only call this in case the format is valid. That's why you can drop the format validation in the value validation. Moreover, you only need to call this if the format is a valid netmask, not if it's a valid prefix. Line 41: isPrefixSupported ? MaskValidator.getInstance().isMaskValid(mask) : MaskValidator.getInstance() Line 42: .isMaskValidPrefixFree(mask); Line 43: if (!isValidFormat) { Line 44: return failWith(getSubnetBadFormatMessage()); http://gerrit.ovirt.org/#/c/35145/4/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 897 Line 898 Line 899 Line 900 Line 901 So basically you still need this exact error message, and there's no need to touch it at all. Line 896: Line 897: @DefaultStringValue("This field can be empty or contain an IP address in format xxx.xxx.xxx.xxx") Line 898: String emptyOrValidIPaddressInFormatMsg(); Line 899: Line 900: @DefaultStringValue("This field must contain a subnet in either of the following formats:\n\txxx.xxx.xxx.xxx where xxx is between 0 and 255\n\txx where xx is between 0-32 (/ prefix is optional, etc. /15 ).") You don't have to mention the optional '/' character in an error message. You've already told them how to insert a valid prefix, no need to confuse them with another optional character. People who use it will just not receive an error message. Line 901: String thisFieldMustContainSubnetFormatAsNetmaskOrPrefixMsg(); Line 902: Line 903: @DefaultStringValue("This field must contain a subnet of the following format:\n\txxx.xxx.xxx.xxx where xxx is between 0 and 255") Line 904: String thisFieldMustContainSubnetFormatAsNetmaskOrPrefixFreeMsg(); -- 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: 4 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
