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

momentjs using deprecated API #1672

Closed
alxndrsn opened this issue Nov 25, 2015 · 14 comments
Closed

momentjs using deprecated API #1672

alxndrsn opened this issue Nov 25, 2015 · 14 comments
Labels
Type: Technical issue Improve something that users won't notice

Comments

@alxndrsn
Copy link
Contributor

Seen on android:

moment construction falls back to js Date

screen shot 2015-11-25 at 10 32 02

@alxndrsn alxndrsn added the Type: Technical issue Improve something that users won't notice label Nov 25, 2015
@garethbowen
Copy link
Member

This generally means we're trying to parse a date which isn't in a standard format.

@ghost ghost assigned SCdF Apr 18, 2016
@ghost ghost added the 1 - Scheduled label Apr 18, 2016
SCdF added a commit that referenced this issue Apr 19, 2016
So we can see both at once.

Issue: #1672
@SCdF
Copy link
Contributor

SCdF commented Apr 19, 2016

This could be legacy data? One example is a child from production whose DOB is the string "Oct 24, 2015". They were created Nov 24, 2015. I haven't been able to reproduce this date style with the current code base.

@SCdF
Copy link
Contributor

SCdF commented Apr 19, 2016

Quick hack, this temp view:

function(doc) {
  if (doc.type === 'person' && doc.date_of_birth && doc.date_of_birth.indexOf(' ') >= 0) {
    emit([new Date(doc.reported_date), doc.date_of_birth]);
  }
}

Finds us ~530 documents with the bad date format in PROD, mostly created between 3rd Nov 2015 and the 29th Feb 2016. There is one lone doc from the 9th April 2016, but apart from that it looks like they were contained in that 3 month period.

@SCdF
Copy link
Contributor

SCdF commented Apr 19, 2016

(date_of_birth are normally stored as YYYY-MM-DD)

@alxndrsn
Copy link
Contributor Author

I think an incorrectly-defined form could be causing these bad dates.

@SCdF
Copy link
Contributor

SCdF commented Apr 19, 2016

Yeah, so this seems to be an old version of the form? Here is the important structure of a failing document:

   "date_of_birth": "Apr 9, 2008",
   "date_of_birth_method": "approx",
   "ephemeral_dob": {
       "dob_calendar": "",
       "age_years": "8",
       "age_months": "0",
       "dob_method": "approx",
       "dob_raw": "Tue, 08 Apr 2008 21:00:00 GMT",
       "dob": "Apr 9, 2008"
   },

Here is one I just created now:

   "date_of_birth": "2015-01-18",
   "dob_method": "approx",
   "dob_calendar": "",
   "age_years": "1",
   "age_months": "3",
   "dob_raw": "2015-01-18",
   "dob_iso": "2015-01-18",

@SCdF
Copy link
Contributor

SCdF commented Apr 19, 2016

And by old version, looking at our LG forms, it's actually the current version? confusing.

@SCdF
Copy link
Contributor

SCdF commented Apr 19, 2016

OK I have the correct versions, we just have some mismatches between some of our forms which I'll raise against those forms.

In any case, the question now becomes:

  1. Are there still ways of creating these bad dates or have all the forms been fixed?
  2. Should we write a migration script to fix the bad values,
  3. Or should we change our code to use moment properly but also deal with these bad values (by detecting them and perhaps converting them in JS)
  4. Or should we just ignore it because we're not going to update moment ever so who cares?

@garethbowen
Copy link
Member

  1. Good idea.
  2. Probably.
  3. I'm not sure how easy it would be to detect them. We could just moment(new Date(...)) but I think it's better to require standardised data rather than trying to handle whatever.
  4. We're definitely going to update moment.

We could also add a validation function or similar so document writes fail when invalid date formats are used, but this is difficult with the configurable nature of the output. 1 & 2 are probably enough for now I think.

@SCdF
Copy link
Contributor

SCdF commented Apr 20, 2016

So this used to be how the form worked, but it was fixed on the 2nd March. It's concerning that someone is still out there with the old form, I'm not really sure how to track down who it is though. Not relevant here though, I'll raise a projects ticket about it.

I'll write a migration script to fix the existing data. Which leaves 3 and 4 non-issues. Depending on what you guys think I could do three as well. I think if we're confident everyone is on the latest forms I don't think it's necessary.

SCdF added a commit to medic/medic-api that referenced this issue Apr 20, 2016
Finds all dates that have a space in them (implying they are the
problematic "17th Apr, 2010" style) and convert them to YYYY-MM-DD as
expected.

Issue: medic/cht-core#1672
@SCdF SCdF assigned alxndrsn and unassigned SCdF Apr 20, 2016
@SCdF
Copy link
Contributor

SCdF commented Apr 20, 2016

Hey @alxndrsn you wanna check out the linked commit? My only real issue with it is that the way we're finding incorrect dobs is by looking for a space (since 2010-01-05 has no space but Jan 5th, 2010 does). I could change it to regex and try to be smarter about it, but if another dob issue comes up we'd need another migration script anyway, since this one would already be run (and it only covers this one format change). It didn't seem worth it,.

@SCdF
Copy link
Contributor

SCdF commented Apr 20, 2016

Ugh let me fix these jshint errors… for some reason my linter doesn't work for medic-api :-/

SCdF added a commit to medic/medic-api that referenced this issue Apr 20, 2016
SCdF added a commit to medic/medic-api that referenced this issue Apr 20, 2016
@garethbowen
Copy link
Member

It's concerning that someone is still out there with the old form

This is really concerning because due to #2134 they might never get the updated version of the form.

I'm not really sure how to track down who it is though.

We should be able to track it down based on who the form was submitted by. Once 2.6.1 is released we could bump all the forms to force them to download to all devices again...

@alxndrsn
Copy link
Contributor Author

Looking good; some comments inline.

@alxndrsn alxndrsn assigned SCdF and unassigned alxndrsn Apr 21, 2016
alxndrsn pushed a commit to medic/medic-api that referenced this issue Apr 21, 2016
* Adds a migration script to fix bad dates

Finds all dates that have a space in them (implying they are the
problematic "17th Apr, 2010" style) and convert them to YYYY-MM-DD as
expected.

Issue: medic/cht-core#1672
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Technical issue Improve something that users won't notice
Projects
None yet
Development

No branches or pull requests

4 participants