Skip to content
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

Implement price/weight normalization behavior #16

Open
Narnach opened this issue Nov 11, 2022 · 5 comments
Open

Implement price/weight normalization behavior #16

Narnach opened this issue Nov 11, 2022 · 5 comments

Comments

@Narnach
Copy link
Collaborator

Narnach commented Nov 11, 2022

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:

  • Barcodes must be able to self-identify the country they are intended for. This breaks down into official rules and our own QM-specific rules. Speculative: implement GTIN::Base#country with official rules and possibly see if we can get this merged into upstream barcodevalidation when we have a working implementation.
    • Our custom classes may not be able to deterministically return a country.
  • Implement GTIN::Base#normalize(country: self.country) which we can use to implement normalization rules. Probably use a helper class per country. Return true/false based on whether normalization could happen or not. We can implement a #normalize! version that raises an error when we don't know the country or are unable to normalize.
    • The optional country argument helps to provide external information about the country a barcode "belongs" to.
  • Implement a #normalized? method that can check if it's normalized or not. I'm unsure what's desired behavior for barcodes that we don't know the normalization behavior for. Do we also need to implement #can_normalize? to make this deterministic?
@Narnach Narnach mentioned this issue Nov 11, 2022
7 tasks
@wvengen
Copy link
Member

wvengen commented Nov 16, 2022

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
Copy link
Collaborator Author

Narnach commented Nov 18, 2022

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
Copy link
Member

wvengen commented Nov 22, 2022

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
Copy link
Collaborator Author

Narnach commented Nov 25, 2022

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
Copy link
Collaborator Author

Narnach commented Dec 9, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants