CRM-21200: Pay Now payment overwrites the contribution

[monishdeb]

Overview

Currently when we do a pay now contribution civicrm_contribution.contribution_page_id and civicrm_contribution.source is updated i.e the configured page for the later pay now payment overwrites the contribution info derived from the original creation of the pay later contribution. This should be a payment that is not overwriting 'reason' that the contribution was originally created.
To replicate:

1. Set Default invoice payment page in CiviContribute Setting (eg contribution page ID = 1|title = Help Support CiviCRM )
2. Add a pay later contribution using Contribution page(eg ID = 3)
3. Click on Pay Now button for a pay later contribution under user dashboard and complete the transaction

Before

Contribution detail like source, campaign_id and contribution_page_id overwritten. Screenshot:

After

1. contribution.contribution_page_id is NOT overwritten.
2. contribution.campaign_id is NOT overwritten.
3. contribution.source is updated with 'Paid later via page ID: 1. Online Contribution: Help Support CiviCRM!'
   Screenshot:
   

---

• CRM-21200: Pay Now payment overwrites the contribution

[highfalutin]

I think this handles the issue well (though it raises the question of whether the source field is being stretched beyond capacity). Tested; the PR behaves as described. @eileenmcnaughton @totten

[eileenmcnaughton]

This seems fine WRT the source - per @highfalutin's testing.

WRT campaign - I guess the argument is that this was originally created via one page (or possibly even an event?) and we want to retain the original campaign. That makes sense I think.

I'm giving this merge ready as it seems OK to merge, but I'd like a little cool down just in case some magic reason why contribution_id might be otherwise present occurs to us.

[eileenmcnaughton]

I guess this does fail the unit test test ... and I mention that because it could easily get re-broken if you don't add one

[monishdeb]

@eileenmcnaughton not sure how to add UT to assert the behaviour of processConfirm(...) also I am pretty sure contribution_id is ONLY set for paynow as identified by $form->_ccid as earlier I've grep'ed the occurrences of it

[highfalutin]

Is there no way to test is_pay_later rather than contribution_id?

[monishdeb]

@highfalutin you want to know if there is a way to use is_pay_later instead of contribution_id here ? Actually on UI end pay now option is only exposed to pay-later online contributions AND also if any default contribution page is set for doing online payment. If these two criterias met, then eventually on submission, contribution_id is set later using arg ccid here

[monishdeb]

@eileenmcnaughton is there any additional fix I need to address?

[eileenmcnaughton]

@monishdeb I guess we should talk more about the test - processConfirm is called from a few forms & I think we can test those form flows - ie. test the submit function, which is what we really care about since we all hope processConfirm will not live forever in anything like it's current form

[monishdeb]

@eileenmcnaughton I have added UT with processsConfirm, please have a look.

[eileenmcnaughton]

we don't really need to wrap this because it is such a shallow wrapper around civicrm_api3('contribution', 'completetransaction', array( I think we should ditch the wrapper & just call the api within the test (which is also how 50% of payment processors will call the completion)

[eileenmcnaughton]

@monishdeb just one minor point regarding the test - I think we can avoid the wrapper - otherwise all good

[monishdeb]

@eileenmcnaughton made the change, can you please check now?

[seamuslee001]

I agree with the changes and i believe that they will fix the issue, I haven't tested it but there seems to be one minor issue with the test but i don't think that should prevent this from being merged

[eileenmcnaughton]

Merging based on merge ready flag having been added a while back & a test having been subsequently added