Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE REQUEST] updateOrderDeliveryPickUp should support multipel different pickup providers #399

Open
macrozone opened this issue Nov 26, 2021 · 3 comments
Labels
feature request Suggest an idea for this project
Milestone

Comments

@macrozone
Copy link
Contributor

Introduction

updateOrderDeliveryPickUp does assume that the order has already a pickup delivery configured, otherwise it will fail silently.

Is your feature request related to a problem? Please describe.

if you have multipe delivery pickup provider, the problem is more severe. You can't change the delivery provider and set a new pickup location at the same time. You would need to call first setOrderDeliveryProvider and then updateOrderDeliveryPickUp .

this is very tedious

Describe the solution you'd like

updateOrderDeliveryPickUp should take a providerId

Describe the design of the solution in detail

simple: updateOrderDeliveryPickUp also allows to change the provider

Describe alternatives you've considered

none

@macrozone macrozone added the feature request Suggest an idea for this project label Nov 26, 2021
@pozylon
Copy link
Member

pozylon commented Nov 29, 2021

Referred mutation:

const UpdateOrderDeliveryPickUpMutation = gql`
  mutation UpdateOrderDeliveryPickUp(
    $orderId: ID!
    $deliveryProviderId: ID!
    $orderDeliveryId: ID!
    $orderPickUpLocationId: ID!
  ) {
    setOrderDeliveryProvider(
      orderId: $orderId
      deliveryProviderId: $deliveryProviderId
    ) {
      _id
    }
    updateOrderDeliveryPickUp(
      orderDeliveryId: $orderDeliveryId
      orderPickUpLocationId: $orderPickUpLocationId
    ) {
      _id
      activePickUpLocation {
        _id
      }
    }
  }
`;

@pozylon
Copy link
Member

pozylon commented Nov 29, 2021

Currently Unchained creates or reuses an existing OrderDelivery when "setOrderDeliveryProvider" is used. So you can't even do the upper mutation because you don't know the correct orderDeliveryId until you have set the delivery provider on the cart.

const delivery = OrderDeliveries.findOne({ orderId, deliveryProviderId });
  const deliveryId = delivery
    ? delivery._id
    : OrderDeliveries.createOrderDelivery({ orderId, deliveryProviderId })._id;

I suppose we do a breaking schema change:

From:

      setOrderDeliveryProvider(orderId: ID!, deliveryProviderId: ID!): Order!
      setOrderPaymentProvider(orderId: ID!, paymentProviderId: ID!): Order!
      updateOrderDeliveryShipping(
        orderDeliveryId: ID!
        address: AddressInput
        meta: JSON
      ): OrderDeliveryShipping!
      updateOrderDeliveryPickUp(
        orderDeliveryId: ID!
        orderPickUpLocationId: ID!
        meta: JSON
      ): OrderDeliveryPickUp!

To:

      setOrderDeliveryProvider(orderId: ID, deliveryProviderId: ID!): Order!
      setOrderPaymentProvider(orderId: ID, paymentProviderId: ID!): Order!
      updateOrderDeliveryShipping(
        orderId: ID
        deliveryProviderId: ID!
        orderDeliveryId: ID! @deprecated(reason: "Use orderId & deliveryProviderId to select the order delivery")
        address: AddressInput
        meta: JSON
      ): OrderDeliveryShipping!
      updateOrderDeliveryPickUp(
        orderId: ID
        deliveryProviderId: ID!
        orderDeliveryId: ID! @deprecated(reason: "Use orderId & deliveryProviderId to select the order delivery")
        orderPickUpLocationId: ID!
        meta: JSON
      ): OrderDeliveryPickUp!
  • we do as @macrozone said and set the order delivery when using one of the updateOrderDeliveryPickUp or updateOrderDeliveryShipping mutations.

The mutation from @macrozone could then be done like this (drop explicit orderId because current cart is assumed):

const UpdateOrderDeliveryPickUpMutation = gql`
  mutation UpdateOrderDeliveryPickUp(
    $deliveryProviderId: ID!
    $orderPickUpLocationId: ID!
  ) {
    updateOrderDeliveryPickUp(
      deliveryProviderId: $deliveryProviderId
      orderPickUpLocationId: $orderPickUpLocationId
    ) {
      _id
      activePickUpLocation {
        _id
      }
      order {
        _id
       delivery { _id }  
      }
    }
  }
`;

@pozylon
Copy link
Member

pozylon commented Apr 5, 2022

We got another issue with a new case also conflicting with the current concept.

It basically boils down to the problem that in order to get a list of pickup locations you have to set the delivery provider on the cart first.

So additionally to these changes, we should find a way to let the user query for pickup locations baed on the PROVIDER.

-> DeliveryProvider.pickUpLocations instead of OrderDelivery.pickUpLocations

@pozylon pozylon added this to the v3.0 milestone Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

2 participants