CRM-19580: Contribution Receipts not including all lines.

[litespeedmarc]

---

• CRM-19580: LIne items are missing from manual receipts when using a price set with multiple membership organization price fields
  https://issues.civicrm.org/jira/browse/CRM-19580

Also clarified input parameter names in CRM_Price_BAO_LineItem::getLineItems, their names were very misleading!

[seamuslee001]

hi @litespeedmarc Marc

would it be possible for you to test these 2 PRs as a fix for your issue as i think these are the same issues #9317 and #9327

ping @eileenmcnaughton @pradpnayak

[eileenmcnaughton]

Yep #9327 is also addressing the same issue - I hit the same test failures you did & fixed them in #9327 - do you want to see if that one addresses your bug. It's kind of hard to know how to integrate this with the previous 2 - I did figure out what needed to change in #9317 but you have added some further clarification to the parameters.

My thinking was to leave dealing with the wierdness around the entity_id until #9327 & #9137 were merged because it's too hard to change too many things at once

[litespeedmarc]

The PR certainly does look like it would. I wonder how these managed to stay broken like this so long? Is this a recent regression, or is it just that my client has really elaborate price sets? I'll verify & update CRM-19580 then. Is there another CRM for this issue that I couldn't find somewhere? I would also like to verify with CRM-15861 integrated, forcing me to rebase & handle conflicts. Double motive here... I really do need both PRs.

[litespeedmarc]

Oh, and by "but you have added some further clarification to the
parameters"... not sure if you mean my changes to LineItem.php (in
PR-9335). You can keep / disregard those. I tend to cleanup little things
that would never otherwise get cleaned up (e.g., redundant assignments,
parameter names, typos, ...). But those were all noop refactors. Though I
have to say "isQuick" being used as argument to indicate to exclude quick
config, really was confusing!! :)

On Sun, Oct 30, 2016 at 8:00 PM, Marc Brazeau litespeedmarc@gmail.com
wrote:

The PR certainly does look like it would. I wonder how these managed to
stay broken like this so long? Is this a recent regression, or is it just
that my client has really elaborate price sets?

I'll verify & update CRM-19580 then. Is there another CRM for this issue
that I couldn't find somewhere?

I would also like to verify with CRM-15861 integrated, forcing me to
rebase & handle conflicts. Double motive here... I really do need both PRs.

[eileenmcnaughton]

HI @litespeedmarc I only discovered the issue (CRM-19580) because I was reviewing @pradpnayak's PR and realised that he was actually fixing a bug. I don't know if @pradpnayak was aware of the bug or not.

I AM aware of CRM-19581 and am hoping to get back to it. I'm hoping we can deal with things in this order

1. confirm & merge Fix line item fetch on membership template with priceset. #9327
2. @pradpnayak makes minor changes to [ready for core review]Code Cleanup, Removed cruft, generalized code #9317 relating to the quick config & we merge that
3. we finish resolving CRM-19581 - I did make some changes when reviewing this that I have not yet passed back.

I've also just submitted #9337 which is minorly related - although it does not actually clash with any of the other patches or need to be incorporated

[eileenmcnaughton]

""isQuick" being used as argument to indicate to exclude quick config, really was confusing!!"

My preference is to remove that parameter altogether and have the template decide what to do with the line items. Each instance I've looked at where that is used it complicates the logic and makes it hard to understand.

[litespeedmarc]

x 2.

My répertoire of anti-pattern names, leaves to desire. Boolean flag as
method arguments, though, is definitively an anti-pattern!
I've probably used it too many times myself!

On Sun, Oct 30, 2016 at 8:09 PM, Eileen McNaughton <notifications@github.com

wrote:

""isQuick" being used as argument to indicate to exclude quick config,
really was confusing!!"

My preference is to remove that parameter altogether and have the template
decide what to do with the line items. Each instance I've looked at where
that is used it complicates the logic and makes it hard to understand.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADVy4935xTD4PYPOQBsq09fY4Bpdod7Rks5q5THPgaJpZM4KkdqK
.

[litespeedmarc]

Closing PR, as this is fixed by #9327.

[eileenmcnaughton]

@litespeedmarc I just merged my patch based on your comments & @seamuslee001 review