[#586] Fix wrong dates due to offset of time #127

georgiana-b · 2016-12-04T19:32:04Z

After a charming journey among the horrors of JS date handling, this is the best thing I could come up with.
Imho expressed in this node-pg-types issue, the Date-only fields should not have any time information attached to them. However, seems that most JS developers (including the ones at node-elasticsearch) don't share my opinion so I had to settle with this for consistency.

opentrials/opentrials#686

coveralls · 2016-12-04T19:34:22Z

Coverage decreased (-1.1%) to 90.892% when pulling 72d6a48 on hotfix/dates-without-offset into 9f168a9 on master.

vitorbaptista

This test shouldn't be here written for trials.registration_date, as it's not only related to it. Instead, extract the pgTypes configuration to a separate file (e.g. config/pg_types.js or similar) and write a test for that (e.g. test/config/pg_types.js).

You can write the test by using the pgTypes.getTypeParser() function and unit testing both functions. This would both guarantee that they're set up correctly and that they behaviour is correct.

vitorbaptista · 2016-12-06T11:19:25Z

config/index.js

+const pgTypes = require('pg').types;
+
+function parseDate(val) {
+  return new Date(Date.parse(val));


Is it possible for parseDate() to be called with null values as well? If so, it needs to handle this case, similarly to parseDateArray

vitorbaptista · 2016-12-06T11:20:53Z

config/index.js

+  return p.parse();
+}
+pgTypes.setTypeParser(1082, parseDate);
+pgTypes.setTypeParser(1182, parseDateArray);


When you move this to a separate file, please remove these magic numbers 1082 and 1182, using a constant like DATE_OID, so it gets more readable.

opentrials/opentrials#586

coveralls · 2016-12-07T07:43:06Z

Coverage increased (+0.3%) to 92.365% when pulling de9a25f on hotfix/dates-without-offset into ae7d4ce on master.

vitorbaptista

Hi @georgiana-b, please check my comments inline. Also, instead of getting the methods on each test (i.e. pgTypes.getTypeParser(DATE_OID), you could get them at the beginning of the test, next to where you define the DATE_OID constant, like const parseDate = pgTypes.getTypeParser(DATE_OID); to make the code a bit more DRY.

vitorbaptista · 2016-12-07T10:20:18Z

test/config/pg_types.js

+
+  it('returns null when value is null', () => {
+    let testDate = null;
+    should.equal(pgTypes.getTypeParser(DATE_OID)(testDate), null);


Use should(pgTypes.getTypeParser(DATE_OID)(testDate)).equal(null) instead, to follow the pattern of the rest of the tests

vitorbaptista · 2016-12-07T10:22:19Z

test/config/pg_types.js

+
+  it('raises when value is invalid', () => {
+    let testDate = 'not a date';
+    pgTypes.getTypeParser(DATE_OID).bind(null, testDate).should.throw(/^Invalid Date/);


Why are you using .bind() here instead of simply calling pgTypes.getTypeParser(DATE_OID)(testDate)? Did you have issues with the latter?

I used .bind() here because if I just do pgTypes.getTypeParser(DATE_OID)(testDate).should.throw(/^Invalid Date/) the tests fail with this error:

1) parseDate raises when value is invalid: Error: Invalid Date at parseDate (config/pg_types.js:9:650) at Context.it (test/config/pg_types.js:25:36)

vitorbaptista · 2016-12-07T10:27:15Z

test/config/pg_types.js

+    let testArray = `{${testDate}}`;
+    let expectedDate = new Date(testDate).toISOString();
+    let resultedArray = pgTypes.getTypeParser(DATE_ARRAY_OID)(testArray);
+    resultedArray.map((date) => date.toISOString()).should.containEql(expectedDate);


Instead of testing that it contains the date, you can use deepEquals, to make sure the entire array is what you expect, not that it has a single element. Like:

should(resultedArray).deepEqual([ expectedDate, ])

I don't think you need to convert toISOString() either.

vitorbaptista · 2016-12-07T10:27:58Z

test/config/pg_types.js

+
+  it('returns null when value is null', () => {
+    let testArray = null;
+    should.equal(pgTypes.getTypeParser(DATE_ARRAY_OID)(testArray), null);


Use should(foo).equal(bar)

vitorbaptista · 2016-12-07T10:28:37Z

test/config/pg_types.js

+  it('returns empty array when there are no values', () => {
+    let testArray = `{}`;
+    let resultedArray = pgTypes.getTypeParser(DATE_ARRAY_OID)(testArray);
+    resultedArray.should.be.empty;


You need to call the function, like should(resultedArray).be.empty(), otherwise it isn't testing anything

vitorbaptista · 2016-12-07T10:33:44Z

test/config/pg_types.js

+
+  it('raises when one of the values is invalid', () => {
+    let testArray = `{2016-07-08, null}`;
+    pgTypes.getTypeParser(DATE_ARRAY_OID).bind(null, testArray).should.throw(/^Invalid Date/);


I'm not sure if this is really throwing an exception. The should.throw() takes a block, like:

should(() => pgTypes.getTypeParser(DATE_ARRAY_OID)(testArray)).throw(/^Invalid Date/)

Also, from the code, it doesn't look like it'll throw an error here, but simply return an array [Date(2016-07-08), null], which seems like the correct behaviour to me.

vitorbaptista · 2016-12-07T10:36:29Z

config/pg_types.js

+    if (!value) {
+      date = null;
+    } else {
+      throw new Error('Invalid Date');


As this is coming from Postgres, I don't think we need to validate dates here, as Postgres would already have done that for us (if I understand it correctly). Here we just need to deal with null values (and maybe undefined, but we can do a general if (!value) { return null; } to cover both cases)

opentrials/opentrials#586

coveralls · 2016-12-12T08:38:38Z

Coverage increased (+0.3%) to 92.322% when pulling ce60542 on hotfix/dates-without-offset into ae7d4ce on master.

georgiana-b · 2016-12-12T12:00:25Z

@nightsh Please take a look at this and approve if you think it's ok.

nightsh · 2016-12-12T13:12:16Z

config/pg_types.js

+
+  const p = pgTypes.arrayParser.create(value, (entry) => {
+    let parsedEntry = entry;
+    if (String(entry).toLowerCase() !== String(null)) {


To clarify why is this here: null value only enters the function as the string 'null'. This condition works with either version of it.

nightsh

Left some comments, please check them out before merging (and push changes if you see fit).

nightsh · 2016-12-12T13:16:29Z

test/config/pg_types.js

+  });
+
+  it('returns empty array when there are no values', () => {
+    let testArray = `{}`;


I would've named this testPgArray, just to avoid any confusions with "regular" arrays delimited by square brackets.

nightsh · 2016-12-12T13:18:47Z

test/config/pg_types.js

+
+});
+
+describe('parseDateArray', () => {


it('calls parseDate() on single array items')

For when you pass it an array of a single item. So we'd avoid creating an interval (unless we really want to).

An array of dates does not represent an interval in this case. It's just a normal array with date values.

nightsh · 2016-12-12T13:21:01Z

config/pg_types.js

+    return parsedEntry;
+  });
+
+  return p.parse();


A bit unclear what p means, unless you go back up to see it's a parser. Might be worth renaming it to parser.

opentrials/opentrials#568

coveralls · 2016-12-12T15:06:50Z

Coverage increased (+0.3%) to 92.322% when pulling 9a4fab7 on hotfix/dates-without-offset into ae7d4ce on master.

[#586] Fix wrong dates due to offset of time

72d6a48

opentrials/opentrials#686

vitorbaptista temporarily deployed to ot-api-next-pr-127 December 4, 2016 19:32 Inactive

georgiana-b added the 4. Ready for Review label Dec 4, 2016

vitorbaptista suggested changes Dec 6, 2016

View reviewed changes

vitorbaptista reviewed Dec 6, 2016

View reviewed changes

[#586] Extract patch in separate file and add dedicated tests

de9a25f

opentrials/opentrials#586

georgiana-b force-pushed the hotfix/dates-without-offset branch from 988363e to de9a25f Compare December 7, 2016 07:40

vitorbaptista deployed to ot-api-next-pr-127 December 7, 2016 07:40 View deployment

vitorbaptista suggested changes Dec 7, 2016

View reviewed changes

[#586] Cover only null or date strings in parseDate

ce60542

opentrials/opentrials#586

nightsh reviewed Dec 12, 2016

View reviewed changes

nightsh approved these changes Dec 12, 2016

View reviewed changes

[#568] Rename variables in pgTypes patch to be more explicit

9a4fab7

opentrials/opentrials#568

georgiana-b merged commit bef5c75 into master Dec 12, 2016

georgiana-b removed the 4. Ready for Review label Dec 12, 2016

georgiana-b deleted the hotfix/dates-without-offset branch December 12, 2016 15:10

georgiana-b mentioned this pull request Dec 12, 2016

Date fields are altered at indexing opentrials/opentrials#586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#586] Fix wrong dates due to offset of time #127

[#586] Fix wrong dates due to offset of time #127

georgiana-b commented Dec 4, 2016

coveralls commented Dec 4, 2016

vitorbaptista left a comment

vitorbaptista Dec 6, 2016

vitorbaptista Dec 6, 2016

coveralls commented Dec 7, 2016

vitorbaptista left a comment

vitorbaptista Dec 7, 2016

vitorbaptista Dec 7, 2016

georgiana-b Dec 11, 2016

vitorbaptista Dec 7, 2016

vitorbaptista Dec 7, 2016

vitorbaptista Dec 7, 2016

vitorbaptista Dec 7, 2016

vitorbaptista Dec 7, 2016

coveralls commented Dec 12, 2016

georgiana-b commented Dec 12, 2016

nightsh Dec 12, 2016

nightsh left a comment

nightsh Dec 12, 2016

nightsh Dec 12, 2016

georgiana-b Dec 12, 2016

nightsh Dec 12, 2016

coveralls commented Dec 12, 2016


		});

		describe('parseDateArray', () => {

[#586] Fix wrong dates due to offset of time #127

[#586] Fix wrong dates due to offset of time #127

Conversation

georgiana-b commented Dec 4, 2016

coveralls commented Dec 4, 2016

vitorbaptista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 7, 2016

vitorbaptista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 12, 2016

georgiana-b commented Dec 12, 2016

Choose a reason for hiding this comment

nightsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 12, 2016