The default rule shipped with the module assumes a certain field name. But if you name your field differently, it will fail. So, instead of using a default rule, we could just generate the right rule when the field is created and remove it when it is deleted?

For that we can use the module name as machine-name prefix instead of the regular 'rules_' prefix - which is for rules created via the admin UI. That way we can make sure we do not run into any conflicts.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cybso’s picture

You are right, I did not think of this when I've created the patch for #1875688: Expose price table to rules. The old implementation just looped through all price table fields, which lead to the situation that you could not have different price tables for different kind of customers.

The idea to add or remove the rule when the field is added or removed sounds right, since the rule would be useless without a price table field. On the other hand, if the user modified the rule and accidentally removed the field, he would to apply his changes again. But in this case he would also have to enter all price table items again, which would make much more effort.

Do you know how to add a rule configuration at runtime without using hook_default_rules_configuration()? I've found rules_config_delete() and rules_config_load(), but no rules_config_add() or something like that. Only rules_import(), which expects the rule configuration as serialized JSON data.

Cybso’s picture

This would also be a fix of #1921668: Support Multiple Price Tables if my assumption made in comment #7 is right.

Cybso’s picture

Assigned: Unassigned » Cybso

Working on it. Will require to set the field's cardinality to > 1 per default. Not sure how to treat existing rule(s).

Cybso’s picture

Status: Active » Needs review
FileSize
2.85 KB

Implemented hook_field_create_field($field) and hook_field_delete_field($field) to create and delete individual rules called "commerce_price_table_{$fieldname}_override_price" for new price table fields that are not named field_price_table.

pcambra’s picture

Thanks for tackling this :)
Comments above

+++ b/commerce_price_table.module
@@ -570,3 +570,76 @@ function commerce_price_table_display_quantity_headers($item) {
+    // The field cardinality *must* be > 1 for this module to make sense
+    // (and for the rule to work).
+    $field['cardinality'] = FIELD_CARDINALITY_UNLIMITED;
+    field_update_field($field);

I don't see a reason to do this, it might make sense to have price tables with 1 value, why not?
In any case this must be by definition and we should not mess with configurations this way.

+++ b/commerce_price_table.module
@@ -570,3 +570,76 @@ function commerce_price_table_display_quantity_headers($item) {
+    // Ensure that no rule with the desired name exists.
+    if (rules_config_load($rulename)) {
+      return;
+    }

If this might happen, why not ensure an unique rule machine name?

+++ b/commerce_price_table.module
@@ -570,3 +570,76 @@ function commerce_price_table_display_quantity_headers($item) {
+    if ($fieldname == 'field_price_table') {

field type I guess?

+++ b/commerce_price_table.module
@@ -570,3 +570,76 @@ function commerce_price_table_display_quantity_headers($item) {
+        'price_table:select' => 'commerce-line-item:commerce-product:'.str_replace('_', '-', $fieldname),

nitpick strtr is faster than str_replace for this small operations

dreditor missed one thing, "Implements hook_X" comment is missing for create and delete field.

Cybso’s picture

I don't see a reason to do this, it might make sense to have price tables with 1 value, why not?

Because the rule expects a List<> field and won't work otherwise. Maybe it's possible to redefine the action parameters to accept multiple field types, I'll check this.

In any case this must be by definition and we should not mess with configurations this way.

According to http://api.drupal.org/api/drupal/modules!field!field.api.php/function/ho... the default cardinality cannot be defined in hook_field_info. Using hook_field_create_field() is the workaround recommend in the comments. (btw, it's only the default value, the form to select the cardinality is shown *after* hook_field_create_field has been displayed so the user is still free in his choice. He'll just get a default selection more reasonable in most situations).

If this might happen, why not ensure an unique rule machine name?

I think the probability that somebody named an existing rule this way is extremely low. Just ensure that the hook doesn't raise an error. But a loop to append _N in this case would be reasonable, too. (Edit: This would it make nearly impossible to automatically remove the rule in hook_field_delete_field without adding a new table or an additional setting to commerce_price_table which refers to the generated rule).

field type I guess?

No, field name is right. It's for the default rule, which refers to field_price_table by default. I've tried to remove the default rule, but I wasn't able to write an update method that replaces it by a custom rule for existing fields in reasonable time. It's easy when the rule hasn't been overridden, but I got problems to convert a customized rule into a custom rule. So I decided that it is OK for me to leave the original rule in place and just add custom rules if somebody chooses another name than field_price_table ;-)

nitpick strtr is faster than str_replace for this small operations

dreditor missed one thing, "Implements hook_X" comment is missing for create and delete field.

Thx, I'll fix this and provide an updated patch by tomorrow or so.

pcambra’s picture

Status: Needs review » Needs work

According to http://api.drupal.org/api/drupal/modules!field!field.api.php/function/ho... the default cardinality cannot be defined in hook_field_info. Using hook_field_create_field() is the workaround recommend in the comments. (btw, it's only the default value, the form to select the cardinality is shown *after* hook_field_create_field has been displayed so the user is still free in his choice. He'll just get a default selection more reasonable in most situations).

I didn't mean in the info, but that part belongs to the create field itself and not the rule generation in case we want to add it.

No, field name is right. It's for the default rule, which refers to field_price_table by default. I've tried to remove the default rule, but I wasn't able to write an update method that replaces it by a custom rule for existing fields in reasonable time. It's easy when the rule hasn't been overridden, but I got problems to convert a customized rule into a custom rule. So I decided that it is OK for me to leave the original rule in place and just add custom rules if somebody chooses another name than field_price_table ;-)

If a rule has been overridden there's nothing the module can or even should do, again, we don't want to mess with user's configuration that way.
I thought we were dealing with creating default rules for all price tables here, if we're going to change a name dependency for another one, there's no point in this patch imho.

For ensuring an unique rule name, we can refer to fago's original comment:

For that we can use the module name as machine-name prefix instead of the regular 'rules_' prefix - which is for rules created via the admin UI. That way we can make sure we do not run into any conflicts.

Setting to NW.

Cybso’s picture

What do you think about this?

1. Rewrite commerce_price_table_default_rules_configuration() so that it searches for all commerce_price_table fields and returns one rule for each field.

2. In hook_field_create_field and hook_field_delete_field, force drupal to refresh the default rules configuration.

This way there shouldn't be any naming conflicts since rules differs between custom and default rules, there is no need to remove the rule if the field is deleted, and the user can still customize the rule and even revert the customizations.

Cybso’s picture

Same problem applies to ief integration. An equivalent solution as suggested in my previous comment for PriceTableCommerceProductInlineEntityFormController should fix this.

pcambra’s picture

I guess that the best way to accomplish this is to loop the fields in hook_default_rules_configuration and generate a rule per table field.

That would drive us to an interesting situation, if you have several fields attached to a same entity type they will end up with several price replacing rules so we could just generate one with the highest price weight, not sure if makes a lot of sense to generate several rules by default that are going to step on top of each other.

pcambra’s picture

We could create a separate issue to manage the unlimited setting by default to price tables.

rockaholiciam’s picture

Any fix for this? I tried using the dev version because of some issues with the other one but this one defines a buggy rule requiring 'commerce-line-item:commerce-product:field-price-table' when even commerce-product isn't available to the rule, let alone the field, and thus it doesn't work once I have created the field with the same name and tried updating the rule with the default selector that had earlier failed.

If I go back to previous rule definition from stable release, if I add a product wheres its quantity doesn't fall in any defined bracket, it picks up the wrong price durigg checkout instead of picking the base price.

Cybso’s picture

I've reserved some time this weekend to fix this

rockaholiciam’s picture

For the wrong price replacement bug in case price falls outside defined quantity brackets, in the method 'commerce_price_table_get_amount_qty' in 'commerce_price_table.module', I replaced:

// Handle the unlimited qty.
 foreach ($items as $item) {
     if ($item['max_qty'] == -1) {
       return $item;
     }
 }

// We fallback to the higher one if no match was found.
 return end($items);

with

// Handle the unlimited qty.
  foreach ($items as $item) {
    if ($quantity >= $item['min_qty'] && $item['max_qty'] == -1) {
      return $item;
    }
  }

// We fallback to the higher one if no match was found.
  return false;

But this should also take into account if the actual/base price is disabled, which it doesn't for now.

Cybso’s picture

Since this is a totally different issue, can you create a separate ticket for this and add some additional details to it? For example, the output of "debug($items);" of this article?

rockaholiciam’s picture

Opened a separate issue: http://drupal.org/node/1980404

Cybso’s picture

Status: Needs work » Needs review
FileSize
4.6 KB

New patch that generates dynamic default rules configurations for each field.

Cybso’s picture

Change: Clear rules' cache only on field operations of type 'commerce_price_table'

sahaj’s picture

Issue summary: View changes

#18 no more matching last dev module:

patching file commerce_price_table.info
Hunk #1 FAILED at 5.
1 out of 1 hunk FAILED -- saving rejects to file commerce_price_table.info.rej
patching file commerce_price_table.module
Hunk #1 succeeded at 525 (offset 23 lines).
patching file commerce_price_table.rules_defaults.inc
Hunk #1 FAILED at 11.
1 out of 1 hunk FAILED -- saving rejects to file commerce_price_table.rules_defaults.inc.rej

Not sure how to fix this, any help welcome.

kscheirer’s picture

Category: Bug report » Task
Priority: Critical » Normal
Status: Needs review » Needs work

Based on #19 patch needs to be updated, and downgraded priority because its not critical, more like a "nice to have" feature.

@sahaj: next step is for someone to reroll this patch against the latest dev and set the issue to "needs review" again.

Cybso’s picture

Only small changes were required to make the patch applicable again. But since I'm not working on the project anymore I cannot check if the patch still works with the latest dev code. sahaj, can you test this and report it here?

Cybso’s picture

Status: Needs work » Needs review
BD3’s picture

The patch in #21 applies cleanly to 7.x-1.2 and is working for me. Sorry, don't have an installation to test on -dev, so I will leave the status as-is: Needs Review.

MHGR’s picture

I have commerce_price_table 7.x-1.2 version. I have applied #21 patch.

then edited Set the unit price to a table based price and added field_price_table in condition,

in Action: Price Table: choosed commerce-line-item:commerce-product:field-price-table:

I am not able to choosing upto "field-price-table"

please help to solve this issue.

MHGR’s picture

Category: Task » Bug report
Status: Needs review » Active
onejam’s picture

I ran into this issue as well, using Drupal Commerce.

Error: unknown action commerce_price_table_override_price_field_product_price_table

I'm using commerce_price_table 7.x-1.x-dev. The patch #21 fixed it.

Thanks,

kscheirer’s picture

Status: Active » Needs review

Issues with patches should be in "needs review" status. I would set this to RTBC based on #23 and #26, but #24 reported issues.