The attached patch introduces five changes to the original commerce_price_table module:

  1. The field used in the price calculation rule is now selected as a parameter of type 'list' using 'commerce-line-item:commerce-product:field-price-table' as default value. This change adds the ability to combine the price calculation with additional conditions. For example, you can add a second price table that will be used only if the user belongs to a special role, like wholesalers.
  2. The field 'max_qty' is removed. Instead, 'min_qty - 1' of the next higher item is used as the maximum value.
  3. Changed table formatting from 'X - Unlimited' into '≥ X'
  4. The validate function now ensures that there is at least one entry with min_qty = 0 and that each min_qty is unique.
  5. I moved min_qty above the amount field, because that feels better for me.

Important: This patch does NOT include an upgrade path, since I started a project from scratch and didn't need it. If this patch is merged, someone has to write an update that alters the field definition.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cybso’s picture

Issue summary: View changes

List formatting

pcambra’s picture

Status: Needs review » Needs work

Important: This patch does NOT include an upgrade path, since I started a project from scratch and didn't need it. If this patch is merged, someone has to write an update that alters the field definition.

Setting to needs work then.

Cybso’s picture

Status: Needs work » Needs review
FileSize
16.61 KB

Cleaned up the code and added an schema update. The update also checks whether the price table quantities are strictly monotone increasing or prints a warning with a link to the product.

Cybso’s picture

The above patch only contains the changes 2 to 5. The first change ("The field used in the price calculation rule is now selected as a parameter of type 'list' using 'commerce-line-item:commerce-product:field-price-table' as default value. This change adds the ability to combine the price calculation with additional conditions. For example, you can add a second price table that will be used only if the user belongs to a special role, like wholesalers.") is now separated from the others, as I think this is a very important one.

pcambra’s picture

Title: Patch: Droped max_qty and make price table more flexible » Expose price table to rules
Status: Needs review » Needs work

I don't really see the need to remove max quantity, you're then assuming that the last interval is min to unlimited, don't you?

Let's focus in the patch in #3 and open a followup to discuss other unrelated stuff.

Patch looks fine, but I'd say to keep the code removed in commerce_price_table_get_amount_qty in case $items is empty.

Cybso’s picture

You're right, I'm assuming that the last interval is min to unlimited. The reason I removed it is because with max_qty you can have gaps and overlapping intervals, and the resulting price would be unpredictable (at least, without looking in the code). Of course, you can do additional checks in the validator hook to prohibit this, but when you look into the function _commerce_price_table_validate_min_max_ranges() from the patch you see that this is somewhat complex.

Additionally, look what happens in the original commerce_price_table_get_amount_qty() if the unlimited entry is omitted, or if there is a gap between the intervals: it just returns the last quantity item. This means, you get a pricing table telling you that you get price A from 1 to 10, price B from 20 to 29, price C from 30 to 100, but you'll also get price C at 15 or 1000. Doesn't sound very logic to me. So, the easiest thing is just to remove max_qty, as it isn't really required for the desired functionality of this module.

As for the empty item table, I think you might right, although this might lead to unexpected results if there are no price table items defined for a product. I've updated the attached patch.

pcambra’s picture

Thanks for the patch updated.

This means, you get a pricing table telling you that you get price A from 1 to 10, price B from 20 to 29, price C from 30 to 100, but you'll also get price C at 15 or 1000. Doesn't sound very logic to me. So, the easiest thing is just to remove max_qty, as it isn't really required for the desired functionality of this module.

That was the intention from the beginning, the missing gaps should fallback to the closest lower level of pricing, I'm open to suggestions on how to fix that, but I don't want to "babysit" this, you might want to have missing intervals for some reason and exposing the thing to rules would allow to handle those more easily.

Cybso’s picture

That was the intention from the beginning, the missing gaps should fallback to the closest lower level of pricing,

But they do not, the existing code just takes the last value from the price table items. Do you have any real-life example where this behavior might be desired?

I think the only other sensible (and expectable) behavior would be to fall back to the products default price if the selected quantity is not within the defined ranges.

pcambra’s picture

Status: Needs work » Needs review

I think the only other sensible (and expectable) behavior would be to fall back to the products default price if the selected quantity is not within the defined ranges.

I'd be ok with that :)

Changing status as for #5

Cybso’s picture

I'll rework this, but since the calculated prices might differ I'll leave the warning from update 7100, even if not database update would be required. I'll also add an extra check to the validate hook that prints an error on overlapping and a warning on gapping intervals.

For the first step, I'll hardcode commerce_price, since it's also hardcoded for hide_default_price. It might make sense to have this field name configurable in future versions. Oh, I'll create a separate issue for this.

pcambra’s picture

Oh, I'll create a separate issue for this.

Nice, thanks! :)

pcambra’s picture

Status: Needs review » Needs work
+++ commerce_price_table/commerce_price_table.module	2013-01-24 17:27:00.444202677 +0100
@@ -453,14 +453,16 @@
+    // Fall back to commerce_price_table-1.1 behaviour

Comment is not clear here, what is 1.1?

Also, provide patch from root, thanks.

Cybso’s picture

Status: Needs work » Needs review
FileSize
3.67 KB

Comment changed ("Fall back to commerce_price_table version 7.x-1.1 behaviour").

pcambra’s picture

Status: Needs review » Needs work

Please describe the behavior, not just reference to a version.

Cybso’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
    // Fall back to commerce_price_table version 1.1 behaviour                                                                                        
    // and look up all price table items in the current product.
pcambra’s picture

Status: Needs review » Fixed

Tweaked a little the comment and committed, thanks!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Added a fifth change

Summit’s picture

Issue summary: View changes

Hi,
Related to: "For example, you can add a second price table that will be used only if the user belongs to a special role, like wholesalers."
Can you also add a second price table for a possible addition to the product. Like a service as printing...which price is based on the quantity of products?
So someone can select Yes/No for the service printing, and if Yes is selected the price of the service is such as stated in the second price table.
(And may be a third price table for another service?

Thanks a lot for your reply in advance. And sorry if remark is not valid for this issue in your perspective.
Greetings, Martijn