Skip to content
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.

Sign up for GitHub

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

Jump to bottom

dev/core#2717 Simplify batch membership renewal #20935

Merged
merged 3 commits into from
Aug 25, 2021
Merged

dev/core#2717 Simplify batch membership renewal #20935

merged 3 commits into from
Aug 25, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 22, 2021 •
edited
Loading

Overview

Simplify batch membership renewal

This builds off #20882 and makes the assumption that we only need 2 code paths in this form

  1. form option is_renew chosen AND a current membership exists => then use the renew code
  2. form option is renew not chosen or no current membership exists => then use the create code.

It removes handling for the (hopefully already unreachable and / or broken)

  • form option is_renew chose and no membership exists
  • for option is_renew chosen but only cancelled or pending memberships exist

Before

A lot of code that was relevant to obscure pathways on the online contribution form (that doesn't have a separate 'new membership' process

After

poof

Technical Details

@monishdeb @kcristiano @jitendrapurohit this is a bit of a bigger change to the batch membership form from a required r-run POV - I'm hoping one of you can help on it

Comments

@civibot
Copy link

civibot bot commented Jul 22, 2021

(Standard links)

@civibot civibot bot added the master label Jul 22, 2021
@eileenmcnaughton eileenmcnaughton changed the title Simplify batch membership renewal dev/core#2717 Simplify batch membership renewal Jul 23, 2021
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb thanks for merging #20882 - I've rebased that out of this now

@kcristiano
Copy link
Member

@eileenmcnaughton I had thought I had commented here. But I must have not.

I did start with an r-run and get the following on batch submission

[13-Aug-2021 09:26:05 America/New_York] PHP Fatal error:  Uncaught TypeError: Return value of CRM_Batch_Form_Entry::legacyProcessMembership() must be an instance of CRM_Member_BAO_Membership, instance of CRM_Member_DAO_Membership returned in /srv/www/buildkit/wpcv/web/wp-content/plugins/civicrm/civicrm/CRM/Batch/Form/Entry.php:1132
Stack trace:
#0 /srv/www/buildkit/wpcv/web/wp-content/plugins/civicrm/civicrm/CRM/Batch/Form/Entry.php(840): CRM_Batch_Form_Entry->legacyProcessMembership('18', '1', Array, 'Batch', Array, Array)
#1 /srv/www/buildkit/wpcv/web/wp-content/plugins/civicrm/civicrm/CRM/Batch/Form/Entry.php(525): CRM_Batch_Form_Entry->processMembership(Array)
#2 /srv/www/buildkit/wpcv/web/wp-content/plugins/civicrm/civicrm/CRM/Core/Form.php(526): CRM_Batch_Form_Entry->postProcess()
#3 /srv/www/buildkit/wpcv/web/wp-content/plugins/civicrm/civicrm/CRM/Core/QuickForm/Action/Upload.php(152): CRM_Core_Form->mainProcess()
#4 /srv/www/buildkit/wpcv/web/wp-content/plugins/civicrm/civicrm/CRM/Core/QuickForm/Action/Upload.php(119): CRM_Core_QuickForm_Action_Upload->realP in /srv/www/buildkit/wpcv/web/wp-content/plugins/civicrm/civicrm/CRM/Batch/Form/Entry.php on line 1132

I'll look further and comment. I want to log what I find as I find it

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano I've fixed ^^

If you are going to do a thorough r-run I could go a bit further with the clean up (the balance of how much to fix depends a bit on whether the reviewer is more code focussed or r-run focussed)

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano ah - now I've refreshed my memory - the renew membership code has 3 paths (it was written for front end forms)

  • the removed one that deals with non-renewals
  • the one I'd like to remove that deals with when is is supposed to be a renewal but they are not a current member
  • the actual renewal

I'm not sure if 2 is reachable - I'll put up a separate PR with it removed & you can tell me if you can figure out the difference.

@eileenmcnaughton eileenmcnaughton mentioned this pull request Aug 13, 2021
Merged
@eileenmcnaughton
Copy link
Contributor Author

OK - here is the PR with the code I'm not sure the reachability of ripped out #21126

@kcristiano
Copy link
Member

@eileenmcnaughton I will look at these over the next few days. Thanks for all the work here.

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano yeah there is quite a bit more to do - but I think there is a big question to answer - how much of the code is actually reachable - which will alter the scope of the next bit.

My goal is to get the code using the Order api

@kcristiano
Copy link
Member

@eileenmcnaughton I've done some more r-run on this one.

The form works, but if you try and rely on the Membership Type to set the end date on new memberships, none is set.

Renewals do extend the membership the number of terms.

I'll also review the alternative Patch.

In additional this shows a regression in the RC - membership Dashboard has a fatal error. I'll open an issue for that.

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano can you outline the steps for "but if you try and rely on the Membership Type to set the end date on new memberships, none is set." - I'll make sure it has test cover & works

@kcristiano
Copy link
Member

More details:

  • Create batch
  • Choose contacts
  • set start date
  • leave end date blank

image

Memberships created without end date

image

mysql sa@(none):wpcvmastercivi_s301l> select id,contact_id,membership_type_id,source,join_date,start_date,end_date from civicrm_membership where source like '%1931%'   
+------+--------------+----------------------+------------+-------------+--------------+------------+
| id   | contact_id   | membership_type_id   | source     | join_date   | start_date   | end_date   |
|------+--------------+----------------------+------------+-------------+--------------+------------|
| 40   | 22           | 1                    | Batch 1931 | 2021-08-15  | 2021-08-15   | <null>     |
| 41   | 169          | 1                    | Batch 1931 | 2021-08-15  | 2021-08-15   | <null>     |
| 42   | 128          | 1                    | Batch 1931 | 2021-08-15  | 2021-08-15   | <null>     |
+------+--------------+----------------------+------------+-------------+--------------+------------+

3 rows in set
Time: 0.007s

@kcristiano
Copy link
Member

For comparison - add via back office - not in batch mode:

image

Results as expected - end date based on Membership Type

image

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano I'm going to dig into this - but I suspect it is unchanged by this PR which I would expect to deal with renewals. This could be a regression though

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano I went back to 5.33 & tested entering a membership without entering the end_date & it was blank back then - potentially we should fix it to set something

@kcristiano
Copy link
Member

Thanks @eileenmcnaughton that makes sense.

@kcristiano
Copy link
Member

@eileenmcnaughton This PR and #21126 now have conflicts. I was satrting to look at 21126 as I expect we want to merge that if possible. This PR is an improvement, so if we want to go in small steps, this looks good to merge once the conflicts are solved.

@eileenmcnaughton
Remove all handling for 'not is renew' from is renew path
c0f6a1d 
This handling is mostly here because the function was 'shared' with multiple forms with different needs.

The expectation is that if the form says to 'renew' we should, with a minor check that the membership
exists
@eileenmcnaughton
Remove unused variables
567e4d7
@eileenmcnaughton
Extract getMembershipParams
6601aa5
@eileenmcnaughton
Copy link
Contributor Author

@kcristiano this is rebased now - are you saying it should be merged now?

@kcristiano
Copy link
Member

@eileenmcnaughton Not yet for merge.

I found one other issue today.

  • Find an expired membership
  • use batch mode to update that contacts memberhip
  • set new expiry date
  • membership status stays expired

image

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano so the expiration date was set in the batch mode - but it didn't update the status (only the date)?

@kcristiano
Copy link
Member

Correct - Running on 5.39 right now to see if the unpatched version works.

@kcristiano
Copy link
Member

kcristiano commented Aug 17, 2021 •
edited
Loading

Just tested on 5.39 and renewing updates expiry date and status for renewed memberships.

@kcristiano
Copy link
Member

I'll also comment on #21126

I just did renewals on that one and the date and ststus upadted, so perhaps we need to focus on that PR as the alternative

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano oh interesting

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 are you ok to merge this - it's a partial of #21126 which @kcristiano has tested & OKd but that one has a test error I need to figure out that doesn't affect this

Then I'll rebase that one & fix the test but it would be easier if this is merged

(note this has changed a bit over time - I just checked & there is just the one commit difference)

@seamuslee001 seamuslee001 merged commit ddffbc4 into civicrm:master Aug 25, 2021
@seamuslee001 seamuslee001 deleted the hhhh branch August 25, 2021 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @kcristiano @seamuslee001