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

CRM-20946, use line total of line item to store in financial item table #10726

Merged
merged 2 commits into from
Jul 30, 2017

Conversation

pradpnayak
Copy link
Contributor


@Edzelopez
Copy link
Contributor

Tested patch for the following scenario:

  1. Added membership with priceset of options $10 and $20.
  2. Set contribution status to pending before saving membership.
  3. Cancelled contribution.

Amounts were not proportionally assigned to entries in entity trxn table.
beforepatch

After patch:
Reversal entries are proportionally assigned.
afterpatch

@pradpnayak pradpnayak changed the title CRM-20946, use line total of line itme to store in financial item table CRM-20946, use line total of line item to store in financial item table Jul 25, 2017
@pradpnayak
Copy link
Contributor Author

@colemanw / @eileenmcnaughton can you please review?

@eileenmcnaughton
Copy link
Contributor

I feel like we have been adding a lot of unit testing covering that function, and the fact this continues to pass unit tests gives me a lot of confidence ... you know where this is going don't you..... if you add a unit test I think this is OK to merge.

@monishdeb
Copy link
Member

@eileenmcnaughton I have added unit test 338a3c3

@eileenmcnaughton
Copy link
Contributor

Looks like editing the xml dataset had some flow on effects...

----------------------------------------
* CRM-20946: Wrong entries in financial table when contribution is cancelled
  https://issues.civicrm.org/jira/browse/CRM-20946
@monishdeb monishdeb force-pushed the CRM-20946 branch 2 times, most recently from b6a03e4 to bd3034b Compare July 30, 2017 10:45
@monishdeb
Copy link
Member

@eileenmcnaughton yup forgot to cleanup those newly added contacts in xml. It's fixed now and tests build passes.

@eileenmcnaughton
Copy link
Contributor

OK - this looks good to me - the change is the variable passed when deciding if it is a negative or positive transaction and the unit test gives me confidence that this part won't regress again when we do further tweaking

@eileenmcnaughton eileenmcnaughton merged commit 51fb7e5 into civicrm:master Jul 30, 2017
@monishdeb monishdeb deleted the CRM-20946 branch August 2, 2017 09:56
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.

5 participants