feat(tax): add universal tax fallback feature#277
feat(tax): add universal tax fallback feature#277mahyarrezghi wants to merge 2 commits intoUltimate-Multisite:mainfrom
Conversation
- Add enable_universal_tax setting toggle - Add universal_tax_rate setting (0-100%) - Implement apply_universal_tax_rate filter - Update translations for new strings - Apply universal rate when no country-specific rates match
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds universal fallback tax rate mechanism to the tax system. Introduces new filter hook and method to apply a universal tax rate when no country-specific rates exist. Extends tax settings with admin fields to enable and configure the universal tax rate globally. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
inc/tax/class-tax.php (1)
174-203: New universal tax settings are consistent with behavior; optional style fix for alignment warningThe two new settings fields look coherent with the runtime logic:
enable_universal_taxis a toggle gated onenable_taxes, matching the runtimewu_should_collect_taxes()+enable_universal_taxchecks.universal_tax_rate:
- Numeric 0–100 with a default of 10%, consistent with the fallback default in
apply_universal_tax_rate.- Properly requires both
enable_taxesandenable_universal_tax, so it only surfaces when relevant.- Copy and translation domain match the rest of the file.
The only remaining item is a non-blocking style warning from the code quality checks about double-arrow alignment on the
'enable_taxes'key at line 199. If you want to satisfy that sniff, you can adjust spacing, e.g.:- 'enable_taxes' => 1, - 'enable_universal_tax' => 1, + 'enable_taxes' => 1, + 'enable_universal_tax' => 1,Functionally everything here aligns with the new feature’s intent.
lang/ultimate-multisite.pot (1)
18475-18510: Universal tax strings are clear; consider tiny wording tweakThe new strings for the universal fallback tax feature (“Universal Tax”, “Enable Universal Tax”, “Universal Tax Rate (%)”, and their descriptions) are clear and aligned with the feature’s behavior.
One optional polish point for consistency/grammar between the two descriptions:
- Current:
- “Enable a fallback universal tax rate when no country-specific rates match.”
- “Tax rate applied when no country-specific rate matches.”
Consider updating the first to singular for consistency:
-Enable a fallback universal tax rate when no country-specific rates match. +Enable a fallback universal tax rate when no country-specific rate matches.This would need to be changed in the PHP source (
inc/tax/class-tax.php:179), then the POT regenerated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
inc/tax/class-tax.php(3 hunks)lang/ultimate-multisite.pot(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
inc/tax/class-tax.php (2)
inc/functions/tax.php (1)
wu_should_collect_taxes(20-23)inc/functions/settings.php (2)
wu_get_setting(37-40)wu_register_settings_field(88-91)
🪛 GitHub Check: Code Quality Checks
inc/tax/class-tax.php
[failure] 124-124:
There should be a comma after the last array item in a multi-line array.
[warning] 105-105:
The method parameter $cart is never used
[warning] 105-105:
The method parameter $tax_category is never used
[warning] 105-105:
The method parameter $country is never used
[warning] 199-199:
Array double arrow not aligned correctly; expected 9 space(s) between "'enable_taxes'" and double arrow, but found 5.
🪛 PHPMD (2.15.0)
inc/tax/class-tax.php
105-105: Avoid unused parameters such as '$country'. (undefined)
(UnusedFormalParameter)
105-105: Avoid unused parameters such as '$tax_category'. (undefined)
(UnusedFormalParameter)
105-105: Avoid unused parameters such as '$cart'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: PHP 7.4
- GitHub Check: PHP 8.4
- GitHub Check: cypress (8.1, chrome)
- GitHub Check: cypress (8.2, chrome)
🔇 Additional comments (3)
inc/tax/class-tax.php (1)
35-35: Filter registration for universal tax fallback looks correctHooking
apply_universal_tax_rateintowu_cart_applicable_tax_rateswith priority10and4accepted args matches the described design and keeps the fallback logic centralized in the tax class.lang/ultimate-multisite.pot (2)
12-14: POT header date update is fine
POT-Creation-Datereflects the new extraction time and is consistent with a regenerated POT; no action needed.
3637-3638: Tax-related source reference additions look consistentThe extra references for existing strings like “Tax Rate”, “Taxes”, and “Tax Rates” pointing into
inc/tax/class-tax.phpcorrectly align these labels with the new tax logic and UI, without altering the translatable text. No further changes required here.Also applies to: 4129-4131, 4940-4943, 18515-18551
inc/tax/class-tax.php
Outdated
| /** | ||
| * Applies universal tax rate when no specific rates are found. | ||
| * | ||
| * @since 2.0.0 | ||
| * @param array $rates Tax rates. | ||
| * @param string $country Country code. | ||
| * @param string $tax_category Tax category. | ||
| * @param object $cart Cart object. | ||
| * @return array | ||
| */ | ||
| public function apply_universal_tax_rate(array $rates, string $country, string $tax_category, $cart): array { | ||
|
|
||
| if (empty($rates) && wu_should_collect_taxes() && wu_get_setting('enable_universal_tax', false)) { | ||
| $universal_rate = wu_get_setting('universal_tax_rate', 10); | ||
|
|
||
| $rates = [ | ||
| [ | ||
| 'id' => 'universal-' . uniqid(), | ||
| 'title' => __('Universal Tax', 'ultimate-multisite'), | ||
| 'country' => '*', | ||
| 'state' => '', | ||
| 'city' => '', | ||
| 'tax_type' => 'percentage', | ||
| 'tax_amount' => $universal_rate, | ||
| 'tax_rate' => $universal_rate, | ||
| 'priority' => 1, | ||
| 'compound' => false, | ||
| 'type' => 'regular', | ||
| 'rate' => $universal_rate, | ||
| ] | ||
| ]; | ||
| } | ||
|
|
||
| return $rates; | ||
| } |
There was a problem hiding this comment.
Universal tax fallback logic is sound; fix trailing comma lint + consider unused param warnings
Functionally this method does what the PR describes:
- Only applies when there are no existing rates, taxes should be collected, and universal tax is enabled.
- Pulls the rate from settings (default 10) and builds a rate array consistent with existing tax rate structures (id, title, country wildcard, percentage type, rate fields, priority, etc.).
- Returns the original
$ratesunchanged when the condition is not met.
Two minor points:
- Trailing comma in multiline array (blocks code quality check)
The last element of the inner array ('rate' => $universal_rate) is missing a trailing comma, which the code-quality check flags as a failure. You can fix it like this:
- 'rate' => $universal_rate,
- ]
+ 'rate' => $universal_rate,
+ ],- Unused parameters in the callback
$country,$tax_category, and$cartare intentionally unused but required by the filter signature. I’d keep them for future-proofing and hook compatibility; if PHPMD noise becomes an issue, you could add a@SuppressWarnings(PHPMD.UnusedFormalParameter)annotation on the method docblock rather than changing the signature.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Applies universal tax rate when no specific rates are found. | |
| * | |
| * @since 2.0.0 | |
| * @param array $rates Tax rates. | |
| * @param string $country Country code. | |
| * @param string $tax_category Tax category. | |
| * @param object $cart Cart object. | |
| * @return array | |
| */ | |
| public function apply_universal_tax_rate(array $rates, string $country, string $tax_category, $cart): array { | |
| if (empty($rates) && wu_should_collect_taxes() && wu_get_setting('enable_universal_tax', false)) { | |
| $universal_rate = wu_get_setting('universal_tax_rate', 10); | |
| $rates = [ | |
| [ | |
| 'id' => 'universal-' . uniqid(), | |
| 'title' => __('Universal Tax', 'ultimate-multisite'), | |
| 'country' => '*', | |
| 'state' => '', | |
| 'city' => '', | |
| 'tax_type' => 'percentage', | |
| 'tax_amount' => $universal_rate, | |
| 'tax_rate' => $universal_rate, | |
| 'priority' => 1, | |
| 'compound' => false, | |
| 'type' => 'regular', | |
| 'rate' => $universal_rate, | |
| ] | |
| ]; | |
| } | |
| return $rates; | |
| } | |
| /** | |
| * Applies universal tax rate when no specific rates are found. | |
| * | |
| * @since 2.0.0 | |
| * @param array $rates Tax rates. | |
| * @param string $country Country code. | |
| * @param string $tax_category Tax category. | |
| * @param object $cart Cart object. | |
| * @return array | |
| */ | |
| public function apply_universal_tax_rate(array $rates, string $country, string $tax_category, $cart): array { | |
| if (empty($rates) && wu_should_collect_taxes() && wu_get_setting('enable_universal_tax', false)) { | |
| $universal_rate = wu_get_setting('universal_tax_rate', 10); | |
| $rates = [ | |
| [ | |
| 'id' => 'universal-' . uniqid(), | |
| 'title' => __('Universal Tax', 'ultimate-multisite'), | |
| 'country' => '*', | |
| 'state' => '', | |
| 'city' => '', | |
| 'tax_type' => 'percentage', | |
| 'tax_amount' => $universal_rate, | |
| 'tax_rate' => $universal_rate, | |
| 'priority' => 1, | |
| 'compound' => false, | |
| 'type' => 'regular', | |
| 'rate' => $universal_rate, | |
| ], | |
| ]; | |
| } | |
| return $rates; | |
| } |
🧰 Tools
🪛 GitHub Check: Code Quality Checks
[failure] 124-124:
There should be a comma after the last array item in a multi-line array.
[warning] 105-105:
The method parameter $cart is never used
[warning] 105-105:
The method parameter $tax_category is never used
[warning] 105-105:
The method parameter $country is never used
🪛 PHPMD (2.15.0)
105-105: Avoid unused parameters such as '$country'. (undefined)
(UnusedFormalParameter)
105-105: Avoid unused parameters such as '$tax_category'. (undefined)
(UnusedFormalParameter)
105-105: Avoid unused parameters such as '$cart'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In inc/tax/class-tax.php around lines 95 to 129, the trailing comma is missing
on the last element of the inner rate array and PHPMD may flag unused parameters
($country, $tax_category, $cart); add a trailing comma after the 'rate' =>
$universal_rate array entry to satisfy the linter, and keep the unused
parameters but add a docblock annotation like
@SuppressWarnings(PHPMD.UnusedFormalParameter) to the method docblock to silence
PHPMD warnings while preserving the filter signature.
superdav42
left a comment
superdav42
left a comment
There was a problem hiding this comment.
I think I'd rather implement this by adding a new option to the country drop down, "Apply to all countries" that could be chosen on the tax rates configuration page. It would probably be simpler to implement and wouldn't need a new setting field. Did you consider this approach?
|
My thinking was: since the user has to go to the tax settings to enable it first, they should also be able to enable universal tax rates there, instead of having to go to a separate page. |
I understand but I want it to be in tax rates configuration page. I don't want to add more settings on the settings page unless they are going to be used by most users. A universal tax seems like an advanced option that belongs on the tax rates page. I've never seen any other software use a "universal tax" so it seems like a foreign concept and would confuse many users. |
Makes sense! I'll edit it. |
|
Dispatching worker to address review changes and fix Code Quality CI failure.
|
…tes UI Replace the settings-based universal tax approach with the reviewer's suggested approach: add an 'Apply to all countries' (*) option to the country dropdown on the Tax Rates configuration page. Changes: - views/taxes/list.php: add '*' option at top of country dropdown - inc/functions/tax.php: handle '*' country as fallback in wu_get_applicable_tax_rates() — applies when no exact-country rates match - inc/tax/class-tax.php: remove apply_universal_tax_rate filter/method and remove enable_universal_tax/universal_tax_rate settings fields - lang/ultimate-multisite.pot: remove obsolete strings, add new 'Apply to all countries' string, update line number references This approach is simpler, consistent with how state/city already use blank/wildcard to mean 'apply to all', and requires no new settings. Addresses review feedback from superdav42 in PR Ultimate-Multisite#277.
|
Thanks for the review @superdav42! I've reworked the implementation to follow your suggested approach. What changed: Instead of a settings toggle + separate rate, the universal/fallback tax is now configured directly on the Tax Rates page by selecting "Apply to all countries" from the country dropdown on any tax rate row. Implementation:
This is simpler and consistent with how state/city already use blank to mean "apply to all". No new settings needed. |
|
Closing this PR in favour of a redesigned implementation based on @superdav42's review feedback. The maintainer requested implementing universal tax as an 'Apply to all countries' option in the country dropdown on the Tax Rates configuration page, rather than as new settings fields. This PR's settings-based approach (enable_universal_tax toggle + universal_tax_rate field) has been replaced with:
A new PR will be opened targeting Superseded by issue #426. |
… option Implements the maintainer's preferred approach from PR #277 review feedback: add an 'Apply to all countries' option to the country dropdown on the Tax Rates configuration page, rather than using new settings fields. Changes: - views/taxes/list.php: add '*' option at top of country dropdown - inc/functions/tax.php: handle '*' country as fallback in wu_get_applicable_tax_rates() — applies when no exact-country rates match - lang/ultimate-multisite.pot: add 'Apply to all countries' string, update line number references for 'Leave blank to apply to all' This approach is simpler, consistent with how state/city already use blank/wildcard to mean 'apply to all', and requires no new settings. Closes #426
|
Hi @mahyarrezghi — thanks for the contribution! The maintainer (@superdav42) has requested a different implementation approach in their review above. Rather than a separate toggle + rate field, the preferred approach is to add an "Apply to all countries" option to the existing country dropdown on the tax rates configuration page. This would be simpler to implement and more consistent with the existing UI. If you'd like to revise the PR with that approach, please reopen it and push the updated implementation. Happy to help review once updated. |
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.