Fix id reference of visits #138
Conversation
hopinxian
added
bug
Codecov Report
@@ Coverage Diff @@
## master #138 +/- ##
============================================
+ Coverage 74.83% 74.88% +0.04%
- Complexity 765 767 +2
============================================
Files 111 111
Lines 2281 2293 +12
Branches 269 272 +3
============================================
+ Hits 1707 1717 +10
Misses 508 508
- Partials 66 68 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Changes requested, certain tests need to be added as mentioned.
Also, are there tests to test if the issue of adding the wrong index has been solved? Having a test which tests if the same visit is added based on what the viewer sees should be added if it does not exist yet.
| @@ -21,7 +27,7 @@ | |||
| public class AddVisitCommandTest { | |||
| @Test | |||
| public void constructor_nullLocation_throwsNullPointerException() { | |||
| assertThrows(NullPointerException.class, () -> new AddVisitCommand(null)); | |||
| assertThrows(NullPointerException.class, () -> new AddVisitCommand(null, null, null)); | |||
There was a problem hiding this comment.
Now that there are three parameters for the constructors, perhaps it could be better if each parameter is tested for NullPointerException in a separate method.
There was a problem hiding this comment.
Have added additional assertions.
| @@ -21,7 +27,7 @@ | |||
| public class AddVisitCommandTest { | |||
| @Test | |||
| public void constructor_nullLocation_throwsNullPointerException() { | |||
There was a problem hiding this comment.
Typo here. Shouldn't be nullLocation but instead the parameter that is null in this method.
| requireAllNonNull(visit); | ||
| toAdd = visit; | ||
| public AddVisitCommand(Index personIndex, Index locationIndex, LocalDate date) { | ||
| requireAllNonNull(personIndex, locationIndex); |
There was a problem hiding this comment.
Shouldn't date be required to be non null as well?
There was a problem hiding this comment.
Thanks for the catch! I have updated it.
Additional tests to confirm the success of adding the correct visits are added under filtered and unfiltered lists.
…hopinxian/tp into v1.2/branch-FixVisitIdReference
|
Thanks for the review. I have improved the Pull Request according to your comments. |
Implement a fix that ensures id of person and location that visit is referring to matches the intended person and location.
Fix #136