fix(input): Take timezone into account when validating minimum and ma… #16390

m-amr · 2018-01-05T00:15:12Z

…ximum date spans.

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix

What is the current behavior? (You can also link to an open issue here)

All details in issue for Issue #16342

What is the new behavior (if this is a feature change)?

All details in issue for Issue #16342

Does this PR introduce a breaking change?

no

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

…ximum date spans. Closes angular#16342 Closes angular#16390

…ximum date spans Closes angular#16342 Closes angular#16390

Narretz

This looks good at first glance.

The tests need some love, however.
In inputSpec, we have a min and a max suite for every type of date input (month, week, date etc), so the we need 2 tests (min +max) for each of them.
Please also note the change request on the test you've already added - this must go into each of the tests that need to be added.

Narretz · 2018-01-08T16:46:17Z

test/ng/directive/inputSpec.js

@@ -840,6 +840,16 @@ describe('input', function() {

        expect($rootScope.form.alias.$error.max).toBeFalsy();
      });
+
+      it('should validate when timezone is provided.', function() {
+        inputElm = helper.compileInput('<input type="date" ng-model="value" name="alias" ' +


Since you are only testing max, please remove min here

Narretz · 2018-01-08T16:47:45Z

test/ng/directive/inputSpec.js

+        $rootScope.$digest();
+
+        expect($rootScope.form.alias.$error.max).toBeFalsy();
+        expect($rootScope.form.alias.$valid).toBeTruthy();


There should be am expectation after this that checks whether the validation also works when the input is updated. You might have to clear the model for that first.

Narretz · 2018-01-08T16:53:18Z

src/ng/directive/input.js

+    }
+
+    function parseDateAndConvertTimeZoneToLocal(value, previousDate) {
+      var parsedDate = previousDate ? parseDate(value, previousDate) : parseDate(value);


Why not simply = parseDate(value, previousDate)? If the previousDate is undefined, it's not used.

They are same but they only different in case you check arguments

function parse(a, b){ console.log(arguments.length) } parse(1) // it will log 1 parse(1, undefined) //it will log 2

if someone in the future decide do something depend on the arguments.length it will not be the same.

I doubt this will happen. Even more so as this is an internal API. Our tests should also catch any problems this might cause.

Narretz · 2018-01-29T14:12:26Z

@m-amr are you still working on this?

m-amr · 2018-01-29T15:00:49Z

@Narretz yes, i am working on it. i will add the missing tests by this week-end.

…ximum date spans Closes angular#16342 Closes angular#16390

Narretz · 2018-02-27T11:44:14Z

Hi @m-amr , sorry for the delay, I didn't get a notification when you updated the PR.
I noticed there's still a test for plain input type="date" missing, and that the test for input type="week" passes even without your change - that is usually a sign that the test isn't correct.

m-amr · 2018-02-27T20:04:41Z

Hi @Narretz ,
Thanks for replying.
I will work on this PR this weekend. i will add all the missing tests.

…ximum date spans Closes angular#16342 Closes angular#16390

googlebot · 2018-03-29T14:08:29Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-03-29T14:08:29Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

m-amr · 2018-03-29T18:19:27Z

@Narretz
Thanks for updating the date-week tests. i didn't understand that this is the problem I got it now.
I have added the missing tests in the previous commit.

Can you please review the PR again and tell me if anything is missing or need to change ?
I am not sure why CLA post on this PR?
I have signed it before.

Narretz · 2018-04-03T19:02:24Z

@m-amr I think I mentioned earlier that the test was not working as intended, but that's okay.
And you don't have to add the changes to your commit, I can squash the changes when I merge the PR.

…ximum date spans Closes angular#16342 Closes angular#16390

Narretz · 2018-04-06T14:45:12Z

Finally merged, thanks @m-amr

…ximum in date types Closes #16342 Closes #16390

googlebot added the cla: yes label Jan 5, 2018

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 0079b78 to 5fb8d31 Compare January 5, 2018 00:27

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

5fb8d31

…ximum date spans. Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 5fb8d31 to ccdb700 Compare January 5, 2018 00:43

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

ccdb700

…ximum date spans. Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from ccdb700 to a0861ea Compare January 5, 2018 00:48

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

a0861ea

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from a0861ea to 8b9af1f Compare January 5, 2018 01:02

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

8b9af1f

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 8b9af1f to 66a8a8e Compare January 5, 2018 01:15

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

66a8a8e

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 66a8a8e to e5401c9 Compare January 5, 2018 01:24

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

e5401c9

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from e5401c9 to 58fd32a Compare January 5, 2018 10:11

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

58fd32a

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 58fd32a to 91418cc Compare January 5, 2018 10:25

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

91418cc

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 91418cc to e1f150c Compare January 5, 2018 16:32

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

e1f150c

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from e1f150c to 76355f1 Compare January 5, 2018 19:18

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

76355f1

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 76355f1 to f4decfb Compare January 5, 2018 21:37

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 5, 2018

fix(input): take timezone into account when validating minimum and ma…

f4decfb

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from f4decfb to 38cda5f Compare January 6, 2018 12:31

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 6, 2018

fix(input): take timezone into account when validating minimum and ma…

38cda5f

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 38cda5f to 483c123 Compare January 6, 2018 13:13

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 6, 2018

fix(input): take timezone into account when validating minimum and ma…

483c123

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 483c123 to cf071f8 Compare January 6, 2018 13:34

m-amr added a commit to m-amr/angular.js that referenced this pull request Jan 6, 2018

fix(input): take timezone into account when validating minimum and ma…

cf071f8

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from cf071f8 to 8d9f9cb Compare January 6, 2018 15:24

Narretz suggested changes Jan 8, 2018

View reviewed changes

Narretz added this to the 1.6.9 milestone Jan 8, 2018

Narretz added component: forms type: bug labels Jan 8, 2018

m-amr closed this Jan 29, 2018

m-amr reopened this Jan 30, 2018

Narretz modified the milestones: 1.6.9, 1.6.10 Feb 2, 2018

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 32d76dd to ee5a110 Compare February 3, 2018 21:56

m-amr added a commit to m-amr/angular.js that referenced this pull request Feb 3, 2018

fix(input): take timezone into account when validating minimum and ma…

ee5a110

…ximum date spans Closes angular#16342 Closes angular#16390

Narretz self-assigned this Feb 12, 2018

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from ee5a110 to 06709f9 Compare March 10, 2018 21:02

m-amr added a commit to m-amr/angular.js that referenced this pull request Mar 10, 2018

fix(input): take timezone into account when validating minimum and ma…

06709f9

…ximum date spans Closes angular#16342 Closes angular#16390

googlebot added cla: no and removed cla: yes labels Mar 29, 2018

fix(input): take timezone into account when validating minimum and ma…

08604c0

…ximum date spans Closes angular#16342 Closes angular#16390

m-amr force-pushed the fix_max_attribute_ignore_timezone branch from 4c75421 to 08604c0 Compare April 3, 2018 20:59

Narretz merged commit b7bb797 into angular:master Apr 6, 2018

Narretz pushed a commit that referenced this pull request Apr 6, 2018

fix(input): take timezone into account when validating minimum and ma…

2f0ac69

…ximum in date types Closes #16342 Closes #16390

This was referenced Apr 18, 2018

Update angular in package.json from 1.6.9 to 1.6.10 ui-router/sample-app-angular-hybrid#69

Closed

Update 4 dependencies from npm JetBrains/ring-ui#226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(input): Take timezone into account when validating minimum and ma… #16390

fix(input): Take timezone into account when validating minimum and ma… #16390

m-amr commented Jan 5, 2018 •

edited

Loading

Narretz left a comment •

edited

Loading

Narretz Jan 8, 2018

Narretz Jan 8, 2018

Narretz Jan 8, 2018

m-amr Jan 8, 2018

Narretz Jan 9, 2018

Narretz commented Jan 29, 2018

m-amr commented Jan 29, 2018

Narretz commented Feb 27, 2018

m-amr commented Feb 27, 2018

googlebot commented Mar 29, 2018

googlebot commented Mar 29, 2018

m-amr commented Mar 29, 2018

Narretz commented Apr 3, 2018

Narretz commented Apr 6, 2018

fix(input): Take timezone into account when validating minimum and ma… #16390

fix(input): Take timezone into account when validating minimum and ma… #16390

Conversation

m-amr commented Jan 5, 2018 • edited Loading

Narretz left a comment • edited Loading

Choose a reason for hiding this comment

Narretz Jan 8, 2018

Choose a reason for hiding this comment

Narretz Jan 8, 2018

Choose a reason for hiding this comment

Narretz Jan 8, 2018

Choose a reason for hiding this comment

m-amr Jan 8, 2018

Choose a reason for hiding this comment

Narretz Jan 9, 2018

Choose a reason for hiding this comment

Narretz commented Jan 29, 2018

m-amr commented Jan 29, 2018

Narretz commented Feb 27, 2018

m-amr commented Feb 27, 2018

googlebot commented Mar 29, 2018

googlebot commented Mar 29, 2018

m-amr commented Mar 29, 2018

Narretz commented Apr 3, 2018

Narretz commented Apr 6, 2018

m-amr commented Jan 5, 2018 •

edited

Loading

Narretz left a comment •

edited

Loading