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.
Continuation of http://drupal.org/node/1875688#comment-6978822:
Fall back to default price if the selected quantity is not within price table ranges, and print an error on overlapping intervals.
Comments
Comment #1
Cybso CreditAttribution: Cybso commentedComment #2
Cybso CreditAttribution: Cybso commentedHere's the patch:
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.
Comment #3
Cybso CreditAttribution: Cybso commentedAdded 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?
Comment #4
Cybso CreditAttribution: Cybso commentedA small change in commerce_price_table_field_validate() for better performance:
Only check for require_complete_intervals if the intervals are incomplete.
Comment #5
pcambraThis is the sort of thing that I meant with "babysit" the price fallback.
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.
Comment #6
Cybso CreditAttribution: Cybso commentedI'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.
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.
Comment #7
Cybso CreditAttribution: Cybso commentedNothing changed but diff headers.
Comment #8
pcambraYou 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.
Comment #9
Cybso CreditAttribution: Cybso commentedI'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)?
Comment #10
Cybso CreditAttribution: Cybso commentedThe 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?
Comment #11
pcambraI 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...
Comment #12
Cybso CreditAttribution: Cybso commentedDo 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).
Comment #13
pcambraOoops crossed messages.
Patch is getting better, I don't think there's any need for the update, let's remove that.
extra space
Uppercase: FALSE & TRUE
You can use entity_load_single here
This message is difficult to understand, some alternatives are suggested in the comments above, and we need a t()
Separate comments here, one for min qty and other for max qty
missing dots
What happens with this TODO?
Missing t()
This could be just one setting
Unit test on this would be wonderful
Comment #14
Cybso CreditAttribution: Cybso commentedIt 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.
Comment #15
Cybso CreditAttribution: Cybso commentedAnnotations 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?
Comment #16
pcambraThey are grouped in Drupal commerce group which is probably the wrong place :)