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

Improve the code and add more details to the doc #183

Merged
merged 1 commit into from Jun 28, 2019

Conversation

Projects
None yet
4 participants
@hacksparrow
Copy link
Member

commented Jun 28, 2019

Closes #69

@hacksparrow hacksparrow force-pushed the improve-example branch from 9e65319 to 5b17e64 Jun 28, 2019

@b-admike
Copy link
Member

left a comment

LGTM overall 👍. My suggestion for the login endpoint is to have a simple paragraph of what's going on and point to the authentication docs instead of explaining what each line of code does.

Show resolved Hide resolved README.md Outdated

@hacksparrow hacksparrow force-pushed the improve-example branch 2 times, most recently from d5c7fd9 to 8f29afa Jun 28, 2019

@hacksparrow hacksparrow force-pushed the improve-example branch 2 times, most recently from b1618e8 to 2bc33e9 Jun 28, 2019

@jannyHou
Copy link
Contributor

left a comment

@hacksparrow LGTM in general 👍 , added a few minor comments

README.md Outdated
1. `User` - representing the users of the system.
2. `Product` - a model which is mapped to a remote service by `services/recommender.service`.
3. `ShoppingCartItem` - a model for representing purchases.
4. `ShoppingCart` - a model to represent a user's shopping cart, can contain many items (`item`) of the type `ShoppingCartItem`.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jun 28, 2019

Contributor

item --> items?

README.md Outdated

This app has five services:

1. `services/hash.password.bcryptjs` - responsible for generating and comparing password hashes.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jun 28, 2019

Contributor

Nitpick: I would suggest organizing the auth related services together

  1. services/recommender.service - responsible for connecting to a "remote" server and getting recommendations for a user. The API endpoint at GET /users​/{userId}​/recommend, is made possible by this service.
  2. services/user-service - responsible for verifying if user exists and the submitted password matches that of the existing user.
  3. services/hash.password.bcryptjs - responsible for generating and comparing password hashes.
  4. services/validator - responsible for validating email and password when a new user is created.
  5. services/jwt-service - responsible for generating and verifying JSON Web Token.
5. `services/validator` - responsible for validating email and password when a new user is created.

## Login

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jun 28, 2019

Contributor

Note: This app contains a login endpoint for the purpose of spike and demo, the authentication for the CRUD operations and navigational endpoints of model User is still in build.

README.md Outdated

The endpoint for logging in a user is a `POST` request to `/users/login`.

Once the credentials are extracted, the logging-in implementation at the controller level is just a four step process. This level of simplicity is made possible by the use of the `UserService` service provided by

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jun 28, 2019

Contributor

Good abstraction for the verification logic in login, we have an auth tutorial showing how the JWT strategy works in https://loopback.io/doc/en/lb4/Authentication-Tutorial.html which you can refer to :)

@jannyHou jannyHou force-pushed the improve-example branch from ce0bf25 to 166e025 Jun 28, 2019

Show resolved Hide resolved README.md Outdated
@b-admike
Copy link
Member

left a comment

Thanks @jannyHou for adding feedback and making changes. LGTM.

README.md Outdated
## Login
## Authentication

*Note: This app contains a `login` endpoint for the purpose of spike and demo, the authentication for the CRUD operations and navigational endpoints of model User is still in build.*

This comment has been minimized.

Copy link
@b-admike

b-admike Jun 28, 2019

Member

nitpick: still in build. -> still in progress.?

@jannyHou jannyHou force-pushed the improve-example branch 2 times, most recently from 852e228 to 31cff9b Jun 28, 2019

@nabdelgadir nabdelgadir force-pushed the improve-example branch 2 times, most recently from 84f4cc7 to 03b867b Jun 28, 2019

docs: add more details to the doc
Add more details to the doc and improve code

Signed-off-by: Hage Yaapa <hage.yaapa@in.ibm.com>
Signed-off-by: jannyHou <juehou@ca.ibm.com>
Signed-off-by: Nora <nora.abdelgadir@ibm.com>
Co-authored-by: Janny <juehou@ca.ibm.com>

@nabdelgadir nabdelgadir force-pushed the improve-example branch from 03b867b to e230ba3 Jun 28, 2019

@nabdelgadir nabdelgadir merged commit 20438d9 into master Jun 28, 2019

3 checks passed

DCO DCO
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@delete-merged-branch delete-merged-branch bot deleted the improve-example branch Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.