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

Fix Ordering Bug for ClientNoteEdit #312

Merged
merged 18 commits into from
Oct 31, 2020

Conversation

rtshkmr
Copy link
Member

@rtshkmr rtshkmr commented Oct 31, 2020

Description

Changed notes from being part of Sets to being part of lists.
The note kept getting popped to the bottom of the list because editing used to be implemented as a removal of a target note and an insertion of its edited form.

Also added the test ClientNoteEditCommandTest#execute_editExistingUntaggedNoteToAddTag_discardsDefaultUntaggedTag() and found a flaw in TagNoteMap initialisation and fixed that.

GUI has been updated too.

Fixes #270 Fixes #296

Testing

NIL

Remarks

NIL

The ordering problem still persists. Changing Client.clientNotes from ObservableSet<Note> to LinkedHashSet<Note>will break the GUI's updating after user issues a valid Client Note edit command.
@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #312 into master will decrease coverage by 0.00%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #312      +/-   ##
============================================
- Coverage     87.14%   87.14%   -0.01%     
- Complexity      928      930       +2     
============================================
  Files           112      112              
  Lines          2420     2427       +7     
  Branches        278      279       +1     
============================================
+ Hits           2109     2115       +6     
  Misses          233      233              
- Partials         78       79       +1     
Impacted Files Coverage Δ Complexity Δ
.../address/logic/commands/ClientNoteEditCommand.java 80.00% <80.00%> (+0.58%) 9.00 <0.00> (ø)
...c/main/java/seedu/address/model/client/Client.java 95.58% <80.00%> (-2.92%) 33.00 <2.00> (ø)
...main/java/seedu/address/model/note/TagNoteMap.java 90.52% <87.50%> (+1.27%) 27.00 <2.00> (+2.00)
...eedu/address/logic/commands/ClientEditCommand.java 94.04% <100.00%> (ø) 13.00 <0.00> (ø)
...ddress/logic/commands/ClientNoteDeleteCommand.java 92.59% <100.00%> (+0.59%) 10.00 <0.00> (ø)
...rc/main/java/seedu/address/model/ModelManager.java 94.78% <100.00%> (ø) 43.00 <0.00> (ø)
src/main/java/seedu/address/model/TbmManager.java 93.61% <100.00%> (ø) 25.00 <1.00> (ø)
...a/seedu/address/model/client/UniqueClientList.java 97.61% <100.00%> (ø) 23.00 <1.00> (ø)
.../java/seedu/address/storage/JsonAdaptedClient.java 100.00% <100.00%> (ø) 20.00 <19.00> (ø)
src/main/java/seedu/address/ui/WidgetViewBox.java 66.66% <100.00%> (ø) 12.00 <1.00> (ø)
... and 2 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 93ff002...6ae998c. Read the comment docs.

@rtshkmr rtshkmr added this to In progress in v1.4 via automation Oct 31, 2020
@rtshkmr rtshkmr added this to the v1.4 milestone Oct 31, 2020
Test added verifies that a previously untagged note will have tag.UNTAGGED removed if the edit adds in a new tag.

TagNoteMap had a undetected flaw where the default tag of Tag.UNTAGGED didn't get added to the uniqueTagMap when constructed. This was causing test case failures.

/**
* Constructor ensures our unique tag map has the UNTAGGED tag.
*/
public TagNoteMap() {
uniqueTagMap.put(Tag.UNTAGGED, Tag.UNTAGGED);
tagToNotesMap.put(Tag.UNTAGGED, new ArrayList<>());
Copy link
Member Author

Choose a reason for hiding this comment

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

took a while to find this 😞

@rtshkmr rtshkmr marked this pull request as ready for review October 31, 2020 08:37
@rtshkmr rtshkmr requested review from raysonkoh and qwoprocks and removed request for raysonkoh October 31, 2020 08:37
@rtshkmr rtshkmr self-assigned this Oct 31, 2020
@rtshkmr rtshkmr requested a review from LeeEnHao October 31, 2020 08:50
v1.4 automation moved this from In progress to Review in progress Oct 31, 2020
Copy link
Member

@qwoprocks qwoprocks 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 is it possible to also add in a more informative message for client note edit? Using String.format to show the user which client, the original note content, and the new note content. Can do in a separate PR if you want but imo its a simple enough change to do in this one.

v1.4 automation moved this from Review in progress to Reviewer approved Oct 31, 2020
@rtshkmr
Copy link
Member Author

rtshkmr commented Oct 31, 2020

@qwoprocks Merged after doing this: 🏃‍♂️

LGTM, but is it possible to also add in a more informative message for client note edit? Using String.format to show the user > which client, the original note content, and the new note content. Can do in a separate PR if you want but imo its a simple enough change to do in this one.

image

@rtshkmr rtshkmr merged commit 9d72b4e into AY2021S1-CS2103T-F11-4:master Oct 31, 2020
v1.4 automation moved this from Reviewer approved to Done Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v1.4
Done
Development

Successfully merging this pull request may close these issues.

[PE-D] Edit Client Note will change order of notes after editing Notes do not follow the same order always
3 participants