user order history component #12

virkt25 · 2018-09-17T13:53:47Z

So I ran into an issue with MongoDB coercing the userId into an Object for the orders so I made changes locally to the connector to take in an option to disable it for each method that I've used. Will make a PR against the connector to fix that.

Also not sure how to pass in a filter right now for testing.

Also ... the API expects userId to be set on an order but the repository throws an error saying it can't be changes even though it's the same. I think the constraint check should be updated to not throw in this case.

fixes loopbackio/loopback-next#1483

blocked by loopbackio/loopback-connector-mongodb#467

raymondfeng · 2018-09-17T14:30:54Z

src/controllers/user-order.controller.ts

+    @param.path.string('userId') userId: string,
+    @requestBody() order: Order,
+  ): Promise<Order> {
+    if (userId !== order.userId) {


We should check existence of order.UserId first. Otherwise, we throw if userId is not in the order.

We don't need to as requestBody validation does that for us. If you make a request with an order not containing a userId, the following error is thrown:

{ "error": { "statusCode": 422, "name": "UnprocessableEntityError", "message": "The request body is invalid. See error object `details` property for more info.", "details": [ { "path": "", "code": "required", "message": "should have required property 'userId'", "info": { "missingProperty": "userId" } } ] } }

Ideally, we should allow the order to not have userId as the purpose of relational method is to enforce the referential integrity. But that can be out of scope for this PR.

raymondfeng · 2018-09-17T14:32:23Z

src/controllers/user-order.controller.ts

+      },
+    },
+  })
+  async create(


For a controller, is createOrder a better name?

raymondfeng · 2018-09-17T14:33:03Z

src/controllers/user-order.controller.ts

+      },
+    },
+  })
+  async patch(


I would like to see a name meaningful for business logic, such as updateOrders.

update does not make it clear whether it's a full replace or a partial patch. Let's use patchOrders please.

raymondfeng · 2018-09-17T14:34:03Z

src/controllers/user-order.controller.ts

+  })
+  async find(
+    @param.path.string('userId') userId: string,
+    @param.query.string('filter') filter?: Filter,


See loopbackio/loopback-next#1679

Will update once it lands.

raymondfeng · 2018-09-17T14:36:03Z

src/models/shopping-cart.model.ts

@@ -25,6 +25,10 @@ export class ShoppingCartItem {
   */
  @property()
  price?: number;
+
+  constructor(item: Partial<ShoppingCartItem>) {
+    Object.assign(this, item);


Is super(item); better?

This is not a derived class, there is no super.

raymondfeng · 2018-09-17T14:39:06Z

Travis is failing with the following error:

․․․․․Unhandled error in POST /users/5b9fb25c1185f50a892ab737/orders: 500 TypeError: Class constructor ShoppingCartItem cannot be invoked without 'new'
    at /home/travis/build/strongloop/loopback4-example-shopping/node_modules/loopback-datasource-juggler/lib/list.js:68:16
    at Array.forEach (<anonymous>)
    at new List (/home/travis/build/strongloop/loopback4-example-shopping/node_modules/loopback-datasource-juggler/lib/list.js:66:9)
    at List (/home/travis/build/strongloop/loopback4-example-shopping/node_modules/loopback-datasource-juggler/lib/list.js:17:12)

I think we need to fix juggler to support class as a model function.

virkt25 · 2018-09-17T14:56:46Z

I think we need to fix juggler to support class as a model function.

See loopbackio/loopback-datasource-juggler#1624

raymondfeng · 2018-09-17T16:41:24Z

The juggler issue is fixed. Please pick up [email protected]

raymondfeng · 2018-09-17T16:42:01Z

Please sign off the commits.

src/controllers/user-order.controller.ts

bajtos · 2018-09-18T08:32:52Z

src/datasources/order.datasource.ts

+import {juggler, AnyObject} from '@loopback/repository';
+const config = require('./mongo.datasource.json');
+
+export class OrderDataSource extends juggler.DataSource {


Is there any particular reason for having different datasource classes for Customer and Order models? Typically, a single datasource should be shared by all models persisted by the same database.

Yes, the reason was to use the same connection information while setting a different database name in the Class for each model.

That said, we default the database name based on the model so I've updated the PR to just have a single DataSource shared by all models.

Yes, the reason was to use the same connection information while setting a different database name in the Class for each model.

I think you may be confusing database and table name?

In SQL world, a single database server can provide multiple different databases as a kind of a multi-tenancy. Each database can have multiple tables. In LoopBack, we map models to tables.

From my experience, a single application typically stores data in a single database. This make it easy to leverage SQL features like the JOIN statement. I am not sure is there are any SQL databases that would support JOIN statements across different databases.

raymondfeng · 2018-09-19T21:48:02Z

test/acceptance/user-order.controller.acceptance.ts

@@ -58,7 +58,7 @@ describe('UserOrderController acceptance tests', () => {

    expect(res.body.orderId).to.be.a.String();
    delete res.body.orderId;
-    expect(res.body).to.deepEqual(order);
+    expect(res.body).to.deepEqual(JSON.parse(JSON.stringify(order)));


order.toJSON() should do the trick, isn't it?

IIRC, order.toJSON() returns an object that includes properties with undefined values.

// our expectation { title: 'My Todo', isCompleted: undefined }

On the other hand, the actual JSON string send by the server and parsed by the client in the test leaves out properties set to undefined.

// the actual value { title: 'My Todo' }

AFAIK, the deep equality check distinguishes between a property set to undefined and a property that does not exist on the object at all.

As a result, the expected object obtained via .toJSON() is rejected as being different from the actual value parsed from a JSON string.

I have encountered this issue myself in loopbackio/loopback-next#1679, I am going to add a new testlab helper to deal with that. See https://github.com/strongloop/loopback-next/blob/b4bba3d09d663b2919bd6ca3c5a5430d70e1d8c7/packages/testlab/src/misc.ts

Based on this discussion, I am going to open a new pull request to add this helper only, so that we can start using it sooner.

Based on this discussion, I am going to open a new pull request to add this helper only, so that we can start using it sooner.

See loopbackio/loopback-next#1733

See also loopbackio/loopback-next#1734

raymondfeng · 2018-09-19T21:49:34Z

test/acceptance/user-order.controller.acceptance.ts

@@ -183,11 +183,11 @@ describe('UserOrderController acceptance tests', () => {
      userId: '',
      total: 99.99,
      products: [
-        {
+        new ShoppingCartItem({


Why do we have to use ShoppingCartItem class?

bajtos · 2018-09-20T07:34:21Z

test/acceptance/user-order.controller.acceptance.ts

+  it('creates an order for a user with a given orderId', async () => {
+    const user = await givenAUser();
+    const userId = user.id.toString();
+    const order = {


Have you considered using a test-data builder here and specify only few property values that are relevant for this test?

Acceptance tests are verifying that all application parts work together, they should not be verifying behavior of individual components (we have unit and integration tests for that).

In this particular case, we should rely on integration tests to verify that all properties provided to orderRepo.create are correctly stored in the database and retrieved back. IMO, we should use const order = givenAnOrder() here, because the actual order details are not important for this test.

bajtos · 2018-09-20T07:41:41Z

test/acceptance/user-order.controller.acceptance.ts

+    const user = await givenAUser();
+    const userId = user.id.toString();
+    const order = givenAOrder();
+    order.userId = userId;


A test-data builder typically accepts a Partial object allowing callers to customize/overwrite default values provided by the builder.

const order = givenAnOrder({userId});

bajtos · 2018-09-20T07:42:46Z

test/acceptance/user-order.controller.acceptance.ts

+      .expect(400);
+  });
+
+  it.skip('throws an error when creating an order for a non-existent user', async () => {});


Please create a follow-up issue (if you haven't do so already) and add a code comment pointing to that issue. (Explain that this test is skipped because the linked issue is not implemented yet.)

Also note that the test-function is optional for both it and it.skip. This line can be simplified as follows:

it.skip('throws an error when creating an order for a non-existent user');

bajtos · 2018-09-20T07:43:15Z

test/acceptance/user-order.controller.acceptance.ts

+      expect(getOrder2).to.deepEqual([savedOrder2]);
+    });
+
+    it.skip('patches orders matching filter for a given user', async () => {});


Ditto.

Please create a follow-up issue (if you haven't do so already) and add a code comment pointing to that issue. (Explain that this test is skipped because the linked issue is not implemented yet.)

bajtos · 2018-09-20T07:43:30Z

test/acceptance/user-order.controller.acceptance.ts

+      expect(getOrder2).to.deepEqual([]);
+    });
+
+    it.skip('deletes orders matching filter for a given user', () => {});


This looks like a duplicated test case?

The first test is to test that all orders are deleted while this one would test deleting only 1 out of the 2 orders by passing an appropriate filter.

bajtos · 2018-09-20T07:45:07Z

test/acceptance/user-order.controller.acceptance.ts

+
+  async function givenAUser() {
+    const user = {
+      email: '[email protected]',


Please use @example.com address. See https://loopback.io/doc/en/contrib/style-guide.html#email-examples

bajtos · 2018-09-20T07:49:34Z

test/acceptance/user.controller.acceptance.ts

    // MongoDB returns an id object we need to convert to string
    // since the REST API returns a string for the id property.
    newUser.id = newUser.id.toString();

    await client.get(`/users/${newUser.id}`).expect(200, newUser.toJSON());
  });

-  function givenAnApplication() {
+  async function setupApplication() {


setupApplication is duplicated in acceptance tests. Can we extract a shared helper please?

I understand setupApplication is updating global variables. Can we leverage object destructuring as an easy-to-use solution?

// the helper async function setupApplication() { // ... return {app, server, client}; } // usage in tests let app: Application; let server: RestServer; let client: supertest.SuperTest<supertest.Test>; before(async () => { // notice no "const", "let" or "var" {app, server, client} = await setupApplication(); });

Also since the function is not setting only the app, but also the server and the client, I think we should use a different name, e.g. setupAcceptanceWorkbench.

raymondfeng · 2018-09-24T20:18:26Z

src/controllers/user-order.controller.ts

+      },
+    },
+  })
+  async createOrders(


It should be createOrder as the method only accepts one order.

src/controllers/user-order.controller.ts

bajtos · 2018-09-25T09:55:26Z

We should probably introduce singular controllers for these operations where the orderId is known.

/users/{userId}/orders/{orderId}

We have discussed this in the past, when implementing "hasMany" relation. Updating a single model via relation endpoint does not make much sense. Once you know the id of the target model (orderId), you can update the model directly via non-relation API (PATCH /orders/{orderId}).

The endpoint /users/{userId}/orders/{orderId} would make sense if we were using composite primary keys (e.g. an order is identified by (userId, orderId), think of how GitHub repositories are identified), but our current implementation does not support composite keys AFAIK.

Will make an issue.

If the issue has been created, could you please cross-link that issue with this pull request

bajtos · 2018-09-25T09:57:28Z

test/acceptance/user-order.controller.acceptance.ts

+
+    // TODO(virkt25): Implement after issue below is fixed.
+    // https://github.com/strongloop/loopback-next/issues/100
+    it.skip('patches orders matching filter for a given user');


FWIW, this can be done now - see loopbackio/loopback-next#1679.

I think it's best to land this PR as-is and leave the fix for a follow-up pull request.

bajtos · 2018-09-25T09:58:56Z

test/acceptance/helper.ts

+export async function setupApplication(): Promise<setupApp> {
+  const app = new ShoppingApplication({
+    rest: {
+      port: 0,


Please use givenHttpServerConfig, see loopbackio/loopback-next#1738.

const app = new ShoppingApplication { rest: givenHttpServerConfig(), });

bajtos · 2018-09-25T09:59:20Z

.travis.yml

+  # Add an IPv6 config - see the corresponding Travis issue
+  # https://github.com/travis-ci/travis-ci/issues/8361
+  - if [ "${TRAVIS_OS_NAME}" == "linux" ]; then
+    sudo sh -c 'echo 0 > /proc/sys/net/ipv6/conf/all/disable_ipv6';


This is not needed if you use givenHttpServerConfig, see my other comment.

bajtos · 2018-09-25T09:59:59Z

test/acceptance/helper.ts

+
+  const client = createRestAppClient(app);
+
+  return {app: app, client: client};


Nitpick: This can be simplified as follows:

return {app, client};

bajtos

Two more tiny comments to consider. No further review is necessary as far as I am concerned.

bajtos · 2018-09-25T16:52:09Z

test/acceptance/shopping-cart.controller.acceptance.ts


 describe('ShoppingCartController', () => {
  let app: ShoppingApplication;
-  let server: RestServer;
  let client: supertest.SuperTest<supertest.Test>;


Please use Client instead of supertest.SuperTest<supertest.Test>.

bajtos · 2018-09-25T16:52:35Z

test/acceptance/user-order.controller.acceptance.ts

+
+describe('UserOrderController acceptance tests', () => {
+  let app: ShoppingApplication;
+  let client: supertest.SuperTest<supertest.Test>;


Ditto, use Client instead of supertest.SuperTest<supertest.Test>.

I fixed that with #16

Signed-off-by: virkt25 <[email protected]>

Signed-off-by: Raymond Feng <[email protected]>

virkt25 requested review from raymondfeng, b-admike, bajtos, dhmlau, hacksparrow and jannyHou September 17, 2018 13:57

raymondfeng reviewed Sep 17, 2018

View reviewed changes

src/controllers/user-order.controller.ts Outdated

},

},

})

async create(

Copy link

Contributor

raymondfeng Sep 17, 2018

Choose a reason for hiding this comment

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

For a controller, is createOrder a better name?

raymondfeng reviewed Sep 17, 2018

View reviewed changes

virkt25 self-assigned this Sep 17, 2018

bajtos reviewed Sep 18, 2018

View reviewed changes

src/controllers/user-order.controller.ts Show resolved Hide resolved

bajtos reviewed Sep 18, 2018

View reviewed changes

virkt25 force-pushed the orders branch 2 times, most recently from 92a0cf5 to 6a6f99b Compare September 19, 2018 17:51

raymondfeng reviewed Sep 19, 2018

View reviewed changes

bajtos reviewed Sep 20, 2018

View reviewed changes

bajtos mentioned this pull request Sep 20, 2018

feat(testlab): add createRestAppClient(), simplify usage in tests loopbackio/loopback-next#1734

Merged

7 tasks

raymondfeng reviewed Sep 24, 2018

View reviewed changes

src/controllers/user-order.controller.ts Show resolved Hide resolved

raymondfeng approved these changes Sep 24, 2018

View reviewed changes

bajtos reviewed Sep 25, 2018

View reviewed changes

virkt25 mentioned this pull request Sep 25, 2018

Relational Controllers loopbackio/loopback-next#1758

Closed

virkt25 force-pushed the orders branch 2 times, most recently from a79d375 to 3264a7d Compare September 25, 2018 15:52

bajtos approved these changes Sep 25, 2018

View reviewed changes

virkt25 added 4 commits September 25, 2018 11:20

feat: add user order history component

c58377c

Signed-off-by: virkt25 <[email protected]>

refactor: simplify test setup

96a4b50

Signed-off-by: virkt25 <[email protected]>

chore: upgrade dependencies

495cce2

Signed-off-by: virkt25 <[email protected]>

test: fix expected value for POST

b832c39

Signed-off-by: virkt25 <[email protected]>

raymondfeng force-pushed the orders branch 2 times, most recently from 119b9e9 to f11cddb Compare September 25, 2018 18:37

chore: use Client instead of supertest

257890c

Signed-off-by: Raymond Feng <[email protected]>

raymondfeng force-pushed the orders branch from f11cddb to 257890c Compare September 25, 2018 18:48

raymondfeng merged commit 0e000ba into master Sep 25, 2018

raymondfeng deleted the orders branch September 25, 2018 19:26


		const client = createRestAppClient(app);

		return {app: app, client: client};

user order history component #12

user order history component #12

Conversation

virkt25 commented Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Sep 17, 2018

virkt25 commented Sep 17, 2018

raymondfeng commented Sep 17, 2018

raymondfeng commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 commented Sep 17, 2018 •

edited

Loading