[ref] Extract activity payment creation

[eileenmcnaughton]

Overview

Minor code cleanup

Before

Code less readable

After

code more readable

Technical Details

I suspect this might also go on the payment class but that might be next - it could also do with a another round of tidy up since it's 2 functions when only one is needed

Comments

The following doesn't block merging this change which is a refactor - but it is commentary on possible next steps

Should the payment.create bao call this? I feel like maybe it should call this if it doesn't result in completetransaction (which would have other activities ) - in our ideal flow we would create an order (pending) process a payment & add a payment when it's complete (potentially completing the order) - if these things happen all at the same time it might be odd to have too many activities created

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[monishdeb]

@eileenmcnaughton two points:

1. Why can't we move addActivityForPayment in Payment BAO after making some additional changes in it, because its only called by Contribution::recordAdditionalPayment in entire codebase. And then we will no longer need to have new fn recordPaymentActivity ?
2. I think its ok to create Activity of type Refund or Payment. And rest of the activities are necessary to indicate contribution is completed, receipt is sent after completeOrder is being called.

[eileenmcnaughton]

@monishdeb so in terms of 1 - this was just intended as a small step - not necessarily the final form of the function. I think addActivityForPayment should be re-merged back into recordPaymentActivity & the function cleaned up as the next step

in terms of 2 - I guess my worry is just that it's a behaviour change if we always create activities - & I feel like some wider buy in is needed

[eileenmcnaughton]

@monishdeb shall we merge this - I think your comments were along the lines of 'could go further' - which I agree with but this seems like a step in the right direction

[monishdeb]

umm yeah rightful step, ok I am gonna merge this! As reviewed earlier, nothing changes since then and all the code is safely shipped to this new fn. Merging now.