Implement API, service, and repository layers for order pickup events #201

shravanhariharan2 · 2021-09-22T21:49:31Z

Logic implementation of order pickup events. API tests will come in a follow-up PR (to keep this PR size smaller).

…es in test factories

github-actions · 2021-09-22T21:49:45Z

Thanks for contributing! If you've made changes to the API's functionality, please make sure to bump the package version—see this guide to semantic versioning for details—and document those changes as appropriate.

shravanhariharan2 · 2021-09-22T21:50:31Z

For some reason the commits are a bit messed up, but the diff should be the same, and it'll all get squashed when merged

shravanhariharan2 · 2021-09-22T21:51:08Z

I guess since we squash and merge, we can't re-use the same branch for multiple PRs unless we do some supreme git magic

sumeet-bansal · 2021-09-23T18:50:23Z

I guess since we squash and merge, we can't re-use the same branch for multiple PRs unless we do some supreme git magic

What do you mean? We can definitely reuse the same branch name after a PR merges but we definitely can't reuse the same branch name for multiple open PRs with different changes.

sumeet-bansal · 2021-09-23T18:52:33Z

API tests will come in a follow-up PR (to keep this PR size smaller).

Let's keep the tests in this PR so we have some confidence in what we're merging.

sumeet-bansal

Couple questions and let's add the tests to this PR but LGTM.

sumeet-bansal · 2021-09-23T18:55:14Z

api/controllers/MerchStoreController.ts

@@ -188,4 +193,44 @@ export class MerchStoreController {
    await this.merchStoreService.updateOrderItems(fulfillOrderRequest.items);
    return { error: null };
  }
+
+  @Get('/pickupEvent/past')


Should this be under the /order prefix instead? And do we need separate past and future endpoints or just a future endpoint? I could imagine reasons to look at past event events because a member want want to see something related to the event, but I can't think of a good reason to scroll through past pickup events.

Also nit but thoughts on /pickup instead of /pickupEvent? It just looks a little neater but I also prefer /first-second instead of /firstSecond because it's a little more readable when looking at requests.

I could imagine reasons to look at past event events because a member want want to see something related to the event, but I can't think of a good reason to scroll through past pickup events.

The reason we have /past as well is so that at the end of the quarter, an admin can look at all the unfulfilled orders from past events and take action on those. In our case, membership will cancel/refund all orders that haven't been fulfilled by the end of the quarter. This route would serve that functionality, instead of having something like GET /order/past with query params or something to get past orders that weren't fulfilled since I think orders temporally are tied to pickup events.

Should this be under the /order prefix instead? Also nit but thoughts on /pickup instead of /pickupEvent? It just looks a little neater but I also prefer /first-second instead of /firstSecond because it's a little more readable when looking at requests.

Good call. I like the idea of /order/pickup since pickup events are entities that are tightly related to orders.

The reason we have /past as well is so that at the end of the quarter, an admin can look at all the unfulfilled orders from past events and take action on those. In our case, membership will cancel/refund all orders that haven't been fulfilled by the end of the quarter. This route would serve that functionality, instead of having something like GET /order/past with query params or something to get past orders that weren't fulfilled since I think orders temporally are tied to pickup events.

What about something like /order/unfulfilled then? That seems more accurate for what the route's intended for. The route would return unfulfilled orders, "unfulfilled" meaning orders with past pickup events that weren't marked as completed.

We're adding a status field to orders where status is one of {PLACED, MISSED, CANCELLED, FULFILLED} with query params added for status in GET /order so I think we can use that for getting all missed orders and taking action. I can remove the /past endpoint

sumeet-bansal · 2021-09-23T18:59:58Z

api/controllers/MerchStoreController.ts

+  @Get('/pickupEvent/past')
+  async getPastPickupEvents(@AuthenticatedUser() user: UserModel): Promise<GetOrderPickupEventsResponse> {
+    const canSeePickupEventOrders = PermissionsService.canSeePickupEventOrders(user);
+    const pickupEvents = await this.merchStoreService.getPastPickupEvents(canSeePickupEventOrders);


For these new endpoints, can we start returning the models instead of the response types? And then do the model → response in the controller? We can do a refactor to move the existing endpoints over to this pattern later.

Sure, is the reasoning because converting the model to response type is the responsibility of the controller, since it's the one that produces a response? And that it shouldn't be the responsibility of the service layer?

sumeet-bansal · 2021-09-23T19:03:48Z

services/MerchStoreService.ts

+    });
+  }
+
+  public async deletePickupEvent(uuid: Uuid): Promise<void> {


How would deleting a pickup event work? Email the attendees, delete the event from their orders, and then delete the event? Should there be a way to do this?

Deleting a pickup event is only allowed for events that don't have orders attached to it, so it'd only be used if an admin creates a 'faulty' event and wants to delete it before anyone uses it.

What if an admin has to cancel an event for some reason?

@steets250 any thoughts on this behavior?

Hmm, I think Sumeet's use case is a good idea in the event that we need to cancel a pick up event. Would we want an order status type that reflects the state of an order where it's pickup event (e.g. PICKUP_CANCELLED) to mark an order as needing to be rescheduled for pickup, or should frontend just check if the pickup event field is empty and allow the user to set a new one?

PICKUP_CANCELED sounds like a good idea to me, so that we can differentiate that case and handle it correctly instead of auto-canceling and -refunding that order at the end of the quarter. Maybe some grace period of a month or end of quarter (whichever comes later) to reschedule?

Sounds good. I think the way membership's doing pick up events is basically until the end of the quarter, so we can just make the users reschedule to any event before the end of the quarter, and if they don't, then membership can just refund those orders in addition to the other missed ones

I'm going to omit this deletePickupEvent change from this PR and implement it in the next PR (which is for handling order states like MISSED, CANCELLED, etc. since it seems to be more relevant to that. For now, I'll delete the DELETE /order/pickup endpoint so that deletion can't be used

tracking in #204

repositories/MerchOrderRepository.ts

Co-authored-by: Sumeet Bansal <[email protected]>

…ast pickup event route

…aseConnection

sumeet-bansal

Changes LGTM. Some nits and a potential NPE.

sumeet-bansal · 2021-09-29T00:23:27Z

migrations/0029-add-order-pickupEvent-field.ts

      type: 'uuid',
    }));
    await queryRunner.createForeignKey(TABLE_NAME, new TableForeignKey({
      columnNames: ['pickupEvent'],
      referencedTableName: 'OrderPickupEvents',
      referencedColumnNames: ['uuid'],
+      onDelete: 'SET NULL',


I think instead of relying on this, we should explicitly set the field to null when it we set the status to PICKUP_EVENT_CANCELED so it's more obvious what happens during a delete.

The problem I'm getting when I remove this, is that when I try to delete the order pickup event, I get the 'foreign key violates non-null constraints', even though the fields themselves are { nullable: true }.

FAIL tests/merch.test.ts ● Test suite failed to run QueryFailedError: update or delete on table "OrderPickupEvents" violates foreign key constraint "Orders_pickupEvent_OrderPickupEvents_uuid_fkey" on table "Orders" at new QueryFailedError (src/error/QueryFailedError.ts:9:9) at PostgresQueryRunner.<anonymous> (src/driver/postgres/PostgresQueryRunner.ts:228:19) at step (node_modules/tslib/tslib.js:143:27) at Object.throw (node_modules/tslib/tslib.js:124:57) at rejected (node_modules/tslib/tslib.js:115:69) at runMicrotasks (<anonymous>)

I think I'll leave this onDelete behavior, but also manually set the orders' pickup event to be null in the delete service.

I thought this PR would leave the delete pickup event endpoint alone for now and get it working in a followup? What I was saying earlier was that in the delete endpoint we should get all the orders for a pickup event, and update the status to PICKUP_CANCELED and the event to the null, and then delete the pickup event, to avoid violating any fkey constraints.

Right, we will handle that in a future PR, but when I removed the onDelete: 'SET NULL' property, then I was getting the above error. I just realized though, that the error was because of DatabaseConnection.clear() deleting all tables asynchronously (link below), so when the OrderPickupEvents table was deleted before the Orders table, the above error was thrown. I didn't realize the order of the table array mattered, so I'll add a comment on that.

https://github.com/acmucsd/membership-portal/blob/master/tests/data/DatabaseConnection.ts#L28

sumeet-bansal · 2021-09-29T01:02:47Z

models/OrderModel.ts

+  // since all orders should either have a pickup event, or no pickup event but with status PICKUP_CANCELLED.
+  @ManyToOne((type) => OrderPickupEventModel,
+    (pickupEvent) => pickupEvent.orders,
+    { eager: true, onDelete: 'SET NULL' })


Same here about onDelete: 'SET NULL'

sumeet-bansal · 2021-09-29T01:08:43Z

models/OrderPickupEventModel.ts

      uuid: this.uuid,
      title: this.title,
      start: this.start,
      end: this.end,
      description: this.description,
    };
+    if (canSeeOrders) pickupEvent.orders = this.orders.map((order) => order.getPublicOrder());


If we get an order, then that order includes a OrderPickupEventModel but the OrderPickupEventModel doesn't include the orders right? So shouldn't this throw some sort of null pointer if we try getting an order? I ran into a similar issue a while back.

Sorry, I'm not quite catching what you're saying here. Can you explain this again?

If you call the GET /merch/order/:uuid endpoint, then you find an OrderModel.

The OrderModel has eager: true set for the pickup event so it includes pickupEvent: OrderPickupEventModel, but the OrderPickupEventModel that's returned as part of the OrderModel doesn't have orders: OrderModel[], right? The OrderPickupEventModel wouldn't have an orders field at all since there's no join on that. So orders might be null or undefined.

When OrderModel.getPublicOrder()'s called, OrderPickupEventModel.getPublicPickupEvent()'s also called, right?

So, if orders is null or undefined, this.orders.map(...) throws some sort of exception, wouldn't it?

That's the issue I ran into a while back, where some getPublic call to one model triggered a getPublic call to another model that didn't have some other Model[].

Side note: should GET /order be GET /orders instead? I think so.

Hope that's clearer, it's kind of a weird case. If it still doesn't make sense, I can pull your branch and test it myself.

Ah okay got it, thanks for the detail. I think this isn't currently an issue though, because OrderModel.getPublicOrder() calls OrderPickupEventModel.getPublicPickupEvent() with canSeeOrders = false (the default boolean value when nothing is passed), so the this.orders.map() check wouldn't be ran since it's false.

The only case I can think of where this line would throw potentially an NPE is when getPublicPickupEvent is called directly on the pickup event that hasn't been queried from the database (i.e., it's been created from the merch factory, because otherwise the orders join happens), but even then it wouldn't because the merch factory sets orders: [] in the pickup event creation.

sumeet-bansal · 2021-09-29T01:33:53Z

services/MerchStoreService.ts

+
+  public async createPickupEvent(pickupEvent: OrderPickupEvent): Promise<OrderPickupEventModel> {
+    return this.transactions.readWrite(async (txn) => {
+      if (pickupEvent.start >= pickupEvent.end) {


Nit: this can happen before the transaction.

sumeet-bansal · 2021-09-29T01:39:11Z

tests/data/MerchFactory.ts

@@ -86,6 +86,7 @@ export class MerchFactory {
      description: faker.lorem.sentences(2),
      start,
      end,
+      orders: [],


Since it's not actually part of the table, this shouldn't be needed right?

The issue's that if it's excluded, then the toEqual check in our test won't pass since every time getFuturePickupEvents is called, then orders: [] will be set. But the 'expected' models don't have orders: [] if this line is removed.

sumeet-bansal · 2021-09-29T01:44:06Z

tests/merch.test.ts

+    const merchStoreController = ControllerFactory.merchStore(conn);
+    await merchStoreController.createPickupEvent({ pickupEvent }, admin);
+
+    const [persistedPickupEvent] = await conn.manager.find(OrderPickupEventModel, { relations: ['orders'] });


Let's use a GET instead of database lookups.

We only have GET for future pickup events, so using database lookups I think is the only way to actually pull all persisted events.

Also, I think we should standardize how these tests are operating on our system. My understanding's that these are 'API unit tests', meaning they're testing each API route as an individual unit, and that we'd eventually have integration tests for testing E2E operations like signing up -> verifying -> attending an event -> viewing leaderboard or logging in -> visit merch store -> select few items -> verify cart -> place order -> select pickup event.

If each test is testing an individual unit, then I don't think we should be using any other API routes than the one we are testing, so that a bug in 1 controller wouldn't affect multiple tests, which makes it easier to debug tests. To me it makes sense to use database lookups since those we have full control over what we're querying (which matters in this case), and so that we aren't relying on the implementation of existing lookups which might be buggy.

If each test is testing an individual unit, then I don't think we should be using any other API routes than the one we are testing, so that a bug in 1 controller wouldn't affect multiple tests, which makes it easier to debug tests. To me it makes sense to use database lookups since those we have full control over what we're querying (which matters in this case), and so that we aren't relying on the implementation of existing lookups which might be buggy.

Agreed but GETs tend to be so simple I think it's fine and even preferable to use those instead of queries.

Also, I think we should standardize how these tests are operating on our system. My understanding's that these are 'API unit tests', meaning they're testing each API route as an individual unit, and that we'd eventually have integration tests for testing E2E operations like signing up -> verifying -> attending an event -> viewing leaderboard or logging in -> visit merch store -> select few items -> verify cart -> place order -> select pickup event.

Can't we do this by calling the controllers like we do now, except in larger tests that more closely resemble actual user flows? I don't know how much work it'd be to setup some other system that launches the backend app and then makes real HTTP requests, but it doesn't seem worth it based on test coverage. What would that test that our current setup doesn't? Everything outside of the controller methods, so (1) ensuring HTTP requests work, (2) the API routes are valid (e.g. GET /event/:uuid doesn't "cover" GET /event/future, where a request to GET /event/future is evaluated as GET /event/:uuid where uuid=future), (3) the routes correctly map to controller methods, (4) request validation.

I think those are all important, but we can assume routing-controllers works as intended for (1) and (3), and I think we can write tests for (2) and (4). (2) maybe by doing something similar to the migrations test, which parses files into ASTs and checks the timestamps in the class names, except the routes test would parse controllers into annotations and verify that no route "covers" any subsequent route using regexes, and (4) by tests that manually validate.

Related to (4), something I've been thinking about wrt API specs and the API client we talked about a while back is if we can have all our controller methods defined as interfaces that the controllers then implement, and one way of doing that would be to accept raw requests in the controller methods and then validate in the method. That'd clean up our method signatures a bit too, since we could do something like

// interface function editMerchItem(uuid, MerchItemEdit); // controller function editMerchItem(@Param uuid: any, merchItemEdit: any) { CommonValidators.validateUuid(uuid); MerchStoreValidators.validateMerchItemEdit(merchItemEdit); }

Don't know if that even works though, and it's not really that relevant to this discussion, except that then our existing tests would also test validation.

sumeet-bansal · 2021-09-29T01:54:54Z

tests/merch.test.ts

+  test('POST /order/pickup fails when pickup event start date is later than end date', async () => {
+    const conn = await DatabaseConnection.get();
+    const admin = UserFactory.fake({ accessType: UserAccessType.ADMIN });
+    const pickupEvent = MerchFactory.fakeOrderPickupEvent({


Nit: move this to before the controller call.

tests/merch.test.ts

sumeet-bansal · 2021-09-29T01:56:44Z

tests/merch.test.ts

+    await ControllerFactory.merchStore(conn)
+      .editPickupEvent({ uuid: pickupEvent.uuid }, editPickupEventRequest, admin);
+
+    const [persistedPickupEvent] = await conn.manager.find(OrderPickupEventModel, { relations: ['orders'] });


Same here with the GET.

tests/merch.test.ts

Co-authored-by: Sumeet Bansal <[email protected]>

…to null for clarity

…elete: 'SET NULL' set

… deletion in test cleanup)

…acmucsd#201) * Add schema and migration for OrderPickupEvent model * Add pickupEvent field to PlaceOrderRequest and made appropriate changes in test factories * Match OrderPickupEvent request object with model * Add pickupEvent field to validators + update tests accordingly * Update merch test changes to use pickup events * Implement service and repository methods for OrderPickupEvents * Implement API layer for order pickup events * Update repositories/MerchOrderRepository.ts Co-authored-by: Sumeet Bansal <[email protected]> * Rename /store/pickupEvent -> /store/order/pickup API route + remove past pickup event route * Change response type of pickup event services to model * Set OrderModel.pickupEvent not null (for event deletion) * Add OrderPickupEvents table to list of tables to delete from in DatabaseConnection * Write tests for order pickup events API * Temporarily remove DELETE route for pickup events * Clean up merch test param usage Co-authored-by: Sumeet Bansal <[email protected]> * Clean up merch test param usage Co-authored-by: Sumeet Bansal <[email protected]> * Clean up merch test some more Co-authored-by: Sumeet Bansal <[email protected]> * Remove onDelete: SET NULL in favor of manually setting pickup events to null for clarity * Add placeOrder verification for pickup event durations + add back onDelete: 'SET NULL' set * Remove onDelete: SET NULL for real this time (plus fix order of table deletion in test cleanup) * Remove redundant { nullable: true } Co-authored-by: Sumeet Bansal <[email protected]>

shravanhariharan2 added 9 commits September 17, 2021 22:24

Add schema and migration for OrderPickupEvent model

e389f92

Add pickupEvent field to PlaceOrderRequest and made appropriate chang…

716a36a

…es in test factories

Match OrderPickupEvent request object with model

d847827

Add pickupEvent field to validators + update tests accordingly

3c02561

Merge branch 'master' into 'store/pickup-events'

342448b

Update merch test changes to use pickup events

2cf82e7

Merge branch 'master' into store/pickup-events

4d4d9d9

Implement service and repository methods for OrderPickupEvents

64c2953

Implement API layer for order pickup events

8417209

shravanhariharan2 requested a review from sumeet-bansal as a code owner September 22, 2021 21:49

sumeet-bansal reviewed Sep 23, 2021

View reviewed changes

shravanhariharan2 and others added 7 commits September 23, 2021 20:04

Update repositories/MerchOrderRepository.ts

133880e

Co-authored-by: Sumeet Bansal <[email protected]>

Rename /store/pickupEvent -> /store/order/pickup API route + remove p…

f92fd1a

…ast pickup event route

Change response type of pickup event services to model

322a04e

Set OrderModel.pickupEvent not null (for event deletion)

245a8cc

Add OrderPickupEvents table to list of tables to delete from in Datab…

da19b30

…aseConnection

Write tests for order pickup events API

7a5a976

Temporarily remove DELETE route for pickup events

eeefa13

shravanhariharan2 requested a review from sumeet-bansal September 28, 2021 04:34

sumeet-bansal approved these changes Sep 29, 2021

View reviewed changes

shravanhariharan2 and others added 5 commits September 28, 2021 19:17

Clean up merch test param usage

f8feaaf

Co-authored-by: Sumeet Bansal <[email protected]>

Clean up merch test param usage

9abb31f

Co-authored-by: Sumeet Bansal <[email protected]>

Clean up merch test some more

b8147d6

Co-authored-by: Sumeet Bansal <[email protected]>

Remove onDelete: SET NULL in favor of manually setting pickup events …

996ed23

…to null for clarity

Add placeOrder verification for pickup event durations + add back onD…

eb7b4a9

…elete: 'SET NULL' set

shravanhariharan2 added 2 commits September 29, 2021 18:56

Remove onDelete: SET NULL for real this time (plus fix order of table…

05de8d5

… deletion in test cleanup)

Remove redundant { nullable: true }

546790e

shravanhariharan2 merged commit 220c93b into master Sep 30, 2021

shravanhariharan2 deleted the store/pickup-events branch September 30, 2021 02:12

Implement API, service, and repository layers for order pickup events #201

Implement API, service, and repository layers for order pickup events #201

Conversation

shravanhariharan2 commented Sep 22, 2021

github-actions bot commented Sep 22, 2021

shravanhariharan2 commented Sep 22, 2021

shravanhariharan2 commented Sep 22, 2021

sumeet-bansal commented Sep 23, 2021

sumeet-bansal commented Sep 23, 2021

sumeet-bansal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shravanhariharan2 Sep 24, 2021 • 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

sumeet-bansal Sep 24, 2021 • 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

sumeet-bansal 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

shravanhariharan2 Sep 29, 2021 • 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

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

shravanhariharan2 Sep 24, 2021 •

edited

Loading

sumeet-bansal Sep 24, 2021 •

edited

Loading

shravanhariharan2 Sep 29, 2021 •

edited

Loading