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

Add income field logic + few minor changes #50

Merged
merged 7 commits into from
Oct 10, 2024

Conversation

ZShunRen
Copy link

@ZShunRen ZShunRen commented Oct 9, 2024

Closes #45.

There is no current field to represent the income of a saved contact.
This information is important to know when serving customers.

Creating this field allows for contacts to have an associated income
level.

As of v1.2, the `add` and `edit` commands can be used to add a new
contact with the income field being mandatory, and edit an existing
contact's income level accordingly.
Tests broke because of the new definition of person and its constructor.
In addition, changes to equality logic broke some existing tests.

Adding income as a parameter fixes the tests.
Adjusting the equality logic tests, such that a person is equal to
another person if their
- name
- email
- address
are equal.

We will need to discuss criteria for two persons being equal to each
other and adjusting our test logic accordingly.
@ZShunRen ZShunRen added the priority.High Must do label Oct 9, 2024
@ZShunRen ZShunRen added this to the v1.2 milestone Oct 9, 2024
Copy link

@itsme-zeix itsme-zeix left a comment

Choose a reason for hiding this comment

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

Great work, LGTM!

For future reference, could we either highlight the minor changes in the description or submit them in a separate PR? This approach will help streamline the review process.

@ZShunRen ZShunRen merged commit 01ab152 into AY2425S1-CS2103T-T14-4:master Oct 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add income as an attribute for a person
3 participants