Skip to content
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

Enforce isSkipLineItem for membership payment entity in Order.create #15891

Merged

Conversation

mecachisenros
Copy link
Contributor

Overview

Enforce isSkipLineItem for membership payment entity in the Order.create API to prevent error on line item update. The line items are already correctly created by the time the membership payment is created so there's no need to update.

Before

Creating an Order with two or more memberships (and using a Price field with a membership type set) will trigger an error when creating the MembershipPayment entity.

DB Error: already exists

UPDATE civicrm_line_item li
LEFT JOIN civicrm_price_field_value pv ON pv.id = li.price_field_value_id
SET entity_table = 'civicrm_membership', entity_id = 898
WHERE pv.membership_type_id = 3
AND contribution_id = 1493 [nativecode=1062 ** Duplicate entry 'civicrm_membership-898-1493-196-114' for key 'UI_line_item_value']

After

No errors, contribution is processed correctly.

Technical Details

Comments

I believe isSkipLineItem was introduced fairly recently, but I could not find a reference to an issue/conversation.

@civibot
Copy link

civibot bot commented Nov 20, 2019

(Standard links)

@civibot civibot bot added the master label Nov 20, 2019
@mecachisenros mecachisenros force-pushed the order-create-isskiplineitem branch from 3afb9cb to 675b7e6 Compare November 20, 2019 13:09
@kcristiano
Copy link
Member

@mecachisenros see a9f44ff and #15142

This looks to be where and when it was added.

I've done an r-run on this without the PR and I can recreate the error. With the PR r-run succeeds.

@eileenmcnaughton any thoughts? Adding to order api makes sense to me.

@seamuslee001
Copy link
Contributor

@eileenmcnaughton @monishdeb does this make sense to you people?

@eileenmcnaughton
Copy link
Contributor

I think this is OK based on @kcristiano testing - I think we could 'do better' but this does fix a bug for now

@eileenmcnaughton eileenmcnaughton merged commit e3af7c8 into civicrm:master Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants