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/core#2717 Use Same order ->payment flow for non recurring back of… #20936

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jul 23, 2021

Overview

dev/core#2717 Use Same order ->payment flow for non recurring back office memberships as for recurrings

Before

If a recurring contribution is created we use the order api, otherwise magic custom code

After

Consistent use of the order api

Technical Details

Note - I'm expecting a test fail on the first run of this - if there isn't one it might mean that there is a missing piece of test cover as I think doing this change had been blocked by not storing membership_num_terms in the line item table (it never worked for recurrings for mulitiple terms iirc

Comments

@civibot
Copy link

civibot bot commented Jul 23, 2021

(Standard links)

@civibot civibot bot added the master label Jul 23, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the mem_order branch 2 times, most recently from 2876cfb to 87725dc Compare July 23, 2021 05:15
@seamuslee001
Copy link
Contributor

@eileenmcnaughton

Form.php:579, After, Priority: High
Expected 1 blank line after function; 2 found

@eileenmcnaughton eileenmcnaughton force-pushed the mem_order branch 8 times, most recently from d7576a6 to 03d9bc3 Compare July 27, 2021 19:47
@eileenmcnaughton
Copy link
Contributor Author

I think the test fails here might actually be 'good' - but I want to get the pre-requisite merged #20941

@eileenmcnaughton eileenmcnaughton force-pushed the mem_order branch 4 times, most recently from 4eef168 to 8e49c13 Compare August 2, 2021 06:21
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb I just rebased the one you just merged out of this - if you look with no whitespace https://github.com/civicrm/civicrm-core/pull/20936/files?w=1 this is less than it looks - but it alters it to do Order->payment.create for non-recurring card payments.

I'll follow this up with a switch to using the v4 membership api (which I'll add) from within the Order.create api - but I wanted to get this test-cover extension first

@monishdeb
Copy link
Member

I have tested on my local, tried different use-cases which touch the code where Order API call is introduced and didn't encounter any breakage/regression. Reviewed the patch. Changes look good and make sense. Merging now.

@monishdeb monishdeb merged commit 7379b91 into civicrm:master Aug 12, 2021
@eileenmcnaughton
Copy link
Contributor Author

Thanks @monishdeb !

@eileenmcnaughton eileenmcnaughton deleted the mem_order branch August 12, 2021 20:31
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