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.

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

CRM-20432: Change Membership status to current when related contribution payment is recorded from pending status. #11125

Merged
merged 3 commits into from
Oct 16, 2017

Conversation

agilewarealok
Copy link
Contributor

@agilewarealok agilewarealok commented Oct 12, 2017

Overview

Pending Contributions which then have a payment recorded are marked completed but do not trigger the related Membership to become current. Membership status remains pending.

Follow the steps to reproduce the bug:

  1. Using membership type, no price set and default status rules
  2. Add membership to contact, set payment status to pending
  3. Note contribution status is pending
  4. Note membership status is pending
  5. Record payment for the contribution
  6. Note contribution status is now completed
  7. Note membership status is still pending
  8. Bug membership status should be current

Before

Recording payment did not update Membership status to current from Pending.

After

It now works as expected, when a pending contribution payment is recorded we update the Membership status to Current.

Technical Details

We've created a unit test for this to prevent failing the above case.

Agileware Ref: CIVICRM-550


@civicrm-builder
Copy link

Can one of the admins verify this patch?

@monishdeb
Copy link
Member

@agilewarealok I cannot replicate this bug in dmaster, please check my screencast below:
test-1
I believe this was fixed in #11006

@agilewarealok
Copy link
Contributor Author

@monishdeb
Yeah It's working as expected. Actually we had this patch to submit some times back, looks like the bug is fixed.

I've removed the patch which was updating the membership status and kept the Unit test as it is. I think that would be helpful to include.

@monishdeb
Copy link
Member

Absolutely, thanks for adding UT to ensure the fix. Will merge it on test build pass

@monishdeb
Copy link
Member

monishdeb commented Oct 13, 2017

@agilewarealok there is a related test build failure - https://test.civicrm.org/job/CiviCRM-Core-PR/17601/ And I agree with this failure that membership status should be New not Current as calculated from membership's start,end and join dates.

@jusfreeman
Copy link
Contributor

And I agree with this that membership status should be New not Current as calculated from membership's start,end and join dates.

@monishdeb so what is your recommendation here? Is it to fix the #11006 logic as part of this PR?

@monishdeb
Copy link
Member

No the #11006 logic is correct. So I am suggesting that 'Current' should be changed to 'New' here https://github.com/civicrm/civicrm-core/pull/11125/files#diff-ec7ce0ba112f5e88dc1d12b6972e09f5R220.

@agilewarealok
Copy link
Contributor Author

@monishdeb Pushed the code with suggested changes.

@monishdeb
Copy link
Member

@agilewarealok great :) One last thing ..can you rebase your PR to remove the false merge commit - 44210fe

Steps to rebase:

$ git fetch upstream master
$ git pull --rebase upstream master
$ git push -f origin CRM-20432

@agilewarealok
Copy link
Contributor Author

@monishdeb I hope it looks good now :)

@monishdeb
Copy link
Member

Indeed :) merged

@monishdeb monishdeb merged commit 34ea575 into civicrm:master Oct 16, 2017
@monishdeb
Copy link
Member

Thanks for your work @agilewarealok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants