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

Add email and website format validation #6494

Merged
merged 4 commits into from
Jun 6, 2019
Merged

Conversation

kymckay
Copy link
Collaborator

@kymckay kymckay commented Jun 5, 2019

Closes #5866

Simple validation following the HTML5 standard for emails as we don't expect POIs to have convoluted email addresses (so there should be minimal false positives). For websites I'm only checking that the scheme is present currently.

Only checks the website and email tags as these are what iD currently supports with fields.

I was unsure how best to handle the reference property for the validationIssue objects because both issues can be present at once on the same entity (so using one function and changing the text via variable isn't viable). Just defined two functions for now since it works, can be refactored as desired.

The main thing to get right here are the translated strings. I think the website one should probably be reworded as it can flag instances where the user sees a seemingly valid site (e.g. www.example.com).

Simple validation following the HTML5 standard for emails as we don't
expect POIs to have convoluted email addresses. Only checks the
`website` and `email` tags as these are what iD currently supports with
fields.
@kymckay kymckay added the validation An issue with the validation or Q/A code label Jun 5, 2019
@mmd-osm
Copy link
Contributor

mmd-osm commented Jun 6, 2019

I think the email check disregards that you can specify multiple email addresses in the value which are being separated by a semicolon: https://overpass-turbo.eu/s/JIF

@kymckay
Copy link
Collaborator Author

kymckay commented Jun 6, 2019

@mmd-osm Good point, easy to fix 👍

kymckay added 3 commits June 6, 2019 14:50
Also improved the UI message to be more clear for websites and simplify
"is tagged with" to "has" which works in context.
I've hijacked the data property for the purposes of changing the message
based on whether multiple values in a list are erroneous
@quincylvania quincylvania merged commit 563f582 into master Jun 6, 2019
@quincylvania quincylvania deleted the validate-format branch June 6, 2019 17:22
@quincylvania
Copy link
Collaborator

quincylvania commented Jun 6, 2019

@SilentSpike Looks great, thanks!! 👏

FYI I'm going to tweak the message strings to make them a little more user-friendly.

@mmd-osm
Copy link
Contributor

mmd-osm commented Jun 6, 2019

EMail addresses can have umlauts and other characters both in their domain name and for the user. The following ones would be incorrectly flagged as invalid: https://www.openstreetmap.org/node/5849987202, https://www.openstreetmap.org/node/6203770400

Then people sometimes add leading and trailing spaces, also combined with a semicolon separator. Those could probably be ignored as well: https://www.openstreetmap.org/api/0.6/node/4237659791

@kymckay
Copy link
Collaborator Author

kymckay commented Jun 7, 2019

The leading and trailing space is stripped out by iD already and not a problem.

Will need to think about how best to handle special characters 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validation An issue with the validation or Q/A code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warnings for website and email address tags with invalid formats
3 participants