Discount price component name is set incorrectly in price calculation rules

Thanks for your reply goldorak.

I completely understand and agree with your points. However, I've looked into it a little deeper and I think that something unexpected is happening, a bug really. I'm changing the title of the issue to make it more accurate, changing the category to bug report and providing a patch.

It turns out that Commerce core (or Commerce Pricing) already defines price component types and when calculating the sell price, it stores the name of the component type in the "name" property of the component (why not call it "type"??)

I think the commerce_discount module is inadvertently setting that name value (i.e. price component type) incorrectly during the sell price calculations.

In the code below, I am showing the structure of the price components for a single line item that has two discounts applied to it, during the sell price calculation:

components[3]
  [0]
  	name='base_price'
  	price[3]
	  amount="1500"
	  currency_code="USD"
	  data[1]
	  	components[0]
	included=true
  [1]
  	name="Some user defined discount name - 10% off"
  	price[3]
  	  amount=-150
  	  currency_code="USD"
  	  data[2]
  	  	discount_name="some_user_defined_discount_name_10_off"
  	  	discount_component_title="Some user defined discount name - 10% off"
  	included=true
  [2]
  	name="A second user defined discount name - $2 off"
  	price[3]
  	  amount=-200
  	  currency_code="USD"
  	  data[2]
  	  	discount_name="second_user_defined_discount_name_2_off"
  	  	discount_component_title="A second user defined discount name - $2 off"
  	included=true

You'll note that the values in components[N]['name'] and components[N]['price']['data']['discount_component_title'] are identical. However, the commerce_pricing module wants the "name" value to represent the name of price component type (see the function definition below.) Based on that, I think "name" should be a fixed value for each type of component, and not a user-defined value.

In commerce_discount.rules.inc, the commerce_discount_add_price_component function calls the commerce_price_component_add function on line 361:

  $line_item_wrapper->commerce_unit_price->data = commerce_price_component_add(
    $line_item_wrapper->commerce_unit_price->value(),
    empty($component_title) ? 'discount' : $component_title,
    $difference,
    TRUE
  );

The second value passed to the function, empty($component_title) ? 'discount' : $component_title, contains either the discount_component_title or 'discount', but, again, per the function definition, that should be the component type name only, i.e., 'discount'.

Here's the commerce_price_component_add function definition from commerce_price.module:

/**
 * Adds a price component to a price's data array.
 *
 * @param $price
 *   The price array the component should be added to.
 * @param $type
 *   The machine-name of the component type to be added to the array.
 * @param $component_price
 *   The price array for the component as defined by the price field.
 * @param $included
 *   Boolean indicating whether or not the price component has already been
 *   included in the price the component is being added to.
 * @param $add_base_price
 *   Boolean indicating whether or not to add the base price component if it is
 *   missing.
 *
 * @return
 *   The updated data array.
 */
function commerce_price_component_add($price, $type, $component_price, $included, $add_base_price = TRUE) {
  // If no price components have been added yet, add the base price first.
  if ($add_base_price && empty($price['data']['components']) && $type != 'base_price') {
    $price['data'] = commerce_price_component_add($price, 'base_price', $price, TRUE);
  }

  $price['data']['components'][] = array(
    'name' => $type,
    'price' => $component_price,
    'included' => $included,
  );

  return $price['data'];
}

So, I think the call to the function should be changed to:

  $line_item_wrapper->commerce_unit_price->data = commerce_price_component_add(
    $line_item_wrapper->commerce_unit_price->value(),
    'discount',
    $difference,
    TRUE
  );

I've tested the patch and discount_component_title remains unchanged and the still presents correctly on the cart, checkout pages, etc.

Let me know what you think, goldorak.

Thanks!

Right! I see that is the problem. Thanks for providing that detail. It happens that the module I am writing eliminates all but the single highest valued discount that applies to a line item. I was only seeing only one discount per item with or without the patch.

Either way, I need a simple, obvious value to test if a component is a discount. So how about a compromise: why not prepend "discount-" to the name of a discount component? For example, "discount-prod1 10%", "discount-prod2 35%"

That would consistently identify discount components, but still provide the distinct values that are needed to present each component individually.

Something to consider.

Thanks!

And in my opinion, it would also keep the commerce discount components in closer compliance with the intent of the architecture of Commerce Pricing module.

Perfecto!