Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cybso’s picture

Assigned: Unassigned » Cybso
Cybso’s picture

Status: Active » Needs review
FileSize
8.97 KB

Here's the patch:

  • Use an empty database update to check for incomplete price range, because the calculations result might differ.
  • On table sorting, also involve max_qty if a['min_qty'] == b['min_qty'] (required for the following change).
  • Fall back to default price, if defined and not empty (preferred), otherwise fall back to nearest price table item (in combination with hide_default_price).
  • Advanced validating of price table intervals:
    • min_qty may not be less than zero.
    • max_qty must be greater or equal to min_qty (or -1).
    • There may be only one infinite item.
    • There may be no overlapping intervals.

The code also checks for gaps, but using drupal_set_message('...', 'warning') would break the Ajax request of IEF, so this is silently ignored for the moment.

Cybso’s picture

Added a configuration parameter require_complete_intervals that disallows gaps or a missing infinite item. @pcambra: I think this solution will satisfy the needs of both of us, doesn't it?

Cybso’s picture

A small change in commerce_price_table_field_validate() for better performance:

Only check for require_complete_intervals if the intervals are incomplete.

pcambra’s picture

Category: bug » task
Status: Needs review » Needs work

This is the sort of thing that I meant with "babysit" the price fallback.

On table sorting, also involve max_qty if a['min_qty'] == b['min_qty'] (required for the following change).
Fall back to default price, if defined and not empty (preferred), otherwise fall back to nearest price table item (in combination with hide_default_price).
Advanced validating of price table intervals:
min_qty may not be less than zero.
max_qty must be greater or equal to min_qty (or -1).
There may be only one infinite item.
There may be no overlapping intervals.

I don't really get why you provide an update, max qty should not be removed.

I think that if there are missing or even overlapping gaps, it's totally ok, we should not validate things like "you've missed qty 13" or "qty 22 falls into two different intervals", the module should be flexible enough to support gaps and people can work their path with rules for this.

Same as the other patches, please provide them from module root.

Cybso’s picture

I think that if there are missing or even overlapping gaps, it's totally ok, we should not validate things like "you've missed qty 13" or "qty 22 falls into two different intervals", the module should be flexible enough to support gaps and people can work their path with rules for this.

I'm not doing this stuff for me, I made these modifications for a company. The people that will maintain prices need this kind of assistance, or my telephone would ring weekly with people asking my why a customer ordered something with a wrong price. In my opinion, at least the check for an overlapping area is an absolute "must be", because the user would not have any change to recognize which price would be used without reading and understanding the module's code.

I don't really get why you provide an update, max qty should not be removed.

max_qty is not removed. But with the new gap fallback to the default commerce price (instead of a random end-of-list-item) the resulting price will change if a user has gaps or a missing infinite item, and he should have a chance to notice that.

Please tell me how a rule would help with that. As far as I see, no rule will be able to analyze the price table ranges for gaps or overlapping intervals.

I'm totally OK with maintaining a private fork for this customer if you don't want to apply this change, but for the sake of compatibility - and since I think other users will benefit from a little more "babysitting" - I hope this code will make it into the original module.

Cybso’s picture

Status: Needs work » Needs review
FileSize
10.74 KB

Nothing changed but diff headers.

pcambra’s picture

Version: 7.x-1.1 » 7.x-1.x-dev
Status: Needs review » Needs work

You don't need to fork anything as far as I'm concerned, you can implement your own custom validation functions, if you think it's not enough, I'm ok with adding an additional hook for more granular validation, but drupal built in hooks should be sufficient here... and definitely no need to mess with prices already existent.

My call would be to handle this with warnings to the user in case he's missed some interval or if the intervals overlap... "This might cause unexpected pricing behaviors if not handled accordingly" could be a good generic message.

Cybso’s picture

I've tried this out, but when I used drupal_set_message("...", "warning") within the validator it breaked IEF's Ajax request when saving the product, so I didn't investigate this further.

Edit Tested it again and it doesn't break the ajax request. Seems that the reason was another error. I'll change the error on overlapping intervals into a warning. What about the optional error on gapping intervals? Since the user can configure this check within the field's settings it doesn't change any existing installation. And the "Only one infinite max quantity is allowed." check (a special case of overlapping intervals)?

Cybso’s picture

Status: Needs work » Needs review
FileSize
12.54 KB

The checks for overlapping and incomplete intervals are both optional, now. They can be enabled through the field settings (disabled by default). If enabled, errors will be returned, otherwise only warnings will be printed.

If you don't want the update checks, just apply the patch and revert all changes to commerce_price_table.install.

Is this agreeable to you?

pcambra’s picture

Status: Needs review » Needs work

I think relaxed warning messages should be the sensible default, maybe it makes sense to have a "strict mode" checkbox in the field instance settings with more strong validation rules...

Cybso’s picture

Do you mean to merge disallow_incomplete_interval and disallow_overlapping_interval into one setting? Beside from this, the -10 patch behaves like your opinion in #11. The only question left is if multiple unlimited items should be allowed or not (currently it isn't).

pcambra’s picture

Ooops crossed messages.

Patch is getting better, I don't think there's any need for the update, let's remove that.

+++ b/commerce_price_table.install
@@ -45,3 +45,93 @@ function commerce_price_table_field_schema($field) {
+ * starting at 0 (1 is allowed, too) and ending at -1. Ranges must be an array of ¶

extra space

+++ b/commerce_price_table.install
@@ -45,3 +45,93 @@ function commerce_price_table_field_schema($field) {
+  $result = true;
...
+          $result = false;
...
+          $result = false;
...
+          $result = false;
...
+          $result = false;
...
+      $result = false;

+++ b/commerce_price_table.module
@@ -190,24 +190,127 @@ function commerce_price_table_field_load($entity_type, $entities, $field, $insta
+  $infinite = false;
+  $incomplete = false;
+  $overlapping = false;
...
+    // min_qty may never be less than zero, and max_qty may never be less then min_qty (exception: -1)
...
+  if ($incomplete !== false || $overlapping !== false || !$infinite) {
...
+    $disallow_incomplete_intervals = false;
+    $disallow_overlapping_intervals = false;
...
+        $disallow_incomplete_intervals = true;
...
+        $disallow_overlapping_intervals = true;
...
+    if ($overlapping !== false) {
...
+    if ($incomplete !== false || !$infinite) {

@@ -466,22 +581,47 @@ function commerce_price_table_get_amount_qty($product, $quantity = 1) {
+      $fallback = true;

Uppercase: FALSE & TRUE

+++ b/commerce_price_table.install
@@ -45,3 +45,93 @@ function commerce_price_table_field_schema($field) {
+    if ($entity = reset(entity_load($entity_type, array($entity_id)))) {

You can use entity_load_single here

+++ b/commerce_price_table.install
@@ -45,3 +45,93 @@ function commerce_price_table_field_schema($field) {
+        drupal_set_message('non monotonically increasing price table quantity range in product '

This message is difficult to understand, some alternatives are suggested in the comments above, and we need a t()

+++ b/commerce_price_table.module
@@ -190,24 +190,127 @@ function commerce_price_table_field_load($entity_type, $entities, $field, $insta
+    // min_qty may never be less than zero, and max_qty may never be less then min_qty (exception: -1)

Separate comments here, one for min qty and other for max qty

+++ b/commerce_price_table.module
@@ -190,24 +190,127 @@ function commerce_price_table_field_load($entity_type, $entities, $field, $insta
+    // Check for overlapping intervals
...
+    // Check for gaps
...
+      // We have an infinite maximum value
...
+    // Get lower bound for next iteration

missing dots

+++ b/commerce_price_table.module
@@ -190,24 +190,127 @@ function commerce_price_table_field_load($entity_type, $entities, $field, $insta
+    // @TODO Always force to have quantity for 1?

What happens with this TODO?

+++ b/commerce_price_table.module
@@ -190,24 +190,127 @@ function commerce_price_table_field_load($entity_type, $entities, $field, $insta
+        drupal_set_message('Incomplete intervals will fall back to the default price.', 'warning');

Missing t()

+++ b/commerce_price_table.module
@@ -263,6 +366,18 @@ function commerce_price_table_field_instance_settings_form($field, $instance) {
+  $form['commerce_price_table']['disallow_incomplete_intervals'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Disallow incomplete intervals'),
+    '#description' => t('Activate this checkbox to disallow gaps between price table intervals.'),
+    '#default_value' => isset($settings['commerce_price_table']['disallow_incomplete_intervals']) ? $settings['commerce_price_table']['disallow_incomplete_intervals'] : NULL,
+  );
+  $form['commerce_price_table']['disallow_overlapping_intervals'] = array(
+    '#type' => 'checkbox',
+    '#title' => t('Disallow overlapping intervals'),
+    '#description' => t('Activate this checkbox to disallow overlapping price table intervals.'),
+    '#default_value' => isset($settings['commerce_price_table']['disallow_overlapping_intervals']) ? $settings['commerce_price_table']['disallow_overlapping_intervals'] : NULL,

This could be just one setting

+++ b/commerce_price_table.module
@@ -490,9 +630,25 @@ function commerce_price_table_get_amount_qty($product, $quantity = 1) {
 function commerce_price_table_sort_by_qty($a, $b) {
   $a_qty = (is_array($a) && isset($a['min_qty'])) ? $a['min_qty'] : 0;
   $b_qty = (is_array($b) && isset($b['min_qty'])) ? $b['min_qty'] : 0;
+
   if ($a_qty == $b_qty) {
-    return 0;
+    // Sort by max_qty
+    $a_qty = (is_array($a) && isset($a['max_qty'])) ? $a['max_qty'] : 0;
+    $b_qty = (is_array($b) && isset($b['max_qty'])) ? $b['max_qty'] : 0;
+    if ($a_qty == $b_qty) {
+        return 0;
+    }
+
+    // Special cases: -1 means infinite
+    if ($a_qty == -1) {
+        return 1;
+    }
+
+    if ($b_qty == 1) {
+        return -1;
+    }

Unit test on this would be wonderful

Cybso’s picture

What happens with this TODO?

It was previously:

@TODO Add extra validations, as no repeating qty and always force to have quantity for 1?.

The check for repeating quantities is just a special case of the check for overlapping intervals.

And since the check for incomplete intervals is a superset of the check for having a min_qty = 1 (or 0) the TODO could be removed completely.

Cybso’s picture

Status: Needs work » Needs review
FileSize
8.66 KB

Annotations 1 to 4 are obsolete, since I removed the update.

Applied the other changes, added the field's label to the warning and fixed an error in commerce_price_table_sort_by_qty().

Unit tests: Do you have any idea why the module doesn't appear in /admin/config/development/testing?

pcambra’s picture

Unit tests: Do you have any idea why the module doesn't appear in /admin/config/development/testing?

They are grouped in Drupal commerce group which is probably the wrong place :)