Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Email validation #241

Merged
merged 15 commits into from
Apr 10, 2021
Merged

Conversation

lue97
Copy link
Collaborator

@lue97 lue97 commented Apr 9, 2021

Apparently the regex checks that came with AB3 is incorrect.

@lue97 lue97 mentioned this pull request Apr 9, 2021
@lue97 lue97 requested review from Assyarul and ivantjh and removed request for Assyarul April 9, 2021 13:43
@lue97 lue97 added this to the v1.4 milestone Apr 9, 2021
@lue97 lue97 requested a review from Assyarul April 9, 2021 13:44
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
contacts in FriendDex. A success message is shown in the status message.

3. Adding a duplicate person. <br>
* Prerequisite: A person with the name `john doe` must already be in FriendDex. <br>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add something here saying that the names are case-insensitive

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this point should belong in the UG

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think its better to state it here as the developer may not have looked at the UG.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a link to the particular section in the UG. Explaining the intricacies of a command shouldn't fall under test manual.

docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
**A**: Email stored in FriendDex is a wilful violation of (RFC 5322)[https://tools.ietf.org/html/rfc5322]. The email
allowed in FriendDex shall adhere to the following rules:
* Emails shall have the format `local-part@label(.label)*`.
* `local-part` contains at least one alphanumeric character with the following special characters <code>.!#$%&'*+\/=?^_`{|}~-</code>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be without?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with is correct

Copy link
Collaborator

@ivantjh ivantjh Apr 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can help reword to "local-part contains at least one alphanumeric character. local-part allows the following special characters .!#$%&'*+/=?^_`{|}~-.

docs/UserGuide.md Outdated Show resolved Hide resolved
private static final String SPECIAL_CHARACTERS = "!#$%&'*+/=?`{|}~^.-";
public static final String MESSAGE_CONSTRAINTS = "Emails should be of the format local-part@domain "
private static final String SPECIAL_CHARACTERS = ".!#$%&'*+/=?`{|}~^.-";
public static final String MESSAGE_CONSTRAINTS = "Emails should be of the format local-part@label(.label)*"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify to English for the format portion, nobody will understand the brackets and asterisk. Emails should be of the format "local-part@label". Multiple labels can follow after provided that they are prefixed with a single period.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that this expression is adequate. If the user doesn't get it he can just refer to point 3. I'll add an example in the usage message

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, up to you. But I still opt for longer explanations for better clarity than brevity.

src/test/java/seedu/address/model/person/EmailTest.java Outdated Show resolved Hide resolved
@lue97 lue97 requested a review from ivantjh April 9, 2021 15:49
Copy link
Collaborator

@ivantjh ivantjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@codecov-io
Copy link

codecov-io commented Apr 10, 2021

Codecov Report

Merging #241 (d7dc18f) into master (cc09c9e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #241   +/-   ##
=========================================
  Coverage     63.51%   63.51%           
  Complexity      800      800           
=========================================
  Files           135      135           
  Lines          3130     3130           
  Branches        348      348           
=========================================
  Hits           1988     1988           
  Misses          998      998           
  Partials        144      144           
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/seedu/address/model/person/Email.java 90.00% <ø> (ø) 7.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc09c9e...d7dc18f. Read the comment docs.

@lue97 lue97 merged commit e009031 into AY2021S2-CS2103T-W14-1:master Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants