Add feature to indicate matriculation year of band member #44

brennalaurentan · 2024-03-27T09:06:44Z

No description provided.

src/test/java/seedu/address/storage/JsonAdaptedPersonTest.java

casaarlai · 2024-03-27T13:56:21Z

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

@@ -5,6 +5,7 @@
 import static seedu.address.logic.parser.CliSyntax.PREFIX_ATTENDANCE;
 import static seedu.address.logic.parser.CliSyntax.PREFIX_BIRTHDAY_DATE;
 import static seedu.address.logic.parser.CliSyntax.PREFIX_EMAIL;
+import static seedu.address.logic.parser.CliSyntax.PREFIX_MATRICULATIONYEAR;


Would it be too much work to make it PREFIX_MATRICULATION_YEAR? In order to follow the existing format of having each word separated with an underscore.

Or is there any reason you chose not to separate matriculation and year?

Thanks for the suggestion! I have updated the code accordingly with the suggested changes.

casaarlai · 2024-03-27T13:56:51Z

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

@@ -22,7 +22,7 @@ public class BirthdayCommand extends Command {

    public static final String MESSAGE_ARGUMENTS = "Index: %1$d, Birthday: %2$s";
    public static final String MESSAGE_USAGE = COMMAND_WORD
-            + ": Edits the remark of the person identified "
+            + ": Edits the birthday of the person identified "


Thanks for taking note and updating this!

triciiaaa

Other than a few naming standard related nits, once you pass CI, LGTM!

triciiaaa · 2024-03-28T06:22:39Z

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

+
+/**
+ * Represents a Person's matriculation year in the address book.
+ * Guarantees: immutable; is always valid


Perhaps you could elaborate how valid is defined?

Thanks for the suggestion! I have updated the code accordingly with the suggested changes.

triciiaaa · 2024-03-28T06:28:49Z

src/test/java/seedu/address/testutil/TypicalPersons.java

@@ -56,10 +62,16 @@ public class TypicalPersons {
    // Manually added - Person's details found in {@code CommandTestUtil}
    public static final Person AMY = new PersonBuilder().withName(VALID_NAME_AMY).withPhone(VALID_PHONE_AMY)
            .withEmail(VALID_EMAIL_AMY).withAddress(VALID_ADDRESS_AMY).withBirthday(VALID_BIRTHDAY_AMY)
-            .withTags(VALID_TAG_FRIEND).build();
+            .withMatriculationYear(VALID_MATRICULATIONYEAR_AMY).withTags(VALID_TAG_FRIEND).build();


Perhaps you could update your variable names to comply with the coding standard?

Thanks for the suggestion! I have updated the code accordingly with the suggested changes.

# Conflicts: # src/main/java/seedu/address/logic/Messages.java # src/main/java/seedu/address/logic/commands/AddCommand.java # src/main/java/seedu/address/logic/commands/AttendanceCommand.java # src/main/java/seedu/address/logic/commands/AttendanceDeleteCommand.java # src/main/java/seedu/address/logic/commands/BirthdayCommand.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/AddressBookParser.java # src/main/java/seedu/address/logic/parser/CliSyntax.java # src/main/java/seedu/address/logic/parser/EditCommandParser.java # src/main/java/seedu/address/logic/parser/ParserUtil.java # src/main/java/seedu/address/model/person/Person.java # src/main/java/seedu/address/model/util/SampleDataUtil.java # src/main/java/seedu/address/storage/JsonAdaptedPerson.java # src/main/java/seedu/address/ui/PersonCard.java # src/main/resources/view/PersonListCard.fxml # src/test/data/JsonSerializableAddressBookTest/duplicatePersonAddressBook.json # src/test/data/JsonSerializableAddressBookTest/typicalPersonsAddressBook.json # src/test/java/seedu/address/logic/commands/CommandTestUtil.java # src/test/java/seedu/address/logic/commands/EditPersonDescriptorTest.java # src/test/java/seedu/address/logic/parser/AddCommandParserTest.java # src/test/java/seedu/address/model/person/PersonTest.java # src/test/java/seedu/address/storage/JsonAdaptedPersonTest.java # src/test/java/seedu/address/testutil/EditPersonDescriptorBuilder.java # src/test/java/seedu/address/testutil/PersonBuilder.java # src/test/java/seedu/address/testutil/PersonUtil.java # src/test/java/seedu/address/testutil/TypicalPersons.java

triciiaaa

Found some typos and missing fields to fix! Will check on the failed TCs again 👍

triciiaaa · 2024-03-29T02:55:10Z

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

@@ -37,6 +39,7 @@ public class AddCommand extends Command {
            + PREFIX_EMAIL + "johnd@example.com "
            + PREFIX_ADDRESS + "311, Clementi Ave 2, #02-25 "
            + PREFIX_BIRTHDAY_DATE + "2000-01-02 "
+            + PREFIX_MATRICULATIONYEAR + "2004"


Can add a space character after 2004 such that the formatting is consistent in the result display!

Current:

Example: add n/John Doe p/98765432 e/johnd@example.com a/311, Clementi Ave 2, #2-25 b/2000-01-02 my/2004i/Flute t/friends t/owesMoney

It should look like:

Example: add n/John Doe p/98765432 e/johnd@example.com a/311, Clementi Ave 2, #2-25 b/2000-01-02 my/2004 i/Flute t/friends t/owesMoney

Thanks for the suggestion, you're right! I have updated the code accordingly with the suggested changes.

triciiaaa · 2024-03-29T03:02:53Z

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

@@ -38,25 +40,30 @@ public class AddCommandParser implements Parser<AddCommand> {
    public AddCommand parse(String args) throws ParseException {
        ArgumentMultimap argMultimap =
                ArgumentTokenizer.tokenize(args, PREFIX_NAME, PREFIX_PHONE, PREFIX_EMAIL,
-                        PREFIX_ADDRESS, PREFIX_BIRTHDAY_DATE, PREFIX_INSTRUMENT, PREFIX_TAG);
+                        PREFIX_ADDRESS, PREFIX_BIRTHDAY_DATE, PREFIX_MATRICULATIONYEAR, PREFIX_BIRTHDAY_DATE,


There is a duplicate PREFIX_BIRTHDAY_DATE. You might need to change one of them to PREFIX_INSTRUMENT as it doesn't account for the instrument field currently.

Thanks for the suggestion, you're right! I have updated the code accordingly with the suggested changes.

triciiaaa · 2024-03-29T03:16:07Z

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

-        return Objects.hash(name, phone, email, address, birthday, instrument, tags, attendances);
+        return Objects.hash(name, phone, email, address, birthday, matriculationYear, tags, attendances);


Was instrument deleted accidentally here?

You're right! I have updated the code accordingly with the suggested changes.

triciiaaa · 2024-03-29T03:25:11Z

src/test/data/JsonSerializableAddressBookTest/typicalPersonsAddressBook.json

+    "matriculationYear": "0000",
    "birthday" : "2000-03-03",


Perhaps can swap the order of matriculationYear and birthday to ensure consistency.

Thanks for the suggestion, you're right! I have updated the code accordingly with the suggested changes.

triciiaaa

Managed to find the errors that caused some of the test cases to fail. Perhaps you can amend accordingly to the review comments and push your changes so that we can help to debug the rest too! 👍

triciiaaa · 2024-03-29T05:43:43Z

src/test/java/seedu/address/logic/parser/AddCommandParserTest.java

+    //    @Test
+    //    public void parse_optionalFieldsMissing_success() {
+    //
+    //        // zero tags
+    //        Person expectedPersonNoTag = new PersonBuilder(AMY).withTags().build();
+    //        assertParseSuccess(parser, NAME_DESC_AMY + PHONE_DESC_AMY + EMAIL_DESC_AMY + ADDRESS_DESC_AMY
+    //                + BIRTHDAY_DESC_AMY + MATRICULATIONYEAR_DESC_AMY, new AddCommand(expectedPersonNoTag));
+    //
+    //        // zero birthday
+    //        Person expectedPersonNoBirthday = new PersonBuilder(AMY).withBirthday().build();
+    //        assertParseSuccess(parser, NAME_DESC_AMY + PHONE_DESC_AMY + EMAIL_DESC_AMY + ADDRESS_DESC_AMY
+    //                + MATRICULATIONYEAR_DESC_AMY + VALID_TAG_FRIEND, new AddCommand(expectedPersonNoBirthday));
+    //
+    //        // zero matriculation year
+    //        Person expectedPersonNoMatriculationYear = new PersonBuilder(AMY).withMatriculationYear().build();
+    //        assertParseSuccess(parser, NAME_DESC_AMY + PHONE_DESC_AMY + EMAIL_DESC_AMY + ADDRESS_DESC_AMY
+    //                + BIRTHDAY_DESC_AMY + VALID_TAG_FRIEND, new AddCommand(expectedPersonNoMatriculationYear));
+    //    }
+
+    //    @Test
+    //    public void parse_optionalFieldsMissing_success() {
+    //
+    //        // zero tags
+    //        Person expectedPersonNoTag = new PersonBuilder(AMY).withTags().build();
+    //        assertParseSuccess(parser, NAME_DESC_AMY + PHONE_DESC_AMY + EMAIL_DESC_AMY + ADDRESS_DESC_AMY
+    //                + BIRTHDAY_DESC_AMY + INSTRUMENT_DESC_AMY, new AddCommand(expectedPersonNoTag));
+    //
+    //        // zero birthday
+    //        Person expectedPersonNoBirthday = new PersonBuilder(AMY).withTags(VALID_TAG_FRIEND)
+    //                .withBirthday(DEFAULT_BIRTHDAY).build();
+    //        assertParseSuccess(parser, NAME_DESC_AMY + PHONE_DESC_AMY + EMAIL_DESC_AMY + ADDRESS_DESC_AMY
+    //                + INSTRUMENT_DESC_AMY + TAG_DESC_FRIEND, new AddCommand(expectedPersonNoBirthday));
+    //
+    //        // zero instrument
+    //        Person expectedPersonNoInstrument = new PersonBuilder(AMY).withTags(VALID_TAG_FRIEND)
+    //                .withInstrument(DEFAULT_INSTRUMENT).build();
+    //        assertParseSuccess(parser, NAME_DESC_AMY + PHONE_DESC_AMY + EMAIL_DESC_AMY + ADDRESS_DESC_AMY
+    //                + BIRTHDAY_DESC_AMY + TAG_DESC_FRIEND, new AddCommand(expectedPersonNoInstrument));
+    //    }


Should there be only one parse_optionalFieldsMissing_success()? Perhaps can include the test for zero matriculation year together with the other test in the duplicate function?

I think the assertParseSuccess arguments should match the person details in TypicalPerson.java. Perhaps @casaarlai can help to double-check on this!

For example, in TypicalPerson.java,

public static final Person AMY = new PersonBuilder().withName(VALID_NAME_AMY).withPhone(VALID_PHONE_AMY) .withEmail(VALID_EMAIL_AMY).withAddress(VALID_ADDRESS_AMY).withBirthday(VALID_BIRTHDAY_AMY) .withMatriculationYear(VALID_MATRICULATIONYEAR_AMY).withInstrument(VALID_INSTRUMENT_AMY) .withTags(VALID_TAG_FRIEND).build();

Hence, in AddCommandParserTest.java,

@Test public void parse_optionalFieldsMissing_success() { // zero tags Person expectedPersonNoTag = new PersonBuilder(AMY).withTags().build(); assertParseSuccess(parser, NAME_DESC_AMY + PHONE_DESC_AMY + EMAIL_DESC_AMY + ADDRESS_DESC_AMY + BIRTHDAY_DESC_AMY + MATRICULATIONYEAR_DESC_AMY + INSTRUMENT_DESC_AMY, new AddCommand(expectedPersonNoTag)); ...

Can apply the same approach for the other tests if similar errors persist!

Works, thanks for suggesting this fix!

triciiaaa · 2024-03-29T06:03:37Z

src/test/java/seedu/address/model/person/PersonTest.java

+    //                + ALICE.getPhone()
+    //                + ", email=" + ALICE.getEmail() + ", address=" + ALICE.getAddress()
+    //                + ", birthday=" + ALICE.getBirthday() + ", matriculation year=" + ALICE.getMatriculationYear()
+    //                + ", instrument= " + ALICE.getInstrument() + ", tags=" + ALICE.getTags()


I think this test case failed due to the additional space behind ", instrument=". Perhaps you can remove the extra space character and run the tests again.

Works, thanks for suggesting this fix!

triciiaaa

Thanks for working on the changes! LGTM 👍

brennalaurentan added 2 commits March 27, 2024 02:03

Update gradle build settings to enable assertions

eee09d8

Add feature to indicate matriculation year of band member

48ca357

brennalaurentan added this to the v1.3 milestone Mar 27, 2024

brennalaurentan requested review from casaarlai and triciiaaa March 27, 2024 09:06

Refactor some tests for matriculation year feature

f4b864f

casaarlai reviewed Mar 27, 2024

View reviewed changes

src/test/java/seedu/address/storage/JsonAdaptedPersonTest.java Outdated Show resolved Hide resolved

casaarlai reviewed Mar 27, 2024

View reviewed changes

Refactor tests for matriculation year feature

a646e5e

triciiaaa reviewed Mar 28, 2024

View reviewed changes

brennalaurentan added 4 commits March 29, 2024 01:02

Refactor tests for matriculation year feature

b0f2fa3

Merge remote-tracking branch 'upstream/master'

d159851

Merge with delete attendance functionality, fix merge conflicts

bd34119

triciiaaa reviewed Mar 29, 2024

View reviewed changes

brennalaurentan added 2 commits March 30, 2024 01:14

Refactor tests for matriculation year functionality

d538540

Fix remaining tests which previously did not pass

bdab7de

triciiaaa approved these changes Mar 30, 2024

View reviewed changes

triciiaaa merged commit bcc39c5 into AY2324S2-CS2103T-T15-3:master Mar 30, 2024
3 checks passed

brennalaurentan linked an issue Apr 1, 2024 that may be closed by this pull request

As a user I can indicate matriculation year information #22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feature to indicate matriculation year of band member #44

Add feature to indicate matriculation year of band member #44

brennalaurentan commented Mar 27, 2024

casaarlai Mar 27, 2024

brennalaurentan Mar 29, 2024

casaarlai Mar 27, 2024

triciiaaa left a comment

triciiaaa Mar 28, 2024

brennalaurentan Mar 29, 2024

triciiaaa Mar 28, 2024

brennalaurentan Mar 29, 2024

triciiaaa left a comment

triciiaaa Mar 29, 2024

brennalaurentan Mar 29, 2024 •

edited

Loading

triciiaaa Mar 29, 2024

brennalaurentan Mar 29, 2024 •

edited

Loading

triciiaaa Mar 29, 2024

brennalaurentan Mar 29, 2024 •

edited

Loading

triciiaaa Mar 29, 2024

brennalaurentan Mar 29, 2024

triciiaaa left a comment

triciiaaa Mar 29, 2024

brennalaurentan Mar 29, 2024

triciiaaa Mar 29, 2024

brennalaurentan Mar 29, 2024

triciiaaa left a comment

		return Objects.hash(name, phone, email, address, birthday, instrument, tags, attendances);
		return Objects.hash(name, phone, email, address, birthday, matriculationYear, tags, attendances);

Add feature to indicate matriculation year of band member #44

Add feature to indicate matriculation year of band member #44

Conversation

brennalaurentan commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triciiaaa 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

triciiaaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brennalaurentan Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brennalaurentan Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brennalaurentan Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triciiaaa 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

triciiaaa left a comment

Choose a reason for hiding this comment

brennalaurentan Mar 29, 2024 •

edited

Loading

brennalaurentan Mar 29, 2024 •

edited

Loading

brennalaurentan Mar 29, 2024 •

edited

Loading