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

Follow up on #12611 - adding in three data-integrity assertions. #12629

Merged
merged 1 commit into from
Aug 7, 2018
Merged

Follow up on #12611 - adding in three data-integrity assertions. #12629

merged 1 commit into from
Aug 7, 2018

Conversation

KarinG
Copy link
Contributor

@KarinG KarinG commented Aug 7, 2018

Overview

#12611 merged in three assertions (commented out as they failed by $5 for unknown reasons at that time). I committed to getting to the bottom of why the GUI produced $5 different results compared to the phpunit test.

Before

Assertions would fail when commented in

After

Assertions now pass when commented in

Technical Details

I ran the entire class CRM_Member_Form_MembershipTest - and all tests pass. Letting Jenkins have a go at it next.

After spending most of today in CRM_Contribute_Form_Contribution submit - it finally dawned on me that the phpunit tests simulate an admin created Price Set -> so is_quick_config must be set to => 0. If it is not set to => 0 then transact essentially creates the civicrm_line_item from the Contribution data and that's off by $5 (= tax_amount).

Todo next: I will commit to try add a similar phpunit test for Edits on Participant registrations with LineItems.

Todo longer term: CRM_Contribute_Form_Contribution submit is a very scary place... having more tests like the ones added here - may allow some much needed refactoring in future.

@civibot
Copy link

civibot bot commented Aug 7, 2018

(Standard links)

@KarinG
Copy link
Contributor Author

KarinG commented Aug 7, 2018

Pinging in those that commented on #12611 - @eileenmcnaughton @mlutfy @JoeMurray

@eileenmcnaughton
Copy link
Contributor

@KarinG despite the urgent and pressing pain in my brain I feel just reading this I think it's a step in the right direction. I'm gonna add merge on pass.

Note that you should check CRM_Contribute_Form_ContributionTest for existing tests on the scary place that is CRM_Contribute_Form_Contribution submit

@KarinG
Copy link
Contributor Author

KarinG commented Aug 7, 2018

Right - so I did not need to make any edits in CRM_Contribute_Form_Contribution submit itself - I ended up fixing how we create the Price Set in that phpunit test that I was looking at (and then I tested all in class CRM_Member_Form_MembershipTest - just to ensure I had not accidentally affected any other test that was perhaps using that Price Set).

@eileenmcnaughton
Copy link
Contributor

test fail unrelated

@eileenmcnaughton eileenmcnaughton merged commit dcef77f into civicrm:master Aug 7, 2018
@KarinG
Copy link
Contributor Author

KarinG commented Aug 7, 2018

Jenkins, test this please

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

Successfully merging this pull request may close these issues.

3 participants