644/datepicker #693

cornelinux · 2017-04-28T11:14:43Z

The dateformat was changed to 2017-04-28T13:14+0200.

General Comment
# Codecov Report

Merging #693 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #693      +/-   ##
==========================================
+ Coverage   95.33%   95.33%   +<.01%
==========================================
Files         121      121
Lines       14755    14762       +7
==========================================
+ Hits        14066    14073       +7
Misses        689      689

Impacted Files	Coverage Δ
privacyidea/api/token.py	`98.01% <ø> (ø)`	⬆️
privacyidea/lib/utils.py	`96.18% <100%> (+0.08%)`	⬆️
privacyidea/lib/tokenclass.py	`99.42% <100%> (-0.01%)`	⬇️
privacyidea/lib/eventhandler/tokenhandler.py	`93.87% <100%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de7ed35...75d6487. Read the comment docs.

File: privacyidea/lib/tokenclass.py:L918-927
I think we need to do something similar as in 75d6487 here:
Suppose a 2.18 token has a next_pin_change token info value in the old DD/MM/YY hh:mm format. Then, parse_date_string will return a naive datetime object, which is then compared against the aware datetime.now(tzlocal()) object, which breaks.
Right, will change this to be on the safe side.
I wanted to start a review, so that I know, that I need to change here:
Please take look at 0cf82fb
File: tests/test_lib_utils.py:L216-230
Fixed in 83769a6
File: privacyidea/static/components/token/controllers/tokenDetailController.js:L1-38
Hmm, will this work on all browsers? We should find out if toTimeString is guaranteed to give the offset relative to GMT (which corresponds to UTC).
Maybe using getTimezoneOffset would be safer.
As you can see i pushed this in a86a0ad.
I rebased my local 644/datepicker branch on master, but was not abel to push it.
Obviously there is no other way then to merge it manually into master. Please drop me a note, when merging is OK.

The datepicker sets the correct timeformat. The timeformat is used in all tests. We need to document the timeformat and we need to be able to *read* the old format from the database and return the new format. Working on #644

working on #644

Working on #644

Closes #644

codecov-io · 2017-04-28T11:14:46Z

Codecov Report

Merging #693 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #693      +/-   ##
==========================================
+ Coverage   95.33%   95.33%   +<.01%     
==========================================
  Files         121      121              
  Lines       14755    14765      +10     
==========================================
+ Hits        14066    14076      +10     
  Misses        689      689

Impacted Files	Coverage Δ
privacyidea/api/token.py	`98.01% <ø> (ø)`	⬆️
privacyidea/lib/policydecorators.py	`98.14% <100%> (ø)`	⬆️
privacyidea/lib/tokenclass.py	`99.42% <100%> (ø)`	⬆️
privacyidea/lib/eventhandler/tokenhandler.py	`93.87% <100%> (+0.12%)`	⬆️
privacyidea/lib/utils.py	`96.18% <100%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de7ed35...a86a0ad. Read the comment docs.

fredreichbier · 2017-04-28T13:53:50Z

privacyidea/lib/tokenclass.py

-        return datetime.datetime.now() > datetime.datetime.strptime(sdate,
-                                                              DATE_FORMAT)
+        #date_change = datetime.datetime.strptime(sdate, DATE_FORMAT)
+        date_change = parse_date_string(sdate)


I think we need to do something similar as in 75d6487 here:
Suppose a 2.18 token has a next_pin_change token info value in the old DD/MM/YY hh:mm format. Then, parse_date_string will return a naive datetime object, which is then compared against the aware datetime.now(tzlocal()) object, which breaks.

Right, will change this to be on the safe side.

I wanted to start a review, so that I know, that I need to change here:

The user does never set a date in next_pin_change manually. This is only achieved by using the tokenclass method. set_next_pin_change.

You are right to use parse_legacy_time here, since it seems it could occur that old timestamps without TZ but with dayfirst are found in the database.

Please take look at 0cf82fb

fredreichbier · 2017-04-28T14:04:46Z

privacyidea/lib/tokenclass.py

            # The last auth is to far in the past
-            if last_success_auth + tdelta < datetime.datetime.now():
+            if last_success_auth + tdelta < datetime.datetime.utcnow():


Right now, last auth is stored without timezone information in the database.
I'm wondering if it would be better to store it with timezone information in order to have a uniform format for datetime values in token info values? If we want to do that, we need to change the right side to an aware datetime object.

In lib/policydecorator.py method auth_lastauth the last authentication is stored as utc. So if we compare utc, we are on the same side, there would be no necissity to store the timezone. It might be faster, but harder to read for admins. I think I am ok with that. Waht do you think?

The last_auth time stamp / token info is generated internally and only used internally. No user input. This is why it was handled different as utc. I am happy to discuss this.

fredreichbier · 2017-04-28T14:07:59Z

privacyidea/lib/utils.py

-            log.debug("Dateformat {1!s} did not match date {0!s}".format(
-                date_string, date_format))
+    try:
+        d = parse_date_string(date_string)


Maybe we should use parse_legacy_time here?

It uses dayfirst=True -- currently, I think parse_date would incorrectly parse dates in the "%d.%m.%Y %H:%M" format (the MM/DD and DD/MM switch).

This way, parse_date would always return aware datetime objects -- better to have a uniform API?

I will add more tests for parse_date and adapt it as needed.

Please see 83769a6
I think this is not the most sensible and most definite way?

fredreichbier · 2017-04-28T14:20:02Z

tests/test_lib_utils.py

        d = parse_date("23.12.16")
-        self.assertEqual(d, None)
+        self.assertEqual(d, datetime(2016, 12, 23, 0, 0))


We should probably add a test for the MM/DD confusion here. Right now, parse_date behaves incorrectly (see above):

>>> privacyidea.lib.utils.parse_date('03.04.16') datetime.datetime(2016, 3, 4, 0, 0)

Fixed in 83769a6

fredreichbier · 2017-04-28T14:26:08Z

privacyidea/lib/eventhandler/tokenhandler.py

@@ -45,6 +45,8 @@
 import json
 import logging
 import datetime
+from dateutil.parser import parse as parse_date_string


dateutil.parser.parse is pretty cool because it supports a lot of formats, but this could also cause problems in the future: If, e.g., a datetime string in the DD/MM/YYYY format is stored in the database for whatever reason, we once again run into the MM/DD/YYYY confusion, but we wouldn't notice, because parse just parses it! Because of that, I'm wondering if we should maybe build a wrapper that validates that the date string is in an expected format first and gives a warning if it isn't.

We have this somehow with parse_legacy_time. But the should^TM be no MM/DD/YY in the database. And starting with 2.19 there will be no such date ever be written to the database again.

This is why till now I was fine with living with that and did try not to complicate things. What do you think?

cornelinux

I committed changes where they seemed obvious. Please take a look at the changes.
I think there are two comments, that need some discussion.

cornelinux · 2017-05-01T08:40:05Z

privacyidea/lib/tokenclass.py

            # The last auth is to far in the past
-            if last_success_auth + tdelta < datetime.datetime.now():
+            if last_success_auth + tdelta < datetime.datetime.utcnow():


In lib/policydecorator.py method auth_lastauth the last authentication is stored as utc. So if we compare utc, we are on the same side, there would be no necissity to store the timezone. It might be faster, but harder to read for admins. I think I am ok with that. Waht do you think?

cornelinux · 2017-05-01T08:40:38Z

privacyidea/lib/tokenclass.py

-        return datetime.datetime.now() > datetime.datetime.strptime(sdate,
-                                                              DATE_FORMAT)
+        #date_change = datetime.datetime.strptime(sdate, DATE_FORMAT)
+        date_change = parse_date_string(sdate)


I wanted to start a review, so that I know, that I need to change here:

cornelinux · 2017-05-01T08:53:25Z

privacyidea/lib/tokenclass.py

            # The last auth is to far in the past
-            if last_success_auth + tdelta < datetime.datetime.now():
+            if last_success_auth + tdelta < datetime.datetime.utcnow():


The last_auth time stamp / token info is generated internally and only used internally. No user input. This is why it was handled different as utc. I am happy to discuss this.

cornelinux · 2017-05-01T08:56:22Z

privacyidea/lib/eventhandler/tokenhandler.py

@@ -45,6 +45,8 @@
 import json
 import logging
 import datetime
+from dateutil.parser import parse as parse_date_string


We have this somehow with parse_legacy_time. But the should^TM be no MM/DD/YY in the database. And starting with 2.19 there will be no such date ever be written to the database again.

This is why till now I was fine with living with that and did try not to complicate things. What do you think?

cornelinux · 2017-05-01T08:58:21Z

tests/test_lib_utils.py

        d = parse_date("23.12.16")
-        self.assertEqual(d, None)
+        self.assertEqual(d, datetime(2016, 12, 23, 0, 0))


cornelinux · 2017-05-01T08:58:49Z

privacyidea/lib/utils.py

-            log.debug("Dateformat {1!s} did not match date {0!s}".format(
-                date_string, date_format))
+    try:
+        d = parse_date_string(date_string)


I will add more tests for parse_date and adapt it as needed.

cornelinux · 2017-05-01T09:05:31Z

tests/test_lib_utils.py

        d = parse_date("23.12.16")
-        self.assertEqual(d, None)
+        self.assertEqual(d, datetime(2016, 12, 23, 0, 0))


Fixed in 83769a6

cornelinux · 2017-05-01T09:06:15Z

privacyidea/lib/utils.py

-            log.debug("Dateformat {1!s} did not match date {0!s}".format(
-                date_string, date_format))
+    try:
+        d = parse_date_string(date_string)


Please see 83769a6
I think this is not the most sensible and most definite way?

cornelinux · 2017-05-01T09:26:09Z

privacyidea/lib/tokenclass.py

-        return datetime.datetime.now() > datetime.datetime.strptime(sdate,
-                                                              DATE_FORMAT)
+        #date_change = datetime.datetime.strptime(sdate, DATE_FORMAT)
+        date_change = parse_date_string(sdate)


The user does never set a date in next_pin_change manually. This is only achieved by using the tokenclass method. set_next_pin_change.

You are right to use parse_legacy_time here, since it seems it could occur that old timestamps without TZ but with dayfirst are found in the database.

cornelinux · 2017-05-01T09:31:38Z

privacyidea/lib/tokenclass.py

-        return datetime.datetime.now() > datetime.datetime.strptime(sdate,
-                                                              DATE_FORMAT)
+        #date_change = datetime.datetime.strptime(sdate, DATE_FORMAT)
+        date_change = parse_date_string(sdate)


Please take look at 0cf82fb

Also parse old format without timezone which is utc. Working on #644

Working on #644

fredreichbier · 2017-05-03T10:20:51Z

privacyidea/static/components/token/controllers/tokenDetailController.js

+        m = (m>9 ? '' : '0') + m;
+        o = date_obj.toTimeString().split(" ")[1];
+        // 10:20:11 GMT+0200 (CEST)
+        o = o.substring(3);


Hmm, will this work on all browsers? We should find out if toTimeString is guaranteed to give the offset relative to GMT (which corresponds to UTC).
Maybe using getTimezoneOffset would be safer.

As you can see i pushed this in a86a0ad.
I rebased my local 644/datepicker branch on master, but was not abel to push it.
Obviously there is no other way then to merge it manually into master. Please drop me a note, when merging is OK.

Closes #644 Closes #693

cornelinux added 4 commits April 27, 2017 18:34

Set the time format to 2017-04-27T16:00+0200

19a280a

The datepicker sets the correct timeformat. The timeformat is used in all tests. We need to document the timeformat and we need to be able to *read* the old format from the database and return the new format. Working on #644

Fix offset test

d513a74

working on #644

Add documentation

619a5a6

Working on #644

also read old validity format

75d6487

Closes #644

cornelinux added this to the 2.19 milestone Apr 28, 2017

cornelinux requested a review from fredreichbier April 28, 2017 11:14

fredreichbier reviewed Apr 28, 2017

View reviewed changes

cornelinux added 2 commits May 1, 2017 11:04

fix parse_date for german dates

83769a6

Allow legacy time stamp in next_pin_change

0cf82fb

cornelinux commented May 1, 2017

View reviewed changes

cornelinux self-assigned this May 1, 2017

cornelinux and others added 4 commits May 2, 2017 17:23

Add timezone to last_auth.

caa3130

Also parse old format without timezone which is utc. Working on #644

Parse DD/MM/YY in javascript

dc7fdc0

Fix missing icon for validity datepicker

9de817f

Working on #644

Use local variables in JS date functions

8e8b7a8

Working on #644

fredreichbier reviewed May 3, 2017

View reviewed changes

use timezoneoffset to determine the timezone

a86a0ad

cornelinux merged commit a86a0ad into master May 3, 2017

cornelinux added a commit that referenced this pull request May 3, 2017

Merge branch '644/datepicker'

bfbd653

Closes #644 Closes #693

tamaro-skaljic deleted the 644/datepicker branch June 27, 2019 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

644/datepicker #693

644/datepicker #693

cornelinux commented Apr 28, 2017 •

edited

codecov-io commented Apr 28, 2017 •

edited

fredreichbier Apr 28, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

fredreichbier Apr 28, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

fredreichbier Apr 28, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

fredreichbier Apr 28, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

fredreichbier Apr 28, 2017

cornelinux May 1, 2017

cornelinux left a comment

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

cornelinux May 1, 2017

fredreichbier May 3, 2017

cornelinux May 3, 2017

644/datepicker #693

644/datepicker #693

Conversation

cornelinux commented Apr 28, 2017 • edited

codecov-io commented Apr 28, 2017 • edited

Codecov Report

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

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

cornelinux 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

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

cornelinux commented Apr 28, 2017 •

edited

codecov-io commented Apr 28, 2017 •

edited