Issue #2840851 by adixon: Price field id and price field value incorrect

[adixon]

As per https://www.drupal.org/node/2840851#comment-11906821

[KarinG]

Hi Alan - trying to reproduce the issue on 4.6. I've got a webform_civicrm webform with

1) a Membership Fee field:

2) a Contribution Amount field (for a Donation)

3) The Contribution page this is linked has main Financial Type: Donation;

4) Submitting: $100 in the Membership Field and $50 in the Donation field -> yields a $150 Contribution in CiviCRM with the following two line-items:

5. I can indeed NOT view these lineItems in the CiviCRM GUI. But they are correct - in that their Financial Types are correct. To confirm this:

6. I looked at issuing a Charitable Tax Receipt for the $150 Contribution -> and it correctly yields a $50 receipt (i.e. not receipting the Membership portion).

7. and I did get the Membership:

Are you looking to make webform_civicrm fetch more information from the Contribution pages - thus requiring less configuration on the webform_civicrm side?

I do agree the NULLs in the Contribution line_item are not nice- and may be a problem at some point - but right now we can sell Membership + Donation on a single form -> and the monies, the membership and the TaxReceipt adds up. If I'm missing something do let me know!

[KarinG]

PS - of course instead of Members Fee - you can do a -user select- and get someone to pick their own Membership Type/Level;

Key is that webform_civicrm will assign the Financial Type of the Contribution page to the Contribution amount; So to sell a membership + donation in webform_civicrm link it to a Contribution page that has Financial Type = Donation; webform_civicrm will find your Membership Financial Type -> form the Membership Type Configuration;

[mfuggle]

I agree that the events described above function correctly. However there are issues that may be unrelated. When viewing the contribution in the contact record it should display the individual line items that have been created in the line items table. Instead it displays a single line item but in the type shows the Financial Type for each of the line items. Furthermore if the View Contribution is used in the submission there are times when it displays the contribution record and times when it give an error that I have reported elsewhere (I will add the link later). I thought that it displayed correctly when there was only a single line item and failed If there were multiple line items but I am not certain that is the case.

[adixon]

1. Yes, I am looking for webform civicrm to pull more information from the civi contribution page, but not to reduce the configuration on the webform.

2. Do check my notes - the key is that webform civicrm doesn't send over line items. CiviCRM can sometimes correctly guess the membership financial type, but it doesn't always. I'd guess it usually does, since this hasn't come up very often. When it chooses the wrong one, it can "break" civicrm.

3. The lack of financial type for the donation portion - on it's own isn't always critical, but it's a symptom of a deeper problem, and indeed can cause issues, depending on how CiviCRM is used.

[KarinG]

webform_civicrm [when configured as described above]:

• does add line_items to the civicrm_line_item table;
• both donation financial_type and membership financial_type in these line_items are correct;
• donation financial_type is identical to the one specified on the Contribution Page (setting) [=1 in my phpMyAdmin screenshot];
• membership financial_type is identical to the one specified on the Membership Type (setting) [=14 in my phpMyAdmin screenshot];

Are you saying this is correct by accident?

[adixon]

[corrected] webform_civicrm does NOT set the price field ids of the line items, civicrm does.

[colemanw]

This module does create line items.

• It calculates them in function tallyLineItems
• It saves them in function processContribution

[adixon]

Sorry, you're right, and the thanks for the links. And here's the patch I'm proposing: https://github.com/colemanw/webform_civicrm/pull/58/files

What my patch does is solve the problem of the price field value id of these line items, which are currently set by CiviCRM.

[KarinG]

Ok - I finally have gotten to the bottom of this. Apologies for the delay...

1. key issue is that webform_civicrm module does not set: $item['price_field_id'] and $item['price_field_value_id']

2. when these are not set CiviCRM thinks a mistake has been made and will look for the best matched price_field_value_id - if a site has hundreds of price_field_value_ids - the results can be quite interesting.

3. on the project that I (finally) encountered this issue on - we were writing a contribution with Membership financial_type_id as a hidden -user-select- on the webform based on (conditionals) Province (to apply the correct amount of GST/HST).

Result: the webform does everything right - the payment amount is correct. But the line_item 'latched' on to the price_set originally configured (way back) for the Membership Type (when you save the Membership Type config a price_field_value is actually created if it does not yet exists); That means CiviCRM rewrote the line_item using that financial_type_id instead of the one webform_civicrm via line_item API had previously saved. That does not change the monies, as these had already been transacted. It does make the invoice quite mangled - as GST/HST was collected but now there is a financial_type_id that has no GST/HST associated with it.

4. first attempt - I tried to be very specific and set the $item['price_field_id'] = NULL; and $item['price_field_value_id'] = NULL;

Ok that worked in that I could see these values be saved, but then later on in CiviCRM [not exactly sure where] - there is panic that these values are NULL and the search for a price_field_value is on - and when found it latches on just like before. This would continue working if you delete all price_field_value fields that can cause interference - but they are so easily re-generated. This is not a permanent solution.

5. second attempt - I created aprice_setthat has 4 fields - one associated with each of the 4 GST/HST rates: I called it Individual Member - WF as I only want to purpose these for a WebForm.

6. next: hooked this price_set up to the Contribution page that is configured as attached to the Webform [telling Webform what Payment Processors e.g. to make available].

7. then in code:

        $price_field_value = $this->fetchPriceField($this->contribution_page['id'], $item['financial_type_id']);
        $item['price_field_id'] = $price_field_value['price_field_id'];
        $item['price_field_value_id'] = $price_field_value['id'];

Where function fetchPriceField - returns 'the correct $price_field_value

8. How to determine the 'correct' one? In this use case all we cared about (to make things add up and make invoice look good) is that we end up with the correct financial_type_id in the line_item. So we pull up the price_field_values that are associated with that contribution page (in table: civicrm_price_set_entity) - and then check to see when we have one that is the one we want;

if ($price_field_value['financial_type_id'] == $financial_type_id) {
  return $price_field_value;
}

9. How to generalize this? Should we also look at Amount? The Amount is set/calculated on the webform - but could perhaps be a criteria to find the best match in other use cases? $financial_type_id match is necessary to guarantee the GST/HST to make it onto the invoice properly.

Ideas? Comments?

Please note that looking for the correct membership_type_id (as Alan did in his PR for his use case) - would have resulted in 4 matches in our use case [they are all of type Individual Membership - some people just paid different GST/HST on it based on where they live] - so I decided not to do that and go straight on the lookout for: financial_type_id

Happy to put some more work into generalizing this - if we can come up with a best (works for most use cases) match plan?

[adixon]

Nice ... yeah, it's a bit complicated.

There are two ways of going about this:

1. Assume that the referenced civicrm contribution page will give us all the clues we need to set the appropriate price field and price field id for each line item. That's the strategy implemented by my patch, though it might be equally well implemented as a pair of patches that pushes some of the functionality over to CiviCRM core.

2. Make no assumptions about the reference civicrm contribution page and provide webform-civicrm with the tools to explicitly set the price field and price field id for each line item.

I'm inclined to support both - i.e. implement 1 to improve the existing behaviour, but also provide an advanced option to be more explicit, with 2.

[KarinG]

Hi Alan,

ad 1: I tried but your strategy does not work for our use case. We have one membership_type_id and 4 potential matches based on that- all with different financial_type_id; we could mix our two strategies together but it's tricky in that there will likely be other use cases where we would still make the wrong decision.

ad 2. I like this idea. That's how CiviCRM native works -> you're in the GUI front or backend and you select a price-set. It's specific.

I can put some work into this -> how about:

• expose the price_field_value_id to the webform (much like we're giving admins the ability to expose financial_type_id right now)
• reduce confusion for admins: only expose the price_field_value_ids belonging to the price_set specifically configured for the contribution form that is attached to the webform (any CiviCRM has a plethora of price_sets - some not visible via the GUI - i.e. the is_quick_config=1);
• default behaviour should be: -automatic- as in do nothing. Which means: let CiviCRM Core do the matching - Options: a) select specific price_field_value_id b) -user select - let admins set price_field_value_id based on other fields using conditionals

Let me know if you have any other thoughts/comments on this.

I think perhaps after this we may be able to say that webform can handle pricesets? :-)

[adixon]

Yes, I think we're agreeing in principle - we want improve default behaviour, but with the ability to override the defaults when necessary. We certainly don't want to make configuration more complicated for those cases where it's working fine as is. I suspect our two different ways of improving the default behaviour may be more closely aligned than you realise, or maybe I'm just misunderstanding something, but in any case, I'd say go ahead and work on some code.