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

[W13-1] InsuRen #91

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

Conversation

zioul123
Copy link

@zioul123 zioul123 commented Oct 2, 2018

Copy link

@asiddharth asiddharth left a comment

Choose a reason for hiding this comment

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

Good job! Some minor UG and DG comments

@@ -50,19 +50,25 @@ e.g. typing *`help`* and pressing kbd:[Enter] will open the help window.
* Items in square brackets are optional e.g `n/NAME [t/TAG]` can be used as `n/John Doe t/friend` or as `n/John Doe`.
* Items with `…`​ after them can be used multiple times including zero times e.g. `[t/TAG]...` can be used as `{nbsp}` (i.e. 0 times), `t/friend`, `t/friend t/family` etc.
* Parameters can be in any order e.g. if the command specifies `n/NAME p/PHONE_NUMBER`, `p/PHONE_NUMBER n/NAME` is also acceptable.
* All commands have a shorthand version for easy access. Simply replace the command word with the shorthand. All other syntax is identical.

Choose a reason for hiding this comment

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

This file still reads as a user guide for AB4

import seedu.address.model.Model;
import seedu.address.model.person.Person;
/**
* Adds a list of pre-established Valid persons to the address book.

Choose a reason for hiding this comment

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

Good job for the header comment!

*/
@Override
public CommandResult execute(Model model, CommandHistory history) {

Choose a reason for hiding this comment

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

Extra blank line. Use blank lines sparingly. It's good to have blank lines to separate logical code blocks, but too many blank lines will increase the height of the code, which increases the amount of scrolling.

@@ -0,0 +1,92 @@
//@@author chantca95

Choose a reason for hiding this comment

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

Maybe better to include this as a part of the class header comment


return new Person(name, phone, email, address, tags, meeting);
}

Choose a reason for hiding this comment

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

Extra blank line. Use blank lines sparingly. It's good to have blank lines to separate logical code blocks, but too many blank lines will increase the height of the code, which increases the amount of scrolling.

* <<AboutUs#, About Us>>
* <<ContactUs#, Contact Us>>

== Acknowledgements

* Some parts of this sample application were inspired by the excellent http://code.makery.ch/library/javafx-8-tutorial/[Java FX tutorial] by
_Marco Jakob_.
* This project was evolved from the "AddressBook-Level4" project created by SE-EDU initiative at https://github.com/se-edu/

Choose a reason for hiding this comment

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

Good job acknowledging AB4!

+
Use case resumes at step 1.


Choose a reason for hiding this comment

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

Maybe good to add some NFRs specific to the product.

Choose a reason for hiding this comment

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

Same comment again.

Copy link

@asiddharth asiddharth left a comment

Choose a reason for hiding this comment

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

Good job overall team.
Here are a few comments in addition to the ones pointed out.

  1. Please change the product description.
    13-2_desc

  2. Upfdate the Contact Us page with the appropriate links/emails.
    13-1cu

  3. Update the introduction in UG to suit your product
    change_intro_13-1

  4. May want to rename your jar and link it correctly.
    13-2_ug1

  5. Please add some coming in 2.0 features.
    2 013-1

  6. Test coverage seems good. Try to add come more to get coverage > 90%
    coverage

README.adoc Outdated
https://www.codacy.com/app/damith/addressbook-level4?utm_source=github.com&utm_medium=referral&utm_content=se-edu/addressbook-level4&utm_campaign=Badge_Grade[image:https://api.codacy.com/project/badge/Grade/fc0b7775cf7f4fdeaf08776f3d8e364a[Codacy Badge]]
https://travis-ci.org/CS2103-AY1819S1-W13-1/main[image:https://travis-ci.org/CS2103-AY1819S1-W13-1/main.svg?branch=master[Build Status]]
https://ci.appveyor.com/project/denzelchung/main-1gn9v/branch/master[image:https://ci.appveyor.com/api/projects/status/0cw1hdcgcqu31k9l/branch/master?svg=true[Build status]]
https://coveralls.io/github/CS2103-AY1819S1-W13-1/main?branch=master[image:https://coveralls.io/repos/github/CS2103-AY1819S1-W13-1/main/badge.svg?branch=master[Coverage Status]]
https://gitter.im/se-edu/Lobby[image:https://badges.gitter.im/se-edu/Lobby.svg[Gitter chat]]

Choose a reason for hiding this comment

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

Can remove/change this link as it points to AB4

* <<AboutUs#, About Us>>
* <<ContactUs#, Contact Us>>

== Acknowledgements

* Some parts of this sample application were inspired by the excellent http://code.makery.ch/library/javafx-8-tutorial/[Java FX tutorial] by
_Marco Jakob_.
* This project was evolved from the "AddressBook-Level4" project created by SE-EDU initiative at https://github.com/se-edu/
* Libraries used: https://github.com/TestFX/TestFX[TextFX], https://bitbucket.org/controlsfx/controlsfx/[ControlsFX], https://github.com/FasterXML/jackson[Jackson], https://github.com/google/guava[Guava], https://github.com/junit-team/junit5[JUnit5]

Choose a reason for hiding this comment

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

Good job on :

Adding UI Mockup
Modifying home page for the product.
Acknowledging AB4
Adding correct links for travis.

{empty}[http://www.comp.nus.edu.sg/~damithch[homepage]] [https://github.com/damithc[github]] [<<johndoe#, portfolio>>]
=== Auyok Sean
image::a19sean.png[width="150", align="left"]
{empty}[https://github.com/A19Sean[github]]

Choose a reason for hiding this comment

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

May be helpful to add the portfolio for each team member. Will be useful to keep everything updated as you progress with the project.

https://nus-cs2103-ay1819s1.github.io/cs2103-website/admin/project-deliverables.html#deliverable-project-portfolio-page-ppp

@@ -94,11 +106,42 @@ Edits the phone number and email address of the 1st person to be `91234567` and
* `edit 2 n/Betsy Crower t/` +
Edits the name of the 2nd person to be `Betsy Crower` and clears all existing tags.

==== Editing by name

Choose a reason for hiding this comment

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

Would be good to add some examples with the shorthand. Same comment applies to all commands.

* `import`
* A file browser will pop up as shown below:
+
image::import.png[width="790"]

Choose a reason for hiding this comment

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

Good job here!

package seedu.address.logic.commands;

import static java.util.Objects.requireNonNull;

Choose a reason for hiding this comment

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

Extra blank line. Use blank lines sparingly. It's good to have blank lines to separate logical code blocks, but too many blank lines will increase the height of the code, which increases the amount of scrolling.

+
Use case resumes at step 1.


Choose a reason for hiding this comment

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

Same comment again.

+
[none]
** 1a1. AddressBook shows an error message.
Step 1 is repeated until the data entered are correct.

Choose a reason for hiding this comment

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

Can modify this to

Use case resumes at Step 1

*MSS*

1. User inputs the customers’ meeting times in their address book entries.
2. User searches for meetings with clients by time, and AddressBook returns the client details if there is a meeting scheduled at that time.

Choose a reason for hiding this comment

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

Each step should alternate between the user and InsuRen as the actor.

*MSS*

1. User requests to add client, specifying the compulsory field (name) and non-compulsory fields (address, email, phone number, and tags).
2. AddressBook stores the new client, and displays a confirmation message.

Choose a reason for hiding this comment

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

Should this not be InsuRen?

Copy link

@asiddharth asiddharth left a comment

Choose a reason for hiding this comment

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

Hi team!
Here are some suggestions regarding your project

  1. w13-1
    Make sure the build passes before the testing session.

Some DG related comments :
Your DG looks very similar to that of the parent repository. You might want to consider doing some of the following things.
1.
w13-1_sc1
As your project is a morph of AB4, it would be better to remove the links to AB1, AB2, AB3 and AB4 in your autopublished website.

2.Good job adding feature implementation details and the activity diagrams for the features. Would be nice to add additional sequence, class diagrams as well for other features if applicable.
3. Many references to AB4. Could help to remove references related to AB4 not relevant to your project application.
4. Add more NFRs?

AyushChatto and others added 19 commits October 29, 2018 22:19
* master:
  Fix formatting issues
  Change meetings alias
  add diagrams for import and export command
  safety store
  Fix formatting issues
  Fix formatting issues
  Check for meeting prefix
  Update test cases
  Add search by meeting command
  WIP
  WIP

# Conflicts:
#	src/main/java/seedu/address/logic/parser/AddressBookParser.java
* tag-delete-tests:
  Update expectedExport.csv to match TypicalPersons.java
  Implement TagCommandParserTest
  Edit TypicalPersons to have separate persons to test tag functionality
  Revert typicalPersonsAddressBook.xml to original addressbook xml file
  Add author tag
  Add TagDeleteCommandTest
  Update TagCommandTest
  Update typicalPersonsAddressBook to a functioning contact list
zioul123 and others added 30 commits November 12, 2018 21:40
Change the "do not use edit or delete" in tags to a recommendation.
* master:
  typo fix and make import sample screenshots bigger 2
  typo fix and make import sample screenshots bigger
  Fix github links.
  Update user-stories, use-case, nfrs and manual testing for tag commands in dev guide.
  Change conditions to lower case.
  Change the "do not use edit or delete" in tags to a recommendation.
* master:
  Update ppp.
  Fix Typo in Portfolio
* master:
  Change image sizes.
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