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

For fixed-width inputs, do not automatically calculate a missing check digit, unless actually told to #263

Open
19 tasks
terryburton opened this issue Feb 16, 2024 · 11 comments

Comments

@terryburton
Copy link
Member

terryburton commented Feb 16, 2024

We've focused a lot on reducing human error in recent releases, e.g. with the GS1 AI syntax linting.

One source of error that I'm aware of is users accidentally missing a digit in the input for a symbology that has fixed-width input (such as EAN-13) and not realising that a check digit has been automatically added.

Given the example of EAN-13, the current behaviour is:

Data Options Result Note
9520123456788 Validate check digit and generate
952012345678 Calculate check digit and generate Check digit calculation occurs automatically

In future, a new calculatecheck option will be introduced, and the behaviour will be changed to:

Data Options Result Note
9520123456788 Validate check digit and generate
952012345678 Error: EAN-13 must be 13 digits, or 12 with calculatecheck option Check digit calculation is no longer automatic
952012345678 calculatecheck Calculate the check digit (as told) and generate

There's a decision to be made over whether or not to silently ignore the new calculatecheck option when a full width input is already provided, option (1):

Data Options Result Note
9520123456788 calculatecheck Validate check digit and generate Ignore being asked to generate a check digit if it is already present

versus option (2):

Data Options Result Note
9520123456788 calculatecheck Error: EAN-13 must be 12 digits with calculatecheck option

My preference is option (2) since this covers the case that a user wants their check digit to be calculated but erroneously enters 13 digits (and the check calculation happens to pass).

The above is an "API change" that maximises the utility of the check digit that is built into these fixed-width symbologies (and their derivatives):

  • ean13
  • ean8
  • upca
  • upce
  • isbn
  • ismn
  • issn
  • databarlimited
  • databaronni
  • databarstackedomni
  • databarstacked
  • databartruncated
  • sscc18
  • ean14
  • itf14
  • identcode
  • leitcode
  • pzn
  • code32
@terryburton
Copy link
Member Author

Soliciting opinions on the above from those who have to handle fallout from downstream or cross-testing:

@gitlost
@metafloor
@semireg
@adamchainz

@semireg
Copy link

semireg commented Feb 16, 2024

This looks good to me and makes sense. Curious to see if @metafloor changes the API to match, or if defaults will be maintained along with a new negated boolean (skip-calculatecheck) to access this new behavior. Thanks for checking with us!

@gitlost
Copy link
Contributor

gitlost commented Feb 16, 2024 via email

@terryburton
Copy link
Member Author

Curious to see if @metafloor changes the API to match, or if defaults will be maintained along with a new negated boolean (skip-calculatecheck) to access this new behavior. Thanks for checking with us!

@semireg For API back compatibility, we could introduce a deprecated BWIPP option calculatecheckifmissing, which frontends could initially set unconditionally to restore the previous behaviour. This would allow for each frontend to transition at its own pace without need to create some tricky shim over the BWIPP inputs to fake up the old behaviour. I would be loathed to document this option (it's not something that I think general users should reply upon), and I would be tempted to remove it at some point in the not too distant future.

@metafloor
Copy link
Contributor

I am traveling this weekend, but would like to add logging to the bwip-js public API to get a sense of how much current usage relies on automatic checkdigit calculation. This is a potentially breaking change for folks who rely on the public API for commercial use.

@metafloor
Copy link
Contributor

I added logging that just records text length for the list of symbols above. The service provider (heroku) restarts the service every 24 hours, and the numbers below represent about 14 hours uptime. Aside from lots of spurious text lengths that would have generated errors, here is sample usage for the public api:

ean13={ ... "12":2934,"13":63885 ... }
ean8={"8":4}
upca={ ... "10":146,"11":18507,"12":8483,"13":54,"14":16}
upce={"5":3,"6":1,"7":3,"8":281,"10":1}
isbn={}
ismn={}
issn={}
databarlimited={"18":19}
databaronni={}
databarstackedomni={}
databarstacked={}
databartruncated={}
sscc18={}
ean14={"17":2,"18":67}
itf14={"13":162,"14":124}
identcode={}
leitcode={}
pzn={}
code32={}

The pairs of digits are "length-of-text":count. As it stands now, there are a significant number of calls that expect the check digit to be calculated automatically. Unfortunately, I have no idea how many distinct users that represents.

My thinking is that when this change hits, bwip-js will add additional logic to the ean, upc and itf14 symbols to automatically add the calculatecheck option if the text length is one digit short. That's an easy fix to maintain backward compatibility for the existing users.

@semireg
Copy link

semireg commented Mar 13, 2024

Nice work on the stats.

By add the calculatecheck option do you mean calculatecheck will default to true in bwip-js but defaults to false in bwip? Or is this new behavior only going to be in the public API to not break things?

@metafloor
Copy link
Contributor

@semireg: I have not decided public api vs base bwip-js, but am leaning towards just the public api. My thinking is that when this change hits, bwip-js will release as a major version change, since it is a breaking api change. That will keep most npm users tied to the previous version until they explicitly choose to make the upgrade. The public api users, on the other hand, likely have minimal visibility into these types of changes

@terryburton
Copy link
Member Author

terryburton commented Mar 13, 2024

My current plan is as follows:

Data Options Result Note
9520123456788 Validate check digit and generate
952012345678 Error: EAN-13 must be 13 digits, or 12 with calculatecheck option Check digit calculation is no longer automatic
952012345678 calculatecheck Calculate the check digit (as told) and generate
9520123456788 calculatecheck Error: EAN-13 must be 12 digits with calculatecheck option
952012345678 calculatecheckifmissing Calculate the check digit (as told) and generate. Deprecated option, for back compat. Can exist alongside calculatecheck in which case calculatecheck is ignored.
9520123456788 calculatecheckifmissing Validate check digit and generate. "

So BWIP-JS et al. can just unconditionally inject calculatecheckifmissing (in these and any other symbologyies, where it is unknown and will therefore be ignored) to obtain the existing behaviour.

Does that work? This way the transition is to simply remove calculatecheckifmissing at the appropriate time.

@metafloor
Copy link
Contributor

@terryburton: For bwip-js, the logic around injecting calculatecheck or unconditionally adding calculatecheckifmissing is a wash. My biggest concern is properly documenting the temporary nature of the work around, and encouraging the users to upgrade their code. That is one limitation of github and especially npm that is tricky to work with.

@terryburton
Copy link
Member Author

My biggest concern is properly documenting the temporary nature of the work around, and encouraging the users to upgrade their code. That is one limitation of github and especially npm that is tricky to work with.

Agreed.

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

No branches or pull requests

4 participants