feat: implement basic user component by virkt25 · Pull Request #5 · loopbackio/loopback4-example-shopping

virkt25 · 2018-08-16T21:18:28Z

implements the remainder of loopbackio/loopback-next#1480

raymondfeng · 2018-08-17T00:50:04Z

package.json

    "@loopback/rest": "^0.19.2",
-    "@loopback/openapi-v3": "^0.12.2"
+    "@types/bcrypt": "^2.0.0",
+    "bcrypt": "^3.0.0",


Please use https://github.com/dcodeIO/bcrypt.js instead.

Any reason for using that modules over the one I'm using?

bcrypt is a binary add-on and it requires compilation.

And we don't want that? CI was failing before because I was running install with --ignore-scripts. The JS version is about 30% slower according to their own docs.

Plain JS module is much more desirable for an example app. It will remove a lot of support issues in the future.

shimks

fire

shimks · 2018-08-16T21:22:25Z

package.json

    "@loopback/repository": "^0.14.2",
    "@loopback/rest": "^0.19.2",
-    "@loopback/openapi-v3": "^0.12.2"
+    "@types/bcrypt": "^2.0.0",


reinstall as dev dependency?

shimks · 2018-08-16T21:26:35Z

src/controllers/user.controller.ts

+    }
+
+    // Validate Password Length
+    if (obj.password.length < 8) {


Is this a temporary solution to the fact that we are not supporting authentication atm? I would have assumed that the password at the framework level would have been hashed

We don't have a default User model like we did in LB3 which is where these checks / hashing would've been implemented. So for now people would have the flexibility to implement User how they want.

shimks · 2018-08-16T21:28:56Z

test/acceptance/ping.controller.acceptance.ts

 import {createClientForHandler, supertest} from '@loopback/testlab';
 import {RestServer} from '@loopback/rest';
-import {ShoppingApplication} from '../';
+import {ShoppingApplication} from '../..';


This is probably a good reminder for us to change the template generated by our app generator so that this file lives in the acceptance folder

@shimks @virkt25 could you please create a follow up issue or open a pull request to fix the template? Otherwise we are most likely going to forget about this.

shimks · 2018-08-17T15:05:15Z

test/acceptance/user.controller.acceptance.ts

+      .send(user)
+      .expect(200)
+      // tslint:disable-next-line:no-any
+      .expect(function(res: any) {


it's not all that important, but we can use the supertest's promise support here:

const response = await client.post('/users').send(user).expect(200); // assertions

src/datasources/db.datasource.ts

@@ -0,0 +1,14 @@
+import {inject} from '@loopback/core';


b-admike · 2018-08-17T16:57:32Z

bin/travis.sh

+
+if [ $TASK = "test" ]; then
+  echo "TASK => MONGODB STARTUP DELAY"
+  sleep 10


Can we have a check for mongodb service / process here instead of using sleep?

We would have to check and sleep / something if the service isn't up and ready yet anyways. This is the fix recommended by Travis themselves, documented here: https://docs.travis-ci.com/user/database-setup/#mongodb-does-not-immediately-accept-connections

b-admike · 2018-08-17T16:58:50Z

bin/setup.sh

@@ -0,0 +1,3 @@
+docker rm -f mongodb_c


Can we have a variable to represent the name of the container? Feel free to ignore as this can be done later if need be.

b-admike · 2018-08-17T17:01:02Z

src/controllers/user.controller.ts

+  ) {}
+
+  @post('/users')
+  async create(@requestBody() obj: User): Promise<User> {


nitpick: I suggest we rename obj to userObj or user or something along those lines

That's the default we generate using the lb4 controller command but a good call. Will update.

b-admike · 2018-08-17T17:04:10Z

src/controllers/user.controller.ts

+  @get('/users/{id}')
+  async findById(@param.path.string('id') id: string): Promise<User> {
+    const user = await this.userRepository.findById(id);
+    delete user.password;


I wonder if juggler can take care of this for us using some sort of flag.

shimks

In this PR, do we want to have a separate datasource and docker configured for just testing? I'd imagine we don't want to run any tests on potentially existing data

shimks · 2018-08-17T17:06:12Z

test/acceptance/user.controller.acceptance.ts

+      .expect(415);
+  });
+
+  it('returns a user with given id when POST /user/{id} is invoked', async () => {


POST -> GET

shimks · 2018-08-17T17:08:48Z

test/acceptance/user.controller.acceptance.ts

+  });
+
+  it('returns a user with given id when POST /user/{id} is invoked', async () => {
+    const userWithId = Object.assign({}, user, {id: userId});


Instead of setting userId in a separate test, I think it's better for the user to be created in the test (through the repository), set the userId, and then call the endpoint. This allows tests to not be dependent upon each other.

Also, the created user instances need to be deleted after each tests

Signed-off-by: virkt25 <taranveer@virk.cc>

bajtos · 2018-08-23T10:25:46Z

src/controllers/user.controller.ts

+
+    // Save & Return Result
+    const savedUser = await this.userRepository.create(user);
+    delete savedUser.password;


While this is good enough for the initial implementation, it is also very brittle. We should find a more robust way how to allow models to hide certain properties from toJSON output.

In the past, I had very good experience with moving the password to a different model (table/collection) and use hasMany relation. As a nice side effect, by keeping a hash of all previous passwords, we can easily implement a password policy like "cannot reuse the same password".

bajtos · 2018-08-23T10:26:11Z

src/controllers/user.controller.ts

+  @get('/users/{id}')
+  async findById(@param.path.string('id') id: string): Promise<User> {
+    const user = await this.userRepository.findById(id, {
+      fields: {password: false},


Ditto here.

bajtos · 2018-08-23T10:28:26Z

test/acceptance/ping.controller.acceptance.ts

 import {createClientForHandler, supertest} from '@loopback/testlab';
 import {RestServer} from '@loopback/rest';
-import {ShoppingApplication} from '../';
+import {ShoppingApplication} from '../..';


@shimks @virkt25 could you please create a follow up issue or open a pull request to fix the template? Otherwise we are most likely going to forget about this.

bajtos · 2018-08-23T10:28:59Z

test/acceptance/user.controller.acceptance.ts

+  let client: supertest.SuperTest<supertest.Test>;
+  const userRepo = new UserRepository(new UserDataSource());
+
+  const user = {


I'd prefer us to follow our own best practices, see Use data builders.

virkt25 self-assigned this Aug 16, 2018

virkt25 requested review from a team and raymondfeng August 16, 2018 21:18

virkt25 force-pushed the user branch from db7117f to 819fda8 Compare August 16, 2018 21:19

raymondfeng reviewed Aug 17, 2018

View reviewed changes

shimks approved these changes Aug 17, 2018

View reviewed changes

b-admike reviewed Aug 17, 2018

View reviewed changes

shimks reviewed Aug 17, 2018

View reviewed changes

shimks approved these changes Aug 17, 2018

View reviewed changes

raymondfeng approved these changes Aug 17, 2018

View reviewed changes

b-admike approved these changes Aug 17, 2018

View reviewed changes

virkt25 added 2 commits August 17, 2018 16:56

feat: implement basic user component

871c1ce

Signed-off-by: virkt25 <taranveer@virk.cc>

ci: update config for tests with mongodb

666dd09

Signed-off-by: virkt25 <taranveer@virk.cc>

virkt25 force-pushed the user branch from 0df6f5e to 666dd09 Compare August 17, 2018 20:56

virkt25 merged commit 0eeb580 into master Aug 17, 2018

virkt25 deleted the user branch August 17, 2018 23:24

bajtos reviewed Aug 23, 2018

View reviewed changes

bajtos mentioned this pull request Aug 23, 2018

feat: adding artifacts for shopping cart #4

Merged

This was referenced Sep 4, 2018

[example-shopping] Cleaup Tests loopbackio/loopback-next#1669

Closed

[User] cleanup tests - use data builders #6

Closed

[Spike][User] Refactor password implementation #7

Closed

Conversation

virkt25 commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shimks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shimks Aug 16, 2018 •

edited

Loading

shimks Aug 16, 2018 •

edited

Loading