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

dev/core#3084 Bug fix on line item financial types #23295

Merged
merged 1 commit into from
Apr 29, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 24, 2022

Overview

https://lab.civicrm.org/dev/core/-/issues/3084

I expect some fixes to v3 order api might be needed to get tests to pass

Before

per steps on https://lab.civicrm.org/dev/core/-/issues/3084

After

Technical Details

Comments

@civibot
Copy link

civibot bot commented Apr 24, 2022

(Standard links)

@civibot civibot bot added the 5.49 label Apr 24, 2022
This initially caused test fails on test written for
https://lab.civicrm.org/dev/core/-/issues/860

However, I think the issue is flaws in the test, which
I have adjusted to have more accurate set up
@eileenmcnaughton
Copy link
Contributor Author

@davejenx this should fix the regression you reported. It caused a test to fail but that test ALSO related to one of your issues - https://lab.civicrm.org/dev/core/-/issues/860 - I concluded the error was in the test & have updated the test to be more internally consistent....

@davejenx
Copy link
Contributor

Fantastic, thanks @eileenmcnaughton ! This does indeed fix the regression. Apologies if one of my tests caused complications.

@demeritcowboy
Copy link
Contributor

I'm confused about how much the goat costs - it looks like $15 for the goat and then $200 in taxes - I hope you at least get good road maintenance out of that. (grin)

Seriously, I don't have a good sense of how this will affect calls to Order api. But it sounds correct.

// We don't permit overrides if there is more than one line.
// The reason for this constraint may be more historical since
// the case could be made that if it is set it should be used and
// we have built out the tax calculations a lot now.
if (!$this->isPermitOverrideFinancialTypeForMultipleLines() && $this->getLineItemCount() > 1) {

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Apr 28, 2022
@eileenmcnaughton
Copy link
Contributor Author

I think it's a bit of a tax rort - you make a tax free donation AND buy a VERY CHEAP GOAT at the same time....

@demeritcowboy
Copy link
Contributor

Hehe.
By the way if I search for "How much does a goat cost?", which has probably put me on all kinds of surveillance watchlists now, there's a surprising range of results, including things like a $3000 Billy Goat Debris Loader, which I both want to know more about and at the same time am scared to find out.

@eileenmcnaughton
Copy link
Contributor Author

maaa-aaaa

@demeritcowboy
Copy link
Contributor

maaa-aaaa

Agreed.

@demeritcowboy demeritcowboy merged commit 62937e7 into civicrm:5.49 Apr 29, 2022
@eileenmcnaughton eileenmcnaughton deleted the 549 branch April 29, 2022 21:36
@eileenmcnaughton eileenmcnaughton mentioned this pull request Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.49 merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants