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

Client Edit, Client Note Edit Bug Fixes #268

Merged
merged 21 commits into from
Oct 29, 2020

Conversation

rtshkmr
Copy link
Member

@rtshkmr rtshkmr commented Oct 29, 2020

Description

Bug: when editing a non-note field of clients, client note wasn't getting retained.

Fixes #264
Fixes #269
Fixes #272 <== this is a minor display bug

Status: desired behaviour is reached.

  • Test method: ClientEditCommandTest#execute_changeClientName_preserveExistingClientNotes()

  • desired behaviuor is seen in the json file:
    before:
    image

    after command : client edit 1 n/ackerman
    image

Testing

Ref to example screenshot above

Remarks

Regression Test Failure was due to the mutability issue again. The following change has been done to TypicalClients#getTypicalClients

    public static List<Client> getTypicalClients() {
        Client alice = new ClientBuilder(TypicalClients.ALICE).build();
        Client benson = new ClientBuilder(TypicalClients.BENSON).build();
        Client carl = new ClientBuilder(TypicalClients.CARL).build();
        Client daniel = new ClientBuilder(TypicalClients.DANIEL).build();
        Client elle = new ClientBuilder(TypicalClients.ELLE).build();
        Client fiona = new ClientBuilder(TypicalClients.FIONA).build();
        Client george = new ClientBuilder(TypicalClients.GEORGE).build();
        return new ArrayList<>(Arrays.asList(alice, benson, carl, daniel, elle, fiona, george));
    }

@codecov
Copy link

codecov bot commented Oct 29, 2020

Codecov Report

Merging #268 into master will increase coverage by 0.14%.
The diff coverage is 88.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #268      +/-   ##
============================================
+ Coverage     87.00%   87.14%   +0.14%     
- Complexity      926      928       +2     
============================================
  Files           112      112              
  Lines          2416     2420       +4     
  Branches        278      278              
============================================
+ Hits           2102     2109       +7     
+ Misses          234      233       -1     
+ Partials         80       78       -2     
Impacted Files Coverage Δ Complexity Δ
...ress/logic/parser/ClientNoteEditCommandParser.java 76.19% <0.00%> (ø) 4.00 <0.00> (ø)
src/main/java/seedu/address/model/TbmManager.java 93.61% <ø> (+1.95%) 25.00 <0.00> (+1.00)
...eedu/address/logic/commands/ClientEditCommand.java 94.04% <100.00%> (+0.22%) 13.00 <1.00> (+1.00)
.../address/logic/commands/ClientNoteEditCommand.java 79.41% <100.00%> (+5.88%) 9.00 <0.00> (ø)
...rc/main/java/seedu/address/model/ModelManager.java 94.78% <100.00%> (+0.09%) 43.00 <0.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 e2f615a...ec36563. Read the comment docs.

@rtshkmr rtshkmr marked this pull request as ready for review October 29, 2020 07:57
@rtshkmr rtshkmr added the priority.High Must do label Oct 29, 2020
@rtshkmr rtshkmr added this to In progress in v1.4 via automation Oct 29, 2020
@rtshkmr rtshkmr added this to the v1.4 milestone Oct 29, 2020
@rtshkmr rtshkmr self-assigned this Oct 29, 2020
v1.4 automation moved this from In progress to Review in progress Oct 29, 2020
@rtshkmr rtshkmr changed the title Client Note Edit Bug Client Edit, Client Note Edit Bug Fixes Oct 29, 2020
src/test/java/seedu/address/testutil/TypicalClients.java Outdated Show resolved Hide resolved
@@ -147,6 +148,7 @@ public void addClient(Client client) {
public void setClient(Client target, Client editedClient) {
requireAllNonNull(target, editedClient);
tbmManager.setClient(target, editedClient);
initialiseTagNoteMap();
Copy link
Member

Choose a reason for hiding this comment

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

is there anything we can do in tagnotemap that would allow us to replace a client instead of doing re-init?

v1.4 automation moved this from Review in progress to Reviewer approved Oct 29, 2020
@rtshkmr rtshkmr merged commit c8ef8d8 into AY2021S1-CS2103T-F11-4:master Oct 29, 2020
v1.4 automation moved this from Reviewer approved to Done Oct 29, 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
2 participants