I'm doing this on a client site right now and thought I'd get a second opinion on the feature for inclusion in Commerce Discount itself.

Basically, I'm adding a "Discount priority" select list (options 1 - 21) to commerce_discount_form. I'll subtract 11 and save the value as the discount Rule's weight. The help text will just indicate that lower numbers indicate higher priority.

I can whip a patch up for this if it tests well. : P

CommentFileSizeAuthor
#10 2550267-10.discount_sort_order.patch2.93 KBrszrama
#3 2550267.discount_sort_order.patch2.5 KBrszrama
#2 sort-order-select-list.png11.01 KBrszrama

Comments

rszrama created an issue. See original summary.

rszrama’s picture

rszrama commented
Title: Add a discount priority form element that builds to the discount Rule's weight » Add a sort order form element that builds to the discount Rule's weight
StatusFileSize
new sort-order-select-list.png11.01 KB

I settled on "Sort order" with a helpful description:

rszrama’s picture

rszrama commented
Status: Active » Needs review
StatusFileSize
new 2550267.discount_sort_order.patch2.5 KB

Ok, realized it was going to be much easier to just write this as a patch against Commerce Discount, because I needed to store the sort order on the discount object in order to build the default rule properly. Attached for review.

Status: Needs review » Needs work

The last submitted patch, 3: 2550267.discount_sort_order.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Does that relate to this at all? https://www.drupal.org/project/commerce_discount_weight

rszrama’s picture

rszrama commented

Nice. It does, this approach is just much simpler. : )

Tests failed because tests are failing, not because of the patch, right?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Tests weren't failing recently, I was quite sure I got them passing... but I've said that before and testbot had managed to prove me wrong(yet Travis and local still have no problem with them)

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@rszrama test likely failed because you've added a field but the import/export test doesn't have that in it.

rszrama’s picture

rszrama commented

Ahh, ok, I'll take a look. : )

rszrama’s picture

rszrama commented
Status: Needs work » Needs review
StatusFileSize
new 2550267-10.discount_sort_order.patch2.93 KB

Patch attached with the test fix. If it passes, I'll commit, as I have another patch incoming that will depend on this one. Slap my hand if that's bad. O: )

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Hold up, will that other module do what you need? Maybe we should consider merging them in instead of this patch?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I've got a request in with @Goz here #2542362: Merge with commerce_discount

rszrama’s picture

rszrama commented

I imagine it might, but it's rather unnecessary. Why add yet another field to the discount form to manipulate a property of the rule like this? In my opinion, the approach here is superior.

rszrama’s picture

rszrama commented

Additionally, I wouldn't use a textfield to set rule weights like that module does. This really deserves a numeric field and widget. I don't even see value validation in that module, nor do I see an awareness that the Rules UI, which may be used to edit a compiled rule after the fact, only supports values from -10 to 10.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Needs work

I'll have to take your word for it until I see review the diff, but they are trying to solve the same problem so likely if this solution is better then I'd expect it will conflict with that solution and make it no longer work regardless. Maybe you can talk to @Goz because he works in the same company as you?

  1. +++ b/commerce_discount.install
@@ -73,6 +73,13 @@ function commerce_discount_schema() {
+        'default' => 10,

    Why start at 10? 

  2. +++ b/commerce_discount.install
@@ -73,6 +73,13 @@ function commerce_discount_schema() {
+        'size' => 'tiny',

    From -128 to 127. Don't some some people have more than 255 discounts? 

  3. +++ b/commerce_discount.rules_defaults.inc
@@ -34,6 +34,7 @@ function commerce_discount_default_rules_configuration() {
+      $rule->weight = !empty($discount->sort_order) ? $discount->sort_order - 11 : -1;

    I'm guessing this -11 is because of the default 10? Or homage to spinal tap? 

  4. +++ b/includes/commerce_discount.admin.inc
@@ -189,6 +189,15 @@ function commerce_discount_form($form, &$form_state, CommerceDiscount $commerce_
+    '#options' => drupal_map_assoc(range(1, 21)),

    Most of these options in drupal are -50 to 50 it seems, but they seem to hit problems of scale if things go bigger. AKA this issue: #1007746: Reordering fails with more than 100 items in a menu Should we adjust?

rszrama’s picture

rszrama commented
Status: Needs work » Needs review

Sure, I can handle internal communication, and I'm not worried about deprecating an in-development module that has minimal usage. ; )

As I mentioned above, the Rules edit form only supports weights of -10 to 10, so if we support any wider range, it will be worthless once you go to customize the rule. We start at 10 because it's roughly in the middle of the stack - for usability, instead of showing a merchant -10 to 10 on the discount form, we should them 1 to 21. I could've gone 11 so it would build to a weight of 0 for the rule, but 10 just looked nicer (and it has the side effect of ensuring Commerce Discount related pricing rules are evaluated before any others that were created with a default weight of 0 via the pricing rules UI).

I think that covers all of points 1-4.

goz’s picture

goz commented

Nice i'll test this patch and this differences with commerce_discount_weight.

Rules only supports 20 weights, but it's a very tiny range. Some clients use hundred, may be thousands of discount. So it can be very hard to order discounts with a 20 range.

In this case, the view in commerce_discount_weight is no more usable (it's better to have an internal ordering political than using drag&drop), so i'm not sure it's a good thing to implement it in commerce_discount.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I worry because I've been punting similar issues to that module.

Dial it to 11! 0 to 20 seems nicer than blackjack. Could it default to 0(aka rules -10)? Sorry for the silly questions I don't have a picture in my head of how the rules UI works thanks for clarification on the reasoning behind 1-21 = -10 to 10.

If I understand correctly you're mapping 11 to 0 so that in the discount UI it's nicer than the rule's UI?

rszrama’s picture

rszrama commented

Yeah, in my mind, you really don't need a priority level for every single discount. It's not likely that 20+ discounts are going to apply to the same order - if so, you might want to dial the merchandising back a little on the site. : P

It's more an issue of when discounts do collide, how do we order them relative to one another. Making use of the -10 to 10 Rules weight does that for us, and it's at least as performant as core Commerce's pricing rules capabilities.

Re: 1 to 21 vs. 0 to 20, we could've gone with that and then just kept the default at 10. I just don't think non-developers are used to counting from 0 vs. counting from 1. It's all about trying to think like a n00b and programming the discount UI to meet their needs, as they're the ones we expect to use this UI anyways.

I didn't default it to the lowest weight out of the box because that's often where we do other things like convert currency or swap in a new base price (cf. Commerce Price Table). We can't prevent someone from doing that, but may as well start 'em in the middle so they know they can sort things up or down.

At the end of the day, the best UI would be drag and drop on the discounts table itself. But so long as we're tied to Rules for execution ordering and still allowing the underlying rules to be overridden, I don't think we have much recourse.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

@rszrama you're UI points in #19 are all well taken. Drag and Drop would be ideal and maybe we can push for that instead of a half measure?

Maybe pie in the sky(mmmm floating pie): Can the rule's weight's be floats so we can use a default weight for all discounts and sort them within that through a the drag and drop. So '10.000001', '10.000002', etc?

Also a better UI when you have more than 20 rows is something like https://www.drupal.org/project/tabledragextra Which can move things to the top instead of tired and frail scroll & drag fingers.

rszrama’s picture

rszrama commented

I don't think it's worth pursuing in this module, to be honest. We could use whatever value we wanted in our column, but if it's not an integer or it exceeds the bounds of -10 to 10, as soon as the Rule is edited in the Rules UI, our data will mismatch.

What probably would be worth pursuing if it isn't automatic is the addition of a Views field definition to at least display / sort a View by the sort order column. I'm happy to spawn a follow-up for that if need be.

The only reason I was pushing to get just this functionality in is because #2557119: Allow discounts to indicate compatibility with other discounts will also require an update function. Wanted to dependably make it use 7006, but we can always renumber later. I didn't consider this feature too important to not use incremental improvement post commit, fwiw. ; )

rszrama’s picture

rszrama commented
Status: Needs review » Fixed

So, I've since discovered that this module uses some magic capability of Entity API to define its Views definitions. I tested out the sort order field and it works without any additional effort. Nice!

I'm going to go ahead and commit this sucker, and we can improve it even further moving forward. I'm pretty convinced we should stick within the paradigm of the Rules weight system for now, but if a solution should ever arise to expand that module's weight capability, we can revisit this here.

  • rszrama committed be5b338 on 7.x-1.x 
    Issue #2550267 by rszrama: add a sort_order column to the Discount table...

Status: Fixed » Closed (fixed)

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