(dev/core/50) Add Separate Sub-tabs for Contributions and Recurring Contributions #11956
Conversation
…utions Changed Contributions Tab template so it uses two sub-tabs within it to show on the first tab, only the contributions table, and on the second tab, just the recurring contributions table. On the Recurring contributions tab, two tables are shown: active recurring contributions vs inactive recurring contributions. Implemented backend logic to load into the page two separate arrays with recurring contributions, the first one with active contributions, the second one with inactive contributions.
CRM/Contribute/Page/Tab.php
Outdated
| * | ||
| * @var array | ||
| */ | ||
| private $inactiveStatuses = array('Cancelled', 'Chargeback', 'Refunded', 'Completed'); |
There was a problem hiding this comment.
This would be better in the ContributionRecur BAO - there may already be something that does this, otherwise would need to create a helper function.
| </div> | ||
| <div id="recurring-subtab" class="ui-tabs-panel ui-widget-content ui-corner-bottom"> | ||
| {if $recur} | ||
| <div class="crm-block crm-contact-contribute-recur"> |
There was a problem hiding this comment.
Can we get additional class selectors on the two div classes here please so they can be uniquely identified for theming/jquery customisation Eg.
<div class="crm-block crm-contact-contribute-recur crm-contact-contribute-recur-active">
<div class="crm-block crm-contact-contribute-recur crm-contact-contribute-recur-inactive">
|
@colemanw do you have an opinion on this? I have a feeling Dgg rejected the same proposed change from Parvez a while ago but I can't think why. Do we need to check support for this change (e.g email dev list & invite people to comment)? |
|
If we do separate it out it would seem weird to do it in the same tpl since other tabs have their own tpl / php classes. The only issue I can think of that might have been Dgg's reason was that the tab should disappear if civicontribute is not enabled (but that seems very fixable) |
MiyaNoctem
changed the title
|
Jenkins re test this please |
|
I think this depends a lot on use-case. Some orgs really care about the difference, others do not. I'm a little worried that users will be surprised by this change and say the recurring contributions have "gone missing" since they are now hidden on initial load. But IMO the ideal UX would probably use a concept more like "filters" than tabs to show just one table but have some buttons at the top to filter out the non-recurring or recurring contributions from display on-the-fly. |
|
Jenkins re test this please |
And added class selectors to each div containing active and inactive recurring contributions.
| @@ -32,6 +32,13 @@ | |||
| */ | |||
| class CRM_Contribute_BAO_ContributionRecur extends CRM_Contribute_DAO_ContributionRecur { | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
@MiyaNoctem can you do git config core.filemode=false to stop committing file permission changes?
(This commit looks like it would be fairly easy to merge by itself it it were in a separate pr - although maybe not the tpl part if it doesn't apply over core by itself)
There was a problem hiding this comment.
File permissions have been fixed.
|
Implemented @mattwire's suggestions, but unsure on how to proceed now. |
|
So the underlying issue is 'does this make sense as a core change' - since it has been 'raised' in a code context we've all jumped into the code without answering that question first. However, we don't have a clear cut process for dealing with how to determine if it was a core change. The move to gitlab & bedding in there has somewhat added to the interim uncertainty. I know this is a source of considerable frustration both for Compucorp & @jusfreeman and also for people trying to figure out how to review these. We should probably figure out the best forum for a discussion on this - probably gitlab with some resulting document. I guess the underlying questions for making code improvements are
In terms of questions 1 & 2 I guess there is an initial triage process whereby someone looks to see if they are clear cut. If not then we need to see if the answer to 3 & 4 is 'no' or 'probably not' & if that seems to be the case then we need a process for canvassing our community. (the triager will not always get it right so an initial 'probably not' is not a guarantee that someone else won't come along & say 'whoa' but that's a side issue). Currently that process is logging an issue in gitlab and emailing the dev list. (Generally I would then close the PR until the 'should we proceed' question is resolved & re-open if it is agreed to be 'yes'). A big gap in this process is what to do if no-one really responds to the proposal & depending on the extent of the change it may be appropriate to deem lack of negative response within a certain timeframe to equal a positive response (assuming the code is also able to be implemented in a way that it has a positive affect on code quality / test coverage or at least not a negative one) or some changes might require a stronger positive response to go in core. I think it's worth considering this one as an extension & if you don't think that makes sense then I think the dev list should be canvassed. I would probably err tend to think this one might fit the no negative given adequate time - e.g 3 weeks) would be enough if people are unresponsive - but the email would also have to have succinctly summarised the impact. Pinging @totten @colemanw @mlutfy @seamuslee001 @seanmadsen @JoeMurray @lcdservices @petednz @agh1 |
|
@eileenmcnaughton Great note!
There was a previous suggestion in the release management process which can potentially help with this issue. That is to form different groups based on people's expertise per CiviCRM component such as event, membership, finance, infrastructure etc. Each group can then regularly check the proposals that are labelled with relevant component. I think it is probably worth it to send these to the dev list or merger room even for a concrete process discussion. |
|
Regarding this PR itself. It is only a partial of the improvement. We are also working on a follow up improvement associated with this to show the contributions that are related to a recurring contribution in view/ edit recurring contribution screens. #11920 |
|
I don't have an issue with the change. A few comments though:
note -- I didn't review code. this is just a comment on the proposed functionality. but I have no objections to it. |
|
@guanhuan "to form different groups based on people's expertise per CiviCRM component such as event, membership, finance, infrastructure etc" - it's a good idea - although the gap is that some functionality is just obscure - e.g @jusfreeman has an interest in improving event copy functionality - no-one else seems to have an opinion at all on that area of code, until it's committed & they want to know who moved their cheese :-) We also get a gap between people who say they are interested & who actually engage - but still the current process of me trying to think of who might be prepared to thoughtfully engage on a particular code area without ALWAYS pinging the same people in case I burn them out, is a bit fraught too |
|
@guanhuan regarding "I think it is probably worth it to send these to the dev list or merger room even for a concrete process discussion." - yep can discuss somewhere like that - at some point some longer version of the above should be in document & shared via dev-list and / or blog |
|
Personally I think this is a nice change. I've always thought that it would be nice if the contributions tab could be rewritten to use datatables, enable sorting on the recurring contributions and implement a view similar to the "Manage Groups" where you can expand/collapse contributions grouped by recurring contribution which I realise is somewhat more than this PR. |
|
Note that @jusfreeman has opened this for process discussion https://lab.civicrm.org/dev/core/issues/97 @guanhuan based on above (& pending @colemanw confirmation) I would say there is support for this & I would be inclined to say it should be a case where you email dev list & say that it will be merged (pending final reviewer confirmation) if people do not express objections on this PR - ie. (@colemanw correct me if I'm wrong) - this feels like it is at the level where some endorsements - which we have - plus lack of a strong negative case would be enough - provided a suitable email goes out & people have at least 10 days to re-act to it. - I do need @colemanw to concur on my take here though as he has raised some concerns |
|
Could you please clarify that you are not moving the Recurring Contributions - but the Recurring Series into a new tab, correct? And that any and all Contributions (regardless of whether they were one-time; attached to a series; or a one time with token (card on file from a recurring series) -> remain in the Contributions tab - and are all visible on first load? With many PRs - we typically bounce around the dev list for opinions and endorsements - about what we all feel is best. I'd like to propose that we start doing a better job of asking clients who heavily use the functionality we're discussing. We have one client who manages close to 6,000 recurring contributions per month; If this is an improvement - they should be excited (and I think they will be); but if not - lets hear their concerns. Not only would we get fantastic feedback [there are some high-end professional people working for some of the larger non-profits] - I'm convinced that engaging our larger clients better is going to help us with credibility and therefore sustainability; Let me know if you'd like me to ask for a large client opinion on this one. |
|
Hi folks, I've given a few thumbs up to responses here - I absolutely agree with @KarinG with regards to clarifying what will be shown on which tab - as a non-technical "expert" user, I've always found the term "Recurring Contribution" confusing - it's difficult to tell if one is talking about an installment of a recurring series or about the recurring series itself. Auth.net and PayPal have views that show recurring subscriptions/ARB details separately from transactions and it is very clear that the purpose of these views is to manage the subscriptions. If it is clear that the "Recurring Contributions" tab displays metadata about recurring subscriptions for management purposes (as Karin indicates), I think this feature would be very useful. I also agree with @KarinG with regards to getting client opinion first before adding to core, but IMO, I think layout changes should always be optional (i.e. in the form of an extension, albeit included with core) - much like Google gives you the option to switch between classic and new views when implementing major changes to Gmail. Alternatively, to help determine if a new view should be made permanent, perhaps usage stats can be gathered from Civi instances either by subliminally collecting the data (or by polling the users outright if that is too big brother). E.g., after a version update, an Admin-level user visits the Contributions tab of a given contact where an invitational message to "Try the new view" will be displayed with Yes/No option selection required to dismiss the message:
Thanks for the time and thought everyone has put into this feature and @eileenmcnaughton, you can always count on me, but you'll have to ping me directly to get my attention (e.g. @nganivet sent me an email to review and weigh in on this). My two cents... |
|
@KarinG Great comment have reposted to https://lab.civicrm.org/dev/core/issues/97#note_4604 |
|
Thanks @tamarmeir - I guess with regards to 'layout changes should always be optional' - there is really a threshold issue. For example I recently merged a change that will add a block to the bottom of the 'View Recurring Contribution' form that shows the contributions associated with that recurring contribution. This is entirely additional information and it is highly consistent with what we have done elsewhere in terms of showing blocks - e.g associated contributions on membership view screen etc. To my mind that is an uncontroversial change but it IS a layout change. The question is whether THIS change crosses the threshold of one that should be optional, given that there are tradeoffs of making it optional. If we decide this should be an optional change realistically that means 'do it in an extension' - although you suggest that it should be included in core there is extra resource involved in that which I don't believe will emerge for this particular improvement (ie. once it is an extension there will be little motivation on anyone's part to make it ship with core), so IF this is a change that people agree is an improvement without notable detriment that we want to be maintaining it makes sense to merge it. If it seems more like 'something that will be of limited interest even for those who use recurring contributions, or possibly detrimental' and / or it adds an unreasonable maintenance burden on the core team or a performance hit (I don't think it does but worth testing) it makes sense to say it should be an extension. This PR is probably right on that threshold, as I think does address a usability issue that many have experienced and have tried to change in the past and that is currently not very well done in core, which is why I struggled to make a call on it & why we have tried to involve others. @KarinG of course you should ask your customer - when we are trying to evaluate a change partners should definitely contact anyone that they think has a worthwhile input on it. |
|
+1 to partners involving clients. |
|
I think we should percolate discussion on this for another 7-10 days & then do an emoji poll on the PR 3 options would be a vote for, a vote against & I'm strongly opposed to this & wish to veto it The point of the 3rd option is not for general principles but it would mean 'I really think this is a negative change from my org's pov & I can explain why'. An example of this from my point of view would be there is a change that was introduced a while back (before the 4.7 series) that is mostly unnoticeable to sites who don't care about the feature - unless they have a big database & high volume. If that came up now I would want the 'this really hurts us' vote to have more weight than a bunch of 'soft +1s' |
|
Just noting that I deployed this on our staging site and showed it to our Donor services Manager to get his thoughts. The performance of the change was fine (which is important for us as we have more than 1 million contributions that have been received as part of a recurring series). The feedback was a soft minus 1 - ie. he actually preferred the scroll down over the extra click but felt it would not adversely affect Donor Services. |
|
As mentioned previously I think it's time for an emoji poll with the following emojis
Most of the people who have provided input have commented above & should see this but @jaapjansma I think you only looped in by email. I'll leave it for a week & then see where the votes lie - since this one is right on the border per my comments above I think this is a good enough process. |
|
I'm going to merge this based on where the votes fell. I would like to see a follow up to add a count to the subtab so people don't need to click there to figure it out. As noted previously I think this one was really right on the border of whether it should be accepted for core or not - per all the discussion - @MiyaNoctem @guanhuan can we move to a process of agreeing things in gitlab before there is a PR in future. That will involve us finalising @jusfreeman's proposal |
|
@eileenmcnaughton thanks for following through this one. We will advise the team to follow the new proposed process and we will move to creating the counter on the subtab shortly. |
Overview
Currently, when viewing contributions on a contact's summary view, there are two tables being shown: one for contributions, the second for recurring contributions. The problem is when a contact has a lot of contributions, recurring contributions kind of get lost within the page, having to scroll down quite a bit to get to the required information. We'd like to have two sub-tabs within the contributions tab, so that we can choose either contributions or recurring contributions as needed. Furthermore, we'd also like to separate active from inactive recurring contributions within the recurring contributions tab, to have easier access to the information that is most likely to be needed.
Before
When viewing contributions on a contact's summary view, two tables being shown: one for contributions, the second for recurring contributions.
After
Changed Contributions Tab template so it uses two sub-tabs within it to show on the first tab, only the contributions table, and on the second tab, just the recurring contributions table.
On the Recurring contributions tab, two tables are shown: active recurring contributions vs inactive recurring contributions.
Implemented backend logic to load into the page two separate arrays with recurring contributions, the first one with active contributions, the second one with inactive contributions.
Issue reported on gitlab issue tracker:
https://lab.civicrm.org/dev/core/issues/50