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] Move use of priceSetID & amount_override to where they are used #16252

Merged
merged 1 commit into from
Jan 9, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Minor code cleanup

Before

$priceFields & $amount_override defined at the start of the fn - giving the impression they are generic in the function

After

$priceFields removed as not necessary, amount_override moved to where it is used. (note that this is all cheap variable parsing)

Technical Details

The field name is misleading since it implies it is useful in general to pass in a priceSetID - however, it has
a very specific usage & meaning & ideally we would deprecate it in favour of something more readable. For now
group the code more logically & comment it

Comments

@civibot
Copy link

civibot bot commented Jan 9, 2020

(Standard links)

@civibot civibot bot added the master label Jan 9, 2020
The field name is misleading since it implies it is useful in general to pass in a priceSetID - however, it has
a very specific usage & meaning & ideally we would deprecate it in favour of something more readable. For now
group the code more logically & comment it
@mattwire
Copy link
Contributor

mattwire commented Jan 9, 2020

Two things. In theory this reduces performance slightly since we'll now be running this code for every checkbox instead of once per priceset. Also amount_override will be reset to NULL each run (if there's a checkbox) and potentially set to a different value. In practise the performance issue is likely to be trivial and amount_override will always evaluate the same variables.
From experience this code is really hard to work with and cleanup like this really helps later on

@mattwire mattwire merged commit 2fc7d58 into civicrm:master Jan 9, 2020
@eileenmcnaughton eileenmcnaughton deleted the line_items_by branch January 9, 2020 20:58
@eileenmcnaughton
Copy link
Contributor Author

@mattwire the overwrite thing is OK because amount Override is only valid if there is only one line item - so only one iteration. The performance is not doing any queries so should be negligable

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