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

Referencing issue #9928 of PrestaShop/PrestaShop #7

Merged
merged 3 commits into from Jan 2, 2019

Conversation

Projects
None yet
6 participants
@borlum
Copy link
Contributor

borlum commented Dec 18, 2018

See my comment (PrestaShop/PrestaShop#9928 (comment)), to issue #9928 of PrestaShop/PrestaShop. Relates to issues with VAT number and EU VAT + B2B.

@jolelievre
Copy link

jolelievre left a comment

Hello @borlum thank you for your PR, although a few mistakes are still present
I originally was not very enthusiastic with this fix, so I searched for the reasons why the Address was badly filled, sadly it requires too many changes in the core code with potential side effects, so in the end your fix is probably the least intrusive

Show resolved Hide resolved VATNumberTaxManager.php Outdated
Show resolved Hide resolved VATNumberTaxManager.php
@borlum

This comment has been minimized.

Copy link
Contributor Author

borlum commented Dec 20, 2018

Hi @jolelievre -- I can not agree with you more; I don't like the fix myself -- but it would take me too long to find the root of the issue.
FYI, I have no stake in PrestaShop myself, but was asked by a friend to help him with these VAT-related issues. I made the fix and after searching around I saw that these changes might still do some good -- thus the pull request.

I ammended the errors you pointed out. It truly was a hotfix :-)

@jolelievre

This comment has been minimized.

Copy link

jolelievre commented Dec 20, 2018

Thank you @borlum I also wanted to fix the core problem, actually the TaxManager is used in various places in the code, from the Product class to the Cart class, the issue is that the used Address is often generated with a few fields but not all of them https://github.com/PrestaShop/PrestaShop/blob/cb3663e22786254b5667a2ffe8011576223ba69c/classes/Product.php#L3411
Whereas it should fetch the user address to get all the informations.

I started to fetch the correct address in various places, but sometimes you simply don't know the customer so you don't have the information to fetch the correct Address, and even if you could I'm afraid sometimes you purposely don't want to use this customer Address so forcing it everywhere might create other unexpected bugs..

Your hot fix has the advantage only modify this module so at least we won't introduce any unexpected behavior for shop which don't use this module.

Our QA team is going to test your fix before we can merge it and upgrade the module.

@jolelievre
Copy link

jolelievre left a comment

Thank you @borlum I would like to have feedback from someone else in @PrestaShop/prestashop-core-developers before sending this to QA.

// Now, check on the cached address object
return (null !== $cached_address
&& !empty($cached_address->vat_number)

This comment has been minimized.

@PierreRambaud

PierreRambaud Dec 20, 2018

Contributor

Why testing vat_number when you use cached_address->id_country, more, the first test is useless because of empty which already tests $cached_address :)

This comment has been minimized.

@jolelievre

jolelievre Dec 20, 2018

no I tested with $cached_address = null and an error is thrown, something like "Can't access vat_number property on non object"
Besides you need to check vat_number because it is only used for Companies which don't belong in your country, "personal" customers still need to pay the taxes

This comment has been minimized.

@borlum

borlum Dec 20, 2018

Author Contributor

Yes, the whole point of my fix is to have a working check of both vat_number and country; to avoid adding VAT for foreign registered businesses :-)

This comment has been minimized.

@PierreRambaud

PierreRambaud Dec 22, 2018

Contributor

@jolelievre http://sandbox.onlinephpfunctions.com/code/af9568e8e5a7e9d806ce1145d0b7c90065c5fdad The empty function catch everything
The null test is useless here.
If you want to have var_number and country values, you need to check both :)

This comment has been minimized.

@borlum

borlum Dec 22, 2018

Author Contributor

What do you mean with 'check both'? You need to clarify a little :-)

This comment has been minimized.

@PierreRambaud

PierreRambaud Dec 23, 2018

Contributor

Like this:

return !empty($cached_address->vat_number)
    && !empty($cached_address->id_country)
    && $cached_address->id_country != Configuration::get('VATNUMBER_COUNTRY')
    && Configuration::get('VATNUMBER_MANAGEMENT')
);

This comment has been minimized.

@borlum

borlum Dec 29, 2018

Author Contributor

I don't mind also adding a check for id_country -- but that has nothing to do with my hotfix. The fix works just fine without, given that country is never empty (it has to be filled out to submit the form).

So it seems somewhat unrelated to my commit / pull request regarding a snappy solution to a major problem for many users :-)

@marionf marionf added QA approved and removed Waiting for QA labels Jan 2, 2019

@PierreRambaud PierreRambaud merged commit adea224 into PrestaShop:dev Jan 2, 2019

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 2, 2019

Thanks @borlum :)

@jf-viguier

This comment has been minimized.

Copy link

jf-viguier commented on 403b8bd Jan 22, 2019

This commit save by day, thanks @borlum
It fix a big bug in Prestashop with vatnumber module and Europe shipping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment