Implement order states for order cancellation, fulfillment, pickup misses, and pickup cancellation #207

shravanhariharan2 · 2021-10-01T05:20:58Z

Details for this feature can be found in the Merch Store Specification on Notion: https://www.notion.so/acmucsd/Merch-Store-Specification-2021-d0d5fa4d16de472aa7e13534e060f8ec (under the "Orders" section of the API routes)

…led emails

github-actions · 2021-10-01T05:21:14Z

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-10-01T05:39:27Z

Tests still need to be written, but I'll publish this PR for now since it's pretty large and might need a bit of time to look at

sumeet-bansal · 2021-10-01T22:34:13Z

api/decorators/Validators.ts

+class OrderStatusEditValidator implements ValidatorConstraintInterface {
+  validate(orderStatus: OrderStatus): boolean {
+    return Object.values(OrderStatus).includes(orderStatus)
+      && orderStatus !== OrderStatus.PLACED
+      && orderStatus !== OrderStatus.FULFILLED;
+  }
+
+  defaultMessage(): string {
+    return 'Order status must be one of [\'PICKUP_MISSED\', \'CANCELLED\', \'PICKUP_CANCELLED\']';
+  }
+}


Error message says status must be one of [...] so let's check for that instead of checking that it's not the other things.

class OrderStatusEditValidator implements ValidatorConstraintInterface { private static allowedStatuses = [ OrderStatus.PICKUP_MISSED, OrderStatus.CANCELLED, OrderStatus.PICKUP_CANCELLED ]; validate(orderStatus: OrderStatus): boolean { return allowedStatuses.includes(orderStatus); } defaultMessage(): string { return `Order status must be one of ${allowedStatuses}`; } }

sumeet-bansal · 2021-10-01T22:36:55Z

api/decorators/Validators.ts

+    return 'Order status must be one of [\'PLACED\', \'FULFILLED\', \'PICKUP_MISSED\', '
+      + '\'CANCELLED\', \'PICKUP_CANCELLED\']';


I think this should print out the array correctly? This way we don't have to update this if we add another order state too.

Suggested change

return 'Order status must be one of [\'PLACED\', \'FULFILLED\', \'PICKUP_MISSED\', '

+ '\'CANCELLED\', \'PICKUP_CANCELLED\']';

return `Order status must be one of ${Object.values(OrderStatus)}`;

migrations/0030-add-order-status-field.ts

sumeet-bansal · 2021-10-02T01:24:04Z

api/controllers/MerchStoreController.ts

+    // status and pickup event are not allowed to be updated at the same time,
+    // since they are not logically related, and thus should never have to be updated at the same request
+    if (status && pickupEvent) throw new UserError('Only status or pickupEvent can be updated at once, not both');
+    // members are only allowed to cancel orders. store admins can perform any operation


Can an order be canceled and then re-placed? Should that be allowed? If it is, then we need to update quantities and all that.

Should this endpoint be split into separate member and admin edits? e.g. DELETE /order/:uuid for members canceling orders and PATCH /order/:uuid for admins canceling orders or something?

Or, PATCH /order/:uuid where the only edit is pickup event, and then a separate DELETE /order/:uuid for canceling. Like, if we consider order status as a state machine, we could expose endpoints as state transitions (e.g. /cancel, /missed) and do the necessary operations than to directly edit the state itself. Or get the state in the endpoint and call different MerchOrderService methods like canceledOrder, missedPickup, canceledPickup, which might all do slightly different things. I think the service methods is the way to go. We're already doing this with fulfillOrderItems.

We could even do PATCH /order/:uuid editMerchOrder/changePickupEvent for editing pickup event and PATCH /order/:uuid/:state changeOrderStatus so we're not just forcing one endpoint to handle all types of edits.

Can an order be canceled and then re-placed? Should that be allowed? If it is, then we need to update quantities and all that.

No, this isn't allowed per store requirements.

Should this endpoint be split into separate member and admin edits? e.g. DELETE /order/:uuid for members canceling orders and PATCH /order/:uuid for admins canceling orders or something?

I like the idea of splitting the giant PATCH /order/:uuid route into separate routes like /cancellation or /missed or /fulfillment. I think in the case with member vs. admin edit, it should be fine to keep them in the same route, since we do the same sort of split with other routes. Also I just realized I wrote some buggy ass code where if an admin cancells a user's order, then the admin gets refunded LMAO

sumeet-bansal · 2021-10-02T01:41:22Z

api/controllers/MerchStoreController.ts

+    return { error: null };
+  }
+
+  @Post('/order/:uuid/fulfillment')


I know, I know, nouns but I kinda like POST /order/:uuid/[fulfill/cancel]. What do you think?

I'm a fan of it too - personally I'm of the opinion of the 'convention' for REST API naming is whatever naming we establish (since I don't think there's a set of 'naming conventions' for APIs). So I don't think we should feel too bad about having verbs in the name - especially since it also distinguishes 'actions' (ie. reset password or fulfill orders) from 'resource operations' (ie. editing a field of a model).

services/MerchStoreService.ts

sumeet-bansal · 2021-10-02T02:08:42Z

services/MerchStoreService.ts

+   */
+  private static getOptionPriceAndQuantitiesFromOrder(order: OrderModel): Map<string, OrderItemPriceAndQuantity> {
+    const optionToPriceAndQuantity = new Map<string, OrderItemPriceAndQuantity>();
+    order.items.forEach((oi) => {


Does forEach process all the elements concurrently?

Seems like it's blocking. IMO we should use for-each for pure functions and prefer for-loops if we're editing maps though. What do you think?

Yes, forEach is a blocking call - https://stackoverflow.com/questions/5050265/javascript-node-js-is-array-foreach-asynchronous

IMO we should use for-each for pure functions and prefer for-loops if we're editing maps though. What do you think?

That makes sense to me - since forEach does by nature look more functional, we can use regular for-loops to emphasize that there's some state being changed within the iteration

sumeet-bansal · 2021-10-02T02:22:42Z

services/MerchStoreService.ts

+      const { uuid } = oi.option;
+      if (optionToPriceAndQuantity.has(oi.option.uuid)) {
+        optionToPriceAndQuantity.set(uuid, {
+          quantity: optionToPriceAndQuantity.get(uuid).quantity + 1,


Nit: we can put optionToPrice.get(uuid) in a variable since we reference it twice.

services/MerchStoreService.ts

sumeet-bansal · 2021-10-02T02:28:39Z

services/MerchStoreService.ts

      const pickupEvent = await orderPickupEventRepository.findByUuid(uuid);
      if (pickupEvent.orders.length > 0) {
        throw new UserError('Cannot delete an order pickup event that has orders assigned to it');
      }
-      // Manually set all the orders' pickup events to null before deleting event
+      // email order cancellation for order, update order status, and set pickupEvent to null before deleting
+      pickupEvent.orders.forEach(async (order) => {


If forEach is blocking then let's do these concurrently so we're not sending emails one at a time and blocking on each one.

Looked at the email template to see if we could just send the same email to all the orderers, and it seems not, but it looks like we don't specify a URL here? Or am I misreading the HTML?

https://github.com/acmucsd/membership-portal/blob/OrderStates/templates/orderPickupCancelled.ejs#L120

And can we send the same email to all the orderers? e.g. "Hey your order got canceled" and check the portal to see which order? Kind of a dick move though, but it means we only send one email.

Good catch, I meant to ping Steven about what URL to use there but I never ended up doing that.

And can we send the same email to all the orderers? e.g. "Hey your order got canceled" and check the portal to see which order? Kind of a dick move though, but it means we only send one email.

Haha interesting idea, I think it might be better UX if we send each person a separate email though so that they can see what specific items were in their cancelled order and so they each get their own URL to reschedule. Might need to look if there's some kind of rate limiting on Sendgrid's end to make sure mass mailing won't fail for some emails.

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

…behavior

…nges

shravanhariharan2 · 2021-11-10T19:33:06Z

Just added the 'pending order cleanup' route for admins to cancel all orders.

TODOs (for myself):

Add activity logs for fulfilling, cancelling, marking as missed, and cancel-all-ing
Add GET /order/pickup/past route (since admins would want to scroll through past pickup events to see if orders have been missed)
(TBD) Add functionality for rescheduling a partially picked up order? e.g. a member can only fulfill a subset of items in 1 event so they should be able to reschedule their pickup to a future one.

shravanhariharan2 · 2021-11-10T19:48:04Z

Will create issues for each of the above, since this PR is getting a bit large. Still need to do some refactoring for code quality, then should be ready for review

shravanhariharan2 · 2021-11-17T01:56:16Z

Actually got around to implementing activity logging + the route in this PR 😅. Should be the last large PR for store in the next few weeks

sumeet-bansal

Didn't look at the tests, maybe Steven or someone else can take a look before merging?

sumeet-bansal · 2021-11-17T02:17:40Z

api/controllers/MerchStoreController.ts

    if (!PermissionsService.canAccessMerchStore(user)) throw new ForbiddenError();
    const canSeeAllOrders = PermissionsService.canSeeAllMerchOrders(user);
-    const orders = await this.merchStoreService.getAllOrders(user, canSeeAllOrders);
+    const orders = await this.merchStoreService.getAllOrders(user, filters.status, canSeeAllOrders);


sumeet-bansal · 2021-11-17T02:18:42Z

api/controllers/MerchStoreController.ts

    @AuthenticatedUser() user: UserModel): Promise<EditMerchOrderResponse> {
-    if (!PermissionsService.canFulfillMerchOrders(user)) throw new ForbiddenError();
+    if (!PermissionsService.canAccessMerchStore(user)) throw new ForbiddenError();


Suggested change

if (!PermissionsService.canAccessMerchStore(user)) throw new ForbiddenError();

if (!PermissionsService.canManageMerchOrders(user)) throw new ForbiddenError();

Members can reschedule their pickup events, so they don't need store manager / store distributor permissions for this route

sumeet-bansal · 2021-11-17T02:18:50Z

api/controllers/MerchStoreController.ts

+
+  @Post('/order/:uuid/cancel')
+  async cancelMerchOrder(@Params() params: UuidParam, @AuthenticatedUser() user: UserModel) {
+    if (!PermissionsService.canAccessMerchStore(user)) throw new ForbiddenError();


Suggested change

if (!PermissionsService.canAccessMerchStore(user)) throw new ForbiddenError();

if (!PermissionsService.canManageMerchOrders(user)) throw new ForbiddenError();

Members can cancel their own orders, so they don't need 'manage merch order' permissions since that's something a store manager / store distributor would need

sumeet-bansal · 2021-11-17T02:20:01Z

api/controllers/MerchStoreController.ts

    return { error: null };
  }

+  @Post('/order/cleanup')
+  async cancelAllPendingMerchOrders(@AuthenticatedUser() user: UserModel): Promise<CancelAllPendingOrdersResponse> {


If this route isn't needed yet, can we make this PR a little smaller and put up another PR when it is needed?

Just confirmed with Steven, we will be rolling with /order/cleanup route and removing the status query param - frontend can do any filtering if needed since there's no current use case for it

services/MerchStoreService.ts

sumeet-bansal · 2021-11-17T02:37:41Z

services/MerchStoreService.ts

+   * @param uuid order uuid
+   * @returns updated order
+   */
+  public async markOrderAsMissed(uuid: Uuid, proxy: UserModel): Promise<OrderModel> {


I think we talked a while back about not using proxy but maybe admin isn't the best alternative? It's better than proxy at least.

Suggested change

public async markOrderAsMissed(uuid: Uuid, proxy: UserModel): Promise<OrderModel> {

public async markOrderAsMissed(uuid: Uuid, admin: UserModel): Promise<OrderModel> {

I was hesitant to use admin since soon we'll have STORE_MANAGER and STORE_DISTRIBUTOR roles who aren't necessarily admins but can mark orders as missed. We can probably leave it as admin for now but change it later to something like manager? But yeah, something that conveys 'on behalf of' would be ideal.

sumeet-bansal · 2021-11-17T02:38:44Z

services/MerchStoreService.ts

+   * @param uuid order uuid
+   * @returns updated order


We can remove these annotations and rename the param orderUuid.

sumeet-bansal · 2021-11-17T02:41:06Z

services/MerchStoreService.ts

+      if (MerchStoreService.isLessThanTwoDaysBeforePickupEvent(order.pickupEvent)) {
+        throw new NotFoundError('Cannot cancel an order with a pickup date less than 2 days away');
+      }
+      const customer = order.user;


Is this used anywhere?

Yeah, it's used in the activity logs to describe who's order has been cancelled (since admins can cancel other members orders in the case of 'cancel all pending orders')

sumeet-bansal · 2021-11-17T02:43:06Z

services/MerchStoreService.ts

+    const refundValue = unfulfilledItems.reduce((refund, item) => refund + item.salePriceAtPurchase, 0);
+    await MerchStoreService.refundUser(user, refundValue, txn);
+
+    // build email with only the unfulfilled items


Nit: I think the code explains itself here.

sumeet-bansal · 2021-11-17T02:51:50Z

services/MerchStoreService.ts

@@ -498,6 +710,25 @@ export default class MerchStoreService {
    }, 0);
  }

+  public async cancelAllPendingOrders(user: UserModel): Promise<void> {


Should this cancel all pending orders or only the ones that were placed a long time ago?

This would cancel all pending orders, since there could be the case where someone orders for the last pickup event of a quarter and wasn't able to make it or wasn't able to fulfill all of it

shravanhariharan2 · 2021-11-17T07:24:01Z

This should be ready to merge now, addressed all of Sumeet's comments and ran through the tests with Steven to see what else needs to be added to get 100% store coverage

…sses, and pickup cancellation (acmucsd#207) * Adds main code for Order Status routes * Add email templates for pickup missed, pickup cancelled, order cancelled emails * Add validators and requests for editing merch orders * Progress on connecting edit order with email service * Fix field accessing for pickup event in templates * Implement editing merch order states v1 * Add migration for Orders.status field * Add status query param to GET /order route * Refund user when order is cancelled * Implement delete functionality for pickup events * Add check for pickup event timeline in order cancellation logic * Rename map variable to mention name Co-authored-by: Sumeet Bansal <[email protected]> * Update services/MerchStoreService.ts Co-authored-by: Sumeet Bansal <[email protected]> * Update services/MerchStoreService.ts Co-authored-by: Sumeet Bansal <[email protected]> * Split PATCH /order/:uuid into separate routes to serve state machine behavior * Add validation for order state machine * Fix method name in suggestion merge * Integrate order verification changes from master into order state changes * Convert forEach to for loop + remove duplicate variable access * fixed various PR comments * Update templates to display proper portal url for cancelling events * Split merch.test.ts into merch.store.test.ts and merch.order.test.ts * Add skeleton test code * Add email mocking + fix archived collection <-> hidden item invariant + write first test * Update date format in email template * Progress on tests * More progress on tests * Add email for updating pickup event for an order + finish order state tests * Rename merch order and merch store test files * Change 'verify' -> 'validate' in merch controller/service * Rename /order/miss -> /order/missed * Implement partial-order cancellation * Remove OrderStates.PARTIALLY_FULFILLED * Implement gradual fulfillment across multiple requests * Implement partial-order cancellation refund test * Write tests for POST /order/cleanup * Add member.reload() calls to make clearer that credits are refreshed * Add activity logging for orders * Add GET /order/pickup/past route and test * Add tests for order activity * lint * Disallow creating pickup events within 2 days of the event start * Ensure item has at least 1 option on creation * make my tests deterministic * Fix minor style guides * Remove GET status filter from /order and rename to GET /orders * linty lint Co-authored-by: Michael Shao <[email protected]> Co-authored-by: Sumeet Bansal <[email protected]>

michl1001 and others added 9 commits September 29, 2021 00:41

Adds main code for Order Status routes

3dc5df3

Merge branch 'master' into 'OrderStates'

62e6a6f

Add email templates for pickup missed, pickup cancelled, order cancel…

7a97d0b

…led emails

Add validators and requests for editing merch orders

dff00e6

Progress on connecting edit order with email service

be30161

Fix field accessing for pickup event in templates

7196fad

Implement editing merch order states v1

e7c4fa5

Add migration for Orders.status field

39fe9ab

Add status query param to GET /order route

7981a6a

shravanhariharan2 added 2 commits September 30, 2021 22:26

Refund user when order is cancelled

5ea7da7

Implement delete functionality for pickup events

9d7df82

shravanhariharan2 marked this pull request as ready for review October 1, 2021 05:39

shravanhariharan2 requested a review from sumeet-bansal as a code owner October 1, 2021 05:39

shravanhariharan2 requested a review from michl1001 October 1, 2021 05:39

shravanhariharan2 linked an issue Oct 1, 2021 that may be closed by this pull request

Implement deletion of order pickup events #204

Closed

Add check for pickup event timeline in order cancellation logic

086c0f8

sumeet-bansal reviewed Oct 2, 2021

View reviewed changes

shravanhariharan2 and others added 11 commits October 5, 2021 21:13

Rename map variable to mention name

6f55b1b

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

Update services/MerchStoreService.ts

1959b5e

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

Update services/MerchStoreService.ts

44a02a6

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

Split PATCH /order/:uuid into separate routes to serve state machine …

b0d9f5e

…behavior

Add validation for order state machine

9ce974a

Fix method name in suggestion merge

1b137b1

Merge branch 'master' into 'OrderStates'

78f19f8

Integrate order verification changes from master into order state cha…

74a5650

…nges

Convert forEach to for loop + remove duplicate variable access

219d5c1

fixed various PR comments

45c9391

Merge branch 'master' into OrderStates

8e969b4

shravanhariharan2 added 10 commits November 7, 2021 09:36

Change 'verify' -> 'validate' in merch controller/service

2b2805a

Rename /order/miss -> /order/missed

9bd117b

Merge branch 'master' into 'OrderStates'

3ec0093

Implement partial-order cancellation

424254f

Remove OrderStates.PARTIALLY_FULFILLED

a94ac88

Implement gradual fulfillment across multiple requests

d59851a

Implement partial-order cancellation refund test

29a3668

Write tests for POST /order/cleanup

961fdb6

Merge branch 'master' into OrderStates

21deff7

Add member.reload() calls to make clearer that credits are refreshed

12b7491

shravanhariharan2 requested a review from sumeet-bansal November 14, 2021 06:40

shravanhariharan2 added 4 commits November 16, 2021 17:31

Add activity logging for orders

5aa373a

Add GET /order/pickup/past route and test

b69ab9d

Add tests for order activity

0dd30ec

lint

ec00336

StormFireFox1 mentioned this pull request Nov 17, 2021

Add Order Limit to Pickup Events #214

Merged

sumeet-bansal approved these changes Nov 17, 2021

View reviewed changes

shravanhariharan2 added 5 commits November 16, 2021 22:15

Disallow creating pickup events within 2 days of the event start

77ccd68

Ensure item has at least 1 option on creation

2feda16

make my tests deterministic

c1d7f77

Fix minor style guides

1763bd1

Remove GET status filter from /order and rename to GET /orders

fd1165f

linty lint

fa2c35d

shravanhariharan2 merged commit c097558 into master Nov 18, 2021

shravanhariharan2 deleted the OrderStates branch November 18, 2021 03:57

		return 'Order status must be one of [\'PLACED\', \'FULFILLED\', \'PICKUP_MISSED\', '
		+ '\'CANCELLED\', \'PICKUP_CANCELLED\']';

	return 'Order status must be one of [\'PLACED\', \'FULFILLED\', \'PICKUP_MISSED\', '
	+ '\'CANCELLED\', \'PICKUP_CANCELLED\']';
	return `Order status must be one of ${Object.values(OrderStatus)}`;

	if (!PermissionsService.canAccessMerchStore(user)) throw new ForbiddenError();
	if (!PermissionsService.canManageMerchOrders(user)) throw new ForbiddenError();

	public async markOrderAsMissed(uuid: Uuid, proxy: UserModel): Promise<OrderModel> {
	public async markOrderAsMissed(uuid: Uuid, admin: UserModel): Promise<OrderModel> {

Implement order states for order cancellation, fulfillment, pickup misses, and pickup cancellation #207

Implement order states for order cancellation, fulfillment, pickup misses, and pickup cancellation #207

Conversation

shravanhariharan2 commented Oct 1, 2021 • edited Loading

github-actions bot commented Oct 1, 2021

shravanhariharan2 commented Oct 1, 2021

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 Oct 12, 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

sumeet-bansal Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shravanhariharan2 commented Nov 10, 2021

shravanhariharan2 commented Nov 10, 2021

shravanhariharan2 commented Nov 17, 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

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 commented Nov 17, 2021

shravanhariharan2 commented Oct 1, 2021 •

edited

Loading

shravanhariharan2 Oct 12, 2021 •

edited

Loading

sumeet-bansal Oct 13, 2021 •

edited

Loading