Fix incorrect display of Line Items created via API when printing invoice (for Participants) #13477

mecachisenros · 2019-01-16T22:55:42Z

Overview

Invoices display incorrect Line Items for Participant registration when creating a contribution via Contribution.create (with skipLineItems => true) and creating the Line Items via LineItem.create afterwards. The same applies creating the contribution using the Order.create, This technique is common and used in modules like Webform CiviCRM or Caldera Forms CiviCRM integrations.

Before

Invoice display incorrect Line Items

After

Technical Details

An example of API calls to reproduce:

// create participants
  $participantOne = civicrm_api3( 'Participant', 'create', [
    'contact_id' => 203,
    'event_id' => 3,
    'role_id' => 1,
    'status_id' => 'Registerd',
    'source' => 'Source',
    'fee_level' => 'Option 1',
    ...
  ] );

  $participantTwo = civicrm_api3( 'Participant', 'create', [
    'contact_id' => 207,
    'event_id' => 3,
    'role_id' => 1,
    'status_id' => 'Registerd',
    'source' => 'Source',
    'fee_level' => 'Option 2',
    ...
  ] );

  // create contribution
  $contribution = civicrm_api3( 'Contribution', 'create', [
    'total_amount' => 24.00,
    'tax_amount' => 4.00,
    'financial_type_id' => 4,
    'contribution_status_id' => 'Completed',
    'payment_instrument_id' => 4,
    'currency' => 'GBP',
    'source' => 'Source',
    'contact_id' => 203,
    'skipLineItem' => 1
    ...
  ] );

  // create line items
  $lineItemOne = civicrm_api3( 'LineItem', 'create', [
    'price_field_id' => 47,
    'label' => 'Option 1',
    'financial_type_id' => 4,
    'non_deductible_amount' => 0.00,
    'price_field_value_id' => 77,
    'tax_rate' => 20.00,
    'tax_amount' => 2.00,
    'qty' => 1,
    'field_title' => 'Event 1',
    'unit_price' => 10.00,
    'line_total' => 10.00,
    'entity_table' => 'civicrm_participant',
    'entity_id' => $participantOne['id'],
    'contribution_id' => $contribution['id']
  ] );

  $lineItemTwo = civicrm_api3( 'LineItem', 'create', [
    'price_field_id' => 48,
    'label' => 'Option 2',
    'financial_type_id' => 4,
    'non_deductible_amount' => 0.00,
    'price_field_value_id' => 77,
    'tax_rate' => 20.00,
    'tax_amount' => 2.00,
    'qty' => 1,
    'field_title' => 'Event 1',
    'unit_price' => 10.00,
    'line_total' => 10.00,
    'entity_table' => 'civicrm_participant',
    'entity_id' => $participantTwo['id'],
    'contribution_id' => $contribution['id']
  ] );

  // create participant payment
  $participantPayment = civicrm_api3( 'ParticipantPayment', 'create', [
    'participant_id' => $participantOne['id'],
    'contribution_id' => $contribution['id']
  ] );
  ...
  // etc

civicrm-builder · 2019-01-16T22:55:43Z

Can one of the admins verify this patch?

civibot · 2019-01-16T22:55:44Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

seamuslee001 · 2019-01-16T23:21:33Z

Jenkins add to whitelist

seamuslee001 · 2019-01-16T23:45:18Z

ping @JoeMurray @monishdeb @eileenmcnaughton

pradpnayak · 2019-01-17T00:45:06Z

@mecachisenros can you please confirm how do you link participant and contribution? In you api example i couldn't see any parameter for contribution_id in lineitem create nor participantpayment create api.

pradpnayak · 2019-01-17T00:48:42Z

CRM/Contribute/Form/Task/Invoice.php

-        $eid = $contribution->_relatedObjects['participant']->id;
-        $lineItem = CRM_Price_BAO_LineItem::getLineItems($eid, 'participant', NULL, TRUE, FALSE, TRUE);
-      }
+      $lineItem = CRM_Price_BAO_LineItem::getLineItemsByContributionID($contribID);


Can you confirm if your commit doesn't cause regression for CRM-16250 ? Since I guess the line CRM_Price_BAO_LineItem::getLineItems($eid, 'participant', NULL, TRUE, FALSE, TRUE); was written to handle multiple participant(shouldn't be a problem since same contribution id is shared between participant and additional participant)

https://github.com/civicrm/civicrm-core/blob/master/CRM/Price/BAO/LineItem.php#L237

That's correct, the line items are created with the same contribution_id, like this:

// civicrm_line_item table export { "civicrm_line_item": [ { "id": 238, "entity_table": "civicrm_participant", "entity_id": 226, "contribution_id": 91, "price_field_id": 47, "label": "Option 1", "qty": 1, "unit_price": 10, "line_total": 10, "participant_count": 1, "price_field_value_id": 77, "financial_type_id": 4, "non_deductible_amount": 0, "tax_amount": 2 }, { "id": 239, "entity_table": "civicrm_participant", "entity_id": 227, "contribution_id": 91, "price_field_id": 47, "label": "Option 2", "qty": 1, "unit_price": 10, "line_total": 10, "participant_count": 1, "price_field_value_id": 78, "financial_type_id": 4, "non_deductible_amount": 0, "tax_amount": 2 } ] }

RE CRM-16250, here's a generated invoice for same Registration (Contact 1 + Contact 2) same PriceSet and fee levels, but registering the participants through Event page (CiviEvent) instead of API, with this patch applied.

And here it's export as well:

{ "civicrm_line_item": [ { "id": 240, "entity_table": "civicrm_participant", "entity_id": 228, "contribution_id": 92, "price_field_id": 47, "label": "Option 1", "qty": 1, "unit_price": 10, "line_total": 10, "participant_count": 1, "price_field_value_id": 77, "financial_type_id": 4, "non_deductible_amount": 0, "tax_amount": 2 }, { "id": 241, "entity_table": "civicrm_participant", "entity_id": 229, "contribution_id": 92, "price_field_id": 47, "label": "Option 2", "qty": 1, "unit_price": 10, "line_total": 10, "participant_count": 1, "price_field_value_id": 78, "financial_type_id": 4, "non_deductible_amount": 0, "tax_amount": 2 } ] }

mecachisenros · 2019-01-17T01:05:35Z

i couldn't see any parameter for contribution_id in lineitem create nor participantpayment create api.

My fault, copy/paste issue, I'll update the description to reflect it.

eileenmcnaughton · 2019-01-17T01:25:09Z

ouch - we need functions like this to not be on the form layer & to be unit tested :-(

KarinG · 2019-01-17T02:02:10Z

@mecachisenros - thank you for this!

The invoice PDF should really just be a nice PDF rendering of the line items that are displayed on the screen - so I like the idea of stripping non-relevant logic from invoice.php - this should just be about the line items at this stage. I'm confirming this is also an issue with Webform CiviCRM (which uses different APIs then you use in Caldera Forms). The Math on the invoice does not add up. It's easy for admins to send out an incorrect invoice. Here's some screenshots to illustrate:

KarinG · 2019-01-17T02:09:26Z

PS - I just applied your commit to our repo - and confirmed it fixes the issue:

mecachisenros · 2019-01-17T02:19:46Z

@KarinG many thanks for your help and time testing!

Caldera Forms CiviCRM uses Order.create, but the issue is present in all my tests, regardless of APIs used, the example above was intended to illustrate a more conventional approach :)

colemanw · 2019-01-17T18:35:02Z

@civicrm-builder retest this please.

kcristiano · 2019-01-19T19:46:28Z

@mecachisenros I have tested against master as well. Against both the demo database and a client database using the Order API as you described. The PR works well to solve the problem.

The failing check is geocoding. @eileenmcnaughton what else is needed to make this merge ready?

eileenmcnaughton · 2019-01-26T23:07:29Z

@kcristiano I'm not terribly comfortable merging this without a test - we can ignore the failing geocoding test though

JoeMurray · 2019-01-30T18:25:17Z

@monishdeb could you provide some direction to @mecachisenros on how to create an appropriate on the order api or a combination of the order api and invoice creation for the use case of an event with multiple line items? @kcristiano believes this is the use case that needs a UT.

KarinG · 2019-01-30T18:44:15Z

Sorry @JoeMurray but you’re missing the point. invoice.php should be rendering the line items that are on the contribution record. There should be no additonal business logic (if event then...). If admins hit Email invoice - that invoice should contain the lineitems that admins are seeing on the screen. Anything else would be incorrect.

kcristiano · 2019-01-30T18:47:13Z

@KarinG That may be my miscommunication. I was talking to @JoeMurray about this PR and that Eileen wanted more test coverage.

As you and I have both tested, we know this PR does render the lines properly and what you see on the screen is now rendered on the invoice. I would just like some guidance on what test coverage we can add to get this tested and merged.

KarinG · 2019-01-30T19:07:44Z

I gave Andrei some direction on what we could do and pointed him to some examples - though not convinced that a test is needed in this case. We’re removing bad logic. It’s highly unlikely that it will ever be merged in again

kcristiano · 2019-01-30T19:08:34Z

Thanks @KarinG

JoeMurray · 2019-01-30T19:12:37Z

So @monishdeb please aim to respond to @eileenmcnaughton 's interest in a test, taking account of rest of discussion here.

KarinG · 2019-01-30T20:06:35Z

Great - thank you

mecachisenros · 2019-01-31T10:40:45Z

@eileenmcnaughton I'm happy to take a stab at the test coverage (it would be my first test for Civi, so please bear with me), my understanding is that there should be two test cases to check the Line Items, one for Event registration via an Event Page, and the second for Event registrations via API, the second will fail without this patch? Or am I getting this wrong/confused?

Guidance would be very much appreciated.

eileenmcnaughton · 2019-01-31T21:00:05Z

@mecachisenros so I guess the goal of the test is to specifically test the output of the html

CRM_Contribute_Form_Task_Invoice::printPDF function

I would normally try to extract out the portion that generates the html & potentially test just that but there are some wierd work-arounds hacked in & already used in
CRM_Contribute_Form_Task_InvoiceTest

In terms of setup for the test - I think you can use whatever combination api & other functions is easiest for you to get the data in .

I'm just confused about your comment though - I haven't fully followed but you say that it will currently fail if you set this up via event page but not api - is that because the data is different or the way the function is called is different

kcristiano · 2019-06-12T16:46:47Z

test this please

mecachisenros · 2019-06-27T16:48:24Z

@kcristiano @eileenmcnaughton apologies for the long wait, some time has passed... but I finally got to prepare the test for this.

Without this patch the test fails, with the patch the test passes, please let me know if there's anything to be amended.

kcristiano · 2019-06-27T23:06:00Z

test this please

eileenmcnaughton · 2019-06-28T00:14:18Z

@mecachisenros thanks for that! The tests are not running due to the patch failing to apply - I think a rebase is needed?

mecachisenros · 2019-06-28T00:47:35Z

@eileenmcnaughton thank you, just did a rebase, can we try again?

eileenmcnaughton · 2019-06-28T00:49:12Z

@mecachisenros so whenever you push to the PR branch it will retrigger the tests - if you look at details above it's now moved past the original issue - but there are formatting issues so the full tests have not run

eileenmcnaughton · 2019-06-28T00:51:17Z

@mecachisenros I've also removed the 'needs-test' flag and added 'has-test' and 'merge on pass' - I appreciate you coming back to this one and adding a test. I know it can be a drag adding them - much as we all understand the value of them

mecachisenros · 2019-06-28T01:17:48Z

Many thanks for the help @eileenmcnaughton, got it now 👍 those errors weren’t showing before, I think it should be fine now (I believe I fixed all the formatting issues).

They are necessary, it’s not a drag at all, it’s more understanding and getting familiar with them, and then writing/adding them :)

eileenmcnaughton · 2019-06-28T01:19:09Z

@mecachisenros thanks!

civibot bot added the master label Jan 16, 2019

pradpnayak reviewed Jan 17, 2019

View reviewed changes

eileenmcnaughton added the needs-test label Feb 24, 2019

retrieve LineItems by contributionID regardless of component

62467b1

mecachisenros force-pushed the invoice-line-items branch from dd4ef13 to 7702be4 Compare June 28, 2019 00:45

eileenmcnaughton added has-test merge on pass and removed needs-test labels Jun 28, 2019

add test for invoice line items

ea75f9b

mecachisenros force-pushed the invoice-line-items branch from 7702be4 to ea75f9b Compare June 28, 2019 01:04

seamuslee001 merged commit 8adb623 into civicrm:master Jun 28, 2019

mecachisenros mentioned this pull request Nov 4, 2019

Pay by check mecachisenros/cf-civicrm#128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incorrect display of Line Items created via API when printing invoice (for Participants) #13477

Fix incorrect display of Line Items created via API when printing invoice (for Participants) #13477

mecachisenros commented Jan 16, 2019 •

edited

Loading

civicrm-builder commented Jan 16, 2019

civibot bot commented Jan 16, 2019

seamuslee001 commented Jan 16, 2019

seamuslee001 commented Jan 16, 2019

pradpnayak commented Jan 17, 2019

pradpnayak Jan 17, 2019 •

edited

Loading

mecachisenros Jan 17, 2019

mecachisenros commented Jan 17, 2019

eileenmcnaughton commented Jan 17, 2019

KarinG commented Jan 17, 2019

KarinG commented Jan 17, 2019

mecachisenros commented Jan 17, 2019

colemanw commented Jan 17, 2019

kcristiano commented Jan 19, 2019

eileenmcnaughton commented Jan 26, 2019

JoeMurray commented Jan 30, 2019

KarinG commented Jan 30, 2019

kcristiano commented Jan 30, 2019

KarinG commented Jan 30, 2019

kcristiano commented Jan 30, 2019

JoeMurray commented Jan 30, 2019

KarinG commented Jan 30, 2019

mecachisenros commented Jan 31, 2019

eileenmcnaughton commented Jan 31, 2019

kcristiano commented Jun 12, 2019

mecachisenros commented Jun 27, 2019

kcristiano commented Jun 27, 2019

eileenmcnaughton commented Jun 28, 2019

mecachisenros commented Jun 28, 2019

eileenmcnaughton commented Jun 28, 2019

eileenmcnaughton commented Jun 28, 2019

mecachisenros commented Jun 28, 2019

eileenmcnaughton commented Jun 28, 2019

Fix incorrect display of Line Items created via API when printing invoice (for Participants) #13477

Fix incorrect display of Line Items created via API when printing invoice (for Participants) #13477

Conversation

mecachisenros commented Jan 16, 2019 • edited Loading

Overview

Before

After

Technical Details

civicrm-builder commented Jan 16, 2019

civibot bot commented Jan 16, 2019

seamuslee001 commented Jan 16, 2019

seamuslee001 commented Jan 16, 2019

pradpnayak commented Jan 17, 2019

pradpnayak Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

mecachisenros Jan 17, 2019

Choose a reason for hiding this comment

mecachisenros commented Jan 17, 2019

eileenmcnaughton commented Jan 17, 2019

KarinG commented Jan 17, 2019

KarinG commented Jan 17, 2019

mecachisenros commented Jan 17, 2019

colemanw commented Jan 17, 2019

kcristiano commented Jan 19, 2019

eileenmcnaughton commented Jan 26, 2019

JoeMurray commented Jan 30, 2019

KarinG commented Jan 30, 2019

kcristiano commented Jan 30, 2019

KarinG commented Jan 30, 2019

kcristiano commented Jan 30, 2019

JoeMurray commented Jan 30, 2019

KarinG commented Jan 30, 2019

mecachisenros commented Jan 31, 2019

eileenmcnaughton commented Jan 31, 2019

kcristiano commented Jun 12, 2019

mecachisenros commented Jun 27, 2019

kcristiano commented Jun 27, 2019

eileenmcnaughton commented Jun 28, 2019

mecachisenros commented Jun 28, 2019

eileenmcnaughton commented Jun 28, 2019

eileenmcnaughton commented Jun 28, 2019

mecachisenros commented Jun 28, 2019

eileenmcnaughton commented Jun 28, 2019

mecachisenros commented Jan 16, 2019 •

edited

Loading

pradpnayak Jan 17, 2019 •

edited

Loading