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

[REF] Cleanup code to determine financial_type_id #19756

Merged
merged 1 commit into from
Mar 11, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 8, 2021

Overview

[REF] Cleanup code to determine financial_type_id

Before

financial_type_id is hard to track as it is passed from array to array & calculated twice

After

retrieved via a consistent function that uses a utils class to retrieve the cached information.

Test cover

Technical Details

Financial type id is really simple on this form - it's either required or
it can be determined form the price set.

However, the code passes the financial_type_id from array to array, calculating
it in more than one place which rather hides the underlying simplicity of it.

This retrieves it using a function that does the same from anywhere in the code.

Note that if someone tried to add it before this->order is built it would
hard fail & kill a bunch of tests. this->order is built at the start of the
submit routine

Comments

@civibot
Copy link

civibot bot commented Mar 8, 2021

(Standard links)

Financial type id is really simple on this form - it's either required or
it can be determined form the price set.

However, the code passes the financial_type_id from array to array, calculating
it in more than one place which rather hides the underlying simplicity of it.

This retrieves it using a function that does the same from anywhere in the code.

Note that if someone tried to add it before this->order is built it would
hard fail & kill a bunch of tests. this->order is built at the start of the
submit routine
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb are you able to merge this - I think it's fairly straight forward/ covered by the test

@monishdeb
Copy link
Member

Patch looks good. Tested on local.

@monishdeb monishdeb merged commit dfaeeac into civicrm:master Mar 11, 2021
@eileenmcnaughton eileenmcnaughton deleted the mem2 branch March 11, 2021 19:26
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.

2 participants