Modify existing feature: add member details (add) #46

ryanongra · 2021-10-10T13:48:34Z

Modified add command to take telegram handles instead of address
- /t tag now recognised as telegram handle
- /tag used for other tags
- automatically adds '@' character is not already preceded by it
Attempted to modify the validation regex to disallow spaces but to no avail :(, to be done in v1.2b
Making tags other than the name optional may take more work as the current implementation requires these parameters to be non-null

Closes #33

yongxiangng

Understandably, this renaming of address to telegram is rather problematic and I apologise for many repeated comments on the same issue (mostly just to mark and flag the issues so we can check if it has been resolved later on)

Here are some common issues I found:

Wild card imports
Address book accidentally refactored into telegram book
Forgetting to rename address value to telegram handle value. e.g. there are still parts where telegram handles are filled with the String value "4th street"
Error messages not renamed and the value not changed
address.png not updated to telegram.png (also change the picture to updated ui)
Several methods name are still not updated and the parameters of certain methods are not updated as well. e.g. setAddress

There might be more things I missed out that I didn't flag out in the review, please be careful and meticulous when refactoring. Once again, I understand this thing is quite a pain and there are many things to change and hard to catch, so it would be best if we all tried to catch the loose ends (5 pairs of eyes are better than 1 pair 👀 👀 👀 👀 👀 )

Please build and run tests before pushing into the master to ensure that you pass the CI. Note the team repo only does checkstyle and tests and doesn't track codecov in the ci.

yongxiangng · 2021-10-11T03:31:34Z

docs/tutorials/RemovingFields.md

-![$address](../images/remove/$address.png)
+![$telegram](../images/remove/$telegram.png)


I don't think you renamed the address.png to telegram.png
Since you're on this, perhaps you can change the address.png file to telegram.png and use this image that has the updated ui mockup

yongxiangng · 2021-10-11T03:31:59Z

docs/tutorials/RemovingFields.md

-    "address": "4th street"
+    "telegram": "4th street"


Needs update

yongxiangng · 2021-10-11T03:33:13Z

docs/tutorials/TracingCode.md

-    3.  Save the address book in the CSV format instead
+    3.  Save the telegram book in the CSV format instead


Be careful not to accidentally refactor address book into telegram book. We should probably rename address book to ForYourInterest some time soon.

yongxiangng · 2021-10-11T03:33:18Z

docs/tutorials/TracingCode.md

-    6.  Add a new entity to the address book
+    6.  Add a new entity to the telegram book


Be careful not to accidentally refactor address book into telegram book

yongxiangng · 2021-10-11T03:34:14Z

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

+        public void setAddress(Telegram telegram) {
+            this.telegram = telegram;


Please refactor the name of this method as well to something like setTelegram

yongxiangng · 2021-10-11T03:50:08Z

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

+import seedu.address.model.person.*;
+import seedu.address.model.person.Telegram;


Wild card import

yongxiangng · 2021-10-11T03:50:20Z

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

+import seedu.address.model.person.*;
+import seedu.address.model.person.Telegram;


yongxiangng · 2021-10-11T03:50:43Z

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

-        address = new Address(DEFAULT_ADDRESS);
+        telegram = new Telegram(DEFAULT_ADDRESS);


DEFAULT_ADDRESS to DEFAULT_TELEGRAM

yongxiangng · 2021-10-11T03:50:51Z

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

@@ -45,7 +42,7 @@ public PersonBuilder(Person personToCopy) {
        name = personToCopy.getName();
        phone = personToCopy.getPhone();
        email = personToCopy.getEmail();
-        address = personToCopy.getAddress();
+        telegram = personToCopy.getAddress();


getTelegram()

yongxiangng · 2021-10-11T03:51:38Z

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

    public PersonBuilder withAddress(String address) {
-        this.address = new Address(address);
+        this.telegram = new Telegram(address);


Update the method name and parameter name. withTelegram, String telegram, this.telegram = new Telegram(telegram)

ryanongra · 2021-10-11T13:57:27Z

Understandably, this renaming of address to telegram is rather problematic and I apologise for many repeated comments on the same issue (mostly just to mark and flag the issues so we can check if it has been resolved later on)

Here are some common issues I found:

Wild card imports

Address book accidentally refactored into telegram book

Forgetting to rename address value to telegram handle value. e.g. there are still parts where telegram handles are filled with the String value "4th street"

Error messages not renamed and the value not changed

address.png not updated to telegram.png (also change the picture to updated ui)

Several methods name are still not updated and the parameters of certain methods are not updated as well. e.g. setAddress

There might be more things I missed out that I didn't flag out in the review, please be careful and meticulous when refactoring. Once again, I understand this thing is quite a pain and there are many things to change and hard to catch, so it would be best if we all tried to catch the loose ends (5 pairs of eyes are better than 1 pair 👀 👀 👀 👀 👀 )

Please build and run tests before pushing into the master to ensure that you pass the CI. Note the team repo only does checkstyle and tests and doesn't track codecov in the ci.

@yongxiangng
Thank you so much for finding all these mistakes 🙏 Sorry for the trouble, I think because I removed some parts when refactoring since a lot of the 'address' keywords are actually part of AddressBook or other libraries which shouldn't be changed, but I removed too many in the end :"). I'll update the PR and run the tests asap.

Fix issue for refactoring address -> telegram

Modify existing feature: add member details (add)

c5c3bc1

ryanongra added the enhancement New feature or request label Oct 10, 2021

yongxiangng reviewed Oct 11, 2021

View reviewed changes

yongxiangng added this to the v1.2 milestone Oct 11, 2021

yongxiangng added the type.Story A user story label Oct 11, 2021

ryanongra added 8 commits October 12, 2021 22:51

Fix refactor

38529eb

Fix issue for refactoring address -> telegram

Fix refactor

09054d9

Fix Failed Checks

2fac5b4

Fix checks

e189d61

Fix checks

f7fbef7

Update JsonAdaptedPerson.java

2a01c04

Fix checkstyle

0ef9be2

Update typicalPersonsAddressBook.json

8136db8

dannytayjy merged commit 4663fbd into AY2122S1-CS2103-T16-4:master Oct 12, 2021

yongxiangng mentioned this pull request Oct 30, 2021

[PE-D] The 4th line in the MemberCard should be Telegram accrount, but now it is showing address #112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify existing feature: add member details (add) #46

Modify existing feature: add member details (add) #46

ryanongra commented Oct 10, 2021

yongxiangng left a comment

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

yongxiangng Oct 11, 2021

ryanongra commented Oct 11, 2021

		![$address](../images/remove/$address.png)
		![$telegram](../images/remove/$telegram.png)

		3. Save the address book in the CSV format instead
		3. Save the telegram book in the CSV format instead

		6. Add a new entity to the address book
		6. Add a new entity to the telegram book

		public void setAddress(Telegram telegram) {
		this.telegram = telegram;

		import seedu.address.model.person.*;
		import seedu.address.model.person.Telegram;

		address = new Address(DEFAULT_ADDRESS);
		telegram = new Telegram(DEFAULT_ADDRESS);

Modify existing feature: add member details (add) #46

Modify existing feature: add member details (add) #46

Conversation

ryanongra commented Oct 10, 2021

yongxiangng 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

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

Choose a reason for hiding this comment

ryanongra commented Oct 11, 2021