Page MenuHomeIn-Portal Phabricator

INP-1746 Correct time parsing in date range filter
ClosedPublic

Authored by erik on Sep 17 2018, 1:41 PM.

Details

Summary

Made changes in dearch by date logic.

Test Plan

Preparation

  1. go to Website & ContentLabels & Phrases section
  2. modify some phrase, if all minutes or days in the "Modified On" column are equal
  3. confirm, that are visible records with different minutes and different days in the "Modified On" column

Date/Time configuration - for current language set:

  1. "Date Format" to Y-m-d
  2. "Time Format" to H:i:s
  3. "Input Date Format" to m/d/Y
  4. "Input Time Format" to g:i:s A

Testing "Modified On" column filter

  1. specify one from date only (no time)
  2. confirm, that records with other dates are filtered out
  3. specify one to date only (no time)
  4. confirm, that records with other dates are filtered out
  5. specify one to date only (with time)
  6. confirm, that records with other dates or minutes are filtered out (ignoring seconds)
  7. specify one from date only (with time)
  8. confirm, that records with other dates or minutes are filtered out (ignoring seconds)
  9. specify both from & to dates with time
  10. confirm, that records with dates between specified datetimes (ignoring seconds) are visible, but other are filtered out
  11. specify both from & to dates with time in output format (without seconds)
  12. confirm, that records with dates between specified datetimes (ignoring seconds) are visible, but other are filtered out
  13. specify both from & to dates with time in input format (with seconds)
  14. confirm, that records with dates between specified datetimes (ignoring seconds) are visible, but other are filtered out
  15. specify word "test" in place of from date and empty to date
  16. confirm, that no records found
  17. specify word "test" in place of to date and empty from date
  18. confirm, that no records found
  19. specify word "test" in place of from date and existing among records to date
  20. confirm, that no records found
  21. specify word "test" in place of to date and existing among records from date
  22. confirm, that no records found
  23. in search filter specify date using format from "Input Date Format" field without time (m/d/Y)
  24. confirm that right search results are shown

Diff Detail

Repository
rINP In-Portal
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

erik created this revision.Sep 17 2018, 1:41 PM
erik edited the test plan for this revision. (Show Details)Sep 17 2018, 1:44 PM
glebs accepted this revision.Sep 18 2018, 4:03 AM
glebs accepted this revision.
glebs edited reviewers, added: glebs; removed: alex.
This revision is now accepted and ready to land.Sep 18 2018, 4:04 AM
alex added a project: Restricted Project.Sep 19 2018, 2:09 AM
alex requested changes to this revision.Sep 19 2018, 3:17 AM
alex added a subscriber: alex.
alex added inline comments.
core/units/helpers/search_helper.php
605 ↗(On Diff #863)
NOTE: battle against malformed dates entered by user

change into:

if ( is_numeric($from) && $to === null ) {

this way to date entered in incorrect format won't be handled as to date not specified

606 ↗(On Diff #863)
  1. change condition from
$from == strtotime(date('Y-m-d', $from) . ' 00:00:00', $from)

into

date('H:i:s', $from) == '00:00:00'
  1. combine with condition from previous IF statement

  • less code
  • purpose of the code is cleaner
610 ↗(On Diff #863)
NOTE: battle against malformed dates entered by user

change into:

if ( $from === null && is_numeric($to) ) {

this way from date entered in incorrect format won't be handled as from date not specified

611 ↗(On Diff #863)
  1. change condition from
$to == strtotime(date('Y-m-d', $to) . ' 00:00:00', $to)

into

date('H:i:s', $to) == '00:00:00'
  1. combine with condition from previous IF statement

  • less code
  • purpose of the code is cleaner
617–620 ↗(On Diff #863)
NOTE: battle against malformed dates entered by user

change into:

if ( is_numeric($from) && $to === null || $from === null && is_numeric($to) ) {
    $from = $from === null ? $to : $from;
    $to = $from;
}

this way we won't replace malformed from/to date entered by user with to/from date, that user entered correctly

622 ↗(On Diff #863)
NOTE: battle against malformed dates entered by user

change into:

if ( is_numeric($from) && is_numeric($to) ) {

this way only correctly entered dates would be used in filter

622–626 ↗(On Diff #863)
NOTE: battle against malformed dates entered by user

Please add else statement, that would set $filter_value variable value to FALSE (as string).


this way if we've submitted filter with incorrectly formatted date, then we'll see 0 records instead of all records

687 ↗(On Diff #863)
NOTE: battle against malformed dates entered by user

return null instead of false


this way recipient would know if user haven't specified any value

717–719 ↗(On Diff #863)

remove else statement, but keep the code inside it


since code inside else statement is always executed it's pointless to wrap return ... statements with it

This revision now requires changes to proceed.Sep 19 2018, 3:17 AM
alex added a comment.EditedSep 19 2018, 3:19 AM

The inline comments with NOTE: battle against malformed dates entered by user line above them are actually UX discovered issues, that weren't part of original task, but without fixing them user would be as confused as now thinking that date entered in incorrect format is actually working as filter, when it's not.

Please also update test plan to reflect handling of incorrectly entered dates.

erik updated this revision to Diff 865.Sep 19 2018, 5:31 AM
erik edited edge metadata.

Made filter out all records when incorrect date entered.

erik edited the test plan for this revision. (Show Details)Sep 19 2018, 5:35 AM
glebs accepted this revision.Sep 20 2018, 6:04 AM
alex requested changes to this revision.Sep 21 2018, 2:39 AM
alex added inline comments.
core/units/helpers/search_helper.php
686 ↗(On Diff #865)
  1. extract $value[$type] into temporary variable
  2. apply all manipulations, that were done on $value[$type] to that temporary variable instead

When performing recursive method call the altered $value argument (trimmed & time added, when specified by user) is given and that results in the parsing error.

Test plan fragment to cover this:

  1. for current language set:
    • "Date Format" to Y-m-d
    • "Time Format" to H:i:s
    • "Input Date Format" to m/d/Y
    • "Input Time Format" to g:i:s A
  2. in search filter specify date using format from "Input Date Format" field without time
This revision now requires changes to proceed.Sep 21 2018, 2:39 AM
alex added inline comments.Sep 21 2018, 2:48 AM
core/units/helpers/search_helper.php
695–703 ↗(On Diff #865)

With current test plan this code is never executed on recursive call to this method. I recommend debugging actual code to verify, that it works.

erik updated this revision to Diff 866.Sep 21 2018, 5:54 AM
erik edited edge metadata.

Fixed bug in recursive call parameter.

erik edited the test plan for this revision. (Show Details)Sep 21 2018, 6:00 AM
glebs accepted this revision.Sep 27 2018, 7:11 AM
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Feb 9, 2:52 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.