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

Make link immutable #218

Merged
merged 22 commits into from
Nov 4, 2021
Merged

Make link immutable #218

merged 22 commits into from
Nov 4, 2021

Conversation

xiangjunn
Copy link
Collaborator

@xiangjunn xiangjunn commented Nov 2, 2021

  • Make link and unlink immutable
  • Shows error message if contact already linked/not linked to event
  • Fix cedit and eedit allowing the removal of a non-existing tag
  • Make link and unlink work with undo
  • Fix cedit and eedit allowing the adding of a already existing tag

@xiangjunn xiangjunn added type.Bug Something isn't working priority.High Should be resolved first, failing which, the project will not work severity.High A flaw that affects most users and causes major problems for users labels Nov 2, 2021
@xiangjunn xiangjunn added this to the v1.4 milestone Nov 2, 2021
@xiangjunn xiangjunn self-assigned this Nov 2, 2021
@xiangjunn xiangjunn added this to In progress in tP Project Dashboard via automation Nov 2, 2021
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #218 (bd4dc5b) into master (1e72508) will increase coverage by 0.47%.
The diff coverage is 89.93%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #218      +/-   ##
============================================
+ Coverage     68.94%   69.41%   +0.47%     
- Complexity     1004     1029      +25     
============================================
  Files           129      129              
  Lines          3355     3404      +49     
  Branches        433      450      +17     
============================================
+ Hits           2313     2363      +50     
+ Misses          857      849       -8     
- Partials        185      192       +7     
Impacted Files Coverage Δ
...rc/main/java/seedu/address/logic/LogicManager.java 51.51% <0.00%> (-1.43%) ⬇️
src/main/java/seedu/address/model/Model.java 100.00% <ø> (ø)
.../seedu/address/storage/JsonAddressBookStorage.java 100.00% <ø> (ø)
...u/address/storage/JsonSerializableAddressBook.java 84.61% <ø> (-0.57%) ⬇️
src/main/java/seedu/address/model/event/Event.java 79.59% <78.94%> (-2.13%) ⬇️
...edu/address/logic/commands/event/ELinkCommand.java 89.47% <87.50%> (-3.39%) ⬇️
...u/address/logic/commands/event/EUnlinkCommand.java 89.13% <87.50%> (-2.71%) ⬇️
...rc/main/java/seedu/address/model/ModelManager.java 79.51% <90.90%> (-1.13%) ⬇️
src/main/java/seedu/address/model/AddressBook.java 93.25% <91.66%> (+4.79%) ⬆️
...u/address/logic/commands/contact/CEditCommand.java 94.87% <92.85%> (-0.37%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e72508...bd4dc5b. Read the comment docs.

@chunweii chunweii self-requested a review November 3, 2021 06:59
@chunweii chunweii removed a link to an issue Nov 3, 2021
Copy link
Collaborator

@chunweii chunweii left a comment

Choose a reason for hiding this comment

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

Some minor but important changes. Otherwise, looks good.

tP Project Dashboard automation moved this from In progress to Review in progress Nov 3, 2021
@chunweii
Copy link
Collaborator

chunweii commented Nov 3, 2021

Also, please add more test cases. See codecov report to see where you are lacking.

tP Project Dashboard automation moved this from Review in progress to Reviewer approved Nov 3, 2021
Copy link
Collaborator

@chunweii chunweii left a comment

Choose a reason for hiding this comment

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

LGTM, but there are some other changes to be made

@chunweii chunweii self-requested a review November 3, 2021 12:13
Copy link
Collaborator

@chunweii chunweii left a comment

Choose a reason for hiding this comment

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

When the user tries to delete a tag that does not exist, the program will give an error and show a red message. However, if the user tries to unlink something that is not linked, the program allows it but displays the message that it is not linked. This is quite inconsistent. Also, you forgot to implement the check for adding new tags if there are already duplicate tags in CEdit and EEdit commands.

tP Project Dashboard automation moved this from Reviewer approved to Review in progress Nov 3, 2021
Copy link
Collaborator

@chunweii chunweii left a comment

Choose a reason for hiding this comment

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

Since I made changes to the PR, I shall not approve this PR. Even though it is showing green, please wait for Janice review

tP Project Dashboard automation moved this from Review in progress to Reviewer approved Nov 4, 2021
Copy link
Collaborator

@janjanchen janjanchen left a comment

Choose a reason for hiding this comment

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

LGTM :) Good job!!

@janjanchen janjanchen merged commit 6b580c5 into master Nov 4, 2021
tP Project Dashboard automation moved this from Reviewer approved to Done Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.High Should be resolved first, failing which, the project will not work severity.High A flaw that affects most users and causes major problems for users type.Bug Something isn't working
3 participants