-
-
Notifications
You must be signed in to change notification settings - Fork 825
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
dev/core#38 Show Recurring Contributions on Membership Modal View #11903
dev/core#38 Show Recurring Contributions on Membership Modal View #11903
Conversation
5377084
to
82ea345
Compare
CRM/Member/BAO/Membership.php
Outdated
* @return array | ||
*/ | ||
public static function getRecurContributions($membershipID) { | ||
$query = " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no API w could use here? Also i don't see contribution_count
being used om the $recurringContributions
array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could use chained API calls, but other than that, I don't think there is a way to get recurring contributions tied to a membership. I'll remove the contribution_count
from the query, though.
CRM/Member/Form/MembershipView.php
Outdated
* | ||
* @var int | ||
*/ | ||
private $_membershipID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we've been moving away from the preceding _ as a matter of style
@mattwire are you able/ willng to review this. My main concern is that I'm pretty sure existing code blocks / functions do much the same as the new ones but I've only skimmed it. I think the idea of adding the block is good in principle. Re whether a unit test is required I think that if we do add these big new functions they should be tested. |
@MiyaNoctem From the screenshots it looks like you developed this before the link was added to "View recurring contribution" in 4.7.31: So I'm not sure if we need to display the full details of the recurring contribution here as well, but I don't think it is a problem if we do either! I don't think we should be touching the BAO class to make form changes and we should be using the API. You can get the recurring contribution ID using:
But contribution_recur_id is already available here and could be used to retrieve details of the recurring contribution for display on the form:
This maybe a useful reference: #11697 |
Speaking just to the question of whether it still makes sense given the addition of view recurring - I think that's ok but it suggests maybe we should be looking at an ajax-addable block that can also be used on the edit page??? (I'm just thinking that's in line with the payments blocks we use elsewhere) |
@mattwire,
Maybe I could create a new API call for this (ie. return al recurring contributions used to pay for a membership)? |
82ea345
to
26f0d17
Compare
So you are basically trying to do something like
|
The API call you suggested kind of works, with a little preprocessing to filter out duplicates. I also moved the code away from the Membership BAO, as @mattwire suggested. Let me know what you guys think. |
Jenkins re test this please |
I'll take a closer look - but I would still want it separate to that form - probably pulled in by ajax - so it can be re-used in other places - e.g the edit form |
Do you have an example I could follow on how to implement loading the table with ajax? Thanks! |
06f2fdb
to
487c45e
Compare
Jenkins re test this please |
Jenkins re test this please |
1 similar comment
Jenkins re test this please |
Looking promisting I think it would be better to more the recurring contributions block to it's own class / own tpl with a menu listing in an xml file. I would be inclined to load by ajax - but @colemanw should be the person to answer the best way to pull it in. To my mind the test to whether it's done the right way should be how easy it would be to add that block to the edit form. At the moment it's part of the view form so it would need refactoring to add to the edit form |
Hi @colemanw, would you please advise on this one too? Thanks! |
Jenkins re test this please |
487c45e
to
6de6b1e
Compare
@guanhuan the question of ajax adding is one thing - but my minimum is that it is done is a re-usable way (if not via ajax the function to add the block should be in it's own class & it's own tpl) - I would suggest adding it both the edit & the view forms as a 'proof' that it is done in a re-usable way - and also because it would be generally useful to have in both places & consistent with other edit pages that show blocks of related entities |
@eileenmcnaughton got it. Sorry I misunderstood your comment earlier. That makes sense. @MiyaNoctem Please see how it is done for "related contributions" and we should make the "related recurring contributions" available in edit modal as well for consistency. I have attached some screenshots. |
@guanhuan, I originally implemented the list following how it was done for "Related Contributions" (ie. load the contributions in the CRM_Member_Form_MembershipView class and send them to the same template used on contributions tab), but I think this is the behaviour @eileenmcnaughton wants us to refrain from using. That being said, I've implemented a new class and template to show the recurring contributions, added a path to Contribute xml and I've loaded the list with an ajax call. Just waiting for Jenkins now... |
@MiyaNoctem the reason I'm not keen on adding another block in an existing class that is not really part of that class per se is that I have reviewed a LOT of PRs that then extract it into a shared class to display in more than one place - so I wanted to start from the assumption it might be used in more than one place |
Understood @eileenmcnaughton, I'll take that into account for any future collaborations. Do you think this is ready to be merged now? |
@MiyaNoctem I tested this & I think it is ready to be merged if you could please squash into one patch & review one proposal - marked inline |
private function getRecurContributions($membershipID) { | ||
$result = civicrm_api3('MembershipPayment', 'get', array( | ||
'sequential' => 1, | ||
'return' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to set limit here - also you can filter on recur here too - ie
'contribution_id.contribution_recur_id.id' => ['IS NOT NULL' => TRUE],
'options' => ['limit' => 0],
$noRecurringContribution = empty($recurringContributionID); | ||
$recurringAlreadyProcessed = isset($recurringContributions[$recurringContributionID]); | ||
|
||
if ($noRecurringContribution || $recurringAlreadyProcessed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the line above you wouldn't get any $noRecurringContribution
f1eeafa
to
6e3ba0b
Compare
@eileenmcnaughton, I've squashed the commits and implemented the changes you suggested... thanks for that. So are we ready now? |
Currently, when viewing a membership from contact's detailed view (on memberships tab), it is hard to tell if a membership has any recurring contributions associated to it, even though you can see all payments done for the membership. Added a recurring contributions section below the contributions table currently being shown, by loading recurring contributions using ajax call to load list of recurring contributions. Added recurring contributionslist to membership edit view using ajax call.
6e3ba0b
to
669fc25
Compare
I think so! |
Overview
Currently, when viewing a membership from contact's detailed view (on memberships tab), it is hard to tell if a membership has any recurring contributions associated to it, even though you can see all payments done for the membership. It would be good if you could see both payments and recurring contributions associated to the membership, similar to how both are shown on Contact's contribution tab.
Before
Only payment contributions were shown on membership modal view.
After
After the table with contributions done for the membership, a new table is shown, with recurring contributions ssociated to the membership.
Technical Details
The same template used on Contact's Contributions tab is used to ensure the same table structure is used for the contributions shown on membership's view.
Comments
Issue was reported on gitlab:
https://lab.civicrm.org/dev/core/issues/38