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-21201: Tax recalculated when pay later contribution is completed using Pay Now #11026

Merged
merged 1 commit into from
Oct 1, 2017

Conversation

monishdeb
Copy link
Member

@monishdeb monishdeb commented Sep 25, 2017

Overview

Steps to replicate:

  1. Add contribution with tax i.e 100 + 10 (tax) = $110 with status as Pending(pay later)
  2. On user dashboard click on Pay Now and complete the transaction

Before

The confirm and thank you page shows correct amount i.e $110 but contribution total amount is updated to $121.00(i.e tax is recalculated for $110 + 11 = $121) and also there is unbalance transaction recorded in financial table.

After

  1. The contribution amount is correctly set to $110
  2. At online registration page, besides total amount field, it tells about the included tax amount.
    image

Technical Details

Moved the form variable assignment into a separate function that also calculates and set tax amount unlike earlier. Unfortunately, cannot add a unit test to assert the contribution details updated after CRM_Contribute_Form_Contribution_Confirm form submission as per 'Pay Now' workflow.


@eileenmcnaughton
Copy link
Contributor

This looks like a code improvement but I have managed to stay blissfully ignorant of the ins & outs of what should happen here.

I like the extraction. @KarinG should weigh in

@alifrumin
Copy link
Contributor

alifrumin commented Sep 29, 2017

I was able to recreate this bug on a clean build by following @monishdeb documentation and can confirm that this patch fixes it.

If @KarinG thinks it looks good I think it is ready to be merged.

@KarinG
Copy link
Contributor

KarinG commented Sep 29, 2017

Thank you! @alifrumin - I'll will look at it today.

@KarinG
Copy link
Contributor

KarinG commented Sep 29, 2017

@monishdeb and @alifrumin - I am trying recreate this but on the user dashboard -> I just have a Print Invoice button [ both on my d47.localhost - as well as on http://dmaster.demo.civicrm.org/civicrm/user?reset=1 ] - any idea as to what I may be missing? Not seeing the Pay Now

image

@alifrumin
Copy link
Contributor

@KarinG in order to get the pay now button on the user dashboard you have to enable deferred payments and set a default page on the civi contribute settings page. I did it on the Jenkins build and will send better documentation in a second when I get to my computer

@KarinG
Copy link
Contributor

KarinG commented Sep 29, 2017

Ah - of course...

@KarinG
Copy link
Contributor

KarinG commented Sep 29, 2017

Ok reproduced that...

@alifrumin
Copy link
Contributor

from there, if you create a pending pay later contribution with a financial type that has taxes, and go to pay for it from the pay now button. It should look good when you pay and on the thank you.

BUT when save a contribution that has been taxed twice

@alifrumin
Copy link
Contributor

let me know if you have questions.. It took me a minute to recreate too

@KarinG
Copy link
Contributor

KarinG commented Sep 29, 2017

Reproduced it -PS: do you know anything about the reasoning behind the fact that in order to get a Pay Now button on the User Dashboard -> one has to enable Deferred Revenue (Accounting)?

@alifrumin
Copy link
Contributor

alifrumin commented Sep 29, 2017

I don't and I had to ask Tommy to figure it out, certainly not super intuitive

@KarinG
Copy link
Contributor

KarinG commented Sep 29, 2017

Ok - great - thanks @monishdeb - this PR is working well - I've confirmed for price - and non-priceset scenarios; I'm comfortable with this being merged;

Two notes

  1. Pay Now buttons should probably be available for organizations that do not use Deferred Revenue Accounting);

  2. I came across another bug re: Sales Taxes while trying to check if this PR affected that Contribution pathway - at first I thought it was a regression (as I remember fixing it) - but it was a different issue - I fixed something related for 4.6; but the issue is different in 4.7;

@monishdeb
Copy link
Member Author

monishdeb commented Sep 30, 2017

Thanks @alifrumin @KarinG for reviewing it :) Regarding 'Pay Now' button appearance in contact dashboard, it's NOT dependent on Deferred Revenue Accounting any more (this was the PR #11003 which fixed it), but on this setting:
screen shot 2017-09-30 at 8 29 19 pm

@monishdeb
Copy link
Member Author

@eileenmcnaughton can you merge it?

@KarinG
Copy link
Contributor

KarinG commented Sep 30, 2017

@monishdeb - Ah great - so one no longer needs to hit the Checkbox Deferred Revenue in order to get to the Default invoice Payment page option. Thanks for pointing to that.

@eileenmcnaughton
Copy link
Contributor

Merging based on review by @KarinG & @alifrumin

@eileenmcnaughton eileenmcnaughton merged commit 2af3fc1 into civicrm:master Oct 1, 2017
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