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

[v1.0][W15-B1] E.T. (Employees Tracker) #43

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

Conversation

IzHoBX
Copy link

@IzHoBX IzHoBX commented Mar 7, 2018

  • Add user stories
  • Add use cases
  • Add NFRs
  • Add lock and unlock command
  • Add rate command
  • Add sort command
  • Add find by any field

@nus-se-pr-bot
Copy link

Hi @IzHoBX, your pull request title is invalid.

For phase A, it should be in the format of [Learning Outcome ID][Team ID] Your Name, where [Learning Outcome ID] has no dashes or spaces (e.g. [W3.1a]) and [Team ID] has one dash only and no spaces (e.g. [W14-A2] means Wednesday 2pm (14 hrs), Phase A, Team 2).

For phase B, it should be in the format of [v1.x][Team ID] Product Name.

Please follow the instructions given strictly and edit your title for reprocessing.

Submit only one learning outcome per pull request (unless otherwise stated in instructions) and do remember to create your branches from the commit where the master branch is pointing at so that each PR you submit only consist of commits meant for the activity.

Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at cs2103-pr-bot and add a link to this PR.

@IzHoBX IzHoBX changed the title [v1.x][W15-B1] E.T. (Employees Tracker) [v1.0][W15-B1] E.T. (Employees Tracker) Mar 8, 2018
Copy link

@devamanyu devamanyu left a comment

Choose a reason for hiding this comment

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

Good coverage of user-stories, use cases, and NFRs. Please address some minor comments mentioned in this review.

A glance at your organization repo showed that some individual PRs were merged despite Travis build failure (e.g., Sort command, Rate command, etc.). Normally it is a good idea to merge only passed builds into the master branch. This will help you in isolating broken commits. Coding standard violation is one of the main issues of Travis build failure and thus sticking to the standards provided is of utmost importance.

Also, it is nice to see that issue-tracker has been set up.

Do not close this PR, as we'll be using this PR for your subsequent milestones!

README.adoc Outdated
@@ -1,7 +1,7 @@
= Address Book (Level 4)

Choose a reason for hiding this comment

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

I guess your project is not Address Book anymore :p, please update the Address Book mentions from all instances. You might also need to update or remove stuff that relates only to address-book and not to your project.

@@ -10,7 +10,8 @@ ifdef::env-github[]
:tip-caption: :bulb:
:note-caption: :information_source:
endif::[]
:repoURL: https://github.com/se-edu/addressbook-level4/tree/master
:repoURL: https://github.com/CS2103JAN2018-W15-B1/main/tree/master


By: `Team SE-EDU`      Since: `Jun 2016`      Licence: `MIT`

Choose a reason for hiding this comment

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

Team name should be different.


|`* * *`|user |add a new person | include employees that have just entered the company

|`* * *`|user |each employee has his/her dedicated timetable | trace progress of that person

Choose a reason for hiding this comment

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

This particular user story doesn't adhere to the "I want to..." prefix. When read together, the whole story should be grammatically coherent.


|`* * *`|user |add notes on a timetable entry or on a person | include more details about the entry

|`* * *`|user |search employees by specific criteria | search employees more easily

Choose a reason for hiding this comment

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

Might be a good idea to enlist a couple of criteria's possible. That way reader can understand which criteria you are planning to consider.


|`*`|user |unlock the addressbook |continue to use the app after leaving it locked

|`*`|user |write a review to an employee that has ever worked below me| information his/her current manager of his/her performance
|=======================================================================

_{More to be added}_

Choose a reason for hiding this comment

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

This comment can be removed if no more stories are planned to be added.

3. User requests to delete a specific person in the list
4. AddressBook deletes the person
1. User requests to list employees
2. AddressBook shows a list of employees

Choose a reason for hiding this comment

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

Its probably best to rename "AddressBook" as this new app is not exactly an "Address Book".

. Should be able to hold up to 1000 persons without a noticeable sluggishness in performance for typical usage.
. A user with above average typing speed for regular English text (i.e. not code, not system admin commands) should be able to accomplish most of the tasks faster using commands than using the mouse.
. Should be able to hold up to 1000 persons without a noticeable sluggishness (i.e. response time >500ms) in performance for typical usage.
. A user with above average typing speed (i.e. >=45 words per minute) for regular English text (i.e. not code, not system admin commands) should be able to accomplish most of the tasks faster using commands than using the mouse.

Choose a reason for hiding this comment

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

Nice to see changes made as per discussion in the tutorial.

YuanQLLer pushed a commit to YuanQLLer/addressbook-level4 that referenced this pull request Mar 18, 2018
wynonaK added a commit to wynonaK/addressbook-level4 that referenced this pull request Mar 23, 2018
@devamanyu
Copy link

Overall User Guide has been updated and reflects the plan. In Developer Guide, I could not find sequence diagrams in the implementations. Generally, its good to add visual cues to aid the reader.
This is particularly important when explaining how to perform features such as adding events on timetables, locking and unlocking of the app, etc.

In the User Guide its good to see the plan for different features correlating to different version releases.

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.

7 participants