We need to implement a price resolver for promotions that will take the purchasable entity and attempt to apply promotions to it.

CommentFileSizeAuthor
#78 interdiff-2805549-69-78.txt1.73 KBs.messaris
#78 2805549-78.patch35.5 KBs.messaris
#69 2805549_interdiff-64-69.txt2.73 KBflocondetoile
#69 2805549-69.patch35.24 KBflocondetoile
#64 expand_calculated_price-2805549-64.patch34.08 KBlisastreeter
#63 expand_calculated_price-2805549-63.patch34.08 KBlisastreeter
#58 expand_calculated_price-2805549-58.patch34.17 KBlisastreeter
#57 expand_calculated_price-2805549-57.patch34.16 KBlisastreeter
#56 expand_calculated_price-2805549-56.patch34.36 KBlisastreeter
#55 expand_calculated_price-2805549-55.patch34.2 KBlisastreeter
#54 expand_calculated_price-2805549-54.patch34.01 KBlisastreeter
#53 expand_calculated_price-2805549-52.patch34.12 KBlisastreeter
#51 expand_calculated_price-2805549-51.patch34.12 KBlisastreeter
#49 expand_calculated_price-2805549-49.patch30.14 KBlisastreeter
#48 expand_calculated_price-2805549-48.patch30.1 KBlisastreeter
#47 expand_calculated_price-2805549-47.patch30.44 KBlisastreeter
#41 expand_calculated_price-2805549-41.patch29.5 KBlisastreeter
#34 expand_calculated_price-2805549-34.patch38.38 KBlisastreeter
#32 expand_calculated_price-2805549-32.patch36.85 KBlisastreeter
#31 expand_calculated_price-2805549-31.patch36.67 KBlisastreeter
#30 expand_calculated_price-2805549-30.patch35.73 KBlisastreeter
#29 expand_calculated_price-2805549-29.patch34.74 KBlisastreeter
#26 2805549-26.patch29.18 KBmglaman
#24 2805549-24.patch26.39 KBmglaman
#20 interdiff-19-20.txt789 bytesflocondetoile
#20 2805549-20.patch27.33 KBflocondetoile
#19 expand_the_calculated-2805549-19.patch27.47 KBedurenye
#19 interdiff-expand_the_calculated-2805549-18-19.txt1.97 KBedurenye
#18 expand_the_calculated-2805549-18.patch25.5 KBedurenye
#17 expand_the_calculated-2805549-17.patch25.76 KBedurenye
#17 interdiff-expand_the_calculated-2805549-15-17.txt1.14 KBedurenye
#15 expand_the_calculated-2805549-15.patch25.51 KBmglaman
#9 create_a_cart_price-2805549-9.patch5.45 KBedurenye
#9 interdiff-create_a_cart_price-2805549-8-9.txt1.16 KBedurenye
#8 create_a_cart_price-2805549-8.patch5.45 KBedurenye
#8 interdiff-create_a_cart_price-2805549-5-8.txt1.03 KBedurenye
#5 create_a_cart_price-2805549-5.patch4.41 KBedurenye
#5 interdiff-create_a_cart_price-2805549-4-5.txt1.84 KBedurenye
#4 create_a_cart_price-2805549-4.patch4.24 KBedurenye

Comments

mglaman created an issue. See original summary.

bojanz’s picture

Title: Create a price resolver for promotions » Create a "Cart price" formatter

The "Calculated price" formatter shows the resolved price, which is performant and good for most use cases (select price by role / country / currency, choose a sale price, etc).

We also want to port the 1.x concept of "how much would this product cost if it was in my cart right now", hence the cart price formatter that would live in commerce_cart. It would create an empty order with the current store and user, add the product to it, then invoke the order refresh process, take the order item's adjusted price, show it.

This would allow people to show promotions directly on the catalog pages, at a performance cost.

Note: This will result in promotions being loaded that only affect the order or the shipping total. Would be great to figure out how to skip those, if possible.

bojanz’s picture

Title: Create a "Cart price" formatter » Create a "Cart price" price formatter
edurenye’s picture

Status: Active » Needs review
StatusFileSize
new4.24 KB

First approach to the solution.

edurenye’s picture

mglaman’s picture

Status: Needs review » Needs work
+++ b/modules/promotion/src/Plugin/Field/FieldFormatter/CartPriceFormatter.php
@@ -0,0 +1,118 @@
+    /** @var \Drupal\commerce_order\OrderItemStorageInterface $storage */
+    $storage = \Drupal::service('entity_type.manager')->getStorage('commerce_order_item');
+    /** @var \Drupal\commerce_order\Entity\OrderItemInterface $order_item */
+    $order_item = $storage->createFromPurchasableEntity($purchasable_entity, ['quantity' => 1]);
+    $order_item->save();
...
+    $order = Order::create([
+      'type' => 'default',
+      'state' => 'completed',
+      'uid' => $this->currentUser->id(),
+      'order_items' => [$order_item],
+    ]);
...
+    $order->save();
+    $items = $order->getItems();

Wait, so this is creating order items and saving them each view? We can actually run unsaved orders and order items through some processes.

However, after reviewing https://github.com/drupalcommerce/commerce/blob/8.x-2.x/modules/order/sr..., I see the order item has a chance of becoming saved.

edurenye’s picture

We need to save the order, as there is done the price refresh and adds the adjustments we need to extract later.

One problem I found playing with this patch is that we can have a situation when the customer did not add any shipping address, then we get the following error:

The website encountered an unexpected error. Please try again later.
TypeError: Argument 1 passed to Drupal\commerce_tax\TaxZone::match() must implement interface CommerceGuys\Addressing\AddressInterface, null given, called in modules/contrib/commerce/modules/tax/src/Plugin/Commerce/TaxType/LocalTaxTypeBase.php on line 281 in Drupal\commerce_tax\TaxZone->match() (line 138 of modules/contrib/commerce/modules/tax/src/TaxZone.php). 

I'm not sure if this is a different issue and we can have this problem without this patch.
And not sure neither how to fix it, what we are supposed to do if the customer did not enter any shipping address? Just skip the taxes, I think is the best option by now, as right now I'm not even showing the price with taxes, that I think this should be configurable somehow.

edurenye’s picture

Status: Needs work » Needs review
StatusFileSize
new1.03 KB
new5.45 KB

This seems to fix the problem, but still there the question to make tax option through configuration.

edurenye’s picture

sorabh.v6’s picture

Assigned: Unassigned » sorabh.v6
sorabh.v6’s picture

Assigned: sorabh.v6 » Unassigned
bojanz’s picture

Title: Create a "Cart price" price formatter » Expand the "Calculated price" resolver with the ability to show prices with promotions/taxes/fees
Status: Needs review » Needs work

We discussed this and agreed that the difference between "Calculated price" and "Cart price" would not be obvious.
So we decided to merge this functionality into the "Calculated price" formatter, which will need to be moved to commerce_order because of that. The formatter will have the following options:
[x] Apply taxes to the calculated price
[x] Apply promotions to the calculated price
[x] Apply fees to the calculated price
All three would be disabled by default.

We'd want to move most of the calculation logic into a \Drupal\commerce_order\PurchasableEntityPriceCalculator (bad name?). We'd basically give the service a purchasable entity and a list of adjustment types, and the service would then call the individual order processors, then call $order_item->getAdjustedUnitPrice(). That means we need to merge #2842256: Provide a getter method for adjusted price of order items first.

bojanz’s picture

Title: Expand the "Calculated price" resolver with the ability to show prices with promotions/taxes/fees » Expand the "Calculated price" formatter with the ability to show prices with promotions/taxes/fees
mglaman’s picture

PR up at https://github.com/drupalcommerce/commerce/pull/781.

A handful of follow ups left.

mglaman’s picture

StatusFileSize
new25.51 KB

Here is an updated patch. Exposes as settings for what adjustments to include. Ditches Refresh for invoking processors directly.

travis-bradbury’s picture

I was working with that patch and had a couple issues.

+    $adjustment_types = $this->getSetting('adjustment_types');
+    if (empty($adjustment_types)) {
+      $summary[] = $this->t('No included adjustment types');
+    }
+    else {
+      $adjustment_type_labels = [];
+      foreach ($adjustment_types as $adjustment_type) {
+        $definition = $this->adjustmentTypeManager->getDefinition($adjustment_type);

This fails because $adjustment_types becomes ['promotions' => 'promotions', 'tax' => 0] or similar and AdjustmentTypeManger::getDefinition(0) doesn't work.

More critically, my site is crashing due to an error when checking whether tax is applicable to the order used for price calculation.

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getStore() on null in Drupal\commerce_tax\Plugin\Commerce\TaxType\TaxTypeBase->resolveCustomerProfile() (line 225 of modules/contrib/commerce/modules/tax/src/Plugin/Commerce/TaxType/TaxTypeBase.php).

Drupal\commerce_tax\Plugin\Commerce\TaxType\TaxTypeBase->resolveCustomerProfile(Object) (Line: 108)
Drupal\commerce_tax\Plugin\Commerce\TaxType\LocalTaxTypeBase->apply(Object) (Line: 40)
Drupal\commerce_tax\TaxOrderProcessor->process(Object) (Line: 121)
Drupal\commerce_order\PurchasableEntityPriceCalculator->calculate(Object, 1, Array) (Line: 195)
Drupal\commerce_order\Plugin\Field\FieldFormatter\PriceCalculatedFormatter->viewElements(Object, 'en') (Line: 80)

I don't have tax selected as an adjustment type for the formatter so I wouldn't want tax calculations to be done, but it is anyway because TaxOrderProcessor is a processor that PurchasableEntityPriceCalculator::calculate() runs before doing the adjustments.

The actual error happens in TaxTypeBase::resolveCustomerProfile() because it tries to get the order item's order. The order item was added to an order earlier but OrderItem::getOrder() returns null and that's unexpected by the profile resolver.

I had a few thoughts:

  1. Have the profile resolver return null when the order item doesn't have an order. This might be worthwhile but it doesn't feel like it's solving the real problem.
  2. Return an order from OrderItem::getOrder() for these order items. This doesn't look straight forward to me since the order isn't saved yet, so it doesn't have an ID for the order item to know about. I'm not sure this would even be desirable behavior.
  3. Not get into tax processing at all when tax isn't desired by the price formatter. This might still still leave a problem if you did want tax included.
edurenye’s picture

Rerolled and apply this #2842920: Use 'maximum fraction digits' from the price formatter in the views handlers to it, as it does not apply the correct decimals.

Still needs the work set in #16 and the rest of points from the PR.

edurenye’s picture

StatusFileSize
new25.5 KB

I rerolled wrongly. Now it's fine.

edurenye’s picture

Fix #16. The method says that must return Null if it can not resolve the customer profile, I fixed that, nothing about the problem when you want to resolve the actual tax.

flocondetoile’s picture

Status: Needs work » Needs review
StatusFileSize
new27.33 KB
new789 bytes

Fixing in #16

This fails because $adjustment_types becomes ['promotions' => 'promotions', 'tax' => 0] or similar and AdjustmentTypeManger::getDefinition(0) doesn't work.

back to NR to launch tests

Status: Needs review » Needs work

The last submitted patch, 20: 2805549-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mglaman’s picture

The fix in #19 needs to be rolled into its own issue. We can keep it here, for, but let's pop it into its own for a proper fix.

mglaman’s picture

Actually for #19 it is not a bug. We're just constructing a faulty order item object. We need to run $order_item->order_id = $order;.

Looks like we'll need a test with a local tax in the system.

mglaman’s picture

Status: Needs work » Needs review
StatusFileSize
new26.39 KB

Here is a patch which includes interdiffs from #17 and #20. I have added test coverage for the profile generation, by making sure the order item has a reference to its order properly.

Status: Needs review » Needs work

The last submitted patch, 24: 2805549-24.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mglaman’s picture

Status: Needs work » Needs review
StatusFileSize
new29.18 KB

Adds testing of the formatter output.
Fixes order test

Next steps: move the formatter to a twig template so we can allow more fancy output.

Status: Needs review » Needs work

The last submitted patch, 26: 2805549-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

lisastreeter’s picture

Matt and I just discussed the issue of the overhead in the purchasable entity price calculator, resulting from running calculations on all the order processors instead of just the ones that provide the specified adjustment types. There is a @todo item related to this (for skipping the AvailabilityOrderProcessor) in the current patch. Matt has suggested that a second tag be added for order processors, an "adjustment provider" tag. An order processor that is an adjustment provider should also list the adjustment types it provides as part of its tag. Essentially, an order processor would be able to say, "I provide adjustments, and I provide these adjustment types."

Adding this second tag will make it possible to limit the order processors ahead of calculation, which will speed up the price calculator service. After calculation, the adjustments can still be filtered by type, as they currently are.

I'm planning to take a look at implementing this, but as I'm new to the concept of "tagged services", I may be a bit slow. I've got some reading to do before I get started with anything... Matt suggested that I post this comment in case anyone else is interested in moving this forward more quickly.

lisastreeter’s picture

StatusFileSize
new34.74 KB

Patch summary:

1. The label for the existing (commerce_price) PriceCalculatedFormatter has been changed from "Calculated price" to "Calculated price (no adjustments)" to differentiate it from the new (commerce_order) PriceCalculalatedFormatter. This addresses the @todo in the last patch: How are we going to handle plugin ID name change?

2. In commerce_promotion.services.yml and commerce_tax.services.yml, an argument, "adjustment_types", was added to the commerce_order.order_processor service tag. The format of this adjustment_types argument is a space-separated list of adjustment types provided by the service. For example, for the commerce_promotion.promotion_order_processor, the tag now looks like this:
{ name: commerce_order.order_processor, priority: 50, adjustment_types: 'promotion' }

@todo: the shipment order processor service provided by the Commerce Shipping module will also need to be updated to declare the type of adjustment it provides; until that's added, shipping type adjustments will not be included in the price calculations.

3. In the PurchasableEntityPriceCalculator class, the addProcessor() method was modified to accept an optional $adjustment_types argument. If an order processor does not provide any adjustment types, it will not be added. If it does provide adjustment types, those types are stored in an array along with the order processor, to be used by the calculate() method. If adjustment types are specified for the PurchasableEntityPriceCalculator calculate() method, order processors are filtered based on these types *before* any order processing occurs.

4. The PurchasableEntityPriceCalculator calculate() method was modified to accept an optional $order_entity argument (type OrderInterface). If provided, the order entity can be used to calculate adjustments instead of the default order created based on the purchasable entity. This expands the functionality of the Calculator. For example...

Currently, promotions based on order email, IP address, or Billing Profile cannot be applied. If relevant, the order entity could be populated with any of this information. Another use-case in which it could potentially be useful is in the admin ui, where it could be used to calculate/display discounts in the context of an existing order. (This is actually the context in which I found this issue--I was working on an Ajax-enabled price widget for the admin ui that would display the calculated price based on the purchasable entity and entered item quantity.)

Next steps:
* move the formatter to a twig template so we can allow more fancy output. (comment #26)
* review/update testing as necessary to confirm only applicable order processors are invoked (based on adjustment types provided).

lisastreeter’s picture

StatusFileSize
new35.73 KB

New patch addresses coding standards issues and incorrect PurchasableEntityPriceCalculatorTest assertions.

lisastreeter’s picture

StatusFileSize
new36.67 KB

(Sorry about all the failed patch posts, I haven't managed to get my local environment set up for running tests yet.) Here is one more attempt to fix the tests.

lisastreeter’s picture

StatusFileSize
new36.85 KB
mglaman’s picture

  1. +++ b/modules/order/src/Plugin/Field/FieldFormatter/PriceCalculatedFormatter.php
    @@ -0,0 +1,196 @@
    + * Plugin implementation of the 'commerce_price_calculated' formatter.
    ...
    + * )
    

    Let's change this to the `commerce_order` tidbit.

  2. +++ b/modules/order/src/PurchasableEntityPriceCalculator.php
    @@ -0,0 +1,257 @@
    +  public function addProcessor(OrderProcessorInterface $processor, $adjustment_types = NULL) {
    

    \o/

  3. +++ b/modules/order/tests/src/Kernel/PurchasableEntityPriceCalculatorTest.php
    @@ -0,0 +1,229 @@
    +    $calculated = $this->priceCalculator->calculate($this->variation, 1, ['test_adjustment_type']);
    +    $this->assertEquals(new Price('12.00', 'USD'), $calculated['original']);
    +    $this->assertEquals(new Price('12.00', 'USD'), $calculated['calculated']);
    ...
    +    $calculated = $this->priceCalculator->calculate($this->variation, 1, ['promotion', 'test_adjustment_type']);
    +    $this->assertEquals(new Price('12.00', 'USD'), $calculated['original']);
    +    $this->assertEquals(new Price('6.00', 'USD'), $calculated['calculated']);
    

    Amazing, thanks!

lisastreeter’s picture

StatusFileSize
new38.38 KB
mglaman’s picture

Status: Needs work » Needs review

Tests pass!

The last submitted patch, 17: expand_the_calculated-2805549-17.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 18: expand_the_calculated-2805549-18.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 19: expand_the_calculated-2805549-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

lisastreeter’s picture

I should have thought of this last night before I started changing PriceFormattersTest to get the patch to pass, but it didn’t occur to me until this evening. There’s a problem in the latest patch related to the commerce_price PriceCalculatedFormatter plugin. I use the commerce_order price calculator to calculate the resolved price. But that creates a dependency for commerce_price on commerce_order that shouldn’t exist.

When I started working on the Issue, the commerce_price calculated price formatter had an error when I viewed products within the admin ui, because $this->currrent_store->getStore() was null (line 121). My first thought was to copy the protected selectStore() method from the PurchableEntityPriceCalculator class into the formatter plugin class. But I didn’t want to duplicate code unnecessarily (and didn’t think about the dependency issue), so instead, I just replaced the problematic code with a call to the PurchasableEntityPriceCalculator’s calculate method.

So…the quick solution at this point is probably to just duplicate the selectStore() method and get all commerce_order dependencies out of both the commerce_price PriceCalculatedFormatter and the PriceFormattersTest. But before I make any more changes, I thought I’d just post this comment first, in case anybody has a suggestion for a better solution.

mglaman’s picture

So in the original formatter we just did

$context = new Context($this->currentUser, $this->currentStore->getStore());

Which does not guard against the fact the stores may not match. This guard is only in the add to cart form. I wonder if we should just simplify our PurchasableEntityCalculator to be simple, too and trust that the passed purchasable entity is purchasable by the current store.

lisastreeter’s picture

StatusFileSize
new29.5 KB

Cleaned up a few things after talking with Matt today, including simplification of PurchasableEntityCalculator:

getStore() method has been removed from the PurchasableEntityCalculator

bojanz’s picture

Status: Needs review » Needs work

Thank you! Great work so far. Rarely is someone so productive in this queue.

+  public function calculate(PurchasableEntityInterface $purchasable_entity, $quantity, array $adjustment_types = [], OrderInterface $order_entity = NULL) {
+
+    if (!empty($order_entity)) {
+      $context = new Context($order_entity->getCustomer(), $order_entity->getStore());
+    }

1) In general, we don't suffix entity variables with _entity. It would be just $order
2) I don't like this approach, it doesn't feel clean. I'd expect the price calculator to always create (and statically cache) its own orders for calculation purposes.

You list the use cases as:

Currently, promotions based on order email, IP address, or Billing Profile cannot be applied. If relevant, the order entity could be populated with any of this information. 

We should populate the email from the current user, and the IP from the current IP.
We can also fire an event that allows the order to be prepared for the calculation context.

Another use-case in which it could potentially be useful is in the admin ui, where it could be used to calculate/display discounts in the context of an existing order.

A recalculation in the context of an existing order would just be a regular order refresh, no?

+  /**
+   * {@inheritdoc}
+   */
+  public function addProcessor(OrderProcessorInterface $processor, $adjustment_types = NULL) {
+    if (!$adjustment_types) {
+      return;
+    }
+    $adjustment_types_array = explode(' ', $adjustment_types);
+    if (!empty($adjustment_types_array)) {
+      $this->processors[] = [
+        'order_processor' => $processor,
+        'adjustment_types' => $adjustment_types_array,
+      ];
+    }
+  }

The code tells us that $adjustment_types is actually required, since the processor isn't added without it. So let's remove the NULL default and simplify the code. The "order_processor" key can also be just "processor".

Question: Can we simplify the general concept by accepting/defining a singular $adjustment_type? Do we have a use case where an order processor might be producing adjustments of multiple types?

+    $elements['adjustment_types'] = [
+      '#type' => 'checkboxes',
+      '#title' => $this->t('Adjustment types'),
+      '#options' => [],
+      '#default_value' => $this->getSetting('adjustment_types'),
+    ];

We'll need to make this a bit nicer. Merchants shouldn't need to know about adjustment types. In a followup we should add a plural_label to adjustment types. That would allow us to implement the original suggested language:
[x] Apply taxes to the calculated price
[x] Apply promotions to the calculated price
[x] Apply fees to the calculated price

+    foreach ($order_item->getAdjustments() as $adjustment) {
+      if (!in_array($adjustment->getType(), $adjustment_types)) {
+        continue;
+      }
+      if ($adjustment->isIncluded()) {
+        continue;
+      }
+
+      $calculated_price = $calculated_price->add($adjustment->getAmount());
+
+      $type = $adjustment->getType();
+      $source_id = $adjustment->getSourceId();
+      if (empty($source_id)) {
+        // Adjustments without a source ID are always shown standalone.
+        $key = count($adjustments);
+      }
+      else {
+        // Adjustments with the same
+        // type and source ID are combined.
+        $key = $type . '_' . $source_id;
+      }
+
+      if (empty($adjustments[$key])) {
+        $adjustments[$key] = [
+          'type' => $type,
+          'label' => $adjustment->getLabel(),
+          'total' => $adjustment->getAmount(),
+          'weight' => $types[$type]['weight'],
+        ];
+      }
+      else {
+        $adjustments[$key]['total'] = $adjustments[$key]['total']->add($adjustment->getAmount());
+      }
+    }

Why do this? Can't we just use $order_item->getAdjustedTotalPrice()?
Our return value could be [$calculated_price, $base_price, $adjustments], where $calculated_price is the adjusted total price, base price is the resolved price, and the adjustments are $order_item->getAdjustments(). In general, we total up the adjustments when we have multiple order items, but here we always assume 1.

--- a/modules/price/src/Plugin/Field/FieldFormatter/PriceCalculatedFormatter.php
+++ b/modules/price/src/Plugin/Field/FieldFormatter/PriceCalculatedFormatter.php
@@ -21,7 +21,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
  *
  * @FieldFormatter(
  *   id = "commerce_price_calculated",
- *   label = @Translation("Calculated price"),
+ *   label = @Translation("Calculated price (no adjustments)"),
  *   field_types = {
  *     "commerce_price"
  *   }

This feels confusing from a UX standpoint. The merchant shouldn't need to know what an adjustment is. Can't we delete the whole formatter, and the matching part of PriceFormattersTest?

mglaman’s picture

On mobile and proper reply incoming later, but

We can also fire an event that allows the order to be prepared for the calculation context.

I told lisastreeter to take this simpler approach to get feedback before going through all the event boilerplate code.

I also copied the order total summary adjustment logic so we can build similar output.

bojanz’s picture

I'm fine with doing the event in a followup.

My thoughts on the adjustment logic still stand. If there is a real need to turn $adjustments into $summarized_adjustments, we'll need to create some kind of helper so that this logic is not duplicated with the OrderTotalSummary.

lisastreeter’s picture

Thank you! Great work so far. Rarely is someone so productive in this queue.

The funny thing is that I'm not even going to use the "calculated price" formatter in my project. I was working on ajax-enabled widgets for the order admin ui and wrote code to determine the calculated price based on the (promotion-only) adjustments that would be applied. Matt saw that what I was going was similar and pointed me to this issue, so I just started working on it. So to respond to your question:

A recalculation in the context of an existing order would just be a regular order refresh, no?

I'm thinking about ajax. The admin user selects the variation, enters a quantity, and (if the price is not overridden) can immediately see the calculated price with a summary of adjustments applied. The actual adjustments can still be set during the order refresh process. But if, in the end, the ideal calculator for this calculator price formatter doesn't work well for the order admin ui price widget I have in mind, it's easy enough to write/use a separate one that does.

Yes, the event firing approach makes sense and would clean up the messiness I've added with the "order_entity" parameter (named that way only to hint that it might not be an actual order but just an order entity created for context. Thanks for pointing out this as a coding standard error!)

I've never worked with events other than in the context of writing event responders, but I'd be happy to learn and give it a try.

For now, I can remove order_entity and the associated code completely, to clean up this patch. And I can populate the order created by the calculator with the IP address and any other information available from the current user.

The code tells us that $adjustment_types is actually required, since the processor isn't added without it. So let's remove the NULL default and simplify the code. The "order_processor" key can also be just "processor".

Got it. Thanks. I'll make the changes. As for the use case where an order processor might be producing adjustments of multiple types, I don't know... Maybe a shipping + fee, if they need to be accounted for separately? Or promotion + custom (not sure why)? Personally, I can't imagine creating an order processor that produces more than one adjustment type...

Agree completely with merchants not knowing about adjustment types. A plural_label would be a good addition to adjustment types.

Also, related to the formatter, I wonder whether it would be helpful to provide options to the merchants:
[x] Show base price
[x] Show details (or something more descriptive, but not "adjustments")

The default case would be to just show the calculated price. The other options would produce a more detailed summary. Or is it better to leave things simpler and assume that the designer would format the data appropriately in a twig template?

Lastly, the problem with deleting the whole original formatter is, I think, BC. When I just created a new formatter with the same plugin id as the original but in the commerce_order namespace, Drupal was not happy with me. But perhaps I was just doing something wrong...

Thank you for the detailed and instructive feedback.

mglaman’s picture

I'd rather put the event in this issue, then make a follow up to build some sort of Adjustment Table thing which can be used here any by OrderTotalSummary.

For the formatter: we can't just delete the previous one, and what if someone uses the price formatter one and has the order module uninstalled? I guess our proper way forward is that the Order module swaps out the class definition for the Price formatter plugin. So that way it is a seamless enhancement?

Question: Can we simplify the general concept by accepting/defining a singular $adjustment_type? Do we have a use case where an order processor might be producing adjustments of multiple types?

I'd be fine with that. If enough people use multiple adjustment types we can change.

lisastreeter’s picture

StatusFileSize
new30.44 KB

Here is an updated patch that addresses the following issues:

We should populate the email from the current user, and the IP from the current IP.
We can also fire an event that allows the order to be prepared for the calculation context.

Done. Though likely my event-related naming needs to be changed?

The code tells us that $adjustment_types is actually required, since the processor isn't added without it. So let's remove the NULL default and simplify the code. The "order_processor" key can also be just "processor".
order_processor has been changed to processor. Removing the NULL default caused an ReflectionException exception, unless I added, "adjustment_type: ''" to the AvailabilityOrderProcessor service tag. So I left the NULL default in the calculator's addProcessor() method. Without it, the patch would also cause errors for anybody with commerce_shipping (or other contrib modules that provide order processors) installed.

Can we simplify the general concept by accepting/defining a singular $adjustment_type?
Yes, done.

Our return value could be [$calculated_price, $base_price, $adjustments], where $calculated_price is the adjusted total price, base price is the resolved price, and the adjustments are $order_item->getAdjustments().
Done, except $calculated_price is the adjusted unit price, not the adjusted total price.

For the formatter: we can't just delete the previous one, and what if someone uses the price formatter one and has the order module uninstalled? I guess our proper way forward is that the Order module swaps out the class definition for the Price formatter plugin. So that way it is a seamless enhancement?
Done

Big thanks to @bojanz and @mglaman for providing help and guidance on all of this!

Still-to-do:
* In a followup we should add a plural_label to adjustment types.That would allow us to implement the original suggested language:
[x] Apply taxes to the calculated price
[x] Apply promotions to the calculated price
[x] Apply fees to the calculated price

* Move the formatter to a twig template so we can allow more fancy output.

* Testing for the new calculator event?

lisastreeter’s picture

StatusFileSize
new30.1 KB
lisastreeter’s picture

StatusFileSize
new30.14 KB
mglaman’s picture

Status: Needs work » Needs review
+++ b/modules/order/src/Event/OrderEvents.php
@@ -145,4 +145,16 @@ final class OrderEvents {
+  const ORDER_PRICE_CALCULATOR = 'commerce_order.order_price_calculator';

purchasable_price_calculator ? no idea, bike shedding ahoy.

Move the formatter to a twig template so we can allow more fancy output.

I think that can be a follow-up. Because we'll bikeshed the output, and want that adjustment summary available.

Testing for the new calculator event?

Yes, we should. We need to prove it works and that it is documented in the code, at least.

+++ b/modules/order/src/PurchasableEntityPriceCalculator.php
@@ -0,0 +1,185 @@
+    $event = new OrderPriceCalculatorEvent($order, $order_item, $this->currentUser);
+    $this->eventDispatcher->dispatch(OrderEvents::ORDER_PRICE_CALCULATOR, $event);

I'm trying to think on how we should test this.

I guess we can add an event listener which checks the purchased entity SKU. If the SKU is XYZ123, then set something on order data. In a processor check if the order data is present, apply XYZ adjustment.

_I think_ we can piggy back that logic into an existing test processor and just add an event subscriber to the order test module.

lisastreeter’s picture

StatusFileSize
new34.12 KB

First attempt at expanded testing

Status: Needs review » Needs work

The last submitted patch, 51: expand_calculated_price-2805549-51.patch, failed testing. View results

lisastreeter’s picture

StatusFileSize
new34.12 KB
lisastreeter’s picture

StatusFileSize
new34.01 KB
lisastreeter’s picture

StatusFileSize
new34.2 KB
lisastreeter’s picture

StatusFileSize
new34.36 KB

The most recent patch is failing because when the order module swaps out the class definition for the Price formatter plugin, the config schema is not updated to include the adjustment_types setting. @mglaman suggested trying to use hook_config_schema_info_alter, but it doesn't seem like that hook can be used to solve this problem. But perhaps I'm just missing something. Any thoughts?

lisastreeter’s picture

StatusFileSize
new34.16 KB
lisastreeter’s picture

StatusFileSize
new34.17 KB
mglaman’s picture

Hrm. I guess the schema checker in the test runs before the alter, which is a bummer. We can actually explicitly bypass checking the schema via a method in the base class. But I'd like to see if we maybe uncovered a core-ish bug.

bojanz’s picture

We should just copy the adjustment_types schema into the commerce_price definition. Simple hack.

lisastreeter’s picture

It was late last night when my last attempt failed, so I didn't comment then. I'm think hook_field_formatter_info_alter is running before the test. What doesn't seem to be working is the field.formatter.settings.commerce_price_calculated schema I put into commerce_order.schema.yml

I found a tutorial last night that stated that when Drupal compiles configuration, it can merge the original configuration (in commerce_price.schema.yml in this case) with the configuration provided by our module, maintaining the original configuration with the configuration we've added. The example in the tutorial was an extension of core schema, but it seemed like it should work for config as well.

I think that config merging isn't happening in the kernel test. I tried adding 'commerce_price' in the $this->installConfig() method call in the PurchasableEntityPriceCalculatorTest setUp() but when I dug deeper into the installConfig() method, it looks like perhaps the /schema config is being ignored? The next thing I thought I'd try was to put the full config for field.formatter.settings.commerce_price_calculated into commerce_order.schema.yml, just to try to make the kernel test happy. (But perhaps that would just make it unhappy in a new way.)

@bojanz - are you saying I should just add the adjustment_types to the commerce_price.schema.yml, even though it's only used/applicable when the commerce_order module is enabled? Just take advantage of the fact that both modules are part of commerce and "cheat" a little?

bojanz’s picture

Yep, better a two line cheat with a 1 line comment explaining why, than advanced acrobatics.

lisastreeter’s picture

StatusFileSize
new34.08 KB

"config cheat" patch

lisastreeter’s picture

StatusFileSize
new34.08 KB
mglaman’s picture

Status: Needs work » Needs review

\o/

gauravjeet’s picture

After applying patch #64 cleanly, got this error:

TypeError: Argument 2 passed to Drupal\commerce\Context::__construct() must implement interface Drupal\commerce_store\Entity\StoreInterface, null given, called in commerce/modules/order/src/PurchasableEntityPriceCalculator.php on line 130 in Drupal\commerce\Context->__construct() (line 46 of modules/contrib/commerce/src/Context.php).

i'm using commerce 2.x-dev
suggestions?

lisastreeter’s picture

@gauravjeet I was getting that same error (and found that I'd also get it if I used the calculated price formatter without the patch.) @mglaman helped me fix it. In my case, it was an issue with not having the default store properly set. I only have one store, so I had assumed it was "default" by default. I'm not 100% sure, but I think I fixed it by just editing the store through the UI and saving it (not making any changes.)

gauravjeet’s picture

@lisastreeter
woaa! that UI store re-save worked.. thats crazy!

thanks

flocondetoile’s picture

StatusFileSize
new35.24 KB
new2.73 KB

Patch #64 works fine. +1 for RTBC
Attached the same patch #64 with a twig template added for the formatter.

joachim’s picture

Patch appears to work well -- got it showing me pro-rata prices from Commerce Recurring.

> Attached the same patch #64 with a twig template added for the formatter.

That seems like feature creep to me. Also, surely this theming could just as well apply to the original formatter that's in the Price module -- therefore the theming should be there too.

mglaman’s picture

That seems like feature creep to me. Also, surely this theming could just as well apply to the original formatter that's in the Price module -- therefore the theming should be there too.

Slightly. I had intended that as a follow-up. But even something basic like this should be fine.

flocondetoile’s picture

> That seems like feature creep to me.

The primary purpose of using a custom theme implantation is to easily, from a theme, customize the calculated price displayed, and if needed to display too the base price available in the variables. Or even display the adjustments if needed.

For example I override this template in my theme as below.

<span class="calculated-price price-item">{{- calculated_price -}}</span>{% if calculated_price != base_price %}<span class="base-price price-item">{{- base_price -}}</span>{% endif %}

> Also, surely this theming could just as well apply to the original formatter that's in the Price module

The orignal formatter have only on value, the price. So it seems to me that It's not really relevant to add a theme on it.

flocondetoile’s picture

@mglaman : I added it now because I would have found it unfortunate to commit this great job without really being able to exploit it right now (especially one of the basic uses is to be able to display the original price in case of price with promotion). Especially that it really lacked very little thing, a basic thing, to then be able to take full control of the display of the price at the level of the layer of the theme.

mglaman’s picture

Yeah, I'm totally fine committing a bare-bone Twig template. One thing we have learned in 2.x is how valuable Twig has been for end users.

s.messaris’s picture

Hi, I applied the patch, but when using the calculated formatter in a view field I don't have the option to include adjustments. Is this by design? Am I doing something wrong? When using the formatter in the display settings I see the option and it works well.

joachim’s picture

> Hi, I applied the patch, but when using the calculated formatter in a view field I don't have the option to include adjustments.

Just tested this -- made a view of Product Variations, added the price field, set the formatter to Calculated price. The adjustment types checkboxes show for me.

s.messaris’s picture

I tested some more, found my problem was that I was trying to add the formatter in a commerce_order_item view, and, because commerce_order_item does not implement PurchasableEntityInterface, PriceCalculatedFormatter::isApplicable() returns FALSE, so it falls back to the PriceDefaultFormatter.

As I understand it, the limitation is logical on this implementation. The problem is, though, that we should have a way to use the formatter for commerce_order_item. For example, I have

  • A product with a price of 10 EUR
  • A promotion giving 10% discount to all products

I expect to be able to show the product price as 9 EUR, both in the product view and the cart. For the product view it is possible with this implementation, but for the cart is not.

s.messaris’s picture

StatusFileSize
new35.5 KB
new1.73 KB

I found I can solve my problem by getting the purchased entity from the order item. I think this will help.

bojanz’s picture

@s.messaris
Please don't hijack this issue, it's long enough as it is.
This issue is clearly about formatting prices on the catalog. The formatter that's being extended isn't even used on order items.

What you need is a views field that will render $order_item->getAdjustedUnitPrice() (or the adjusted total price), but since that's not usual cart behavior I would advise you to reconsider and rediscuss with your client.

s.messaris’s picture

I'm sorry if this was out of the scope of this issue, but since the option to use the calculated price formatter shows up in the view, I found it bad UX to select it and get the default formatter functionality. I, for one, spent some time trying to figure out why I was not getting the adjustments options, and I am sure some other developers will have the same issue, don't you agree?

agoradesign’s picture

tried #69 +1 for RTBC

bojanz’s picture

Assigned: Unassigned » bojanz
Status: Needs review » Needs work

This issue lands today!

Reviewed it several times, and spoke to mglaman about some of the changes I want to make:

1. Rename PurchasableEntityPriceCalculator to just PriceCalculator. It's less precise, but more friendly.
2. Pass $context to ->calculate(), to allow callers to get user/store specific prices.
3. Implement a custom CompilerPass cause the current one requires us to do the $adjustment_type = NULL + empty check dance in addProcessor().
4. addProcessor() wasn't added to the interface
5. We need a static cache of created dummy orders, for performance reasons
6. Remove the custom event, we can rely on the existing order create event/hook. We'll add an additional flag that the event/hook can use to identify dummy orders.
7. Use plural labels for the adjustment types setting in the formatter.

I've already addressed the feedback above and am now updating tests.
Also thinking about replacing the array result with a typed one (PriceCalculationResult)

agoradesign’s picture

sounds great :)

  • bojanz committed d006d19 on 8.x-2.x authored by lisastreeter
    Issue #2805549 by lisastreeter, edurenye, mglaman, flocondetoile, bojanz...
bojanz’s picture

Status: Needs work » Fixed

Three people did major work on the patch:

- I committed #2943525: Clean up PriceCalculatedFormatter and gave edurenye commit credit there.
- This issue's commit credit goes to lisastreeter.
- Opened #2943579: Use a twig template for rendering calculated prices to give flocondetoile credit there.

Also opened #2943585: Use a value object for the result returned from commerce_order's PriceCalculator for the value object I mentioned in #82.

Thanks, everyone! See you in followups.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Leagnus’s picture

  del