feat: add crud relation methods #1403

jannyHou · 2018-06-08T18:38:53Z

connect to #1376

Add two more relation methods:

delete
patch

bajtos · 2018-06-11T14:19:41Z

packages/repository/src/repositories/relation.repository.ts

+   * @param entity 
+   * @param options 
+   */
+  update(entity: DataObject<T>, options?: Options): Promise<boolean>;


I am not a fan of the name update, because it's not clear whether it's performing a patch (update of a subset of properties provided in entity) or a full replace (setting the target model instance exactly to the property values provided in entity).

Let's pick one of the more descriptive names (depending on the actual implementation in juggler): patch or replace.

@bajtos good point, I was following the naming convention in legacy-juggler-bridge.
IMO:

It makes sense that the crud methods in legacy-juggler-bridge.ts align with the names in lb3 juggler

Given a new opportunity to build lb4 relation from scratch, relation cruds can follow a new naming conversion, which reflects the http verb better and also distinguish partial update(patch) and full update(replace) in a more clear way.

It's a little weird that the Entity crud methods and relation crud methods use different naming conversion...

// entity crud method customer.update() // relation crud method customer.order.patch()

I am going to sacrifice point 3 in this PR, name the function as patch, because given enough bandwidth IMO we should correct some old naming in juggler, not the other way around.
Let's discuss a better strategy to make the naming conversion more consistent across different repositories out of this story. Also we have a story to split the repository interface: #1356.

bajtos · 2018-06-11T14:20:08Z

packages/repository/src/repositories/relation.repository.ts

+   * @param entity 
+   * @param options 
+   */
+  save(entity: DataObject<T>, options?: Options): Promise<T | null>;


What's the difference between update and save? I am proposing to omit the save method.

@bajtos save means patchOrCreate

While...I guess you would say that's not a good/clear name again lol, at least that's my thought, went into the code to see the difference.

bajtos · 2018-06-11T14:22:03Z

packages/repository/src/repositories/relation.repository.ts

+   * 
+   * @param dataObjects 
+   */
+  createAll(dataObjects: DataObject<T>[]): Promise<T[]>;


AFAIK, juggler does not provide a reliable batch create where all create requests are send in a single request or transaction and where either all or no entities are created. Without that, this tool is just a syntactic sugar for dataObjects.map(dto => repo.create(dto)), which adds too little value IMO.

I am proposing to remove createAll from LB4 API.

bajtos · 2018-06-11T14:23:39Z

packages/repository/src/repositories/relation.repository.ts

+   * @param where 
+   * @param options 
+   */
+  updateAll(


Is this going to perform a partial update (patch) or a full replace? I believe our connectors are performing a partial update, let's call this method patchAll.

I am also not sure if the suffix All is descriptive enough, can we come up with a better name please? E.g. patchMany or patchByQuery.

bajtos · 2018-06-11T14:24:54Z

packages/repository/src/repositories/relation.repository.ts

    return await this.targetRepository.find(
      constrainFilter(filter, this.constraint),
      options,
    );
  }
+
+  async update(entity: DataObject<T>, options?: Options): Promise<boolean> {
+    return await this.targetRepository.update(entity, options);


I think you forgot to apply constrainFilter?

It would be great if you could work on this in a test-first/test-driven way - start with a failing unit or integration test and only then write an implementation.

🙈 Sorry I should have marked this PR as not ready for review

t would be great if you could work on this in a test-first/test-driven way - start with a failing unit or integration test and only then write an implementation.

True 👍 The missing function is constrainDataObject IIUC

bajtos

Test cases to add:

patch, replace and patchAll cannot modify the foreign key (reference) in the target model

bajtos · 2018-06-14T08:54:46Z

packages/repository/src/repositories/relation.repository.ts

+   * Create multiple target model instances
+   * @param dataObjects
+   */
+  createAll(dataObjects: DataObject<T>[]): Promise<T[]>;


Can we remove this API please?

See #1403 (comment)

AFAIK, juggler does not provide a reliable batch create where all create requests are send in a single request or transaction and where either all or no entities are created. Without that, this tool is just a syntactic sugar for dataObjects.map(dto => repo.create(dto)), which adds too little value IMO.

I am proposing to remove createAll from LB4 API.

I am ok to remove the batch create api. Any objections? @strongloop/sq-lb-apex @raymondfeng ?

bajtos · 2018-06-14T08:56:58Z

packages/repository/src/repositories/relation.repository.ts

+   * @param where Instances within the where scope are deleted
+   * @param options
+   */
+  deleteAll(where?: Where, options?: Options): Promise<number>;


I am proposing to rename this method to deleteMany or deleteByQuery. I find the name deleteAll misleading, because were are not going to delete all records when a query was provided.

sounds good to me, deleteMany sounds better.

bajtos · 2018-06-14T08:57:18Z

packages/repository/src/repositories/relation.repository.ts

+   * @param where Instances within the where scope are patched
+   * @param options
+   */
+  patchAll(


Similarly here, maybe patchMany or patchByQuery would be a better name?

bajtos · 2018-06-14T08:58:48Z

packages/repository/src/repositories/relation.repository.ts

+    );
+  }
+
+  async delete(entity: DataObject<T>, options?: Options): Promise<boolean> {


I think delete should accept the entity id only?

@bajtos delete and patch are sugar functions for deleteById and patchById

bajtos · 2018-06-14T09:01:18Z

packages/repository/src/repositories/relation.repository.ts

+    options?: Options,
+  ): Promise<number> {
+    return await this.targetRepository.updateAll(
+      dataObject,


I think you need to call constrainDataObject?

Test case:

Category hasMany products, using product.categoryId as the FK.

create releation repository to access products of a category with id CATID

call patchAll({categoryId: 123}) - this moves the products from CATID to category 123, I think it's not allowed.

@bajtos Good catch! I added the test case in unit test.

bajtos · 2018-06-14T09:03:56Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+      isDelivered: true,
+    };
+    const newData = Object.assign({id: order.id}, fieldToPatch);
+    const isPatched = await customerOrders.patch(toOrderEntity(newData));


const newData = Object.assign({id: order.id}, fieldToPatch); const isPatched = await customerOrders.patch(toOrderEntity(newData));

I find this API cumbersome to use. Let's use patchById please.

const isPatched = await customerOrders.patchById(order.id, fieldsToPatch);

bajtos · 2018-06-14T09:05:07Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+    const order = await customerOrders.create(originalData);
+    delete order.customerId;
+
+    const isDeleted = await customerOrders.delete(toOrderEntity(order));


Similarly here - why do we need to provide the full entity to delete it?

Here is what I would like to see instead:

await customerOrders.deleteById(order.id);

true. fixed after rebasing

bajtos · 2018-06-14T09:06:14Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+    const expectedResult = {
+      id: order.id,
+      description: 'new order',
+      // @jannyhou: investigating why it's undefined, pretty sure it's not caused by `constrainData`


IIUC, your newData does not provide any value for isDelivered, therefore the model instance returned by subsequent queries return isDelivered: undefined.

@bajtos My expectation is the field doesn't show up at all, like this:

{ id: 1 description: 'new order', customerId: 1 }

thought?

bajtos · 2018-06-14T09:08:43Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+      id: order.id,
+      description: 'new order',
+    };
+    const newEntity = toOrderEntity(newData);


I don't really understand the benefits of toOrderEntity, what's wrong with the following code that does not this helper?

const newData = new Order( id: order.id, description: 'new order', });

jannyHou · 2018-06-14T14:16:58Z

@bajtos I am following the list in acceptance criteria #1376 to implement those 6 relation crud methods, honestly I didn't think too much about why pick those 6...since as a team we agreed and groomed like that.

According to our discussion, I agree that

patchById is better patch,
deleteById is better delete,
rename deleteAll to deleteMany
rename patchAll to patchMany
remove createAll

So the updated list to implement would be

patchById
deleteById
replaceById
deleteMany
patchMany

note: we don't have replaceMany in juggler IIRC, so skip it in this story

@bajtos @strongloop/sq-lb-apex @raymondfeng thought?

bajtos · 2018-06-14T14:21:20Z

I am following the list in acceptance criteria #1376 to implement those 6 relation crud methods, honestly I didn't think too much about why pick those 6...since as a team we agreed and groomed like that.

No worries, you did the right thing. It's just as I was reviewing the code it occurred to me the proposed API is not very easy to follow from user's perspective and thus I proposed to take this opportunity to improve it.

So the updated list to implement would be

patchById

deleteById

replaceById

deleteMany

patchMany

Your list looks perfect to me 👍

jannyHou · 2018-06-14T14:26:08Z

@bajtos 😎 cool, if there is no objection from team that will be our new list. Thanks!

b-admike · 2018-06-14T15:03:19Z

I agree with your proposed list as well @jannyHou 👍

jannyHou · 2018-06-14T20:00:23Z

@bajtos I had a talk with @b-admike and know why we the original list doesn't contain *ById. And also, the DefaultCrudRepository api I called in this PR is not correct.

For patch(or patchById), we cannot call targetRepository's update or updateById method, since the where clause is hard-coded, there is no way to apply fk constraint, see:

// DefaultCrudRepository
  updateById(id: ID, data: Partial<T>, options?: Options): Promise<boolean> {
    const idProp = this.modelClass.definition.idName();
    const where = {} as Where;
    where[idProp] = id;
    return this.updateAll(data, where, options).then(count => count > 0);
  }

By calling it, a customer can delete an order that doesn't belong to it!

Therefore patch or patchById or patchMany can only call updateAll from DefaultCrudRepository:

// DefaultCrudRepository
  updateAll(
    data: Partial<T>,
    where?: Where,
    options?: Options,
  ): Promise<number> {
    return ensurePromise(this.modelClass.updateAll(where, data, options)).then(
      result => result.count,
    );
  }

So patchMany is good in my PR. If I add patchById, it will be a sugar function of patchMany:

patchById(id: ID, data: Partial<T>, options?: Options): Promise<T> {
  const idProp = hmmmm....;
  await this.patchMany(data, constrainWhere({idProp: id}), options);
}

Currently TargetRepository is extending EntityCrudRepository, which is an interface that doesn't provide the function to get id name, to fix it, TargetRepository would extend DefaultCrudRepository instead. Then we can do const idProp = this.targetRepository.modelClass.definition.idName() in the code above^.

Give it a quick try, it also needs lots of change to constrain* functions.

Considering PR #1383 is landing soon, I will get back to this PR after 1383 merges, otherwise our change to constrain util functions are overlapping.

bajtos · 2018-06-15T13:04:44Z

By calling it, a customer can delete an order that doesn't belong to it!

Good catch! Obviously, enforcing the relation constrain is a must have for all relation methods.

To be honest, I was never sure what's the benefit of using endpoints like PATCH /customers/{id1}/orders/{id2} - once we have the id of the related model, we can always call the non-relation API directly: PATCH /orders/{id2}.

One can argue that PATCH /customers/{id1}/orders/{id2} allows us to not expose /orders API and require all clients to access orders via /customers namespace. It would make sense to me if the order key was composed from customer id and order id, similarly to how GitHub identifies issues (org/user name + project name + project-local issue number). When using a primary key that's decoupled from relations, e.g. MongoDB's GUID or even a numeric Order id that's unique for all orders, I don't see much benefits.

Maybe it's time to revisit the relation API design and ask ourselves what use cases/database designs we would like to support? I feel that's out of scope of this story and DP3 though.

For DP3, I am proposing to simplify the relations API to the following methods:

find, create (already exists)
patchMany
deleteMany

Currently TargetRepository is extending EntityCrudRepository, which is an interface that doesn't provide the function to get id name, to fix it, TargetRepository would extend DefaultCrudRepository instead. Then we can do const idProp = this.targetRepository.modelClass.definition.idName() in the code above^.

I don't think it's a good idea to couple relation repositories with our default implementation of EntityCrudRepository. Have you tried to access the definition from the target model/entity directly, bypassing the target repository instance? Look how DefaultCrudRepository is obtaining definion.idName() from the model/entity constructor, I think we should be able to do the same.If not, then the relation repository should receive idName in a constructor argument.

jannyHou · 2018-06-15T16:43:23Z

One can argue that PATCH /customers/{id1}/orders/{id2} allows us to not expose /orders API and require all clients to access orders via /customers namespace. It would make sense to me if the order key was composed from customer id and order id, similarly to how GitHub identifies issues (org/user name + project name + project-local issue number). When using a primary key that's decoupled from relations, e.g. MongoDB's GUID or even a numeric Order id that's unique for all orders, I don't see much benefits.

I agree with ^

My only concern is, from UX's perspective, say we expose an API called customer.orders.patchById when Order has a composed unique id(id + customerId), but the api doesn't work when Order uses a single unique id(id), it will confuse users.
Actually as long as we call an api that takes a where filter, the implementation for single Id or composed Id would be same, and the connector(or the api being called) will decide how to generate the Id.

jannyHou · 2018-06-15T16:50:23Z

I don't think it's a good idea to couple relation repositories with our default implementation of EntityCrudRepository

Agree 👍 I will try another approach.

find, create (already exists)
patchMany
deleteMany

Based on this list, I think we can also support replaceById, since the api under the hood takes in a where filter 🥇 .
Then as the first stage's relation crud api, we have the apis map to GET PATCH PUT DELETE POST.

bajtos · 2018-06-18T14:21:40Z

My only concern is, from UX's perspective, say we expose an API called customer.orders.patchById when Order has a composed unique id(id + customerId), but the api doesn't work when Order uses a single unique id(id), it will confuse users.
Actually as long as we call an api that takes a where filter, the implementation for single Id or composed Id would be same, and the connector(or the api being called) will decide how to generate the Id.

I share your concerns. Unfortunately I don't have any suggestions how to approach this :(

Based on this list, I think we can also support replaceById, since the api under the hood takes in a where filter 🥇 .

That would be great 👍

Then as the first stage's relation crud api, we have the apis map to GET PATCH PUT DELETE POST.

Awesome 👌

jannyHou · 2018-06-25T05:17:59Z

@bajtos On a second thought, I changed the function names from deleteMany, patchMany to simply delete, patch:

find also represents an operation that finds many instances and takes in a query, so I believe a simple name without word like *many or *withQuery would be able to imply those information.
other functions like patchById and deleteById will be shortcuts of them, so keep the main function's name concise.

While I don't have very strong opinion on the naming convention, will respect team's suggestion.

bajtos

Nice!

Please improve the tests per my comments below.

Additionally, please modify the tests to create also an order linked to a different customer and verify that the relation methods are not touching orders of that different customer!

bajtos · 2018-06-25T06:36:12Z

packages/repository/src/repositories/relation.repository.ts

@@ -41,7 +64,7 @@ export class DefaultHasManyEntityCrudRepository<
   */
  constructor(
    public targetRepository: TargetRepository,
-    public constraint: AnyObject,
+    public constraint: AnyObject, // public idName: string


Forgotten comment?

bajtos · 2018-06-25T06:38:01Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+    await createSamples();
+    await testPatch();
+
+    async function testPatch() {


What is the purpose of nesting the test code inside testPatch? We usually inline the test code in the test function itself.

it('can patch many instances', async () => { await createSamples(); const patchObject = {description: 'new order'}; // etc. });

bajtos · 2018-06-25T06:43:28Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+      expect(arePatched).to.equal(2);
+      const patchedItems = await customerOrders.find();
+      expect(patchedItems).to.have.length(2);
+      patchedItems.forEach(order => {


Executing the same assertion multiple times in a loop usually produces unhelpful test failure messages. My proposal:

const actualData = _.pick(patchedItems, ['customerId', 'description']); expect(actualData).to.eql([ {customerId: existingCustomerId, description: 'new order'}, {customerId: existingCustomerId, description: 'new order'}, ]);

Also please verify the following aspects of the patch operation:

The operation touched only the properties specified in the data (i.e. description) and no other properties were changed (e.g. isDelivered).

What happens when the data is trying to change the relation key (customerId) - do we silently ignore that value or reject the request with an error? I personally prefer the error to tell the clients they are doing something wrong.

bajtos · 2018-06-25T06:47:21Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+    }
+  });
+
+  async function createSamples() {


This test helper makes tests difficult to understand because it's not clear what kind of sample data was created. At minimum, please rename it to createTwoSamples().

To be honest, I think this helper function is a wrong abstraction. A better solution is to create a test-data builder as described in our docs and then call this builder multiple times in each test.

it('can patch many instances', async () => { await givenCustomerOrder({description: 'order 1'}); await givenCustomerOrder({description: 'order 2'}); // run the test });

bajtos

Almost there! 👍

bajtos · 2018-06-26T08:52:42Z

packages/repository/src/repositories/constraint-utils.ts

@@ -55,6 +55,7 @@ export function constrainDataObject<T extends Entity>(
 ): DataObject<T> {
  const constrainedData = cloneDeep(originalData);
  for (const c in constraint) {
+    if (constrainedData[c]) throw new Error(`Field "${c}" cannot be changed!`);


I think we use "property" rather than "field", would you mind rewording this message for consistency?

Also, this check is not going to catch the case where constrainedData[c] is set to a falsey value like 0 or null. Please fix, e.g. by using the following condition: if (c in constrainedData).

good catch!

bajtos · 2018-06-26T08:53:57Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+    expect(deletedOrders).to.equal(2);
+    const relatedOrders = await customerOrders.find();
+    // tslint:disable-next-line: no-unused-expression
+    expect(relatedOrders).to.be.empty;


Please note that empty is a function, see http://shouldjs.github.io/#assertion-empty

Please remove no-unused-expression override too.

Good catch! Probably because I didn't call empty as a function, tslint was complaining unused expression. Removed those comments.

bajtos · 2018-06-26T08:54:12Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+    await givenCustomerOrder({description: 'order 1'});
+    await givenCustomerOrder({description: 'order 2'});
+    const deletedOrders = await customerOrders.delete();
+    // tslint:disable-next-line: no-unused-expression


Why is this override necessary? Can you remove it please?

bajtos · 2018-06-26T08:54:16Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+    await givenCustomerOrder({description: 'order 2', isDelivered: false});
+    const patchObject = {description: 'new order'};
+    const arePatched = await customerOrders.patch(patchObject);
+    // tslint:disable-next-line: no-unused-expression


Why is this override necessary? Can you remove it please?

b-admike

Nice work! LGTM.

b-admike · 2018-06-26T14:41:49Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+  it('can patch many instances', async () => {
+    await givenCustomerOrder({description: 'order 1', isDelivered: false});
+    await givenCustomerOrder({description: 'order 2', isDelivered: false});
+    const patchObject = {description: 'new order'};


I'm being picky here, but can we patch the isDelivered property to be true and keep the descriptions as it? Feel free to ignore.

ah that's a more practical use case 👍

shimks

LGTM. after seeing the comment down below

packages/repository/test/acceptance/has-many.relation.acceptance.ts

+    const deletedOrders = await customerOrders.delete();
+    expect(deletedOrders).to.equal(2);
+    const relatedOrders = await customerOrders.find();
+    expect(relatedOrders).to.be.empty();


shimks · 2018-06-26T16:22:20Z

packages/repository/test/acceptance/has-many.relation.acceptance.ts

@@ -94,6 +94,18 @@ describe('HasMany relation', () => {
    expect(relatedOrders).to.be.empty();
  });

+  it('does not delte instance not belongs to model from', async () => {


does not delete instances that don't belong to the constrained instance

jannyHou requested review from bajtos and raymondfeng as code owners June 8, 2018 18:38

bajtos suggested changes Jun 11, 2018

View reviewed changes

jannyHou mentioned this pull request Jun 13, 2018

hasMany relation CRUD methods #1376

Closed

6 tasks

jannyHou force-pushed the relation/crud-methods branch from 5b81005 to ae98552 Compare June 13, 2018 22:50

jannyHou changed the title ~~[WIP, tests TBD]feat: add crud relation methods~~ [WIP, tsdocs to be add]feat: add crud relation methods Jun 13, 2018

jannyHou changed the title ~~[WIP, tsdocs to be add]feat: add crud relation methods~~ [tsdocs to be add]feat: add crud relation methods Jun 13, 2018

jannyHou force-pushed the relation/crud-methods branch from ae98552 to 8a3d476 Compare June 14, 2018 00:09

jannyHou changed the title ~~[tsdocs to be add]feat: add crud relation methods~~ feat: add crud relation methods Jun 14, 2018

bajtos suggested changes Jun 14, 2018

View reviewed changes

jannyHou force-pushed the relation/crud-methods branch 3 times, most recently from 473a212 to 5a1589a Compare June 25, 2018 05:01

bajtos suggested changes Jun 25, 2018

View reviewed changes

jannyHou force-pushed the relation/crud-methods branch from 73eec5c to f8db64b Compare June 25, 2018 21:54

bajtos reviewed Jun 26, 2018

View reviewed changes

jannyHou force-pushed the relation/crud-methods branch from ac573e9 to 1a4927a Compare June 26, 2018 13:52

bajtos approved these changes Jun 26, 2018

View reviewed changes

b-admike approved these changes Jun 26, 2018

View reviewed changes

jannyHou force-pushed the relation/crud-methods branch from 1a4927a to 5956e63 Compare June 26, 2018 15:35

shimks approved these changes Jun 26, 2018

View reviewed changes

shimks reviewed Jun 26, 2018

View reviewed changes

feat: add crud relation methods

6c10d74

jannyHou force-pushed the relation/crud-methods branch from c20a174 to 6c10d74 Compare June 26, 2018 16:37

jannyHou merged commit 1fdae63 into master Jun 26, 2018

raymondfeng deleted the relation/crud-methods branch June 27, 2018 21:53

feat: add crud relation methods #1403

feat: add crud relation methods #1403

Conversation

jannyHou commented Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

jannyHou Jun 13, 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

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

jannyHou Jun 14, 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

jannyHou Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Jun 14, 2018

bajtos commented Jun 14, 2018

jannyHou commented Jun 14, 2018

b-admike commented Jun 14, 2018

jannyHou commented Jun 14, 2018 • edited Loading

bajtos commented Jun 15, 2018

jannyHou commented Jun 15, 2018

jannyHou commented Jun 15, 2018 • edited Loading

bajtos commented Jun 18, 2018

jannyHou commented Jun 25, 2018

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

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

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

b-admike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimks left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

jannyHou commented Jun 8, 2018 •

edited

Loading

jannyHou Jun 13, 2018 •

edited

Loading

jannyHou Jun 14, 2018 •

edited

Loading

jannyHou Jun 14, 2018 •

edited

Loading

jannyHou commented Jun 14, 2018 •

edited

Loading

jannyHou commented Jun 15, 2018 •

edited

Loading