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

Check for membership type fee before applying tax #19007

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

alexymik
Copy link
Contributor

@alexymik alexymik commented Nov 20, 2020

Overview

Membership Types have an optional minimum fee which cause a PHP notice when no minimum is set and tax is applied.

Before

minimum_fee is used directly and can be unset.

After

Check for minimum_fee using array_key_exists() before using.

Technical Details

In CRM_Member_BAO_MembershipType::getAllMembershipTypes(), minimum_fee can be unset which causes a

Notice: Undefined index: minimum_fee

When trying to calculate minimum_fee_with_tax.

@civibot
Copy link

civibot bot commented Nov 20, 2020

(Standard links)

@civibot civibot bot added the master label Nov 20, 2020
@eileenmcnaughton
Copy link
Contributor

@alexymik thanks for this - I have a feeling it might be better to set minimum_fee_with_tax to 0 rather than leave unset since that would mean functions that don't use this out put don't need to handled it not being set.

Or maybe we should ensure minimum_fee is 0 if not set in the function this calls.

@alexymik
Copy link
Contributor Author

alexymik commented Nov 22, 2020

I think the Membership Type API call should always return minimum_fee, with 0 for an unset minimum, but for now I've made the change you suggested.

@eileenmcnaughton
Copy link
Contributor

@alexymik thanks - I'm happy to merge this if we can get the style issue sorted - it's just a trailing white space

@alexymik
Copy link
Contributor Author

Sorry about that, web editor mishap.

@eileenmcnaughton
Copy link
Contributor

@alexymik one last request - can you squah your commits into a single commit?

@eileenmcnaughton
Copy link
Contributor

Thanks!

@seamuslee001 seamuslee001 merged commit a7077eb into civicrm:master Nov 23, 2020
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