Issue 1541:FieldValidator: Increase the rule for validation checking

Issue 1541:FieldValidator: Increase the rule for validation checking

[damith, 2014-01-25 06:49:17]

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
File src/main/java/teammates/common/util/FieldValidator.java (right):

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/util/FieldValidator.java:267: //Allows any space,
apostrophe, hyphen, comma, dot, plus, underscore, tilde, at mark, hash, dollar
sign, percent sign, ampersand, asterisk, slash, brackets
Do you have test cases to cover all these symbols?

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/util/FieldValidator.java:463: * the specified
length {@code maxLength}, and also does not contain invalid characters.
mention what is valid/invalid?

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/util/FieldValidator.java:478:
Assumption.assertTrue("\""+value+"\""+  "is expected to be trimmed.",
isTrimmed(value));
This assumption is not mentioned in the method comment. Again, it may be cheaper
to trim than to verify the assumption.

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/util/FieldValidator.java:734: return
Pattern.compile(REGEX_NAME, Pattern.CANON_EQ).matcher(value).matches();
Do we have test cases containing é etc?

[Oo Theong Siang, 2014-01-25 08:29:21]

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
File src/main/java/teammates/common/util/FieldValidator.java (right):

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/util/FieldValidator.java:267: //Allows any space,
apostrophe, hyphen, comma, dot, plus, underscore, tilde, at mark, hash, dollar
sign, percent sign, ampersand, asterisk, slash, brackets
Is this character restriction rule seem okay? Actually I have more restricted
rule initially, but some test cases elsewhere were broken, so i allowed more
symbols to be used.

There are a few key questions here:
i) How lenient should we go for name validation? I know that we should allow
more symbols as possible, but we are not sure whether certain
symbols/combination of character will cause trouble/harm to our system. It might
not be a wise choice to not validate at all, on other hand it is also hard to
think of what symbols might be used by users.. 
ii) Should we come up with separate rules for each type of name? (person name,
institute name, course name, team name, evaluation/feedback name) Since certain
name might have more strict/relax format.

On 2014/01/25 06:49:17, damith wrote:
> Do you have test cases to cover all these symbols?

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/util/FieldValidator.java:478:
Assumption.assertTrue("\""+value+"\""+  "is expected to be trimmed.",
isTrimmed(value));
For this actually i just follow the convention from other methods in this class.
Since these methods are supposed to check if user input are in correct format,
maybe we shouldn't trim them here? As the trimming has been done in Logic layer.

On 2014/01/25 06:49:17, damith wrote:
> This assumption is not mentioned in the method comment. Again, it may be
cheaper
> to trim than to verify the assumption.

[damith, 2014-01-25 09:11:26]

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
File src/main/java/teammates/common/util/FieldValidator.java (right):

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/util/FieldValidator.java:267: //Allows any space,
apostrophe, hyphen, comma, dot, plus, underscore, tilde, at mark, hash, dollar
sign, percent sign, ampersand, asterisk, slash, brackets
Yes it is ok. perhaps we can disallow | and % because they are used for special
purposes in our system, just to be on the safe side.
We can use the same policy for all names, for now.

https://codereview.appspot.com/56790043/diff/1/src/main/java/teammates/common...
src/main/java/teammates/common/util/FieldValidator.java:478:
Assumption.assertTrue("\""+value+"\""+  "is expected to be trimmed.",
isTrimmed(value));
We can leave it this for consistency. Add //TODO: reconsider this assumption.

[damith, 2014-01-26 00:06:42]

https://codereview.appspot.com/56790043/diff/70001/src/main/java/teammates/co...
File src/main/java/teammates/common/util/FieldValidator.java (right):

https://codereview.appspot.com/56790043/diff/70001/src/main/java/teammates/co...
src/main/java/teammates/common/util/FieldValidator.java:299: public static final
String REASON_START_WITH_NON_ALPHANUMERIC_CHAR= "is started with a
non-alphanumeric character";
starts with a ...

https://codereview.appspot.com/56790043/diff/70001/src/main/java/teammates/co...
src/main/java/teammates/common/util/FieldValidator.java:317: "All %s must be
started with an alphanumeri character, and cannot contain any vertical bar (|)
or percent sign (%%).";
... must start with ...
there is a typo:alphanumeri
... vertical bar (|) or percent (%%) signs.