Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comments
Comment #1
Cybso CreditAttribution: Cybso commentedYou 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.
Comment #2
Cybso CreditAttribution: Cybso commentedThis would also be a fix of #1921668: Support Multiple Price Tables if my assumption made in comment #7 is right.
Comment #3
Cybso CreditAttribution: Cybso commentedWorking on it. Will require to set the field's cardinality to > 1 per default. Not sure how to treat existing rule(s).
Comment #4
Cybso CreditAttribution: Cybso commentedImplemented 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.
Comment #5
pcambraThanks for tackling this :)
Comments above
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.
If this might happen, why not ensure an unique rule machine name?
field type I guess?
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.
Comment #6
Cybso CreditAttribution: Cybso commentedBecause 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.
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 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).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 ;-)
Thx, I'll fix this and provide an updated patch by tomorrow or so.
Comment #7
pcambraI 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.
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:
Setting to NW.
Comment #8
Cybso CreditAttribution: Cybso commentedWhat 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.
Comment #9
Cybso CreditAttribution: Cybso commentedSame problem applies to ief integration. An equivalent solution as suggested in my previous comment for PriceTableCommerceProductInlineEntityFormController should fix this.
Comment #10
pcambraI 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.
Comment #11
pcambraWe could create a separate issue to manage the unlimited setting by default to price tables.
Comment #12
rockaholiciam CreditAttribution: rockaholiciam commentedAny 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.
Comment #13
Cybso CreditAttribution: Cybso commentedI've reserved some time this weekend to fix this
Comment #14
rockaholiciam CreditAttribution: rockaholiciam commentedFor 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:
with
But this should also take into account if the actual/base price is disabled, which it doesn't for now.
Comment #15
Cybso CreditAttribution: Cybso commentedSince 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?
Comment #16
rockaholiciam CreditAttribution: rockaholiciam commentedOpened a separate issue: http://drupal.org/node/1980404
Comment #17
Cybso CreditAttribution: Cybso commentedNew patch that generates dynamic default rules configurations for each field.
Comment #18
Cybso CreditAttribution: Cybso commentedChange: Clear rules' cache only on field operations of type 'commerce_price_table'
Comment #19
sahaj CreditAttribution: sahaj commented#18 no more matching last dev module:
Not sure how to fix this, any help welcome.
Comment #20
kscheirerBased 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.
Comment #21
Cybso CreditAttribution: Cybso commentedOnly 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?
Comment #22
Cybso CreditAttribution: Cybso commentedComment #23
BD3 CreditAttribution: BD3 commentedThe 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.
Comment #24
MHGR CreditAttribution: MHGR commentedI 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.
Comment #25
MHGR CreditAttribution: MHGR commentedComment #26
onejam CreditAttribution: onejam commentedI 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,
Comment #27
kscheirerIssues with patches should be in "needs review" status. I would set this to RTBC based on #23 and #26, but #24 reported issues.