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

Standardise Storage and Loading Order for Client Notes #320

Merged

Conversation

rtshkmr
Copy link
Member

@rtshkmr rtshkmr commented Oct 31, 2020

Description

As per title.

Fixes #319

Testing

NIL, just works man:

image
image

Remarks

NIL

@rtshkmr rtshkmr added priority.Medium Nice to have severity.Medium A flaw that causes occasional inconvenience to some users but they can continue to use the product. labels Oct 31, 2020
@rtshkmr rtshkmr added this to the v1.4 milestone Oct 31, 2020
@rtshkmr rtshkmr added this to In progress in v1.4 via automation Oct 31, 2020
@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #320 into master will decrease coverage by 0.04%.
The diff coverage is 92.30%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #320      +/-   ##
============================================
- Coverage     87.26%   87.21%   -0.05%     
+ Complexity      961      958       -3     
============================================
  Files           113      113              
  Lines          2512     2510       -2     
  Branches        287      286       -1     
============================================
- Hits           2192     2189       -3     
  Misses          235      235              
- Partials         85       86       +1     
Impacted Files Coverage Δ Complexity Δ
...du/address/storage/JsonSerializableTbmManager.java 92.00% <88.88%> (+0.69%) 6.00 <0.00> (+1.00)
src/main/java/seedu/address/model/TbmManager.java 93.18% <100.00%> (-0.57%) 22.00 <0.00> (-3.00)
.../java/seedu/address/storage/JsonAdaptedClient.java 100.00% <100.00%> (ø) 20.00 <0.00> (ø)
...a/seedu/address/model/client/UniqueClientList.java 95.23% <0.00%> (-2.39%) 22.00% <0.00%> (-1.00%)

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 ef8a057...c096268. Read the comment docs.

@rtshkmr rtshkmr changed the title Standardise Storage and Loading Order Standardise Storage and Loading Order for Client Notes Oct 31, 2020
Copy link

@raysonkoh raysonkoh left a comment

Choose a reason for hiding this comment

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

Please add in countryNotesManager.clear() in TbmManager#setCountryNotes method. This is quite important. Otherwise I'm ok with the changes.

*/
public void setNotes(List<Note> notes) {
countryNotesManager.clear();

Choose a reason for hiding this comment

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

This is important!! don't delete this

Copy link
Member

Choose a reason for hiding this comment

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

my bad, added it back

@@ -61,11 +61,10 @@ public TbmManager toModelType() throws IllegalValueException {
}
tbmManager.addClient(client);
}
for (JsonAdaptedNote note: notes) {
for (JsonAdaptedNote note: countryNotes) {

Choose a reason for hiding this comment

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

Renaming note to countryNote might be clearer

Copy link
Member

Choose a reason for hiding this comment

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

renamed to jsonAdaptedCountryNote

"countryCode": "NULL_CC",
"tags": ["godLike"]
"countryNotes": [ {
"contents": "too hot, hot damn",

Choose a reason for hiding this comment

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

Made me chuckle

v1.4 automation moved this from In progress to Review in progress Oct 31, 2020
Copy link

@raysonkoh raysonkoh left a comment

Choose a reason for hiding this comment

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

LGTM

v1.4 automation moved this from Review in progress to Reviewer approved Nov 1, 2020
@qwoprocks qwoprocks merged commit 5782b84 into AY2021S1-CS2103T-F11-4:master Nov 1, 2020
v1.4 automation moved this from Reviewer approved to Done Nov 1, 2020
@rtshkmr rtshkmr added the type.Bug A bug label Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.Medium Nice to have severity.Medium A flaw that causes occasional inconvenience to some users but they can continue to use the product. type.Bug A bug
Projects
No open projects
v1.4
Done
Development

Successfully merging this pull request may close these issues.

Bug: Ensure order of Client Notes is maintained at exit and start-up
3 participants