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-T12-1] +Work #110

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

Conversation

seahlynn
Copy link

@seahlynn seahlynn commented Oct 1, 2019


|`* * *` |user |delete a person |remove entries that I no longer need
|`* * *` |project leader |change task status | ger reminder of the progress of each task
Copy link

Choose a reason for hiding this comment

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

Small spelling error here


|`* * *` |user |find a person by name |locate details of persons without having to go through the entire list
|`* * *` |project leader |sync team members schedules |Find a time slot where the maximum number of people, if not all, can attend
Copy link

Choose a reason for hiding this comment

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

The capitalisation should be consistent. "find" is capitalise but before that is not


|`* *` |user |hide <<private-contact-detail,private contact details>> by default |minimize chance of someone else seeing them by accident
|`* *`|Project leader with frequent meetings|Create a new meeting with recommended time and desired location|View my next meeting in the home page
Copy link

Choose a reason for hiding this comment

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

Probably can split the function and benefit into 2 user stories since it does not seems to match.
As a project leader, I want to create meeting so that I can secure the meeting slot at my desired time and place.
As a project leader, I want to view my next meeting in the homepage so that I know my schedule at a glance.


|`*`|Project leader with a long-term project with ad-hoc members|Change members working on a task|Reassign tasks to incoming members

|`*`|Project leader operating keeping track of purchases|Tag the inventory purchase to the member who bought it|Produce an accurate claims report at the end of the project
Copy link

Choose a reason for hiding this comment

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

"Project leader operating keeping track of purchases" I don't seem to understand this. Could you rephrase it in a better way?

* prefers typing over mouse input
* is reasonably comfortable using CLI apps

*Value proposition*: manage contacts faster than a typical mouse/GUI driven app
*Value proposition*: manage tasks assigned to project mates, finding common time slots and keep track of inventory faster than GUI apps.

[appendix]
== User Stories
Copy link

Choose a reason for hiding this comment

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

Generally well structured and have all 3 parts present


|`*` |user with many persons in the address book |sort persons by name |locate a person easily
|=======================================================================
Copy link

Choose a reason for hiding this comment

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

Did not really talk about the other type of users for this project. For example, project leader can assign tasks but did not talk about if the teammates can see the tasks

3. The given index is invalid.
4. +work shoes an error message
+
Use case resumes at step 2

[appendix]
== Non Functional Requirements
Copy link

Choose a reason for hiding this comment

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

  1. Are the NFRs are really Non-Functional?
  • Yes because it is not related to any function. They are function requirements related to performance, user-friendliness.
  1. Is each NFR well-defined (i.e., possible to decide when it has been met)?
  • NFR 1 is a well-defined NFR, but very generic
  • NFR 2: it is ambiguous as to what ‘noticeable sluggishness’ means. How many seconds/milliseconds of lag would it be before we notice the sluggishness?
  • NFR 3: it is a measurable NFR. Since the wording is ‘accomplish most of the tasks faster using commands’, are there any specific tasks where it would take longer to accomplish?
  • Other feedback: the NFRs are quite generic, and not specific to the application.
  1. Is each NFR reasonably achievable?
  • NFRs are reasonably achievable
  1. Are more relevant NFRs have been left out?
  • Left out NFRs related to security. A possible NFR would be: only team leader can change the allocation of tasks to team members.

Copy link

@iskandarzulkarnaien iskandarzulkarnaien left a comment

Choose a reason for hiding this comment

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

Good work in general! However there are formatting issues with use cases. Please refer to this

Comment on lines 340 to 341
2. The user does not specify name
3. +work shows an error message

Choose a reason for hiding this comment

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

Numbering format is wrong. Should start from the step number where the extension occurs. Refer to this.

Comment on lines 357 to 359
2. The list is empty
+
Use case resumes at step 2.
Use case ends

Choose a reason for hiding this comment

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

I think it would be better to have the system feedback to the user that the list is empty instead of simply terminating

Comment on lines 331 to 334
*Main Success Scenario*

1. User requests to list persons
2. AddressBook shows a list of persons
3. User requests to delete a specific person in the list
4. AddressBook deletes the person
1. User requests to add a team member
2. User specifies the name of the team member

Choose a reason for hiding this comment

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

This seems to be that at step 1, the user types in the command, then +Work waits for the user to type in the name of the team member. May I suggest adding a step in between where +Work notifies user to key in the name of team member

Choose a reason for hiding this comment

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

This applies for your other use cases too

Choose a reason for hiding this comment

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

Also, before use case ends, it may be good to notify the user that their action was successful.

*Main Success Scenario*

1. User requests to list team members
2. +work displays list of team members

Choose a reason for hiding this comment

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

Seems like your app name is inconsistent in its capitalization. Some places have +work and others have +Work. Best to make it consistent.


*Main Success Scenario*

1. User requests to mark a task as ‘doing

Choose a reason for hiding this comment

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

Missing close inverted comma

Comment on lines 493 to 520
=== Use case: Remove a task from the dashboard

*Main Success Scenario*

1. User requests to remove a task from the dashboard
2. User specifies the task ID
3. +work updates the dashboard
+
Use case ends

*Extensions*

2. User specifies an invalid task ID
3. +work shows an error message
+
Use case ends


[discrete]
=== Use case: Remove a task from the dashboard

*Main Success Scenario*

1. User requests to remove a task from the dashboard
2. User specifies the task ID
3. +work updates the dashboard
+
Use case ends

Choose a reason for hiding this comment

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

Seems to be duplicated by mistake

Comment on lines 553 to 555
1. User adds timetable of team mates
2. User requests to generate availability timings of team members
3. +work displays list of timings where the most number of team members are available

Choose a reason for hiding this comment

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

Same formatting issue here with extensions

@@ -64,9 +63,9 @@ image::LogicClassDiagram.png[]
[discrete]

Choose a reason for hiding this comment

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

Diagrams do not have updated class name (it is still named as AddressBookParser). I think the ProjectDashBoardParser should be connected to the XYZCommandParser in this case.

@@ -98,9 +115,9 @@ image::LogicClassDiagram.png[]
*API* :

Choose a reason for hiding this comment

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

The UI Component diagram is a bit difficult to understand. What are the functionalities of the UserView classes (Navigator / Controller / Main / Update)? Why is the UserViewMain connected to the Logic?

Choose a reason for hiding this comment

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

While the implementation may be unclear to us, the puml diagram is actually quite well done.


[[Design-Model]]
=== Model component

.Structure of the Model Component
image::ModelClassDiagram.png[]
image::ModelClassDiagramNew.png[]

Choose a reason for hiding this comment

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

Could consider putting the name/price/taskStatus under the attributes of the main classes.
Tag is an association, not a composition. Might want to change the notation for the tag.

* does not depend on any of the other three components.

[NOTE]
As a more OOP model, we can store a `Tag` list in `Address Book`, which `Person` can reference. This would allow `Address Book` to only require one `Tag` object per unique `Tag`, instead of each `Person` needing their own `Tag` object. An example of how such a model may look like is given below. +
As a more OOP model, we can store a `Tag` list in `Address Book`, which `Member` or 'Task' can reference. This would allow `Address Book` to only require one `Tag` object per unique `Tag`, instead of each `Member` or 'Task' needing their own `Tag` object. An example of how such a model may look like is given below. +

Choose a reason for hiding this comment

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

Don't forget to change the address books to your application! :)

Step 1. The user launches the application. The `ProjectCalendar` will be initialised based on the
saved `ProjectCalendar`.

Step 2. The user imports members' calendars by executing `impport-calendar`.

Choose a reason for hiding this comment

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

import (spelling error)

gabrielseow and others added 30 commits November 11, 2019 22:19
Added more sample memebrs
fix paste to command box not working
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.

9 participants