-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert to Order API for line item support #33
Conversation
I have applied the patch on one of our staging site to test it correctly records separate line-item and and financial item on purchasing an order with multiple products. I have added an order with two product items:
Here's the financial records before and after applying the patch: BEFORE
AFTER
As you can see in the later case it fixes two issues:
Hence, the current patch confirms that it correctly records the financial information on purchasing a product. |
The current patch fixes another issue mentioned in #32 where I the update status code wasn't able to change the Donation's status to 'Cancelled' or 'Refunded' on cancelling or refunding a order. Here's the screenshot of both the cases of: Ensured that financial adjustment are correct. |
'0' => 3, | ||
), | ||
$params['line_items'][0]['line_item'][] = array( | ||
'price_field_id' => '1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please replace the hardcoded ID with api call?
'trxn_id' => $txn_id, | ||
'invoice_id' => $invoice_id, | ||
'source' => $source, | ||
'receive_date' => $order_paid_date, | ||
'contribution_status_id' => $contribution_status_id, | ||
// 'contribution_status_id' => $contribution_status_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like to know why this line is commented out!!.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably actually should be hard-coded to 'Pending' and then a payment recorded for completed WooCommerce orders, as per what the Order API expects.
@@ -677,7 +676,7 @@ public function add_contribution( $cid, &$order ) { | |||
* @since 2.0 | |||
* @param array $params The params to be passsed to the API | |||
*/ | |||
$contribution = civicrm_api3( 'Contribution', 'create', apply_filters( 'woocommerce_civicrm_contribution_create_params', $params ) ); | |||
$contribution = civicrm_api3( 'Order', 'create', apply_filters( 'woocommerce_civicrm_contribution_create_params', $params, $order ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have put the new code in and it seems to fix the problem of the financial contribution link, but the plugin also seems to be trying to create a membership which is failing as follows:
Error Details
An error of type E_ERROR was caused in line 133 of the file /home/rjakn8hl475q/public_html/shop/wp-content/plugins/civicrm/civicrm/api/api.php. Error message: Uncaught CiviCRM_API3_Exception: [constraint violation: DB Error: constraint violation thrown
Maybe it is because I have set the WooCommerce product to set to Contribution Type = Membership in Civi although it has worked in the past. Can you tell me what the code is trying to do with creating a membership record in Civi because I can't see anywhere in WooCommerce where this is set or how it is triggered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ralphyke yes, setting the Financial Type to Member Dues seems to (obscurely) attempt to create a Membership, I think I will strip that out for now as it's not the best implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if you can strip that out and then I can put in the new version again, that would be great
Thanks @agileware-fj for the PRs and @monishdeb for reviewing. I'll review and test and report back. |
@monishdeb could you test for multiple line items to ensure that financial items are created properly in that use case. Thanks, @mecachisenros for your work on this. JMA would like to leave the work and testing for the membership stuff to you and others as we focus on the financial stuff. Thanks again @agileware-fj for your patches. Cheers, |
This and the other two PRs were funded work as part of a WordPress / CiviCRM integration project. We were planning on releasing these changes at some stage once the project was done, @JoeMurray your recent comment about maintainers and line items was the prompt to bring that plan forward :) |
@JoeMurray yes I have tested with multiple line-items and posted my findings above. I have tested with two items:
And using chain API result, that the line-items and financial items are created correctly. |
@monishdeb @agileware-fj @agileware-justin @JoeMurray
|
Forgot to mention, added basic Membership implementation. The membership is configurable in the CiviCRM Settings panel in the Edit/Add Product screen in Woo. All membership default/settings (i.e. period, plan, etc.) are based on how the Membership is configured in CiviCRM. |
Thanks you so much guys for getting this PR merged. Much needed change to get the financial entries recorded for the purchased order. Also enables line-item editor to edit the associated line-items properly now, unlike earlier which throws an error due to missing financial-item, which fixed by this patch. |
This converts the integration to use the Order API to allow adding line items that are linked properly to the resulting contribution.
This might be related to or at least interfere with work on #32