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

Catalog price resolver #288

Merged
merged 37 commits into from
Feb 9, 2021
Merged

Catalog price resolver #288

merged 37 commits into from
Feb 9, 2021

Conversation

Mikearaya
Copy link
Contributor

@Mikearaya Mikearaya commented Jan 5, 2021

I've added the field on the top level product object so all products (simple, bundle configurable) return the same price list. this will remove the need for query.productCatalogPrices

@Mikearaya Mikearaya requested a review from pozylon January 5, 2021 14:59
@pozylon
Copy link
Member

pozylon commented Jan 11, 2021

@Mikearaya I thought a lot about this PR and what we want to achieve. Dumping the price tables doesn't feel right to me. Currently we have SimpleProducts which can have 1 valid catalog price and 1 simulated price based on the current context. That's fine and correct.

But let's assume we have 5 Simple Products that are behind a Proxy (Configurable Product) like this:

Name Type  Catalog Price Status
A, color red SimpleProduct  5 USD Active
B, color red SimpleProduct  20 USD Active
C, color blue SimpleProduct  5 USD, 4 USD if more than 5 ordered Active
D, color blue SimpleProduct  15 USD, 10 USD if more than 5 ordered Draft
E, color blue SimpleProduct  1 USD Draft

If you have a ConfigurableProduct that could end up in one of the above SimpleProducts, the range we want to show is:
Catalog Price Range: 5-20 $

The simulated price range for a ConfigurableProduct is different though because the query accepts a quantity:
Simulated Price Range with quantity 10: 4-20 $
Simulated Price Range with quantity 10 and vectors color=blue: 4 $
Simulated Price Range with quantity 10 and vectors color=red: 5-20 $

So maybe ConfigurableProduct should be extended like this:

catalogPriceRange(
        quantity: Int
        vectors: [ProductAssignmentVectorInput!]
        includeInactive: Boolean = false
      ): ProductPriceRange!
simulatedPriceRange(
        quantity: Int
        vectors: [ProductAssignmentVectorInput!]
        includeInactive: Boolean = false
        currency: String
        useNetPrice: Boolean = false
      ): ProductPriceRange!

@Mikearaya
Copy link
Contributor Author

Yes dumping the product catalogue price was used based on the conversation on the issue I.e. to let the frontend extract the min and max value of the product.
But like you stated above it's trickier with catalog product.
I'm extending the product helper with simulatedPriceRange and catalogPriceRange to handle each case.
One clarification I need is how do you set the logic for a price to apply for a certain quantity level. Is it through maxQuantity or some other means?.

@pozylon
Copy link
Member

pozylon commented Jan 14, 2021

Absolutely, we should expose a function to query for these quantity levels and the maxQuantity in ProductPrice could do it but it's not really a good API for a frontend developer. The reason I have used Query.productCatalogPrices before is to kind of split the "private" pricing data which is only editable and querable by admins from the public data, (same like translatedProductTexts).

In the db, pricing is stored in this way:

pricing: Array,
    'pricing.$': Object,
    'pricing.$.isTaxable': Boolean,
    'pricing.$.isNetPrice': Boolean,
    'pricing.$.countryCode': String,
    'pricing.$.currencyCode': String,
    'pricing.$.amount': Number,
    'pricing.$.maxQuantity': Number,

Currently the ProductPrice type is modeled after that but at the same time wraps some stuff into the Money type.

type ProductPrice {
      _id: ID!
      isTaxable: Boolean!
      isNetPrice: Boolean!
      country: Country!
      price: Money!
      maxQuantity: Int
    }

It's kind of a mix of different attributes + the price and it does not really make sense to reuse it for catalogPrice and simulatedPrice on the product level because that's interpreted data.

Currently the SimpleProduct.catalogPrice looks like this: catalogPrice(quantity: Int = 1): ProductPrice so it takes the quantity + the locale information from the graphql context and looks for the right entry from the catalog price table ("pricing" in db).

simulatedPrice does more and uses the dynamic pricing logic but at the moment it's inconsistent with catalogPrice as it also lets you manually "choose" an alternate currency (which is nice and we needed it for Weng): simulatedPrice( **currency: String** useNetPrice: Boolean = false quantity: Int = 1 ): ProductPrice (it's clear though that useNetPrice is not relevant for a catalog price because that is a dynamic pricing only configuration parameter)

So I suggest maybe we have to change the API like this:

Simple Product:

  • Breaking Change catalogPrice(quantity: Int = 1): ProductPrice -> catalogPrice(quantity: Int = 1, **currency: String**): Price
  • Breaking Change simulatedPrice( currency: String useNetPrice: Boolean = false quantity: Int = 1 ): ProductPrice -> simulatedPrice( currency: String useNetPrice: Boolean = false quantity: Int = 1 ): Price
  • Breaking Change: type ProductPrice { _id: ID! isTaxable: Boolean! isNetPrice: Boolean! country: Country! price: Money! maxQuantity: Int } -> type ProductCatalogPrice { _id: ID! isTaxable: Boolean! isNetPrice: Boolean! country: Country! currency: Currency! amount: Int! maxQuantity: Int }
  • Add type Price: type Price { isTaxable: Boolean!, isNetPrice: Boolean!, amount: Int!, currency: String! }
  • Add type PriceLevel: type PriceLevel { minQuantity: Int, maxQuantity: Int, price: Price }
  • New field: leveledCatalogPrices(currency: String): [PriceLevel!]!

Then we should also audit the whole API and propably replace all occurences of "Money" with Price as the Money type kind of sucks when using the API.

@pozylon
Copy link
Member

pozylon commented Jan 14, 2021

So basically I'm not happy with the current state of the GraphQL schema as a whole when it comes to price display. But maybe it's not the right time to refactor this?

@Mikearaya
Copy link
Contributor Author

I agree it's a big change and a lot of breaking changes but at the same time if I complete the work queue filter PR there is probably one ticket for me to take (create repo adding forget password flow) so I have time to engage in this change until the print ends and ready for release, we defiantly will need a few back and forth to stay on the same track but it's doable.
on the other hand, there are other breaking changes and if we want to make the migration a little painful we can schedule this for the next sprint release but still work on it to make everything sound.

@Mikearaya
Copy link
Contributor Author

what is the purpose of leveledCatalogPrices

@Mikearaya
Copy link
Contributor Author

Mikearaya commented Jan 14, 2021

also, should we add another field minQuantity to make it more descriptive for this purpose? simulatedPrice inconsistency with catalog price because of its dynamic pricing logic is really useful and should still support that in my opinion.

@Mikearaya
Copy link
Contributor Author

Mikearaya commented Jan 18, 2021

@pozylon I've added the initial fields we discussed (simulatedPriceRange and catalogPriceRange) they are using the product price maxQuantity field for their quantity requirement for the price will apply to. I think the latter breaking changes you mentioned can be applied on the next release after we test with this.
this is just a sketch I think we can start with, check it out when you have the time and let me know

@pozylon
Copy link
Member

pozylon commented Jan 25, 2021

what is the purpose of leveledCatalogPrices

When you proposed dumping the catalog price table it would have exposed the price levels (different maxQuantity), which was not in my mind but I certainly see that for some customers we might render that to the customer in a UI like:

Price: USD 8
Bulk discounts:
- Pay USD 5 / item if you order 50
- Pay USD 4 / item if you order 200

So in order to do that you would use leveledCatalogPrices + catalogPrice

@pozylon
Copy link
Member

pozylon commented Jan 25, 2021

minQuantity

See my comment about the type PriceLevel, I'd still only store maxQuantity but in the GraphQL API show the complete ranges with min & max quantity.

price maxQuantity
50 5 -> 0 - 5
40 10 -> 6 - 10
100 -> 11 - Infinity

@pozylon
Copy link
Member

pozylon commented Jan 25, 2021

SimpleProduct catalogPrice/simulatedPrice -> Still only return exactly 1 price depending on parameters and context

SimpleProduct leveledCatalogPrices -> can return multiple prices because there are multiple records for same currency / country with different maxQuantities

ConfigurableProduct catalogPriceRange/simulatedPriceRange -> can return multiple prices because it's a configurable product having more than 1 simple products to choose from depending on variants. It will iterate over the simple products and call catalogPrice/simulatedPrice

@pozylon
Copy link
Member

pozylon commented Jan 25, 2021

So catalogPriceRange,simulatedPriceRange,catalogPrice,simulatedPrice all take a quantity argument and the quantity level is basically "locked" but leveledCatalogPrices will not take the quantity

@Mikearaya
Copy link
Contributor Author

@pozylon the price range resolvers are added including leveledCatalogPrice now remaining is converting money to Price plus other type changes suggested and making the functions comply with them.
Do you think we should implement those changes with this PR or should we take it as a task on its on. The breaks are huge might touch order or any other are that use this functions which might take time.
What do you think on how to approach this?

@Mikearaya Mikearaya force-pushed the catalog-price-resolver branch 3 times, most recently from e791cc9 to 142ebe6 Compare February 1, 2021 17:52
@Mikearaya
Copy link
Contributor Author

@pozylon What we have discussed so far has been implemented now my next step is replacing all traces of Money type but since now prices apply differently based on the quantity of order (that's what we are displaying with leveled catalog price) we should also add this to the logic order pricing and it would be helpful and faster for me if you could share your thoughts on how I should approach it rather than trying to decipher the whole process of order pricing.

@Mikearaya
Copy link
Contributor Author

DeliveryFee.price: Money

type DeliveryFee {  _id: ID!  isTaxable: Boolean!   isNetPrice: Boolean!   country: Country!   price: Money!   }

can be replaced with price if it didn't have country field
this will affect type DeliveryProvider.simulatedPrice
this in turn will affect

  • Order.supportedDeliveryProviders
  • OrderDeliveryShipping.provider
  • OrderDeliveryPickUp.provider
  • OrderDelivery.provider
  • SubscriptionPayment.provider
  • Stock.deliveryProvider
  • Dispatch.deliveryProvider
  • Query.deliveryProvider
  • Query.deliveryProviders
  • Mutation.removeDeliveryProvider
  • Mutation.updateDeliverProvider
  • Mutation.createDeliveryProvider

OrderDelivery.fee:Money

interface OrderDelivery {  _id: ID!   provider: DeliveryProvider  status: OrderDeliveryStatus   delivered: Date   fee: Money  meta: JSON  discounts: [OrderDeliveryDiscount!]  }

will be

interface OrderDelivery {  _id: ID!   provider: DeliveryProvider  status: OrderDeliveryStatus   delivered: Date   fee: Price  meta: JSON  discounts: [OrderDeliveryDiscount!]  }
this will also trickle down to
type OrderDeliveryPickUp and type OrderDeliveryShipping

this has been affected by the change to DeliveryProvider type already but changes to this alone will affect

  • OrderDeliveryDiscount.delivery
  • Order.delivery
  • Mutation.updateOrderDeliveryShipping
  • Mutation.updateOrderDeliveryPrickUp

`OrderDiscountable.total

interface OrderDiscountable {  _id: ID!  orderDiscount: OrderDiscount!   total: Money!   }
will become
interface OrderDiscountable {  _id: ID!  orderDiscount: OrderDiscount!   total: Price!   }

this change in turn will trickle down to

OrderGlobalDiscount
OrderPaymentDiscount
OrderDeliveryDiscount
OrderItemDiscount

OrderDiscount.total

type OrderDiscount {  _id: ID!   trigger: OrderDiscountTrigger!  code: String   order: Order!   interface: DiscountInterface   total: Money!  discounted: [OrderDiscountable!]  }
will be
type OrderDiscount {  _id: ID!   trigger: OrderDiscountTrigger!  code: String   order: Order!   interface: DiscountInterface   total: Price!  discounted: [OrderDiscountable!]  }

and trickle down to affect types

OrderDiscountable
OrderGlobalDiscount
OrderPaymentDiscount
OrderDeliveryDiscount
OrderItemdiscount

This in turn will affect

  • Order.discounts
  • Mutation.addCartDiscount
  • Mutation.removeCartDiscount

OrderItem.unitPrice && OrderItem.total

type OrderItem {
      _id: ID!
      product: Product!
      order: Order!
      quantity: Int!
      originalProduct: Product!
      quotation: Quotation
      unitPrice: Money
      total(category: OrderItemPriceCategory): Money
      discounts: [OrderItemDiscount!]
      dispatches: [Dispatch!]
      configuration: [ProductConfigurationParameter!]
    }

will become

type OrderItem {
      _id: ID!
      product: Product!
      order: Order!
      quantity: Int!
      originalProduct: Product!
      quotation: Quotation
      unitPrice: Price
      total(category: OrderItemPriceCategory): Price
      discounts: [OrderItemDiscount!]
      dispatches: [Dispatch!]
      configuration: [ProductConfigurationParameter!]
    }

this will affect

  • OrderItemDiscount.item
  • Order.items
  • Mutation.addCartProduct
  • Mutation.addMultipleCartProducts
  • Mutation.addCartQuotation
  • Mutation.updateCartItem
  • Mutation.removeCartItem

Order.total

type Order {
      _id: ID!
      user: User
      status: OrderStatus
      created: Date
      updated: Date
      ordered: Date
      orderNumber: String
      confirmed: Date
      fullfilled: Date
      contact: Contact
      country: Country
      meta: JSON
      currency: Currency
      billingAddress: Address
      delivery: OrderDelivery
      payment: OrderPayment
      items: [OrderItem!]
      discounts: [OrderDiscount!]
      total(category: OrderPriceCategory): Money
      documents(type: OrderDocumentType = CONFIRMATION): [Media!]!
      supportedDeliveryProviders: [DeliveryProvider!]!
      supportedPaymentProviders: [PaymentProvider!]!
      subscription: Subscription
      logs(limit: Int = 10, offset: Int = 0): [Log!]!
    }

will be

type Order {
      _id: ID!
      user: User
      status: OrderStatus
      created: Date
      updated: Date
      ordered: Date
      orderNumber: String
      confirmed: Date
      fullfilled: Date
      contact: Contact
      country: Country
      meta: JSON
      currency: Currency
      billingAddress: Address
      delivery: OrderDelivery
      payment: OrderPayment
      items: [OrderItem!]
      discounts: [OrderDiscount!]
      total(category: OrderPriceCategory): Price
      documents(type: OrderDocumentType = CONFIRMATION): [Media!]!
      supportedDeliveryProviders: [DeliveryProvider!]!
      supportedPaymentProviders: [PaymentProvider!]!
      subscription: Subscription
      logs(limit: Int = 10, offset: Int = 0): [Log!]!
    }

this will affect

  • Mutation.createCart
  • Mutation.emptyCart
  • Mutation.checkoutCart
  • Mutation.updateCart
  • Mutation.setOrderDeliveryProvider
  • Mutation.setOrderPaymentProvider
  • Mutation.removeOrder
  • Mutation.confirmOrder
  • Mutation.payOrder
  • Mutation.deliverOrder
  • Query.orders
  • Query.order
  • type Log.order
  • type SubscriptionPeriod.order
  • type User.cart & type User.orders
  • type OrderGlobalDiscount.order and type OrderDiscount.order
  • type OrderItem.order
  • type OrderPickUpLocation.order

OrderPayment.fee

interface OrderPayment {
      _id: ID!
      provider: PaymentProvider
      status: OrderPaymentStatus
      fee: Money
      paid: Date
      meta: JSON
      discounts: [OrderPaymentDiscount!]
    }

will become

interface OrderPayment {
      _id: ID!
      provider: PaymentProvider
      status: OrderPaymentStatus
      fee: Price
      paid: Date
      meta: JSON
      discounts: [OrderPaymentDiscount!]
    }

inherit down to

OrderPaymentInvoice
OrderPaymentCard
OrderPaymentGeneric

@pozylon
Copy link
Member

pozylon commented Feb 1, 2021

Mhm, I don’t get what exactly you want to change regarding the order pricing. I was not really thinking to tamper the order pricing. Can you try to explain by showing me some pseudo schema or concept?

@pozylon
Copy link
Member

pozylon commented Feb 1, 2021

Oh okay wow just missed each other,

@Mikearaya
Copy link
Contributor Author

after auditing the whole schema for Money type replacing it with Price type doesn't seem like the right move as some of the fields in Price type are not relevant some situations

@Mikearaya
Copy link
Contributor Author

Mhm, I don’t get what exactly you want to change regarding the order pricing. I was not really thinking to tamper the order pricing. Can you try to explain by showing me some pseudo schema or concept?

i mean since now the price applied to the order will differ based on the quantity a person orders i was thinking there should be some update there too to handle that logic

let say a product default price is $100 but in bulk order say 5 items that will drop to $90. so when calculating the order total price the quantity of the item in question should be considered

@pozylon
Copy link
Member

pozylon commented Feb 1, 2021

Order Pricing uses pretty much the same price logic like price simulation so it will automatically gain that new feature I guess nothing to change there.

@pozylon
Copy link
Member

pozylon commented Feb 1, 2021

Order Pricing will go through all items, take the quantity of the items, let it run through product pricing, sum it all up then add delivery and payment fees.

@pozylon
Copy link
Member

pozylon commented Feb 1, 2021

Product pricing takes the correct price level through the normal catalog price plugin

@pozylon
Copy link
Member

pozylon commented Feb 1, 2021

DeliveryFee.price: Money

type DeliveryFee {  _id: ID!  isTaxable: Boolean!   isNetPrice: Boolean!   country: Country!   price: Money!   }

can be replaced with price if it didn't have country field
this will affect type DeliveryProvider.simulatedPrice

I don‘t know why country is there, does not make sense to me, so I think that is a great idea to change

@Mikearaya
Copy link
Contributor Author

@pozylon Noted. I was thinking the same too, left them out until we are happy with the actual changes. I'll add the tests.

@pozylon
Copy link
Member

pozylon commented Feb 9, 2021

@Mikearaya In a new branch, else brain is going to 💨

@Mikearaya
Copy link
Contributor Author

@pozylon 😆 i agree. ok do the honor and merge this one🎀

@pozylon pozylon merged commit 5846fa4 into master Feb 9, 2021
@pozylon pozylon deleted the catalog-price-resolver branch February 9, 2021 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants