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

ISBN inputs should not allow text #1194

Closed
jdlrobson opened this issue Sep 27, 2018 · 19 comments
Closed

ISBN inputs should not allow text #1194

jdlrobson opened this issue Sep 27, 2018 · 19 comments

Comments

@jdlrobson
Copy link
Collaborator

input type="number" is well supported on phones and prevents you from entering text into a field where numbers are needed.

Change input type to number so this is not possible:
2018-09-10 14 53 04

@tabshaikh tabshaikh self-assigned this Sep 29, 2018
@tabshaikh tabshaikh added the WIP label Sep 29, 2018
@linusheck
Copy link

I looked into this, this will probably break more things than it fixes.

Many ISBNs do have letters:
ISBN-10s end with Xes
ISBNs often have the prefixes "ISBN-13" or "ISBN-10" or "ISBN: "

@tabshaikh
Copy link
Collaborator

@glatteis Ohhh

@jdlrobson
Copy link
Collaborator Author

Thanks for properly looking into this @glatteis !
Maybe we could use built in form validation here instead, https://developer.mozilla.org/en-US/docs/Learn/HTML/Forms/Form_validation

At the current time, it's possible to submit a bad ISBN and lose all the other form data. It would be good to protected our users against this. What do you think? If so, should we open a new ticket or amend this one?

@LeadSongDog
Copy link

There are a slew of longstanding issues with ISBNs, particularly #27, #49, #142, #565 , #609
There should be one umbrella discussion somewhere that eventually gets to all of these subissues:

  1. an agreed single format to be stored,
  2. cleanup of existing data into that format,
  3. automatic cleanup of all the variant inputs, whether from user forms or imports
  4. external links correctly generated (amazon etc.)
  5. checksum validation
  6. existence checks (try Worldcat, LoC, BL, BNF, ... ) to rule out books perpetually "in cataloguing", that were never published

@LyzardKing
Copy link

Could this be something good enough to use?
https://formvalidation.io/guide/validators/isbn/
There's also an open source fork somewhere on github...
It seems to avoid most of the issues @LeadSongDog points to I believe

@tabshaikh tabshaikh removed their assignment Oct 10, 2018
@tabshaikh
Copy link
Collaborator

@LyzardKing ya it could be used, maybe we should have a discussion on this @mekarpeles

@tfmorris
Copy link
Contributor

The formvalidation commercial product is a whole suite of validators which seems like overkill -- and it's not open source.

There are a number of reasonable looking alternatives here: https://github.com/search?l=JavaScript&q=isbn+validator&type=Repositories

@jdlrobson
Copy link
Collaborator Author

Please consider html5 form validation and the pattern attribute. I agree with @tfmorris that we should avoid that particular product.

Let's keep it simple and future proof!

https://developer.mozilla.org/en-US/docs/Learn/HTML/Forms/Form_validation

@brad2014
Copy link

@tabshaikh This is marked WIP in 2018. Is it still in progress?

@linusheck
Copy link

linusheck commented Apr 29, 2019

@brad2014 my very personal opinion is that client-side validation of ISBNs is a bad idea, because it's just too complex.
I would suggest closing this issue (maybe someone can re-open it once they have a good solution to this, which doesn't seem to be the case right now).

@jdlrobson
Copy link
Collaborator Author

Works for me. Let's close this in lieu of a concrete solution.

@LeadSongDog
Copy link

@glatteis @jdlrobson I don’t get it. How does closing an issue help find a solution??

@brad2014
Copy link

brad2014 commented May 3, 2019

Since the issue reported is "ISBN inputs should not allow text", and the research is "ISBN should allow text", I'm marking this "Close: Not an Issue". Other issues related to ISBN validation, storage and indexing remain open.

@LeadSongDog
Copy link

LeadSongDog commented May 4, 2019

@brad2014 Perhaps we are talking past each other here. ISBN inputs, like any other inputs, will vary in form, accuracy, validity etc. That does not mean we need to store the variations: the stored form should be as good as we can manage to achieve.

Either strip out the superfluous hyphens or ensure they are always present. Either choose 10 digits, 13, or both.l for storage. Always ensure the number represents a real edition found in real libraries. For searches ensure all the various input forms match the consistent stored form. None of this happens if the issue is closed.

@jdlrobson
Copy link
Collaborator Author

jdlrobson commented May 4, 2019

@LeadSongDog let's create a more specific issue capturing the new requirements to avoid confusion. We obviously want to allow text, so a more specific bug would be "ISBN inputs should always be in a valid format". Sound good?

(I think using HTML5 validation using regex is still a cheap and valid way to do this)

@LeadSongDog
Copy link

@jdlrobson is that saying to reject input in other formats, or to rework them to an agreed single format?

@jdlrobson
Copy link
Collaborator Author

Whatever you think best. My original proposal was a little poorly thought out as I thought just having numbers was enough.. so I'm sorry for creating a lot of confusion on this issue. I think a new issue will clear up that confusion by rescoping the problem.

@hornc
Copy link
Collaborator

hornc commented May 5, 2019

I'm just seeing this thread now, it seems to relate to an issue I opened recently where previously existing js client side ISBN and other validation has dropped off: #2090

@LeadSongDog
Copy link

Given that all ISBNs really boil down to 9 significant digits plus various padding and one checkdigit (calculated one of two ways, depending on -10 or -13), can we not get on with saying we will store one specific format, so the validation, verification, and search have something on which to focus?

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

8 participants