-
-
Notifications
You must be signed in to change notification settings - Fork 821
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 taxTerm at top level on all forms #31539
base: master
Are you sure you want to change the base?
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
ce99fc9
to
a511509
Compare
@eileenmcnaughton I understand the intent here but worry that putting it at the top-level is a bit of a sledgehammer. We don't seem to have any other similar assigns at this level and I would think that taxTerm sits at the same level as other vars such as language, currency, etc. which aren't currently assigned at this level. If we added taxTerm I imagine there's other vars that should similarly be added. There do seem to be some helper functions in |
Seems like good idea to assign it at top level. I am wondering if it should be part of config object rather than at form level? We have things like currency as a part of config object which is already assigned to the template. |
@kurund if the config object is already assigned wouldn't it already be assigned then? Are settings already magic-assigned that way (much as I hate those magic assigns) |
Sorry, missed your comment. When I checked last time it wasn't part of config object. |
My comments are non-blocking by the way, whatever @eileenmcnaughton and @kurund can agree on :-) |
Overview
A brief description of the pull request. Keep technical jargon to a minimum. Hyperlink relevant discussions.
Before
per #31532 the tax term (e.g VAT, GST) is only available if assigned
After
This, along with probably some other organizational settings, should really always be available to templates as it is frequently required
Technical Details
If this is merged there are other places it could be removed from
We could go with a crmSetting type smarty call - but it might be more disruptive
Comments