Add status attribute + other minor fixes #108
Conversation
Remove status which will be added in another PR.
There is no way to indicate if follow-up action is needed for a particular customer, which is usually the case with financial services. Adding the status attribute helps the user differentiate between clients who may need follow-up action, and works in tandem with remark, where case details can be inserted. Other small changes include: - Refactoring `Person::isSamePerson` method to use name, phone and email as checks for equality. - Refactoring `Person::equals` method to use `Person::isSamePerson` method when checking for equality, which makes the code more maintainable as equality logic is only specified once.
ZShunRen
requested review from
FionaQY,
iamdiluxedbutcooler,
itsme-zeix and
colinhia
and removed request for
iamdiluxedbutcooler
| @@ -39,7 +40,9 @@ public class PersonCard extends UiPart<Region> { | |||
| @FXML | |||
| private Label remark; | |||
| @FXML | |||
| private FlowPane assignedTier; | |||
| private FlowPane assignedStatus; | |||
There was a problem hiding this comment.
Is there a need for both assignedStatus and assignedStatusAndTier ?
There was a problem hiding this comment.
Thank you for spotting this, I have amended it accordingly.
| * Returns true if a given string is a valid status value. | ||
| * | ||
| * @param test The string to be tested | ||
| */ |
There was a problem hiding this comment.
Would be better to have @return tag in the Javadoc comment and explaination of what boolean return value means :D
There was a problem hiding this comment.
Hi I have added this, thank you pleng.
| } | ||
|
|
||
| /** | ||
| * Represent what values Status can take. |
There was a problem hiding this comment.
Consider moving the enum to its own file if it might need to be referenced independently 👍🏻 This can be applied to other enums in our project too!
There was a problem hiding this comment.
I think for now since the enum is related to Status, we can keep it here, but we can possibly refactor the enums to CommandCommons to reduce coupling in the future.
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { |
There was a problem hiding this comment.
Good that you don't forget to override hash code! :D
| /** | ||
| * Converts a given {@code Status} into a {@code JsonAdaptedStatus} for Jackson use. | ||
| */ | ||
| public JsonAdaptedStatus(Status source) { |
There was a problem hiding this comment.
Consider adding null check for source parameter!
There was a problem hiding this comment.
Hi, will add the requirenonnull check!
| Person validPerson = new PersonBuilder().withTier("GOLD").withRemark("big brained").build(); | ||
| public void execute_newPersonWithTierAndRemarkAndStatus_success() { | ||
| Person validPerson = | ||
| new PersonBuilder().withTier("GOLD").withRemark("big brained").withStatus("urgent").build(); |
There was a problem hiding this comment.
Good that you not forget to test the status attributes in person!
There is no way to indicate if follow-up action is needed for a
particular customer, which is usually the case with financial services.
Adding the status attribute helps the user differentiate between clients
who may need follow-up action, and works in tandem with remark, where
case details can be inserted.
Other small changes include:
Refactoring
Person::isSamePersonmethod to use name, phone and email aschecks for equality.
Refactoring
Person::equalsmethod to usePerson::isSamePersonmethod when checking for equality, which makes the code more
maintainable as equality logic is only specified once.
Added more tests into
AddCommandParser.javato test the new flags like income, etc.Refactoring one of the tests in
AddCommandParserTestwhich used a normal set, which did not preserve the order of duplicates, to a linked set which preserves order.Closes Add status attribute #116.