activeadmin-plugins / active_admin_datetimepicker

:calendar: active_admin_datetimepicker gem
MIT License
72 stars 48 forks source link

Fix wrong value for lteq_datetime predicate #63

Closed workgena closed 4 years ago

workgena commented 4 years ago

[refs #61]

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.2%) to 95.363% when pulling 548d64d66d6c2d132c4613bef73839b99bd1ce9a on workgena:61_wrong_sql_query_lteq_datetime into 62443e61f1d4c3353e8fa15de588586617bb0ddc on activeadmin-plugins:master.

workgena commented 4 years ago
image
workgena commented 4 years ago

I only don't like the situation with timezones(client and server).

Current situation: The browser sends selected time, and this time will be passed to SQL query as is. The user should know what timezone database-server uses, usually it is UTC.

This is common misunderstanding situation. And I don't know how to handle it. Or it is not the responsibility of a this plugin?

workgena commented 4 years ago

@senid231 @Fivell perhaps you have better solution? Feel free to change/add/replace the PR.

Fivell commented 4 years ago

@workgena searching should be performed in the same timezone by default, not sure why not :)

workgena commented 4 years ago

After some research I'm intent to merge current solution. It fixes the problem with

  config.add_predicate 'lteq_datetime',
    arel_predicate: 'lt',
    formatter: ->(v) { v + 1.day }

https://github.com/activeadmin/activeadmin/blob/a496b8c74356880bab934202994ac43117752e54/lib/ransack_ext.rb#L17-L19

The problem with timezone is complicated and is out of scope #61 , so it would be better to solve it in another PR.

workgena commented 4 years ago

NOTICE:

With this change we introduce new predicate gteq_datetime_picker. It means the HTML will be different from previous versions:

<!-- old -->
<input id="q_created_at_gteq_datetime">

<!-- new -->
<input id="q_created_at_gteq_datetime_picker">

If somebody relies on this in Capybara tests, then such tests should be updated.