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

[CS2103T-F14-2] TutorAid #91

Open
wants to merge 724 commits into
base: master
Choose a base branch
from

Conversation

caesarpjz
Copy link

@@ -297,33 +298,33 @@ Priorities: High (must have) - `* * \*`, Medium (nice to have) - `* \*`, Low (un
|Priority |As a ... |I want to ... |So that I can...
|`* * *` |new user |see usage instructions |refer to instructions when I forget how to use the App

|`* * *` |user |add a new person |
|`* * *` |user |add a new task | check the details of the task when I want

Choose a reason for hiding this comment

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

User vs Tutor: Do they refer to the same type of user? Would appreciate a bit more details with respect to the differences between these two types of users.

Choose a reason for hiding this comment

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

Task: Perhaps a bit more detail with respect to what a task refers to, currently we're not sure what a task is.

Copy link

@huiminlim huiminlim left a comment

Choose a reason for hiding this comment

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

Our team reviewed your use cases and Developer guide and we find that generally, the use cases were coherent and well written. There are some minor issues, so you can improve your developer guide after reading our review.

1. User requests to list tasks
2. TutorAid shows a list of tasks
3. User requests to delete a specific task in the list
4. TutorAid deletes the person

Choose a reason for hiding this comment

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

Should be "task" instead of "persons"


[discrete]
=== Use case: Delete person
=== Use case: Delete task

Choose a reason for hiding this comment

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

Nice flow of use case, but can also include in user stories since it is a feature for the user.


|`*` |user with many persons in the address book |sort persons by name |locate a person easily
|`*` |caring tutor |check the upcoming events |remind my students

Choose a reason for hiding this comment

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

Events: Does event refer to class schedules or to things like deadlines (ie are regularly scheduled or one-off things)?

*Example*
The following activity diagram summarizes what happens when a user executes a new command.

image::AddEarningsActivityDiagram.png[]

Choose a reason for hiding this comment

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

The branching node (input is valid) only has one branch. Maybe include a second arrow for else that goes to the end of the diagram? Also, "input is valid" should be contained within square brackets ([input is valid]).

Choose a reason for hiding this comment

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

Design diagrams have been unchanged since AB3. From how it looked like in Implementation, I would assume that there have been some changes to the architecture? I could be mistaken, of course.

@@ -153,6 +153,73 @@ Classes used by multiple components are in the `seedu.addressbook.commons` packa

This section describes some noteworthy details on how certain features are implemented.

=== Earnings Feature

Choose a reason for hiding this comment

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

No updates to Architecture diagram

following input from the user: `Date`, `ClassId`, `Amount`, which will construct `Earnings` objects.

.Add Earnings Command Sequence Diagram
image::AddEarningsSequenceDiagram.png[]

Choose a reason for hiding this comment

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

image
Is your class still called AddressBookParser? Should it be renamed?

alages97 pushed a commit to alages97/main that referenced this pull request Oct 26, 2019
b123b123z and others added 30 commits November 11, 2019 21:37
edited note error messages and examples
Merge pull request #392 from kerwin97/master
Merge pull request #22 from AY1920S1-CS2103T-F14-2/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet