Skip to content

Fix wrong value for lteq_datetime predicate #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

workgena
Copy link
Contributor

[refs #61]

@coveralls
Copy link

coveralls commented Oct 14, 2019

Coverage Status

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

@workgena
Copy link
Contributor Author

image

@workgena
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Fivell
Copy link
Member

Fivell commented Oct 15, 2019

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

@workgena workgena force-pushed the 61_wrong_sql_query_lteq_datetime branch from 79c3b2f to 548d64d Compare October 16, 2019 17:07
@workgena
Copy link
Contributor Author

workgena commented Oct 16, 2019

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 workgena requested a review from Fivell October 16, 2019 20:06
@workgena
Copy link
Contributor Author

workgena commented Oct 16, 2019

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.

@workgena workgena changed the title WIP: Fix wrong value for lteq_datetime predicate Fix wrong value for lteq_datetime predicate Oct 16, 2019
@workgena workgena merged commit bd577c5 into activeadmin-plugins:master Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants