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

Fix separate payment membership test to create valid financial transa… #20436

Merged
merged 1 commit into from
May 28, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 28, 2021

Overview

Fix separate payment membership test to create valid financial transactions

This fixes the membership submit tests in this class to be submitting price set values that are valid for the form rather than doing weird test-specific overrides.

I also did a lot of standardisation & cleanup

Before

The various tests to submit the contribution page with membershps were forcing amounts through that didn't relate to the actual price set options selected (or not). As a result efforts to ensure line items, tax and payments added up to the total were failing.
I'd already done a big cleanup but still

image

After

Consistent and valid values used

image

Technical Details

There is no actual change to what is tested. The only changes outside tests are really in the test harness - although the order function will be useful if we ever get to cleaning up the rest of that form.

Comments

Note the Order class is commented as not supported for use outside of core and all existing uses of it are well covered by tests

@civibot
Copy link

civibot bot commented May 28, 2021

(Standard links)

@@ -2055,10 +2055,6 @@ public static function submit($params) {
$form->controller = new CRM_Contribute_Controller_Contribution();
$params['invoiceID'] = md5(uniqid(rand(), TRUE));

// We want to move away from passing in amount as it is calculated by the actually-submitted params.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is only used by ContributionPage.submit api which is likely only used but the tess

…tions

The issue is inconsistency on the test side, not the core code
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 are you able to merge this so I can re-test the other change?

@seamuslee001
Copy link
Contributor

I did a long look and I'm pretty confident the tests aren't actually changing here so I think this is all good

@seamuslee001 seamuslee001 merged commit ed642f8 into civicrm:master May 28, 2021
@seamuslee001 seamuslee001 deleted the mem_test_fix branch May 28, 2021 04:04
@eileenmcnaughton
Copy link
Contributor Author

thanks @seamuslee001 there is a lot there!

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.

2 participants