Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We need a way to control how the promotion will show up in the order's subtotal. This should be another text field which allows store managers to control the promotion's display.
To do
- Add tests
- Remove coupon code label option. (#39)
Comment | File | Size | Author |
---|---|---|---|
#59 | promotion-display-name-59.png | 40.92 KB | bojanz |
#59 | 2770731-59-promotion-display-name.patch | 22.85 KB | bojanz |
| |||
#55 | promotion-display-name.png | 124.62 KB | bojanz |
#55 | 2770731-55-promotion-display-name.patch | 11.91 KB | bojanz |
| |||
#51 | adjustment-label-update-2770731-51.patch | 18.46 KB | TwiiK |
Comments
Comment #2
mglamanThis is currently blocked by some items in #2763705: Implement a usage API which allow the offer plugin to know the promotion its for.
Comment #3
mglamanComment #4
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedAdds 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.
Comment #5
mglamanPromotion entities won't be rendered, and this setting doesn't affect views, so let's just remove it (I don't think we set it for any of the others.)
:gasp: oh no! ;)
Let's keep it nonrequired, saying it'll otherwise default to the promotion name. On ::preSave we can populate it if empty from the name, yeah?
Comment #6
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedUpdated w/ changes.
https://github.com/drupalcommerce/commerce/pull/706
Comment #7
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commentedRTBC +1
Comment #8
mglamanThere's no test coverage in the PR.
Comment #9
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedFlipped back to RTBC as there is actually test coverage, talked to mglaman and it was just a mistake.
Comment #10
bojanz CreditAttribution: bojanz at Centarro commentedComment #11
heddnLooks 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.
Nit: there's still a few more more mentions of this @TODO in OrderFixedAmountOff, OrderPercentageOff, OrderItemPercentageOff. But that could be fixed on commit.
Comment #12
heddnActually, 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.
Comment #13
bojanz CreditAttribution: bojanz at Centarro commentedI'll wrap this up.
Comment #14
heddnhttps://github.com/drupalcommerce/commerce/pull/775 is the re-roll.
Comment #15
finnetested the patch, works fine.
code looks good too.
Comment #16
kala4ekAs 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.
Comment #17
kala4ekEverything is good. My fault.
Comment #18
kala4ekThere 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.
Comment #19
francois o CreditAttribution: francois o commentedUsing 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):
Comment #20
kala4ek@francois, thank for the report, just yesterday faced with the same problem.
Here is the new patch which fixes it.
Comment #21
Hubbs CreditAttribution: Hubbs at Acro Commerce commentedThis patch applies and works great. Screenshots of results.
Cart summary
Cart
Comment #22
s.messaris CreditAttribution: s.messaris commentedWe have been using the patch in #20 without problems. I would like to see this commited!
Comment #23
s.messaris CreditAttribution: s.messaris commentedPatch in #20 does not apply to 8.x-2.4.0.
Comment #24
s.messaris CreditAttribution: s.messaris commentedWorking on the reroll.
Comment #25
s.messaris CreditAttribution: s.messaris commentedRerolled #20.
Comment #27
istavros CreditAttribution: istavros commentedpatch from #25 breaks functionality
Comment #28
istavros CreditAttribution: istavros commentedFixed patch from #25.
It was missing a use statement in modules/promotion/src/Plugin/Commerce/PromotionOffer/PromotionOfferInterface.php
Comment #29
edurenye CreditAttribution: edurenye as a volunteer commentedComment #32
heddnNot 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.
Comment #33
heddnAnd 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.
Comment #34
mglamanI see zero test coverage.
Comment #35
mglamanComment #36
travis-bradbury CreditAttribution: travis-bradbury at Acro Commerce commentedI 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:
Perhaps PromotionOfferBase should be doing the labeling so offers extending it don't need to copy code?
Comment #37
Hubbs CreditAttribution: Hubbs at Acro Commerce commentedPatch 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.
Comment #38
fastangel CreditAttribution: fastangel at Adyax commentedHere a reroll and working fine with commerce 2.8
NOTE: Pending implement tests.
Comment #39
bojanz CreditAttribution: bojanz at Centarro commentedThe 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.
Comment #40
s.messaris CreditAttribution: s.messaris commentedComment #41
Hubbs CreditAttribution: Hubbs at Acro Commerce commentedPatch #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.
Comment #42
fastangel CreditAttribution: fastangel as a volunteer and at Adyax commentedYes Sorry here the right fix.
Comment #43
Hubbs CreditAttribution: Hubbs at Acro Commerce commentedUpdated patch. This just removes from the previous patch a duplicate use class that was being called in OrderPercentageOff.php.
Comment #44
Hubbs CreditAttribution: Hubbs at Acro Commerce commentedUpdated patch. One more correction. There was some space before the opening PHP tag in BuyXGetY.php that was causing some havoc.
Comment #45
fotograafinge CreditAttribution: fotograafinge commented@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.
Comment #46
mglamanThe 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.
Comment #47
FiNeX CreditAttribution: FiNeX as a volunteer commentedHi, I've updated the patch for the latest -dev version.
Comment #48
TwiiK CreditAttribution: TwiiK commentedI 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.
Comment #49
kimlop CreditAttribution: kimlop commentedthe patch has problems with the new version of commerce module: 8.x-2.13
Comment #50
rjdjohnston CreditAttribution: rjdjohnston commented+1 @kimlop
Comment #51
TwiiK CreditAttribution: TwiiK commentedThis 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
Comment #52
travis-bradbury CreditAttribution: travis-bradbury at Acro Commerce commentedThere's an @todo for this issue that can be removed.
Updated the issue summary to add two things from the maintainers that are getting buried by other comments.
#39
#46
Comment #53
bojanz CreditAttribution: bojanz at Centarro commentedKeep 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.
Comment #54
bojanz CreditAttribution: bojanz at Centarro commentedTaking this over.
Fixed #2986802: Promotions can't be translated as a blocker.
Comment #55
bojanz CreditAttribution: bojanz at Centarro commentedFirst 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.
Comment #56
MegaChriz CreditAttribution: MegaChriz as a volunteer commented@bojanz
Since you asked for feedback on Slack, here's my personal opinion:
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.
Comment #57
bojanz CreditAttribution: bojanz at Centarro commented@MegaChriz
Assuming that we keep the fallback to "Discount", what should the description for the Display name field be? How do we explain that?
Comment #58
MegaChriz CreditAttribution: MegaChriz as a volunteer commented@bojanz
I think the following description for the promotion display name would be a good one:
Comment #59
bojanz CreditAttribution: bojanz at Centarro commentedThis patch addresses the feedback and keeps the fallback to t('Discount'). Tests have been updated. Attaching new screenshot.
Comment #60
bradjones1This might be reeaallll esoteric but the embedded translation could actually be like:
?
Comment #61
bojanz CreditAttribution: bojanz at Centarro commentedOkay, 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.
Comment #62
MegaChriz CreditAttribution: MegaChriz as a volunteer commented@bojanz
Regarding moving
"Discount"
to a placeholder, the advantages are:The disadvantages are:
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.Comment #63
travis-bradbury CreditAttribution: travis-bradbury at Acro Commerce commentedI 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.
Comment #64
bradjones1Yes.
Comment #65
joekersThanks for the patch, it's working really well for me.
Comment #67
bojanz CreditAttribution: bojanz at Centarro commentedLooks like we have consensus. Applied the suggestion from #60 and committed. Thanks, everyone!