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

Remove use of overrideDefaultCurrency method from eventInfo page. #22802

Merged
merged 1 commit into from
Mar 9, 2022

Conversation

braders
Copy link
Contributor

@braders braders commented Feb 20, 2022

Overview

Updates the civicrm/event/info route to show the current currency in multi-currencies setups.

Before

As discussed in https://lab.civicrm.org/dev/core/-/issues/2930#note_66911, CRM_Contribute_BAO_Contribution_Utils::overrideDefaultCurrency does not work correctly with the new money functions recently introduced in 386fe6c and d6d93aa.

Whilst there is a good argument for updating CRM_Contribute_BAO_Contribution_Utils::overrideDefaultCurrency to work with the new functions for backwards compatiability, I think moving away from overriding the default currency is a cleaner long-term solution.

After

The overrideDefaultCurrency method is no longer used on the eventInfo page, and the correct currency is used, even if the event and default currencies are different.

The getTaxLabel method has been updated to accept a new optional currency argument, in order to facilitate this change.

Comments

I've kept this PR minimal to start, and so have just focussed on this single eventInfo page - other pages can be handled through follow up PRs.

@civibot
Copy link

civibot bot commented Feb 20, 2022

(Standard links)

@civibot civibot bot added the master label Feb 20, 2022
@braders braders force-pushed the feature/eventinfo-currency branch from 13b4d4e to 9062791 Compare February 20, 2022 14:28
@mattwire mattwire merged commit 0d1b338 into civicrm:master Mar 9, 2022
@mattwire
Copy link
Contributor

mattwire commented Mar 9, 2022

Hi @braders the overrideDefaultCurrency causes various problems. Good to see it being phased out!

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