CRM-21784 Display custom data when viewing recurring contributions #11697

mattwire · 2018-02-20T17:43:48Z

Overview

You can add custom data fields to recurring contributions easily via the custom data admin UI. However, it's not possible to view them anywhere except via the API. This PR fixes that so they show up when viewing the recurring contribution.

Before

Custom data not visible.

After

Custom data visible, using the same method as for other entities.

Note: processor column is no longer visible and has been moved to a separate issue: https://lab.civicrm.org/dev/financial/issues/4

Technical Details

This PR does 3 things:

Switch the data source for the recurring contributions on the contribution tab to use the API instead of BAO/DAO functions. This simplifies the code and makes all fields available to smarty so the tab can be easily customised by extensions (eg. to highlight ones with a particular custom field value). This also has the side-benefit of deprecating a BAO function that is only used once.
Switch the data source for the "View recurring contribution" page to use the API instead of BAO/DAO functions. This simplifies the code and makes all fields available so that the standard "view custom data" functions and smarty tpl can automatically display custom data in the standard format. This also makes "financial_type" visible at all times even if it cannot be changed - it makes sense to stop it being edited but I don't think it makes sense to hide it from view by default as well.
Modify the "View recurring contribution" page smarty template to add in the standard custom data template and allow custom data to magically appear.

CRM-21784: View custom data for recurring contributions

eileenmcnaughton · 2018-02-20T21:01:16Z

@guanhuan looks like something you would have an interest in - would you like to assign a reviewer

guanhuan · 2018-02-26T10:38:53Z

@eileenmcnaughton thanks for mentioning! I was on holiday last week but will assign someone to review this soon.

guanhuan · 2018-02-26T17:56:44Z

@omarabuhussein please review this

omarabuhussein · 2018-02-28T18:39:06Z

So after reading the technical details part ( I didn't checked the code yet) :

1- everything about the ticket indicate that it is about showing custom fields, but you only made another change which show the processor field in the rec contributions table , I don't think it is a good idea to do that and it is better to move that to a different PR even if it is just a small change.

2- I am not sure about switching to use APIs instead of BAO/DAO methods, I personally disagree with this concept, But I know this is something we discussed before with other people (including the core team) but still not sure if the core team reached to a conclusion about that.

BTW, this sentence caught my attention :

This also has the side-benefit of deprecating a BAO function that is only used once.

I don't think for a function to be used once is a bad thing, reusing the code is not the only reason for creating new functions, another reason is to make the code more readable and clear.

@mattwire

omarabuhussein · 2018-02-28T19:04:14Z

So I just took a quick look on the code and I think I need to wait a bit until I have an answers for my questions above.

eileenmcnaughton · 2018-02-28T23:02:40Z

Regarding the deprecated function - I think that is a good move to deprecate that function & I think the usage of the api in this generally in line with what we have been doing in the code.

I agree that the addition of the processor field could be contentious & should be a separate issue - in general there is a desire to make those fields profile-configurable across all the displays and I don't think it's just a minor change on this PR. @mattwire I think you should move that out of this PR.

Note that other than code review this PR will require someone to confirm they have UI tested the change & the UI change is appropriate before it can be merged. I mention this because there are a few PRs which have stalled because people have done extension review of code style but not given the code a UI test or filled the review template & I've been left feeling unable to merge them despite a lot of work having gone into it.

omarabuhussein · 2018-03-01T11:51:23Z

Regarding the deprecated function - I think that is a good move to deprecate that function & I think the usage of the api in this generally in line with what we have been doing in the code.

if this is the case then I will review based on that then.

Note that other than code review this PR will require someone to confirm they have UI tested the change & the UI change is appropriate before it can be merged. I mention this because there are a few PRs which have stalled because people have done extension review of code style but not given the code a UI test or filled the review template & I've been left feeling unable to merge them despite a lot of work having gone into it.

I will do the UI testing.

omarabuhussein

Left few notes and questions, Beside that just one thing, Can you prepend the commit messages with the ticket number. that's beside moving "showing the payment processor" to another PR. I will do Manual testing after everything is fine,

omarabuhussein · 2018-03-01T16:56:22Z

CRM/Contribute/Page/Tab.php

        // no action allowed if it's not active
-        $params[$ids]['is_active'] = ($recur['contribution_status_id'] != 3);
+        $recurContributions[$recurId]['is_active'] = (!CRM_Contribute_BAO_Contribution::isContributionStatusNegative($recurDetail['contribution_status_id']));


maybe this is more readable :

$statusID = $recurDetail['contribution_status_id']; $recurContributions[$recurId]['is_active'] = !(CRM_Contribute_BAO_Contribution::isContributionStatusNegative($statusID));

omarabuhussein · 2018-03-01T17:06:14Z

CRM/Contribute/Page/Tab.php

@@ -131,44 +130,52 @@ public function browse() {
    $controller->run();

    // add recurring block
-    $action = array_sum(array_keys($this->recurLinks()));
-    $params = CRM_Contribute_BAO_ContributionRecur::getRecurContributions($this->_contactId);
+    try {


I know it was already like this, but better to improve it now, all the logic under "add recurring block" can be moved to a new private method with proper name (e.g addRecurringContributionsBlock).

@omarabuhussein Yes that's a good idea. It would be nice to refactor this in future to either create a separate tab, or introduce sortable columns etc. on the existing tab. Moving to the API means this is supported by the code, moving it to it's own function makes any future refactoring easier.

omarabuhussein · 2018-03-01T20:10:15Z

CRM/Financial/BAO/PaymentProcessor.php

+   *
+   * @return null|string
+   */
+  public static function getPaymentProcessorName($paymentProcessorId) {


so if the BAO calls getting replaced by API calls elsewhere above, why to create new BAO method (even if it at the end use the API) instead of calling the API directly, because to be honest I am not clear on the line between when to use the API directly and when to use BAO methods in civicrm core code.

@omarabuhussein @eileenmcnaughton Good question! I wanted to to something consistent (it's possible to have a recurring contribution with no payment processor ID, or an invalid one). I couldn't find a way of doing this via the UI without duplicating around 6 lines of code (which I abstracted into getPaymentProcessorName). Any thoughts?

I guess my position is that it's good to use the api because it's standardised & various hooks (include api-layer) work if you use the api but when there is a re-usable chunk then it makes sense to wrap that in a function. In this case the handling of the unknown situation is an extra bit of handling that the function provides. I would have thought that the FKs on the contribution_recur table would have meant we should not see an invalid processor id - but I'm not opposed to handling the posibility.

CiviCRM is never going to be a purist's codebase :-) which does mean we can't rely on rules as easily as we might in other codebases.

okay then :)

omarabuhussein · 2018-03-01T20:10:59Z

CRM/Financial/BAO/PaymentProcessor.php

+   * @return null|string
+   */
+  public static function getPaymentProcessorName($paymentProcessorId) {
+    // if there is a payment processor ID, get the name of the payment processor


I don't really think this ID check is necessary

@omarabuhussein Ok, removed.

omarabuhussein · 2018-03-01T20:13:29Z

templates/CRM/Contribute/Form/Selector.tpl

@@ -26,6 +26,10 @@
 {include file="CRM/common/pager.tpl" location="top"}

 {strip}
+  <div class="description">
+    {ts}Click arrow to view payment details.{/ts}


I am not a native English speaker so I might be wrong, but isn't it more correct to be :

Click on the arrow to view the payment details.

@omarabuhussein @eileenmcnaughton I'm not sure that this text actually adds any value to the UI and might be better removed altogether?

ohh - I shy away totally at UI decisions. Gramatically I think no 'the' is probably better than with the 'the' - but I lean heavily on people like Andrew Hunt, Stoob, Pete D & Lcdweb for UI decisions - I didn't ping them because I thought pinging all 4 might be overkill :-)

I meant removing the entire description :-) Thoughts @agh1 @lcdservices ?

according to @lcdservices

And at this point, the arrows are ubiquitous throughout the system, so it may not need explanation.

I think I agree, so I think removing the entire description make sense here.

lcdservices · 2018-03-05T13:46:40Z

If we leave the text I would leave as is (no "the").
Re: removing altogether... I'd be fine with that. It's kind of in an awkward place. And at this point, the arrows are ubiquitous throughout the system, so it may not need explanation.

omarabuhussein · 2018-03-07T14:23:44Z

@mattwire so other than removing the message as well as removing the extra field from recurr contribution table and do it separate PR, I think all looks fine now, I will do some UI testing now and let you know here once done.

omarabuhussein · 2018-03-07T15:34:49Z

So just finished testing, I can see that this change allow only for viewing the recurring contribution custom fields but it does not allow to edit them from UI, shouldn't editing be allowed too ?

@mattwire

mattwire · 2018-03-09T12:41:15Z

@omarabuhussein I've added custom data editing now and removed the "Click arrow to view payment details" text. Could you check the editing part and confirm if it is ok?

omarabuhussein · 2018-03-12T11:12:08Z

sure @mattwire

omarabuhussein · 2018-03-12T18:30:47Z

@mattwire editing works fine now , So it is approved from my side. But please update all commit messages to start with ticket number.

@eileenmcnaughton unless you have anything to say, I think this fine to be merged after the commit messages get updated with the ticket number.

…on tab

… details

mattwire · 2018-03-16T16:48:43Z

@omarabuhussein @eileenmcnaughton Ok, updated commit messages and rebased. Good to merge?

omarabuhussein · 2018-03-16T16:52:13Z

from my side yes @mattwire

eileenmcnaughton · 2018-03-18T22:57:19Z

test this please

mattwire · 2018-03-20T11:32:23Z

test this please

eileenmcnaughton · 2018-03-24T02:53:55Z

I'm going to merge this based on @omarabuhussein review & it generally being a good idea to be able to see this custom data. I have some reservations about the implementation because I would like it to be generic enough to apply to 'any' entity - but I don't think it makes sense to block this until that is resolved

dvhirst · 2018-03-26T23:42:24Z

Thanks for this! It is something I've been seeking, and now it's here without my even asking for it. Nice work!

civicrm-builder added the master label Feb 20, 2018

mattwire force-pushed the CRM-21784_recur_customdata branch 2 times, most recently from ec7defe to e5b17ac Compare February 20, 2018 19:36

omarabuhussein suggested changes Mar 1, 2018

View reviewed changes

mattwire force-pushed the CRM-21784_recur_customdata branch from e5b17ac to e65014d Compare March 5, 2018 10:28

mattwire force-pushed the CRM-21784_recur_customdata branch 2 times, most recently from 4221691 to 0993a4f Compare March 9, 2018 12:40

mattwire added 5 commits March 16, 2018 16:47

CRM-21784 Switch to API to get recurring contributions for contributi…

9e475c2

…on tab

CRM-21784 Switch contribution recur view to use API to retrieve recur…

46097c9

… details

CRM-21784 Show custom data on recurring contributions

24eba10

CRM-21784 Clean up contribution tab templates

9806f96

CRM-21784 Support editing custom data via UI for ContributionRecur

0699333

mattwire force-pushed the CRM-21784_recur_customdata branch from 0993a4f to 0699333 Compare March 16, 2018 16:48

eileenmcnaughton merged commit 2fb7a32 into civicrm:master Mar 24, 2018

mattwire mentioned this pull request Mar 31, 2018

dev/core#38 Show Recurring Contributions on Membership Modal View #11903

Merged

mattwire deleted the CRM-21784_recur_customdata branch September 25, 2018 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21784 Display custom data when viewing recurring contributions #11697

CRM-21784 Display custom data when viewing recurring contributions #11697

mattwire commented Feb 20, 2018 •

edited

Loading

eileenmcnaughton commented Feb 20, 2018

guanhuan commented Feb 26, 2018

guanhuan commented Feb 26, 2018

omarabuhussein commented Feb 28, 2018 •

edited

Loading

omarabuhussein commented Feb 28, 2018

eileenmcnaughton commented Feb 28, 2018

omarabuhussein commented Mar 1, 2018

omarabuhussein left a comment •

edited

Loading

omarabuhussein Mar 1, 2018

omarabuhussein Mar 1, 2018

mattwire Mar 5, 2018

omarabuhussein Mar 1, 2018

mattwire Mar 5, 2018

eileenmcnaughton Mar 5, 2018

omarabuhussein Mar 7, 2018

omarabuhussein Mar 1, 2018

mattwire Mar 5, 2018

omarabuhussein Mar 1, 2018

mattwire Mar 5, 2018

eileenmcnaughton Mar 5, 2018

mattwire Mar 5, 2018

omarabuhussein Mar 7, 2018

lcdservices commented Mar 5, 2018

omarabuhussein commented Mar 7, 2018 •

edited

Loading

omarabuhussein commented Mar 7, 2018

mattwire commented Mar 9, 2018

omarabuhussein commented Mar 12, 2018

omarabuhussein commented Mar 12, 2018

mattwire commented Mar 16, 2018

omarabuhussein commented Mar 16, 2018

eileenmcnaughton commented Mar 18, 2018

mattwire commented Mar 20, 2018

eileenmcnaughton commented Mar 24, 2018

dvhirst commented Mar 26, 2018

CRM-21784 Display custom data when viewing recurring contributions #11697

CRM-21784 Display custom data when viewing recurring contributions #11697

Conversation

mattwire commented Feb 20, 2018 • edited Loading

Overview

Before

After

Technical Details

eileenmcnaughton commented Feb 20, 2018

guanhuan commented Feb 26, 2018

guanhuan commented Feb 26, 2018

omarabuhussein commented Feb 28, 2018 • edited Loading

omarabuhussein commented Feb 28, 2018

eileenmcnaughton commented Feb 28, 2018

omarabuhussein commented Mar 1, 2018

omarabuhussein left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcdservices commented Mar 5, 2018

omarabuhussein commented Mar 7, 2018 • edited Loading

omarabuhussein commented Mar 7, 2018

mattwire commented Mar 9, 2018

omarabuhussein commented Mar 12, 2018

omarabuhussein commented Mar 12, 2018

mattwire commented Mar 16, 2018

omarabuhussein commented Mar 16, 2018

eileenmcnaughton commented Mar 18, 2018

mattwire commented Mar 20, 2018

eileenmcnaughton commented Mar 24, 2018

dvhirst commented Mar 26, 2018

mattwire commented Feb 20, 2018 •

edited

Loading

omarabuhussein commented Feb 28, 2018 •

edited

Loading

omarabuhussein left a comment •

edited

Loading

omarabuhussein commented Mar 7, 2018 •

edited

Loading