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

Reimplement date field validator as validation rule #138

Merged

Conversation

1ec5
Copy link
Member

@1ec5 1ec5 commented Sep 22, 2022

Replaced the date format validator from #81 with a user-friendlier check within the existing invalid format validation rule. The new rule comes with a one-click option to reformat a malformed but understandable date, wrapping around months and years as necessary, as well as an option to remove the malformed date.

-00004003-9-53 July 4, 1776 1970-1/2038-1 2022-23

Fixes OpenHistoricalMap/issues#445.

Comment on lines +236 to +318
expect(iD.utilNormalizeDateString('-4003').value).to.eql('-4003'); // beyond displayable year range but still valid
expect(iD.utilNormalizeDateString('31337').value).to.eql('31337');
expect(iD.utilNormalizeDateString('-31337').value).to.eql('-31337');
Copy link
Member Author

@1ec5 1ec5 Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These years are well beyond the minimum year of 3999 BCE set in OpenHistoricalMap/issues#28 and the maximum year equivalent to the current year at any given time. However, these year thresholds are only for display and filtering purposes. A truly primordial natural feature shouldn’t prevent the user from saving, nor should a building slated for demolition in a year’s time. If we want these to be hard limits, then the API should enforce them and expose them via the capabilities API method.

});
it('rejects malformed dates', function() {
expect(iD.utilNormalizeDateString('1970-01--1')).to.eql(null);
expect(iD.utilNormalizeDateString('197X')).to.eql(null); // no EDTF for now
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validating the start_date:edtf and end_date:edtf keys should wait until we have dedicated fields for these keys. Parsing EDTF is somewhat more complex than parsing ordinary ISO 8601, since we can’t lean on the browser to bear the brunt of parsing them.

modules/util/util.js Outdated Show resolved Hide resolved
Comment on lines +571 to +651
// Fall back on whatever the browser can parse into a date.
date = new Date(raw);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fallback is what allows for some natural language syntax to be parsed successfully. The specific supported natural language formats depends on the browser, language settings, etc., so it’s only a best-effort fallback. Anything produced by this code path still results in an error presented to the user, but there’s a suggestion to reformat the tag value to what the Date constructor interprets the value as.

expect(iD.utilNormalizeDateString('1970-12-32').value).to.eql('1971-01-01');
expect(iD.utilNormalizeDateString('1970-02-29').value).to.eql('1970-03-01');
expect(iD.utilNormalizeDateString('1970-00').value).to.eql('1969-12');
expect(iD.utilNormalizeDateString('1970-23').value).to.eql('1971-11'); // no EDTF for now
Copy link
Member Author

@1ec5 1ec5 Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve already made the mistake of entering EDTF year-season combinations into start_date and end_date a few times. It’ll be nice to get warned about this mistake instead of having to notice that the time slider never filters the feature out.

Added a date format validator to the invalid format validation rule.
@1ec5 1ec5 changed the base branch from staging-old to staging September 30, 2022 01:34
@1ec5
Copy link
Member Author

1ec5 commented Sep 30, 2022

I rebased the most substantial part of this PR onto the latest staging branch for OpenHistoricalMap/issues#395. I omitted the commits that rolled back #81, which have been completely superseded by this PR.

@batpad
Copy link

batpad commented Sep 30, 2022

@1ec5 this looks fantastic, thank you so much!

@danrademacher am a bit confused with the branches :( - but do you guys have everything you need to integrate? Let us know if we can help!

@danrademacher
Copy link
Member

@batpad Yes, I think we have what we need -- @1ec5 helped chart the way to a new staging that's based on latest upstream, which is what we have now.

@danrademacher
Copy link
Member

Just tested this locally -- the screenshots should have alerted me to how awesome this is, but using it live re-inforced -- it is very cool!

@danrademacher danrademacher merged commit b355d7b into OpenHistoricalMap:staging Oct 1, 2022
@1ec5 1ec5 deleted the ohm-date-validator-445 branch October 1, 2022 01:46
@todrobbins
Copy link

@danrademacher is this now live on https://www.openhistoricalmap.org/?

@danrademacher
Copy link
Member

Unfortunately, no. We have been having a bunch of problems with our local development setup after catching up with upstream OSM and iD, and iD is currently failing on our staging site -- but for reasons that we're still trying to figure out... Documented in OpenHistoricalMap/ohm-deploy#189 (.local dev issues) and at OpenHistoricalMap/issues#476 (iD failing on staging). We escalated this to get more help from our friends at DevSeed and I hope tomorrow we will have progress on local dev that will allow us to fix new iD inside rails and get this out to the community!

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