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

Finalise PPP for Wei Jie #194

Merged
merged 10 commits into from
Nov 9, 2020
Merged

Conversation

WeiJie96
Copy link

@WeiJie96 WeiJie96 commented Nov 5, 2020

Close #192

@WeiJie96 WeiJie96 added this to the v1.4 milestone Nov 5, 2020
@WeiJie96 WeiJie96 marked this pull request as ready for review November 6, 2020 17:46
@codecov-io
Copy link

Codecov Report

Merging #194 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #194   +/-   ##
=========================================
  Coverage     68.35%   68.35%           
  Complexity      562      562           
=========================================
  Files            93       93           
  Lines          1833     1833           
  Branches        218      218           
=========================================
  Hits           1253     1253           
  Misses          499      499           
  Partials         81       81           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f778c6...26524c2. Read the comment docs.

Copy link

@Joven-Heng Joven-Heng left a comment

Choose a reason for hiding this comment

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

I was really thinking of us adding one more section as difficulties faced, especially emphasising on how we were a three man team and like the different unique difficulties we faced doing this project. After all, our problems that we have are likely extremely different compared to a 5 man team.

Also, I am not going to request changes or what ya >< Its up to you to do these changes, after all this your individual component HAHA

* Highlights:
This enhancement affects existing commands (such as `ListCommand` and `AddCommand`), and commands to be added in future (such as `FindCommand`).
It required an in-depth analysis of design alternatives.
The implementation too was challenging as it required changes to existing commands.

Choose a reason for hiding this comment

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

Just a suggestion, maybe you can elaborate on how is it challenging, or highlight one or two cases that were really difficult for you. Or you can edit the DG, then link it there also.

Copy link
Author

Choose a reason for hiding this comment

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

Updated

* Highlights: **Coming Soon**
* What it does: It allows the user to add active clients into the archive. The opposite can be done as well; archived clients can be made active again.
To support the archiving feature, users can also view the archive, as well as to switch back to view the active clients.
* Justification: This feature improves the product significantly because a user can add clients which are not currently relevant to the user into the archive.

Choose a reason for hiding this comment

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

For this, I think it may be better if you differentiate between insurance agent and client. You used user a few times, it might be confusing, besides linking it back to the target audience would help to solidify this case.

Copy link
Author

Choose a reason for hiding this comment

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

I would make the change, but actually our DG and UG references "user" most of the time; but for PPP I'll update

* PRs reviewed (examples with non-trivial review comments):
[\#50](https://github.com/AY2021S1-CS2103-T16-2/tp/pull/50), [\#97](https://github.com/AY2021S1-CS2103-T16-2/tp/pull/97), [\#124](https://github.com/AY2021S1-CS2103-T16-2/tp/pull/124)
* Maintained the issue tracker.

Choose a reason for hiding this comment

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

I would say you are a really solid code quality checker. Anyways I would suggest like we meet on Monday and go through our good points (or bad) in person, since it is much easier to like see others pros and cons instead of writing it yourself.

Copy link
Author

Choose a reason for hiding this comment

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

Added a short point for this 👍

* What it does: **Coming Soon**
* Justification: **Coming Soon**
* Highlights: **Coming Soon**
* What it does: It allows the user to add active clients into the archive. The opposite can be done as well; archived clients can be made active again.

Choose a reason for hiding this comment

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

Use the feature instead of it. (Just minor thing)

Copy link
Author

Choose a reason for hiding this comment

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

Done

An initial detailed proposal was suggested to save the archive in a separate `.json` file, using appropriate additional classes and methods.
However, this would be very time-consuming as care needs to be taken to ensure that the implementation of the reading and saving of the 2 different storages, and updating of the models are correct.
Given the tight timeline and limitations of a 3-person team, the current implementation of storing the active and archived clients in the same file was decided instead.
More details of the analysis can be found in the Developer Guide.

Choose a reason for hiding this comment

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

link to dg

@Joven-Heng Joven-Heng merged commit 0793889 into AY2021S1-CS2103-T16-2:master Nov 9, 2020
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.

Finalise PPP
3 participants