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

Update UGDG #149

Merged
merged 9 commits into from
Oct 29, 2018
Merged

Update UGDG #149

merged 9 commits into from
Oct 29, 2018

Conversation

Kelly9373
Copy link
Collaborator

No description provided.

@coveralls
Copy link

coveralls commented Oct 28, 2018

Pull Request Test Coverage Report for Build 516

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 91.816%

Totals Coverage Status
Change from base Build 506: 0.0%
Covered Lines: 1941
Relevant Lines: 2114

💛 - Coveralls

@Kelly9373
Copy link
Collaborator Author

For issue: #141


*Step 1.* The user launches the application for the first time. The user's email address is string `"default"` by default.

*Step 2.* The user executes `check`. The command executes `ModelManager#getMyEmail()`, which calls `UserPrefs#getDefaultEmail()` and returns user's email stored in `UserPref`. The app checks if user's email equals to `"default"`. In this case, they are equal, so it throws a `CommandException`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use a sequence diagram.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean a sequence diagram to show the whole procedure or just this step?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just this step. Though feel free to add more diagrams in other places if you think it helps.


. Send a reminder email successfully.

`add` a loan with a valid customer's email (but you own it of course). Execute `check` command. Then use the `setemail` command to set the sender's email. After that, execute `remind x/correctpassword n/Name b/BikeId`. +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be converted to point form to make things clearer.

Copy link
Collaborator

@OrangeJuice7 OrangeJuice7 left a comment

Choose a reason for hiding this comment

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

I've corrected some language styling. Please verify that I did not misinterpret anything.

Overall comments:
The procedures have quite a few if-else cases (such as the example usage scenario Step 3). I would suggest indenting the individual cases to make things clearer.

When specifying a loan to send a reminder to, you can use the LoanID instead of specifying a Name and Bike, as the latter is more complicated and may not uniquely identify a loan.

May I also suggest renaming the check command to checkemail, so that what it does is clearer?

docs/UserGuide.adoc Outdated Show resolved Hide resolved
Copy link
Collaborator

@OrangeJuice7 OrangeJuice7 left a comment

Choose a reason for hiding this comment

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

Just two more issues, otherwise it looks fine.

@OrangeJuice7
Copy link
Collaborator

"CommandException" in the diagram is spelled incorrectly.

Copy link
Collaborator

@OrangeJuice7 OrangeJuice7 left a comment

Choose a reason for hiding this comment

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

I've fixed a few more language issues, mostly for the example usage scenario Step 3. Please review it, and merge if you're satisfied.

@Kelly9373 Kelly9373 merged commit 4fb314e into CS2103-AY1819S1-F10-2:master Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants