Fix/3574 date field doesnt work when date is entered manually #3724

tramuntanal · 2018-06-27T09:21:50Z

🎩 What? Why?

Remove the readonly attribute from date fields and add a text help with the localized format to be used by the user to enter the date manually.

📌 Related Issues

Fixes The input type date_field does not work properly when the date is entered manually #3574

📋 Subtasks

Add CHANGELOG entry
[-] Add documentation regarding the feature
[-] Add/modify seeds
Add tests

📷 Screenshots (optional)

…is_entered_manually

tramuntanal · 2018-07-05T10:52:10Z

Hi @decidim/lot-core , I need to know why dates in english don't use the standard mm/dd/yyyy format. It is a problem to make the datepicker and the FormObjects to use the same parse formats.

mrcasals · 2018-07-05T13:00:39Z

@tramuntanal what format are they using?

tramuntanal · 2018-07-06T07:52:00Z

I think that FormObjects are picking en.date.formats.decidim_short: "%d/%m/%Y", but I can't find were it is configured.

mrcasals · 2018-07-09T07:15:05Z

@tramuntanal a quick search gives me this:

decidim/decidim-core/config/locales/en.yml

Line 52 in c8c316a

decidim_short: "%d/%m/%Y"

decidim/decidim-core/config/locales/en.yml

Line 731 in c8c316a

decidim_short: "%d/%m/%Y %H:%M"

tramuntanal · 2018-07-09T07:54:50Z

Hi @decidim/lot-core ,
Sorry I guess I did not explain myself correctly. I have two questions:

Why English locales use d/m/y format when it should be m/d/y?
How is that FormObjects use d/m/y format? were in the code are FormObject configured to use this format?

mrcasals · 2018-07-09T08:22:49Z

Oh, I misunderstood the problem LOL sorry 😅

For English locales: Maybe it's a typo? Or maybe I did it intentionally (I always thought m/d/y makes zero sense, but who am I to judge?). Let's consider this a typo 😂

For the FormObjects: let me check it first, but maybe it's just using a Date.new(string) and tries to deduce the format

tramuntanal · 2018-07-10T11:57:30Z

FormObjects use Virtus coercion system which is not documented but it seems to use Date.parse for date attributes. Date.parse is also scarcely documented although the "expected" correct format seems to be 'yyyy-mm-dd'. After some stackoverflowing it seems that Date.parse works by parsing by guess, this is why Decidim works with 'dd/mm/yyyy'. Doing exploration through the rails console one can see that it correctly resolves dates in the format 'dd/mm/yyyy' but fails to parse the native english 'mm/dd/yyyy' format, impeding the parsing of localized dates (dates in all Decidim's supported locales).

irb(main):029:0> I18n.locale
=> :en
irb(main):032:0> Date.parse('09/27/2017')
Traceback (most recent call last):
        2: from (irb):32
        1: from (irb):32:in `parse'
ArgumentError (invalid date)
irb(main):033:0> Date.parse('27/09/2017')
=> Wed, 27 Sep 2017
irb(main):036:0> Date.parse('2017-09-27')
=> Wed, 27 Sep 2017

Summarizing, as we use Rectify, and thus Virtus, which rely on Date.parse for parsing dates, the best will be to always force the user to enter dates in the 'yyyy-mm-dd' format, or alternatively use a most common and also supported 'dd/mm/yyyy' format. This will affect users entering dates manually whatever is their locale and rendered dates (datepickers will remain the same as now).

cc/ @decidim/lot-core @decidim/product @decidim/lot-px any thoughts on this?

oriolgual · 2018-07-11T09:10:12Z

Actually, mm/dd/yyyy is mostly used at USA, so it isn't that bad to use dd/mm/yyyy (which makes more sense to me 😅).

…as it is used also in decidim-verificators, and may be used in any public view.

tramuntanal · 2018-07-13T14:00:37Z

Hi @decidim/lot-core ,
I have to do the refactor in this commit in order for the datepicker to work in accordance with manual date entrering.
Also, I noticed that the datepicker format is declared in decidim-admin but datepicker also appears in decidim-verifications public part and in the future may appear in any public view so I think it must belong to core.
The question is how should I proceed to move the keys from decidim-admin to decidim-core? is this PR ok? If not, which is the procedure?

(This PR is still WIP)

mrcasals · 2018-07-16T07:48:46Z

@tramuntanal yes, it's OK if you do it in this PR! 😄

…is_entered_manually

deivid-rodriguez · 2018-08-06T10:57:50Z

I removed some i18n keys that are now unused.

That makes me wonder if we should actually keep them. I originally suggested the removal in #3724 (comment), but I'm now not sure about it. Previously, the formatted dates from pickers could be customized to use their own formatting, now they use decidim_short, so if you change that formatting, it will change all over the application (not only on pickers). I'm not sure if this configurability is actually needed, but restoring the old way would make the diff in the this patch smaller.

Thoughts?

deivid-rodriguez · 2018-08-06T13:52:07Z

Nah, I think it's fine to leave it as it is.

mrcasals · 2018-08-07T07:10:07Z

@deivid-rodriguez I'll wait for @tramuntanal's opinion and, if he thinks everything looks good, I'll merge this 😄

tramuntanal · 2018-08-27T10:52:11Z

decidim-core/lib/decidim/attributes/localized_date.rb

+        return value unless value.is_a?(String)
+        Date.strptime(value, I18n.t("date.formats.decidim_short"))
+      rescue ArgumentError
+        nil


Very clever @deivid-rodriguez !
Creating a custom attribute coercer is much better design than overriding coercible.
Just a note, why swallow the ArgumentError? This will make parsing errors much more difficult to find, I know that they should not happen in production but still when testing they happened to me and was tedious to bind the cause to the specific line.

The situation is described here. Basically, we don't want end users to be able to crash our server at their will, so we need to rescue and swallow. We could keep the nil return but add some logging in between or even append an error to the attribute, but I think the current version is enough for such an edge case...

OK, I understand now. I did not get that the user would provoke a crash when reading your comment before. Indeed raising an error is not what we want.

deivid-rodriguez · 2018-08-27T11:54:44Z

@tramuntanal Could you merge latest master to bring this PR up to date?

…is_entered_manually

mrcasals · 2018-09-10T07:03:26Z

I've rescheduled a CI job, it failed due to a random error

tramuntanal · 2018-09-12T07:17:12Z

Thank you @mrcasals , this PR is now green!

mrcasals · 2018-09-12T07:21:46Z

@deivid-rodriguez do you mind reviewing the code? 😄

deivid-rodriguez

It should be the same patch as before, only rebased. Right @tramuntanal? It looks good to me from a quick look, except for the change log.

deivid-rodriguez · 2018-09-12T10:47:04Z

CHANGELOG.md

+- **decidim-initiatives**: Only show initiative types fomr the current tenant [\#3887](https://github.com/decidim/decidim/pull/3887)
+- **decidim-core**: Allows users with admin access to preview unpublished components [\#4016](https://github.com/decidim/decidim/pull/4016)
+- **decidim-proposals**: Rename "votes" column to "supports" when exporting proposals [\#4018](https://github.com/decidim/decidim/pull/4018)
+


I don't think we've fixed this many things in this PR. Merge problem?

This seems to be a problem with a rebase after we released the 0.14.0 version.

I've just fixed the CHANGELOG @mrcasals

@deivid-rodriguez , yes, I only merged master to solve conflicts.

Ok! I'll merge this as soon as CI passes!

…is_entered_manually

deivid-rodriguez · 2018-09-13T23:46:54Z

Going in finally!

tramuntanal · 2018-09-14T06:42:27Z

👏 🎉 🎉 🎉

tramuntanal added 5 commits June 22, 2018 10:36

[REFACTOR] Remove hidden field.

4aeb961

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

e5eb60a

…is_entered_manually

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

5ab5cd2

…is_entered_manually

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

be7235a

…is_entered_manually

[REFACTOR] Allow users to enter date fields manually.

cfb00f4

tramuntanal self-assigned this Jun 27, 2018

ghost added the status: WIP label Jun 27, 2018

tramuntanal added 5 commits June 27, 2018 11:24

Add Changelog entry.

68e215b

Normalize locales.

c721c3b

[FIX] Check classes with class not with string.[ci skip]

761d62f

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

f54cb6c

…is_entered_manually

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

bbac361

…is_entered_manually

[REFACTOR] Move datepicker format form decidim-admin to decidim-core …

a759b9d

…as it is used also in decidim-verificators, and may be used in any public view.

tramuntanal added 6 commits July 16, 2018 10:42

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

dca3d81

…is_entered_manually

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

a9ccdbe

…is_entered_manually

[REFACTOR] Rename I18n key.

98cbe76

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

db6df8f

…is_entered_manually

Rubocopify.

bb51ff9

[TEST|REFACTOR] Update tag ids for date input fields.

f224407

tramuntanal force-pushed the fix/3574-date_field_doesnt_work_when_date_is_entered_manually branch from 8ba234f to f224407 Compare July 17, 2018 16:19

mrcasals previously approved these changes Aug 6, 2018

View reviewed changes

Remove now unused i18n keys

1adeb07

deivid-rodriguez dismissed mrcasals’s stale review via 1adeb07 August 6, 2018 10:49

Fixup changelog entry with link to PR

ea7a7b2

deivid-rodriguez removed the status: WIP label Aug 6, 2018

tramuntanal commented Aug 27, 2018

View reviewed changes

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

f8a1006

…is_entered_manually

ghost added the status: WIP label Aug 27, 2018

tramuntanal added 2 commits September 5, 2018 16:48

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

0983a41

…is_entered_manually

[FIX] In CHANGELOG.md, lists should be surrounded by blank lines

0bc7e8c

tramuntanal removed the status: WIP label Sep 12, 2018

deivid-rodriguez reviewed Sep 12, 2018

View reviewed changes

tramuntanal added 2 commits September 13, 2018 22:25

Merge branch 'master' into fix/3574-date_field_doesnt_work_when_date_…

decba25

…is_entered_manually

[DOC] Fix bad CHANGELOG after automatic merge.

0d03062

ghost added the status: WIP label Sep 13, 2018

tramuntanal removed the status: WIP label Sep 13, 2018

deivid-rodriguez approved these changes Sep 13, 2018

View reviewed changes

deivid-rodriguez merged commit 7c44534 into master Sep 13, 2018

deivid-rodriguez deleted the fix/3574-date_field_doesnt_work_when_date_is_entered_manually branch September 13, 2018 23:47

aitorlb mentioned this pull request Aug 30, 2019

Fix: AuthorizationFormBuilder does not detect attribute types #5315

Merged

2 tasks

andreslucena mentioned this pull request Sep 8, 2021

Fix birthday attribute type in Assembly Members #8311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/3574 date field doesnt work when date is entered manually #3724

Fix/3574 date field doesnt work when date is entered manually #3724

tramuntanal commented Jun 27, 2018 •

edited

tramuntanal commented Jul 5, 2018

mrcasals commented Jul 5, 2018

tramuntanal commented Jul 6, 2018 •

edited

mrcasals commented Jul 9, 2018

tramuntanal commented Jul 9, 2018

mrcasals commented Jul 9, 2018

tramuntanal commented Jul 10, 2018

oriolgual commented Jul 11, 2018

tramuntanal commented Jul 13, 2018 •

edited

mrcasals commented Jul 16, 2018

deivid-rodriguez commented Aug 6, 2018

deivid-rodriguez commented Aug 6, 2018

mrcasals commented Aug 7, 2018

tramuntanal Aug 27, 2018

deivid-rodriguez Aug 27, 2018 •

edited

tramuntanal Aug 27, 2018

deivid-rodriguez commented Aug 27, 2018

mrcasals commented Sep 10, 2018

tramuntanal commented Sep 12, 2018

mrcasals commented Sep 12, 2018

deivid-rodriguez left a comment

deivid-rodriguez Sep 12, 2018

mrcasals Sep 13, 2018

tramuntanal Sep 13, 2018

tramuntanal Sep 13, 2018

deivid-rodriguez Sep 13, 2018

deivid-rodriguez commented Sep 13, 2018

tramuntanal commented Sep 14, 2018

Fix/3574 date field doesnt work when date is entered manually #3724

Fix/3574 date field doesnt work when date is entered manually #3724

Conversation

tramuntanal commented Jun 27, 2018 • edited

🎩 What? Why?

📌 Related Issues

📋 Subtasks

📷 Screenshots (optional)

tramuntanal commented Jul 5, 2018

mrcasals commented Jul 5, 2018

tramuntanal commented Jul 6, 2018 • edited

mrcasals commented Jul 9, 2018

tramuntanal commented Jul 9, 2018

mrcasals commented Jul 9, 2018

tramuntanal commented Jul 10, 2018

oriolgual commented Jul 11, 2018

tramuntanal commented Jul 13, 2018 • edited

mrcasals commented Jul 16, 2018

deivid-rodriguez commented Aug 6, 2018

deivid-rodriguez commented Aug 6, 2018

mrcasals commented Aug 7, 2018

tramuntanal Aug 27, 2018

Choose a reason for hiding this comment

deivid-rodriguez Aug 27, 2018 • edited

Choose a reason for hiding this comment

tramuntanal Aug 27, 2018

Choose a reason for hiding this comment

deivid-rodriguez commented Aug 27, 2018

mrcasals commented Sep 10, 2018

tramuntanal commented Sep 12, 2018

mrcasals commented Sep 12, 2018

deivid-rodriguez left a comment

Choose a reason for hiding this comment

deivid-rodriguez Sep 12, 2018

Choose a reason for hiding this comment

mrcasals Sep 13, 2018

Choose a reason for hiding this comment

tramuntanal Sep 13, 2018

Choose a reason for hiding this comment

tramuntanal Sep 13, 2018

Choose a reason for hiding this comment

deivid-rodriguez Sep 13, 2018

Choose a reason for hiding this comment

deivid-rodriguez commented Sep 13, 2018

tramuntanal commented Sep 14, 2018

tramuntanal commented Jun 27, 2018 •

edited

tramuntanal commented Jul 6, 2018 •

edited

tramuntanal commented Jul 13, 2018 •

edited

deivid-rodriguez Aug 27, 2018 •

edited