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

Fix missing financial_account_id for recurring contributions #898

Open
wants to merge 1 commit into
base: 6.x
Choose a base branch
from

Conversation

mattwire
Copy link
Collaborator

Overview

Both price_field_id and price_field_value_id need to be set when creating the lineitems.
For memberships we were retrieving that data but not using it. For contributions we were only retrieving the default price_field_id and not setting price_field_value_id.

This resulted in errors such as found in https://lab.civicrm.org/extensions/mjwshared/-/issues/20 where CiviCRM ends up not setting the financial_account_id.

Before

"Invalid" lineitems created.

After

Valid lineitems created.

Technical Details

CiviCRM has a default "Price field ID" and a default "Price field value ID" which should be used if nothing else is specified. For memberships we can map them to existing ones.

Comments

Copy link
Collaborator

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested by different combinations of amounts on the webform and this works as expected. Thanks @mattwire

@mattwire
Copy link
Collaborator Author

mattwire commented Aug 2, 2023

I think the fix actually needs to be in CiviCRM core as this won't work with multiple line items - see civicrm/civicrm-core#26974

@mattwire mattwire marked this pull request as draft October 9, 2023 08:52
@mattwire mattwire marked this pull request as ready for review October 9, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants