Ethan826
commented
3 years ago @se-panfilov, thanks for this helpful library! We're getting a similar problem.
TL:DR: If (1) the countryList has multiple countries, (2) the country list includes Brazil, and (3) Brazil appears in the list before the correct country, it appears a bug in the new Brazil code always returns the validator for Brazil. This wasn't caught by the tests because none of the tests appears to cover country lists with multiple countries.
I apologize that I don't have the time to dig far enough into the project structure and how VATs work to offer a PR, but I hope this writeup is useful in lieu of a PR. And I could certainly be mistaken in my understanding!
Test to prove something is wrong
The problem can be reproduced by changing one of the spec files, for example the UK one, thus:
import { unitedKingdom, brazil } from '../index';
import { codes, invalid, name, valid, validOnlyByFormat } from './countries_vat_lists/unitedKingdom.vat';
import { addCharsToString, checkInvalidVat, checkOnlyValidFormatVat, checkValidVat } from './utils';
const countriesToTest = [brazil, unitedKingdom];
describe('United Kingdom', () => {
it('should return "true" result for valid VATs', () => {
valid.forEach((vat) => checkValidVat(vat, countriesToTest, codes, name));
});
it('should return "true" for "isSupportedCountry" and "isValidFormat" fields, but "false" for "isValid" for VATs that match format but still invalid', () => {
validOnlyByFormat.forEach((vat) => checkOnlyValidFormatVat(vat, countriesToTest, codes, name));
});
it('should return "true" result for valid VATs with extra dash characters', () => {
valid.map((vat) => addCharsToString(vat, '-')).forEach((vat) => checkValidVat(vat, countriesToTest, codes, name));
});
it('should return "true" result for valid VATs with extra space characters', () => {
valid.map((vat) => addCharsToString(vat, ' ')).forEach((vat) => checkValidVat(vat, countriesToTest, codes, name));
});
it('should return "false" result for invalid VATs', () => {
invalid.forEach((vat) => checkInvalidVat(vat, countriesToTest));
});
});The tests fail. But if the countriesToTest array is reversed, [unitedKingdom, brazil], the tests pass.
Why the test fails
The issue is in this code. If multiple countries are selected and Brazil gets hit first while iterating over the countriesList, the expression !isVATStartWithCountryCode(country.name) will always return true and so Brazil will always get returned.
In other words, if Brazil is among the countries being checked for, and the correct country hasn't been hit yet, Brazil will always get returned. (That's also pretty likely to happen given that Brazil starts with B and the full list is alphabetical.)
Fixes and assumptions
One possibility is to rip out the Brazil code pending a fuller solution, as the library seems pretty substantially broken right now. We downgraded to 2.4.0. @DonutLaser, maybe that's the best solution for the time being?
I don't know enough about VAT numbers to know how to rewrite the Brazil special-case logic without actually running the validation code. And more to the point, the library seems to have a pretty strong assumption that there's a cheap threshold test for VAT countries before getting down to the nitty-gritty of validating the number. In other words, until Brazil got added, it was easy to look at the first few letters of the cleaned VAT and say, "If this VAT is valid for anybody, it's valid for country X."
Brazil seems to break that assumption. So it seems like there may need to be some re-thinking of how to cheaply identify the VAT country if it isn't prefixed by the country code.
And even if that is addressed for Brazil, I don't know enough about VAT numbers to know if there is more than one country like Brazil. Is it possible to have a VAT code that could be valid in more than one country? How should that get handled? Because I don't know whether that's a real problem or if Brazil is truly a special case, I don't know how best to solve the issue. If I did, I'd offer a PR, and I apologize for not having the ability to dig into this enough to offer one. @andrebazoli, maybe you have some additional insights here, and please do correct me if I'm getting anything wrong.
What test to consider adding
The test isolation is great for each country, but there should be some tests that exercise multiple countries in the countryList and ensure that various edge cases are covered in terms of interaction, probably by checking that behavior is correct if a country is early in the list, late in the list, etc.
andrebazoli
PierreF
mits87
SergioGMB
se-panfilov
result.country.nameis Brazil and isValid = false even though both of these things aren't actually true about this VAT code