Branch input validation #75

chowyiyin · 2019-10-05T04:02:33Z

Added policy class and new fields to person.
Added addpolicycommand.
UI can now display new person fields.
Tests are incomplete (6 still failing).

Remove policy field when adding new person

To do: should remove policy prefix

Change ListCommand to ListPeopleCommand

Not finished

…from the edit command

To do: should remove policy prefix

Change ListCommand to ListPeopleCommand

Not finished

…aptedPolicy

…n/main into branch-input-validation # Conflicts: # docs/UserGuide.adoc # src/main/java/seedu/address/logic/commands/AddPolicyCommand.java # src/main/java/seedu/address/logic/commands/EditCommand.java # src/main/java/seedu/address/logic/parser/AddCommandParser.java # src/main/java/seedu/address/logic/parser/AddPolicyCommandParser.java # src/main/java/seedu/address/logic/parser/AddressBookParser.java # src/main/java/seedu/address/logic/parser/ParserUtil.java # src/main/java/seedu/address/model/Model.java # src/main/java/seedu/address/model/person/DateOfBirth.java # src/main/java/seedu/address/model/person/Nric.java # src/main/java/seedu/address/model/person/Person.java # src/main/java/seedu/address/model/policy/Coverage.java # src/main/java/seedu/address/model/policy/EndAge.java # src/main/java/seedu/address/model/policy/Policy.java # src/main/java/seedu/address/model/policy/Price.java # src/main/java/seedu/address/model/policy/StartAge.java # src/main/java/seedu/address/model/policy/UniquePolicyList.java # src/main/java/seedu/address/model/policy/exceptions/DuplicatePolicyException.java # src/main/java/seedu/address/model/util/SampleDataUtil.java # src/main/java/seedu/address/storage/JsonAdaptedPerson.java # src/main/java/seedu/address/storage/JsonAdaptedPolicy.java

…ail is invalid

chaitanyabaranwal · 2019-10-06T10:13:48Z

src/test/data/JsonSerializableAddressBookTest/typicalPersonsAddressBook.json

    "phone" : "98765432",
    "email" : "johnd@example.com",
    "address" : "311, Clementi Ave 2, #02-25",
-    "tagged" : [ "owesMoney", "friends" ]
+    "date of birth" : "12.12.1922",
+    "policies" : [ ["SeniorCare", "SeniorCare", "months/12", "$1000" ],


Extra square bracket

chaitanyabaranwal · 2019-10-06T10:14:31Z

src/test/data/JsonSerializableAddressBookTest/typicalPersonsAddressBook.json

    "phone" : "87652533",
    "email" : "cornelia@example.com",
    "address" : "10th street",
-    "tagged" : [ "friends" ]
+    "date of birth" : "14.2.2019",
+    "policies" : [ ["Infant Insurance", "Infant Insurance", "years/2", "$1500"] ],


Why is this wrapped under an extra set of square brackets?

chowyiyin · 2019-10-06T10:57:52Z

actually im not sure how json works so idk how to the policies are supposed to be converted to the jsonadaptedpolicy🤔

…

On Sun, Oct 6, 2019 at 6:14 PM Chaitanya Baranwal ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/test/data/JsonSerializableAddressBookTest/typicalPersonsAddressBook.json <#75 (comment)> : > "phone" : "87652533", "email" : ***@***.***", "address" : "10th street", - "tagged" : [ "friends" ] + "date of birth" : "14.2.2019", + "policies" : [ ["Infant Insurance", "Infant Insurance", "years/2", "$1500"] ], Why is this wrapped under an extra set of square brackets? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#75?email_source=notifications&email_token=AM4VMSZIU5B7URVJJ2RPV23QNG3APA5CNFSM4I5WGAL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHAILSQ#pullrequestreview-297829834>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AM4VMSYCXTLBSZRQI564EK3QNG3APANCNFSM4I5WGALQ> .

larrylawl · 2019-10-06T13:36:01Z

src/main/java/seedu/address/model/person/Nric.java

+public class Nric {
+
+    public static final String MESSAGE_CONSTRAINTS =
+            "This is not a valid Singapore Idenfication number.\n" +


Typo "Idenfication"

larrylawl · 2019-10-06T13:56:47Z

src/main/java/seedu/address/model/person/Phone.java

-            "Phone numbers should only contain numbers, and it should be at least 3 digits long";
-    public static final String VALIDATION_REGEX = "\\d{3,}";
+            "Phone numbers should only contain numbers, and it should be at least 3 digits long.\n";
+    public static final String VALIDATION_REGEX = "\\d{3,8}";
    public final String value;


Maybe we can consider this phone validation regex instead? I think it suits sg context better.

https://www.regextester.com/104145

larrylawl · 2019-10-06T14:04:46Z

wow this is alot of work! thanks so much yi yin! I didn't have much time to look through the code in detail, but I did smoke tests and they work fine (other than input validation for phone number).

i'd also suggest to add // TODO: ... in places which the team needs to look into further in the future (such as the tests which are failing), so that we don't forget about what exactly needs to be fixed.

larrylawl · 2019-10-06T18:16:49Z

src/main/java/seedu/address/storage/JsonAdaptedPolicy.java

+                             @JsonProperty("tagged") List<JsonAdaptedTag> tagged) {
+        this.name = name;
+        this.description = description;
+        this.coverage = coverage;


Yo i'm trying to fix the test cases.

Test case:toModelType_validPersonDetails_returnsPerson
In SampleDataUtil, new Coverage(" days/10 months/11 years/12")
However in the model, coverage takes in the argument 10 days 11 months 12 years, which leads to coverage being null as it is not able to parse the correct format using getCoverageBreakdown.

However, is coverage meant to be doing the parsing? cuz in sample data util, the arguments are the content itself.

ybchen97

Sup Yiyin, awesome work here! That's a monster of a pr, thanks for lifting this heavy load. I have left a few comments here and there, the ones with "Note:" are not directed towards you, they just there to note down some potential discrepancies, or raise up certain issues which we could discuss about as a group. Other than that there's one thing I want to mention:

I realised that our addperson command allows the user to add policies and tags when creating a new contact, but editperson does not allow that, which might be a little confusing for both the user and us. I was thinking that since we already have the commands assignpolicy, unassignpolicy, addtag, deletetag why not just separate the adding/editing of policies and tags entirely and leave that responsibility to the various commands mentioned. This means that addperson and editperson only touches the particulars of the contact, while anything else related to policies and tags will have their own helper commands like the ones raised above. This allows us and our users to draw a distinct line between things related to person, tags, and policies, which i think makes the user workflow simpler. The same goes for addpolicy and editpolicy.

I think we should discuss this, let us know what you and oli think during your meeting tonight! Thanks again :D

ybchen97 · 2019-10-07T09:07:38Z

src/main/java/seedu/address/logic/parser/CliSyntax.java

    public static final Prefix PREFIX_PHONE = new Prefix("p/");
    public static final Prefix PREFIX_EMAIL = new Prefix("e/");
    public static final Prefix PREFIX_ADDRESS = new Prefix("a/");
+    public static final Prefix PREFIX_DATE_OF_BIRTH = new Prefix("dob/");
+    public static final Prefix PREFIX_POLICY = new Prefix("pol/");
    public static final Prefix PREFIX_TAG = new Prefix("t/");



Small thing, but might want to add a comment here to denote policy prefixes

ybchen97 · 2019-10-07T09:19:01Z

src/main/java/seedu/address/logic/commands/AddPolicyCommand.java

+    public CommandResult execute(Model model) throws CommandException {
+        requireNonNull(model);
+
+        model.addPolicy(toAdd);
+        return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd));
+    }


Note:

Duplicate policy not yet implemented, also resulting in no CommandException being thrown (referencing to AddCommand.java)

ybchen97 · 2019-10-07T10:13:08Z

src/main/java/seedu/address/logic/commands/ListPeopleCommand.java

@@ -8,9 +8,9 @@
 /**
 * Lists all persons in the address book to the user.
 */
-public class ListCommand extends Command {
+public class ListPeopleCommand extends Command {


Note:

What purpose does this command actually serve to the user, since the main page already lists all the contacts in the addressbook?

ybchen97 · 2019-10-07T10:32:30Z

src/main/java/seedu/address/model/ModelManager.java

+    @Override
+    public void addPolicy(Policy policy) {
+        addressBook.addPolicy(policy);
+        updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS);


Note:

addPolicy should update policy list instead of person list?

Change duplicate person error

chowyiyin added 30 commits October 1, 2019 23:07

Change birthday to date of birth

906b3c9

Remove policy field when adding new person

Add prefixes for policy and NRIC, Date of Birth and Policy

3453717

To do: should remove policy prefix

Add AddCommandPolicy command

928424a

Change ListCommand to ListPeopleCommand

Rename ListCommand to ListPeopleCommand

7341d11

Add classes for Date of birth and NRIC

6d77702

Create policy class and classes for fields

45634f1

Add parser and command for adding policy

aeb3a5e

Add exceptions for policies

31b9f90

Edit classes that creates new Person to take in the additional fields

8cfa801

Add policy methods to model and model manager

7d4c87c

Add parsers for new fields for person and policy

f7f2196

Not finished

Create JsonAdaptedPolicy class

b64e121

Not finished

Add policies to addressbook

10c9441

Change b/BIRTHDAY to dob/DATE_OF_BIRTH and removed criteria and tags …

0e39109

…from the edit command

Add prefixes for policy and NRIC, Date of Birth and Policy

2da3ee8

To do: should remove policy prefix

Add AddCommandPolicy command

7661520

Change ListCommand to ListPeopleCommand

Rename ListCommand to ListPeopleCommand

3625047

Add classes for Date of birth and NRIC

5270c5f

Create policy class and classes for fields

fa0259e

Add parser and command for adding policy

88e43f7

Add exceptions for policies

c8cf768

Edit classes that creates new Person to take in the additional fields

ab71da0

Add policy methods to model and model manager

567d54f

Add parsers for new fields for person and policy

d61d4e4

Not finished

Create JsonAdaptedPolicy class

938a6bf

Not finished

Add policies to addressbook

d81130e

Update message usage for AddCommand and AddPolicyCommand

18f4b72

Edit validation regex

8094675

Edit minor errors in previous refactoring

a60682a

Change sample data to match new validation regex

039d6ad

chowyiyin added 18 commits October 4, 2019 16:08

Add policies to Person and add getter for criteria for Policy

f442199

Add JsonAdaptedPolicy to JsonAdaptedPerson and add criteria to JsonAd…

8747699

…aptedPolicy

Edit fxml to display NRIC, date of birth and policy names

c40833c

Add PolicyCard class to handle display of policies

39a69e8

Remove johndoe.adoc

8e6d738

Edit to follow coding standards

0b0f476

Display policy name instead of all details of policy

5676c8c

Edit typicalPersonsAddressBook.json to include new fields

81bdee8

Edit tests

ecc01ec

Edit equals method of Date Of Birth

52e8b0b

Remove tests involving editing of Person's tags

777f3d2

Add nric, date of birth and policies oto get PersonDetail(Person) method

8eb8f81

Edit tests to make tests pass

0b409e3

Add some missing information

d375bf2

Fix AddPolicyCommand

e7b9564

Fix coverage parsing

8b355fc

Display valid phone number and/or email when nric, phone number or em…

212ceb1

…ail is invalid

chaitanyabaranwal reviewed Oct 6, 2019

View reviewed changes

larrylawl reviewed Oct 6, 2019

View reviewed changes

ybchen97 reviewed Oct 7, 2019

View reviewed changes

Change phone regex and changed test cases to validate phone numbers

0ac5c74

Change duplicate person error

larrylawl merged commit 3088530 into AY1920S1-CS2103-F09-4:master Oct 8, 2019

nus-se-bot mentioned this pull request Oct 10, 2019

Feedback on week 8 project progress for CS2103-F09-4 #85

Closed

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Branch input validation #75

Branch input validation #75

chowyiyin commented Oct 5, 2019

chaitanyabaranwal Oct 6, 2019

chaitanyabaranwal Oct 6, 2019

chowyiyin commented Oct 6, 2019 via email

larrylawl Oct 6, 2019

larrylawl Oct 6, 2019

larrylawl commented Oct 6, 2019 •

edited

larrylawl Oct 6, 2019

ybchen97 left a comment

ybchen97 Oct 7, 2019

ybchen97 Oct 7, 2019

ybchen97 Oct 7, 2019

ybchen97 Oct 7, 2019

Branch input validation #75

Branch input validation #75

Conversation

chowyiyin commented Oct 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chowyiyin commented Oct 6, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larrylawl commented Oct 6, 2019 • edited

Choose a reason for hiding this comment

ybchen97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larrylawl commented Oct 6, 2019 •

edited