Problem/Motivation

Currently, commerce_price_component_types() doesn't include user-defined discounts.

Proposed resolution

I've added a patch to add this info to the price components info array, using discount labels as the component display title. Also, I've changed the price component key to "discount|machine_name" instead of "discount|label" since the label can be changed through the UI which may break code relying on the old label.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#8 Edit_Order_discount_-_fixed___Site-Install.png30.21 KBjoelpittet
#3 interdiff.txt643 bytesjoelpittet
#3 add_active_discounts_to-2433273-3.patch1.62 KBjoelpittet
commerce_discount-add_price_component_info-1.patch1.62 KBbgilhome

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Thank you, I'm glad you changed it to machine name.

deggertsen’s picture

deggertsen commented

+1 RTBC

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: Add active discounts to price component types info array » Add active discounts 'display_title' to price component types data array
Issue summary: View changes
StatusFileSize
new add_active_discounts_to-2433273-3.patch1.62 KB
new interdiff.txt643 bytes

Coding standards comma added, no need to change status. This one is a nice one.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs profiling, +Performance

Let's see what kind of performance impact this may have.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I've committed the discount_name (aka machine name) part of the patch.

Let's look at if the first part the component type info is needed, and if it's slow?

  • joelpittet committed d7cd968 on 7.x-1.x 
    Issue #2433273 by joelpittet, bgilhome: Use discount machine name for...
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue tags: -needs profiling, -Performance +Needs tests

Doesn't need profiling but needs tests.

  1. +++ b/commerce_discount.module
@@ -7,6 +7,23 @@
+  $discounts = entity_load('commerce_discount', FALSE, array('status' => TRUE));

    Would this mean disappearing labels if a discount was disabled? 

  2. +++ b/commerce_discount.module
@@ -7,6 +7,23 @@
+      'title' => $discount->label,
+      'display_title' => $discount->label,

    Do they both need to be defined as the same thing? And shouldn't they be the "component title" because UI says that is what shows to the customer.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
StatusFileSize
new Edit_Order_discount_-_fixed___Site-Install.png30.21 KB

Visuals for your consideration:

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

So I wonder if this should be closed as "works as designed"? Commerce Discount adds its component and label properly. The benefit of hook_commerce_price_component_type_info() might be reusing a discount's price component else where? But I think that'd be bad juju to encourage that.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Fixed
Issue tags: -Needs tests

Agreed, if someone wants to use the label like that, it's in that patch for the taking.

Closing as fixed as I've committed the important part regarding the machine name.

Status: Fixed » Closed (fixed)

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