Add edit journal #135
Merged
Add edit journal #135
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was
linked to
issues
Oct 24, 2020
Closed
Codecov Report
@@ Coverage Diff @@
## master #135 +/- ##
============================================
+ Coverage 66.04% 68.39% +2.35%
- Complexity 613 649 +36
============================================
Files 104 106 +2
Lines 2188 2329 +141
Branches 232 258 +26
============================================
+ Hits 1445 1593 +148
+ Misses 648 638 -10
- Partials 95 98 +3
Continue to review full report at Codecov.
|
|
Should be ready for merging now. There's still some tests that haven't been updated for AddJournalEntryCommand and its parser, but I'll leave that for another PR. |
jazerler
commented
Oct 26, 2020
Comment on lines
+72
to
+78
| public static Title parseTitle(String title) throws ParseException { | ||
| requireNonNull(title); | ||
| String trimmedTitle = title.trim(); | ||
| if (!Title.isValidTitle(trimmedTitle)) { | ||
| throw new ParseException(Title.MESSAGE_CONSTRAINTS); | ||
| } | ||
| return new Title(trimmedTitle); |
There was a problem hiding this comment.
This is a new method that replaces the old new Title(parseName(name).get()), since the ParseException from that throws the message constraints from Name rather than Title.
| Email.EMPTY_EMAIL, | ||
| Address.EMPTY_ADDRESS, | ||
| new HashSet<>(), | ||
| UUID.fromString("e26616c9-c740-4d86-861e-733a4d377a3e") |
There was a problem hiding this comment.
The UUID here just uses the one from Alice Pauline in the typical persons addressbook.
It's just a placeholder that doesn't actually get used anywhere.
Comment on lines
+54
to
+55
| public static final String CONTACT_DEFAULT_UUID = "e26616c9-c740-4d86" | ||
| + "-861e-733a4d377a3e"; |
There was a problem hiding this comment.
This is the same UUID as used in ParserUtil's parseContacts. Again, it's just a placeholder that's used to make sure the equals method doesn't just break.
joshualiangxy
approved these changes
Oct 26, 2020
docs/DeveloperGuide.md
Outdated
|
|
||
| #### Current Implementation | ||
|
|
||
| Similar to the existing `editc` and `addj` commands, the `EditJournalEntryParser` makes use of `ParserUtils` to split up user input into arguments, which are then used to create an `EditEntryDescriptor` that contains the details of the journal properties to be edited. |
| ### Editing a contact: `editc` | ||
|
|
||
| Edits an existing person in the address book. | ||
|
|
| ### Editing a journal entry: `editj` | ||
|
|
||
| Edits an existing entry in the journal. | ||
|
|
joshualiangxy
approved these changes
Oct 26, 2020
Bug: EditEntryDescriptor's getContacts method throws an InvocationTargetException.
Note that the current implementation of the toString method is not properly formatted, and merely uses the verbose toString method of Person.
Bugs that have not been fixed: * Entry titles are allowed to be empty * Empty title entries are not shown properly in the UI * EditJournalEntryCommandParser does not reject usage with no arguments
Bug with testability: The contacts generated within the journal parsers each have their own UUIDs, which cannot be tested for by comparing the resulting command with another expected command. Suggested fix: Change the parseContacts method to return a list of name Strings rather than Persons.
parseContact now intialises each Person with the specified Name field and each field's empty constant and uses a constant UUID. Although the UUID used is the same as one of the test Persons, it should not pose any problems as only the Name field of the resulting Persons is used.
jazerler
force-pushed
the
add-edit-journal
branch
from
4f0cc90
to
278b9d8
Compare
joshualiangxy
approved these changes
Oct 26, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Not ready to merge, as I have yet to write tests for this.
Had to modify the toString() method of Entry in logic to have it show that contacts were being edited, but it's not very pretty and will probably have to be rewritten soon.
Storage and UI are also missing contact representation, so contacts aren't saved to the json file orshown in Entry.