Add dynamic shipping tax calculation #176

BorisovskiP · 2020-06-28T12:29:10Z

Please make sure these boxes are checked before submitting your PR - thank you!

Pull request is based against develop branch
README.md reflects changes (if applicable)
New files contain a license header

Issue

This PR fixes issue #22 .

Proposed changes

...

sprankhub · 2020-06-28T13:00:38Z

i18n/de_DE.csv

@@ -55,3 +55,5 @@ SWIFT,SWIFT
 "CMS Page for Shipping Info","CMS-Seite für Versandkosten"
 "Show ""incl. Shipping Cost"" instead of ""excl. Shipping Cost""","Zeige ""inkl. Versandkosten"" anstatt ""exkl. Versandkosten"""
 "Display Delivery Time on Product Listing","Lieferzeit auf Produktübersichtsseiten anzeigen"
+"No dynamic shipping tax calculation","Keine dynamische Versandsteuerberechnung"


Please re-generate the English language file and then edit the German file. Hint:

n98-magerun2.phar i18n:collect-phrases -o app/code/FireGento/MageSetup/i18n/en_US.csv app/code/FireGento/MageSetup/

It seems that you re-generated the English file. Great! Now IMHO, we need to update the German file accordingly (add the strings to the position similar to the EN file, not at the end of the file).

sprankhub · 2020-06-28T13:01:49Z

composer.json

-    "magento/module-store": "*",
-    "magento/module-backend": "*",
-    "magento/framework": "*"
+    "php": "~7.1.0|~7.2.0|~7.3.0",


Please revert this to ~7.2.0||~7.3.0.

sprankhub · 2020-06-28T13:02:15Z

Plugin/Email/Model/Source/Variables.php

@@ -8,7 +8,9 @@
 namespace FireGento\MageSetup\Plugin\Email\Model\Source;

 /**
- * Plugin to add additional config variables to be inserted into emails.
+ * Class Variables


Please revert this change.

sprankhub · 2020-06-28T13:03:27Z

Model/Config.php

@@ -13,6 +13,9 @@
 */
 class Config implements ConfigInterface
 {
+    public const DYNAMIC_TYPE_DEFAULT = 0;


I think these should be more specific (saying that this is related to the shipping tax). Additionally, what do you think of putting them to the source model directly?

sprankhub · 2020-06-28T13:03:48Z

Model/System/Config/Source/Tax/Dynamic.php

+    /**
+     * @var array
+     */
+    protected $options;


Switch to private.

sprankhub · 2020-06-28T13:04:25Z

Plugin/Tax/Config/ShippingTaxPlugin.php

+
+class ShippingTaxPlugin
+{
+    public const CONFIG_PATH_DYNAMIC_SHIPPING_TAX_CLASS = 'tax/classes/dynamic_shipping_tax_class';


Please add a new method in the Config model for retrieving this configuration and use it.

sprankhub · 2020-06-28T13:05:12Z

Plugin/Tax/Config/ShippingTaxPlugin.php

+        Cart $cart,
+        Session $customerSession,
+        GroupRepository $groupRepository,
+        Proxy $taxCalculation


We should not depend on a proxy.

The Plugin does not have a around method. I think the plugin will not work in general.

sprankhub · 2020-06-28T13:07:29Z

@BorisovskiP, please also check the failing build (probably code style issues).

sprankhub · 2020-07-01T18:31:56Z

Model/Config.php

+     */
+    public function getDynamicShippingConfigPath(): string
+    {
+        return self::CONFIG_PATH_DYNAMIC_SHIPPING_TAX_CLASS;


I do not think this adds much value. I think we should add a new method in \FireGento\MageSetup\Model\System\Config, which returns the actual configuration value of that option.

sprankhub · 2020-07-01T18:32:50Z

Plugin/Tax/Config/ShippingTaxPlugin.php

+     *
+     * @return bool|mixed|null
+     */
+


Please remove this line.

sprankhub · 2020-07-01T18:33:18Z

composer.json

-    "magento/module-store": "*",
-    "magento/module-backend": "*",
-    "magento/framework": "*"
+    "php": "~7.2.0|~7.3.0",


Please fix the formatting.

sprankhub · 2020-07-01T18:34:30Z

i18n/de_DE.csv

@@ -55,3 +55,5 @@ SWIFT,SWIFT
 "CMS Page for Shipping Info","CMS-Seite für Versandkosten"
 "Show ""incl. Shipping Cost"" instead of ""excl. Shipping Cost""","Zeige ""inkl. Versandkosten"" anstatt ""exkl. Versandkosten"""
 "Display Delivery Time on Product Listing","Lieferzeit auf Produktübersichtsseiten anzeigen"
+"No dynamic shipping tax calculation","Keine dynamische Versandsteuerberechnung"


It seems that you re-generated the English file. Great! Now IMHO, we need to update the German file accordingly (add the strings to the position similar to the EN file, not at the end of the file).

sprankhub · 2020-07-01T18:36:24Z

Model/System/Config/Source/Tax/Dynamic.php

+ */
+class Dynamic implements OptionSourceInterface
+{
+    public const DYNAMIC_TYPE_SHIPPING_TAX_DEFAULT = 0;


Maybe DYNAMIC_SHIPPING_TAX_NO?

sprankhub · 2020-07-01T18:36:36Z

Model/System/Config/Source/Tax/Dynamic.php

+class Dynamic implements OptionSourceInterface
+{
+    public const DYNAMIC_TYPE_SHIPPING_TAX_DEFAULT = 0;
+    public const DYNAMIC_TYPE_HIGHEST_PRODUCT_TAX = 1;


Maybe DYNAMIC_SHIPPING_TAX_HIGHEST_PRODUCT_TAX?

barryvdh · 2021-05-12T10:20:44Z

Plugin/Tax/Config/ShippingTaxPlugin.php

+     * @throws \Magento\Framework\Exception\LocalizedException
+     * @throws \Magento\Framework\Exception\NoSuchEntityException
+     */
+    private function getTaxPercent(int $productTaxClassId, $store): int


The returnType of the rate is float, not int

barryvdh · 2021-05-12T10:21:02Z

Plugin/Tax/Config/ShippingTaxPlugin.php

+        $group              = $this->groupRepository->getById($groupId);
+        $customerTaxClassId = $group->getTaxClassId();
+
+        $request = $this->taxCalculation->getRateRequest(null, null, $customerTaxClassId, $store);


This is not actually set in the constructor.

barryvdh · 2021-05-12T10:32:58Z

I'm using a module based on this, but I encounter a loop for the payment methods. I think the collections of the tax percentage might trigger the plugin again, so I added a static variable that prevents loops (doesn't run on nested calls). Not sure if thats always required.

Also it seems the lower method is never called to get the tax percentage. At least it's not functioning right now, not sure if the fallback is needed at all?

gino2014 · 2022-08-07T17:53:13Z

Hi Magesetup2 Team and all who are looking for a solution,

as a suggestion you could use this extension

https://github.com/JaJuMa-GmbH/magento-2-dynamic-shipping-tax

Add dynamic shipping tax calculation

33a2e69

sprankhub self-requested a review June 28, 2020 12:59

sprankhub requested changes Jun 28, 2020

View reviewed changes

Petar added 2 commits June 28, 2020 15:57

Fix review issues and travis errors

48d7a9a

Use method to get configuration

9bb2804

BorisovskiP requested a review from sprankhub June 30, 2020 10:32

sprankhub requested changes Jul 1, 2020

View reviewed changes

aboritskiy mentioned this pull request Dec 21, 2020

Dynamic shipping tax class calculation #192

Closed

2 tasks

barryvdh reviewed May 12, 2021

View reviewed changes

BorisovskiP closed this Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic shipping tax calculation #176

Add dynamic shipping tax calculation #176

BorisovskiP commented Jun 28, 2020

sprankhub Jun 28, 2020

sprankhub Jul 1, 2020

sprankhub Jun 28, 2020

sprankhub Jun 28, 2020

sprankhub Jun 28, 2020

sprankhub Jun 28, 2020

sprankhub Jun 28, 2020

sprankhub Jun 28, 2020

frostblogNet Jul 5, 2023

sprankhub commented Jun 28, 2020

sprankhub Jul 1, 2020

sprankhub Jul 1, 2020

sprankhub Jul 1, 2020

sprankhub Jul 1, 2020

sprankhub Jul 1, 2020

sprankhub Jul 1, 2020

barryvdh May 12, 2021

barryvdh May 12, 2021

barryvdh commented May 12, 2021

gino2014 commented Aug 7, 2022

Add dynamic shipping tax calculation #176

Add dynamic shipping tax calculation #176

Conversation

BorisovskiP commented Jun 28, 2020

Issue

Proposed changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sprankhub commented Jun 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barryvdh commented May 12, 2021

gino2014 commented Aug 7, 2022