Sc 63647/submission of json objects to string fields #170

henriquelakiap · 2024-04-11T14:33:15Z

Description of the change

Add string validation and is valid/invalid format operators to first_name type

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Configuration change
Technical Debt
Documentation

Related tickets

Ticket SC-63647

Checklists

Development and Testing

Lint rules pass locally.
The code changed/added as part of this pull request has been covered with tests, or this PR does not alter production code.
All tests related to the changed code pass in development, or tests are not applicable.

Code Review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached.
At least two engineers have been added as "Reviewers" on the pull request.
Changes have been reviewed by at least two other engineers who did not write the code.
This branch has been rebased off master to be current.

Tracking

Issue from Shortcut/Jira has a link to this pull request.
This PR has a link to the issue in Shortcut.

QA

This branch has been deployed to staging and tested.

…first_name operators

henriquelakiap · 2024-04-11T15:04:33Z

lib/types/date.js

@@ -10,6 +10,7 @@ const formats = [
  'ddd MMM DD YYYY', // 'Mon Jun 02 2014'
  'MMM DD YYYY',     // 'Jun 02 2014'
  'M/D/YYYY',        // '6/2/2014', '06/02/2014'
+  'MM/DD/YYYY',      // '06/02/2014'


This mapping scenario was missing here and was causing failures in the unit tests

henriquelakiap · 2024-04-11T15:04:40Z

lib/types/date.js

@@ -20,6 +21,7 @@ const formats = [
  'ddd DD MMM YYYY',   // 'Fri 18 July 2014'
  'DD MMM YYYY',       // '18 July 2014'
  'D/M/YYYY',          // '18/7/2014'
+  'DD/MM/YYYY',        // '18/07/2014'


This mapping scenario was missing here and was causing failures in the unit tests

Hmm. This worries me some. I don't understand why these date tests started failing, after working fine as of the previous release (here). What changed?

I do see the failures locally, too. But when I applied just this change, for example, it caused other failures, such as:

06/02/2014 should return a Date object: AssertionError: expected '2014-02-06T00:00:00.000Z' to equal '2014-06-02T00:00:00.000Z' + expected - actual -2014-02-06T00:00:00.000Z +2014-06-02T00:00:00.000Z

Which I suppose your other changes fix. But that's a functional change, not just a test fix. Customers may be relying on LeadConduit treating 06/02/2014 as June 2nd, and not February 6th.

It seems that something changed in moment from version 2.29.4 to 2.30.0. In our last versioning of types, version 2.29.4 was used. By enforcing it here, the tests pass without adding these two scenarios. There was likely a bug in this conversion that has been fixed.

Regarding clients sending dates like 06/02/2014 being converted to June 2nd and not Feb 6th, this might also be happening in cases with D/M/YYYY and M/D/YYYY formats already mapped previously. In such cases, the first format passed in the formats array will always be the one that moment returns.

-- new info: the change in moment was on isValid function; before v2.30, isValid('18/07/2014',['D/M/YYYY']) was returning true. Now we need to add the correct mask DD/MM/YYYY, otherwise it will return false. Moment conversion still working

We were masking this issue in the tests for European formats because we were using a day that can't be a month (18).

In this scenario, we would have to think about how to proceed, such as opting for (and accepting) a standard conversion to US format.

A change in moment, that explains it! Thank you for finding that!

I think the thing to do in this PR is lock moment to 2.29.4, and open another story to, just as you say, think about how to proceed.

cgrayson

The changes for first_name look good, but the fixes to the unit tests have me worried. I'd really like to know why those tests started failing.

I also wonder if other types that are basically just strings should get this same protection. I think that would just be last_name and city.

cgrayson · 2024-04-11T17:01:34Z

lib/types/date.js

@@ -20,6 +21,7 @@ const formats = [
  'ddd DD MMM YYYY',   // 'Fri 18 July 2014'
  'DD MMM YYYY',       // '18 July 2014'
  'D/M/YYYY',          // '18/7/2014'
+  'DD/MM/YYYY',        // '18/07/2014'


Hmm. This worries me some. I don't understand why these date tests started failing, after working fine as of the previous release (here). What changed?

I do see the failures locally, too. But when I applied just this change, for example, it caused other failures, such as:

06/02/2014 should return a Date object: AssertionError: expected '2014-02-06T00:00:00.000Z' to equal '2014-06-02T00:00:00.000Z' + expected - actual -2014-02-06T00:00:00.000Z +2014-06-02T00:00:00.000Z

Which I suppose your other changes fix. But that's a functional change, not just a test fix. Customers may be relying on LeadConduit treating 06/02/2014 as June 2nd, and not February 6th.

…_name and city

…d function o newer versions

cgrayson

Thanks for the changes, I appreciate it. You left a stray .only in a test file but otherwise LGTM 👍

cgrayson · 2024-04-11T20:54:39Z

test/date.test.js

@@ -1,7 +1,7 @@
 const { assert } = require('chai');
 const date = require('../lib/types/date');

-describe('Date', function () {
+describe.only('Date', function () {


…d function o newer versions

henriquelakiap added 2 commits April 10, 2024 15:44

Add string validation to first_name

779ca94

Add string validation to first_name and 'is valid/invalid format' to …

73db70e

…first_name operators

henriquelakiap requested review from johnrb2 and cgrayson and removed request for johnrb2 April 11, 2024 14:33

henriquelakiap marked this pull request as draft April 11, 2024 14:34

Add string validation to first_name

131c82c

henriquelakiap had a problem deploying to test April 11, 2024 14:37 — with GitHub Actions Failure

Add date mapping for MM/DD/YYYY and DD/MM/YYYY

b436b0c

henriquelakiap temporarily deployed to test April 11, 2024 15:02 — with GitHub Actions Inactive

Add date mapping for MM/DD/YYYY and DD/MM/YYYY

715e8b4

henriquelakiap temporarily deployed to test April 11, 2024 15:03 — with GitHub Actions Inactive

henriquelakiap commented Apr 11, 2024

View reviewed changes

henriquelakiap marked this pull request as ready for review April 11, 2024 15:05

cgrayson requested changes Apr 11, 2024

View reviewed changes

Add string validation and 'is valid/invalid format' operators to last…

dfa0dbf

…_name and city

henriquelakiap temporarily deployed to test April 11, 2024 18:06 — with GitHub Actions Inactive

henriquelakiap requested a review from cgrayson April 11, 2024 18:07

Lock moment to version 2.29.4 to avoid a different behavior in isVali…

441e285

…d function o newer versions

henriquelakiap temporarily deployed to test April 11, 2024 19:43 — with GitHub Actions Inactive

cgrayson previously approved these changes Apr 11, 2024

View reviewed changes

Lock moment to version 2.29.4 to avoid a different behavior in isVali…

c91b200

…d function o newer versions

henriquelakiap dismissed cgrayson’s stale review via c91b200 April 11, 2024 21:04

henriquelakiap temporarily deployed to test April 11, 2024 21:04 — with GitHub Actions Inactive

henriquelakiap requested a review from cgrayson April 11, 2024 21:05

cgrayson approved these changes Apr 11, 2024

View reviewed changes

henriquelakiap merged commit 237084a into master Apr 12, 2024
2 checks passed

henriquelakiap deleted the sc-63647/submission-of-json-objects-to-string-fields branch April 12, 2024 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sc 63647/submission of json objects to string fields #170

Sc 63647/submission of json objects to string fields #170

henriquelakiap commented Apr 11, 2024

henriquelakiap Apr 11, 2024

henriquelakiap Apr 11, 2024

cgrayson Apr 11, 2024

henriquelakiap Apr 11, 2024 •

edited

Loading

henriquelakiap Apr 11, 2024

henriquelakiap Apr 11, 2024

cgrayson Apr 11, 2024

cgrayson left a comment

cgrayson Apr 11, 2024

cgrayson left a comment

cgrayson Apr 11, 2024

Sc 63647/submission of json objects to string fields #170

Sc 63647/submission of json objects to string fields #170

Conversation

henriquelakiap commented Apr 11, 2024

Description of the change

Type of change

Related tickets

Checklists

Development and Testing

Code Review

Tracking

QA

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henriquelakiap Apr 11, 2024 • edited Loading

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

cgrayson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgrayson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henriquelakiap Apr 11, 2024 •

edited

Loading