Add a display name to promotions

Adds a display_name field, for now i set it to Required, but maybe in the future we should have it auto populate from the name maybe? I just left it simple for now.

Updated w/ changes.

https://github.com/drupalcommerce/commerce/pull/706

RTBC +1

Flipped back to RTBC as there is actually test coverage, talked to mglaman and it was just a mistake.

Looks good to me as well. I'm about to use it now that conditions landed for promotions a few days ago. There's an update path. And it seems to do what it says.

+++ b/modules/promotion/commerce_promotion.install
@@ -98,3 +98,27 @@ function commerce_promotion_update_8202() {
+    ->setTranslatable(TRUE)

+++ b/modules/promotion/src/Plugin/Commerce/PromotionOffer/PromotionOfferBase.php
@@ -129,8 +129,7 @@ abstract class PromotionOfferBase extends ExecutablePluginBase implements Promot
-      // @todo Change to label from UI when added in #2770731.

Nit: there's still a few more more mentions of this @TODO in OrderFixedAmountOff, OrderPercentageOff, OrderItemPercentageOff. But that could be fixed on commit.

Actually, there's conflicts on the PR. This needs to be NW. But we can pick up those nits at the same time. so no big deal.

I'll wrap this up.

https://github.com/drupalcommerce/commerce/pull/775 is the re-roll.

tested the patch, works fine.
code looks good too.

As soon Display Name field is not required, we must have a fallback to the "Discount" string, otherwise, right after applying the patch and before re-saving current promotions - a site is down.

Everything is good. My fault.

There are cases when multiple coupons from the same promotion could be applied. For such cases, I added the checkbox "Display Entered Coupon" to the Promotion if it's checked - coupon code would be used as a label, Display name otherwise.

Patch attached.

Using patch in #18, I get an error when implementing a promotion with no coupon (Fixed amount off the order total):

TypeError: Argument 2 passed to Drupal\commerce_promotion\Entity\Promotion::apply() must implement interface Drupal\commerce_promotion\Entity\CouponInterface, none given, called in /app/web/modules/contrib/commerce/modules/promotion/src/PromotionOrderProcessor.php on line 54 in Drupal\commerce_promotion\Entity\Promotion->apply() (line 517 of /app/web/modules/contrib/commerce/modules/promotion/src/Entity/Promotion.php) #0 /app/web/modules/contrib/commerce/modules/promotion/src/PromotionOrderProcessor.php(54): Drupal\commerce_promotion\Entity\Promotion->apply(Object(Drupal\commerce_order\Entity\Order)) #1 /app/web/modules/contrib/commerce/modules/order/src/OrderRefresh.php(155): Drupal\commerce_promotion\PromotionOrderProcessor->process(Object(Drupal\commerce_order\Entity\Order)) #2 /app/web/modules/contrib/commerce/modules/order/src/OrderStorage.php(92):

@francois, thank for the report, just yesterday faced with the same problem.
Here is the new patch which fixes it.

This patch applies and works great. Screenshots of results.

Cart summary

Cart

We have been using the patch in #20 without problems. I would like to see this commited!

Patch in #20 does not apply to 8.x-2.4.0.

Working on the reroll.

Rerolled #20.

patch from #25 breaks functionality

Fixed patch from #25.

It was missing a use statement in modules/promotion/src/Plugin/Commerce/PromotionOffer/PromotionOfferInterface.php

Not sure why the php 7.2 failures. I've requeued testing. I'm also linking in #2939636: Add Discount (promotion) as line item, since this is important. I've actually seen this issue tagged in several places as a blocker, so I'm going to bump priority.

And actually, I think I can RTBC this thing. If the php 7.2 comes back as a failure, obviously we'll have to do more research.

I don't like the interface changes since it'll break sites that have a module implementing the old PromotionOfferInterface (as in #2973367: Incompatible with commerce_promotion). All of those custom or contrib promotions would also need to do this little coupon/show coupon code dance in order to honour the configuration:

+    if (!$coupon) {
+      $label = $promotion->getDisplayName();
+    }
+    else {
+      $label = $promotion->getDisplayCouponName()
+        ? $this->t('Coupon @code', ['@code' => $coupon->getCode()])
+        : $promotion->getDisplayName();
+    }

Perhaps PromotionOfferBase should be doing the labeling so offers extending it don't need to copy code?

Patch doesn't apply for Commerce 2.8.

Without looking at it, I'm guessing it's because of backwards compatibility breaks - https://www.drupal.org/node/2982334

I might get to this today, but if anyone else is quicker then, by all means, have at it!

EDIT: Reroll is beyond my abilities. Someone else will have to clear this up.

Here a reroll and working fine with commerce 2.8

NOTE: Pending implement tests.

The coupon code stuff is out of scope for this issue. I am generally not convinced it should be done at all.
Open a separate feature request, please.

Patch #38 does apply, but when adding a new promotion via the Promotions UI, I get an HTTP 500 error with the following in the logs.

[notice] Update started: commerce_promotion_post_update_9
PHP Fatal error:  Declaration of Drupal\commerce_promotion\Plugin\Commerce\PromotionOffer\OrderPercentageOff::apply(Drupal\Core\Entity\EntityInterface $entity, 
 Drupal\commerce_promotion\Entity\PromotionInterface $promotion) must be compatible with Drupal\commerce_promotion\Plugin\Commerce\PromotionOffer\PromotionOfferInterface::apply(Drupal\Core\Entity\EntityInterface $entity, 
 Drupal\commerce_promotion\Entity\PromotionInterface $promotion, ?Drupal\commerce_promotion\Entity\CouponInterface $coupon = NULL) in /.../web/modules/contrib/commerce/modules/promotion/src/Plugin/Commerce/PromotionOffer/OrderPercentageOff.php on line 21

Yes Sorry here the right fix.

Updated patch. This just removes from the previous patch a duplicate use class that was being called in OrderPercentageOff.php.

Updated patch. One more correction. There was some space before the opening PHP tag in BuyXGetY.php that was causing some havoc.

@Hubbs, I keep getting this error after using patch #44:

InvalidArgumentException: Missing required property label. in Drupal\commerce_order\Adjustment->__construct() (line 73 of modules/contrib/commerce/modules/order/src/Adjustment.php).

When removing the patch, the error disappears.

Hi, I've updated the patch for the latest -dev version.

The coupon should not have a different label. It should show the promotion which redeemed it. The coupon redemption form tells the user their applied coupon.

I disagree with this. I see no reason why this couldn't be there as an option. Seeing the actual coupon code as the adjustment label is how I expect this to behave and how my clients are used to it behaving. I don't intend on using the coupon redemption form for viewing/removing coupons, I'm going to change it so it's just a remove-link which removes the applied coupon directly so In my scenario the adjustment label is the only place where you would see the applied coupon code.

As for the patch, the coupon was never passed to order level offers so they were never able to use the coupon label as the adjustment label. I've updated the patch to fix this.

the patch has problems with the new version of commerce module: 8.x-2.13

patching file modules/promotion/src/Entity/Promotion.php
Hunk #1 succeeded at 115 (offset 31 lines).
Hunk #2 FAILED at 506.
Hunk #3 succeeded at 652 with fuzz 1 (offset 72 lines).
1 out of 3 hunks FAILED -- saving rejects to file modules/promotion/src/Entity/Promotion.php.rej

+1 @kimlop

This should apply cleanly to 2.13 and dev.

The reason for the last one failing in 2.13 were these two issues which didn't really affect the patch in any way, they just caused a few lines of code to be shuffled around:
https://www.drupal.org/node/3045308
https://www.drupal.org/node/3042257

There's an @todo for this issue that can be removed.

         // @todo Change to label from UI when added in #2770731.
-        'label' => t('Discount'),
+        'label' => $label,

Updated the issue summary to add two things from the maintainers that are getting buried by other comments.

#39

The coupon code stuff is out of scope for this issue. I am generally not convinced it should be done at all.
Open a separate feature request, please.

#46

The coupon should not have a different label. It should show the promotion which redeemed it. The coupon redemption form tells the user their applied coupon.

Also. No tests.

Keep in mind that the current patch is incompatible with Shipping beta8: #3110065: ShipmentPromotionOfferBase::apply has wrong signature.

That is because my #39 was never addressed. Modifying the apply() signature is a BC break, and out of scope for this issue.

Taking this over.

Fixed #2986802: Promotions can't be translated as a blocker.

First stab. Still want to expand the offer tests to ensure the right labels are used.

The team discussed making the field required, but that would be a BC break and would require updating a thousand tests.
Second option was required + default value such as "Discount", but I am not sure people will want to continue using such a generic label. Our competitors tend to either use the promotion name or to have a separate display name field.

Attaching screenshot. Suggestions welcome on how to tweak the field descriptions.

@bojanz
Since you asked for feedback on Slack, here's my personal opinion:

• Make the display name optional.
• When the display name is not provided, fallback to 'Discount' as user interface translated string (thus through t()).

I think this is the least disruptive for existing sites. One of my clients for example, uses the admin label for categorizing discounts, for example "coupon_10%_foo_bar", indicating a coupon discount of 10% for products (or product groups) 'foo' and 'bar'. So a fallback to the admin label isn't preferable for that client.
The alternative, updating all existing promotions with display label 'Discount' (via a database update), would probably be disruptive on multilingual sites, since then that label becomes an user defined translatable instead of user interface translatable: a different translation group. Multilingual sites will then have to retranslate that label in all languages that they support, or update all promotions with a language neutral label.

@MegaChriz
Assuming that we keep the fallback to "Discount", what should the description for the Display name field be? How do we explain that?

@bojanz
I think the following description for the promotion display name would be a good one:

t('The customer-facing name, shown at checkout. When leaving this empty, %default_display_name will be displayed instead.', [
  '%default_display_name' => t('Discount'),
])

This patch addresses the feedback and keeps the fallback to t('Discount'). Tests have been updated. Attaching new screenshot.

+++ b/modules/promotion/src/Entity/Promotion.php
@@ -590,6 +605,22 @@ class Promotion extends CommerceContentEntityBase implements PromotionInterface
+      ->setDescription(t('If provided, shown on the order instead of "Discount".'))

This might be reeaallll esoteric but the embedded translation could actually be like:

t('If provided, shown on the order instead of "@translated".', ['@translated' => t('Discount')])

?

Okay, so we have the same proposal in #58 and #60. The reason why I didn't move "Discount" to a placeholder is because I felt that the string is less clear in this version, especially considering that the gain of reusing the translation for 1 word is slim. However, I will happily change my mind if more people agree. Holding off the commit of this patch until this is clearer.

@bojanz
Regarding moving "Discount" to a placeholder, the advantages are:

• The actual label that is used as fallback is seen.
• Translators can make no mistake in translating the fallback label in this string differently.

The disadvantages are:

• Users viewing the admin interface in a language other than English, might see a string in mixed languages (assuming that for the new description, it is a lot less likely it has a translation).
• If an user views the admin interface in a language other than English, but not the common site language, the user might have to think for a second to realize the fallback label is not in his/her language when the site is viewed in an other language. This is especially a disadvantage when the translation of 'Discount' in their language is ambiguous, though if this is the case, I assume that the ambiguity will also occur as soon as the description string has a translation.
• It might take translators some extra effort to understand in which context the description string is used.

I would be in favor for moving "Discount" to a placeholder, mainly because then there is no way for translators to make an error in translating "Discount" in the description differently.

I agree with using a placeholder for "Discount". If the purpose is to show the word as the customer will see it a translator doesn't need it to appear in the context of the rest of the description. That it's quoted in the sentence is a sign that it's separate: one would want to translate it by itself.

But the color of this bikeshed is fine. That could be changed and committed or just committed as-is and someone can post a patch in a new issue if they don't like it.

But the color of this bikeshed is fine. That could be changed and committed or just committed as-is and someone can post a patch in a new issue if they don't like it.

Yes.

Thanks for the patch, it's working really well for me.

Looks like we have consensus. Applied the suggestion from #60 and committed. Thanks, everyone!