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

Assign totalTaxAmount more consistently #23038

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Assign totalTaxAmount more consistently

@civibot
Copy link

civibot bot commented Mar 25, 2022

(Standard links)

@civibot civibot bot added the master label Mar 25, 2022
@eileenmcnaughton
Copy link
Contributor Author

@colemanw @seamuslee001 @demeritcowboy should be an easy merge

if ($invoicing) {
$form->assign('totalTaxAmount', $totalTaxAmount);
}
$form->assign('totalTaxAmount', Civi::settings()->get('invoicing') ? ($totalTaxAmount ?? NULL) : NULL);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think totalTaxAmount would always be set to zero so the ($totalTaxAmount ?? NULL) is proably redundant actually (since it would only kick in if it actually were null

@colemanw
Copy link
Member

colemanw commented Apr 4, 2022

@eileenmcnaughton can you explain why in the diff it originally called

Civi::settings()->get('contribution_invoice_settings')

But now it calls

Civi::settings()->get('invoicing')

What's the difference?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw contribution_invoice_settings was a REALLY dumb idea. They created a setting that was an array of settings that had no metadata. Quite a long time ago we separated into separate settings but there is still a wrapper to allow access of the no-longer-existing setting. Once we no longer refer to it in core we can deprecate that wrapper

@colemanw colemanw merged commit dead29a into civicrm:master Apr 6, 2022
@colemanw colemanw deleted the assign3 branch April 6, 2022 00:30
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