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

dev/membership/#16 Fix duplicate lineitems when creating a recurring membership via backend #15334

Merged
merged 1 commit into from
Sep 23, 2019

Conversation

mattwire
Copy link
Contributor

Overview

See https://lab.civicrm.org/dev/membership/issues/16

Before

Cannot create new recurring membership via credit card via backend

After

Can

Technical Details

We just need to update the check for a contribution ID.

Comments

@MegaphoneJon Please can you test?

@civibot
Copy link

civibot bot commented Sep 19, 2019

(Standard links)

@civibot civibot bot added the master label Sep 19, 2019
@MegaphoneJon
Copy link
Contributor

I just did an r-run and can confirm that this fixes the problem. I don't have a super-strong sense of this part of the code, but the change looks sensible from what I understand.

@seamuslee001
Copy link
Contributor

@mattwire should we put this against the RC?

@MegaphoneJon
Copy link
Contributor

@seamuslee001 IMO definitely, since this is a regression from 5.16.

@mattwire mattwire changed the base branch from master to 5.18 September 20, 2019 13:01
@civibot civibot bot added 5.18 and removed master labels Sep 20, 2019
@mattwire mattwire force-pushed the membership16_fixdupelineitems branch from 6b757e0 to c80e665 Compare September 20, 2019 13:02
@mattwire
Copy link
Contributor Author

@MegaphoneJon @seamuslee001 Rebased against 5.18 RC @eileenmcnaughton are you able to take a quick look at this fix for the regression?

@seamuslee001
Copy link
Contributor

@mattwire, Eileen has been away i think and not sure when she will come back

@mattwire
Copy link
Contributor Author

@seamuslee001 Ok, I thought it would be good to get a second opinion from someone who's familiar with the cleanup/refactoring we're doing in this area. That said, I'm happy with the patch, it's simple and clear. It's also been tested by @MegaphoneJon so your call if you'd like to merge :-)

@seamuslee001
Copy link
Contributor

Given Jon's testing i'm going to merge this and if @eileenmcnaughton wants something else she can make the code changes

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.

3 participants