-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add datafields into Loan class #124
Add datafields into Loan class #124
Conversation
Pull Request Test Coverage Report for Build 402
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can update the javadoc for Loan.isSameLoan()
to say "Returns true if both loans have the same customer name, NRIC, bike, and at least one other field (Email, Phone, LoanRate, LoanTime)."
Otherwise quite alright. I'll open an issue for the parts which create a new Bike on the spot.
+ PREFIX_PHONE + "PHONE " | ||
+ PREFIX_EMAIL + "EMAIL " | ||
+ PREFIX_ADDRESS + "ADDRESS " | ||
+ PREFIX_BIKE + "BIKE " | ||
+ PREFIX_LOANRATE + "LOANRATE " | ||
+ PREFIX_LOANTIME + "LOANTIME " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not correct. Loan start time is NOT a parameter when adding a loan!
The start time based on when the loan was created.
+ "[" + PREFIX_PHONE + "PHONE] " | ||
+ "[" + PREFIX_EMAIL + "EMAIL] " | ||
+ "[" + PREFIX_ADDRESS + "ADDRESS] " | ||
+ "[" + PREFIX_BIKE + "BIKE] " | ||
+ "[" + PREFIX_LOANRATE + "LOANRATE] " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LoanTime is not supposed to be editable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, I referenced the wrong line. This comment references line 56, which is 1 line below.
For issue: #103