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.

Sign up for GitHub

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

Jump to bottom

CRM-20276 update line item financial amount when updating line item. #10235

Merged
merged 2 commits into from
Apr 27, 2017
Merged

CRM-20276 update line item financial amount when updating line item. #10235

merged 2 commits into from
Apr 27, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 25, 2017 •
edited by civicrm-builder
Loading

We don't have to leave the e-notice in, but I suspect that line is never called & it would be good
to figure that out rather than leave it 'in case', hence the enotice

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton eileenmcnaughton force-pushed the follow_on branch 9 times, most recently from 2bf71c7 to fb081b6 Compare April 26, 2017 23:58
eileenmcnaughton and others added 2 commits April 27, 2017 15:10
@eileenmcnaughton @monishdeb
CRM-20276 update line item financial amount when updating line item.
8a47705 
We don't have to leave the e-notice in, but I suspect that line is never called & it would be good
to figure that out rather than leave it 'in case', hence the enotice
@monishdeb
minor change
7c63ab2
@monishdeb
Copy link
Member

monishdeb commented Apr 27, 2017 •
edited
Loading

@eileenmcnaughton I have reviewed the patch and I like the refactoring changes. However this doesn't fix the original issue CRM-20276 where I am still getting the incorrect financial_item.amount, reason why I have commented out here. So merging it now as the PR #10172 is dependent on it and will fix the original issue.

@monishdeb monishdeb merged commit a80d386 into civicrm:master Apr 27, 2017
@eileenmcnaughton eileenmcnaughton deleted the follow_on branch April 27, 2017 20:50
@eileenmcnaughton
Copy link
Contributor Author

Thanks @monishdeb & yeah - I agree with your analysis

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @monishdeb @civicrm-builder