q-m / questionmark-barcodes

barcodevalidation with Questionmark-specific (internal) barcodes
https://github.com/marketplacer/barcodevalidation
MIT License
2 stars 0 forks source link

Implement price/weight normalization behavior #16

Open Narnach opened 1 year ago

Narnach commented 1 year ago

Status: rough draft

We've got country-specific price/weight normalization logic implemented in Questionmark's Hoard system for the Netherlands and (IIRC) Belgium. A PR can start by extracting the logic and sharing it.

Design idea:

wvengen commented 1 year ago

Great to move this logic here, yes. Note that one cannot deduce the country from the barcode, but it is an input. Perhaps we can instantiate a barcode with a country parameter. This is needed elsewhere too, e.g. if we implement operator == it would also need to do normalisation, I think it belongs in an instance attribute.

Links focused on NL situation: http://developers.thequestionmark.org/2016/12/14/partial-barcode-sql http://developers.thequestionmark.org/2017/02/13/storing-barcodes

Narnach commented 1 year ago

Note that one cannot deduce the country from the barcode, but it is an input.

My understanding was flawed, thanks for correcting me 😄

In that case it makes sense to have #country accessor as well, so we can base normalization behavior based on this being present. It also feeds reliably into #can_normalize? because we can then use the presence + value of country in addition to other factors.

wvengen commented 1 year ago

I'd expected country as input when initializing the class. I don't think that would fit with Barcodevalidation, so indeed an accessor would make sense - or a function to set it, e.g.

BarcodeValidation.scan('1234567890').country(:nl)

We could consider if there could be a default country or not. Right now, an instance of our apps has a static country from an environment variable, there it would make our code easier. But we may add multi-country support to single app instances, then the default would need to be set on each request / for each BarcodeValidation instance (also fine).

Narnach commented 1 year ago

I'd lean towards the "manually pass a country to an instance" approach to keep the behavior as generic as possible and don't get in the way of future multi-tenancy situations, or legitimate cases where you'd want to compare normalized barcodes from multiple countries.

Narnach commented 1 year ago

Another aspect that's useful to call out here is that normalization requires we split a barcode into its component parts: prefix, affix, check_digit, and that affix can thus be split into product id, price and weight.