Improve GridFilter's filtering of date fieldType for specific operators

[saba-mo]

See #3338 (partially resolves ticket)

I'm not sure if we need to consider changes to the Values tab as well. I don't believe it is used for the date fieldType.

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

• Caught up with develop branch as of last change.
• Added CHANGELOG entry, or determined not required.
• Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
• Updated doc comments / prop-types, or determined not required.
• Reviewed and tested on Mobile, or determined not required.
• Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

[amcclain]

No need to call moment again here, as you already have a moment.

[TomTirapani]

Hi Saba - thanks for filing the issue and taking it on!

However, unless I'm misunderstanding something, the special handling for > & <= on Dates comes from ActivityTrackModel's implementation of FilterChooserModel - the actual default behaviour for FilterChooser and Grid Filtering is the same. Merging this PR would actually be introducing a discrepancy rather than resolving one. A better solution would be to make sure ActivityTrackModel's grid filtering uses the same fieldSpecs via GridModel.filterModel.

That said, I wonder if the special handling is something we want to always do for Dates in the FieldFilter's testFn? Wouldn't we always want to work with the end of day in those cases?

[saba-mo]

Hi Tom, sorry for not providing the full context. Yes, the default behavior is currently the same, and yes, this PR would introduce a discrepancy that would require the developer to customize filterChooser to behave similarly if utilizing both filters. The end goal is to set this as the default behavior for both, but wanted to get in a partial fix as available.

And yes, we always want to work with the end of day in these two cases. I will look into FieldFilter's testFn as you suggest - maybe that's the place to make the change for both!

[amcclain]

Need to be careful that we don't prevent FieldFilter from doing true datetime comparisons.

The grid filter case is somewhat special as it presents a date input w/o time component when building a filter for a date column, so we know that the user can only provide date-level granularity and therefore we want to take this approach to comparisons.

That said, FieldFilter could check and if its value (i.e. the provided value that should be checked against record-level values) was a LocalDate then it could act this way - which is maybe what you were thinking. Then the grid filtering component could always emit LocalDates unless the developer customized it otherwise. Note that is not what it's currently doing:

hoist-react/desktop/cmp/grid/impl/filter/custom/CustomRow.ts

Line 85 in b440cae

valueType: fieldSpec.fieldType as 'localDate' | 'date'

[lbwexler]

Like the idea of recognizing LocalDate in the test function, and having the UI emit LocalDate whenever it is not displaying time --- think that really leverages LocalDate appropriately.

[saba-mo]

I may be misunderstanding, but if we change the valueType to strictly 'localDate' in CustomRow and then have FieldFilter still call parseFieldValue with the column's fieldType of 'date' (like the activity tracking example), we get a Date back for the prior day. So checking if it was a localDate and, if so, setting it to end of day still leaves us at the end of the prior day.

[TomTirapani]

Was thinking about this, and realised the root issue with this tab is that we're trying to filter a Date as if it was a LocalDate; I think that's what we should be trying to solve.

A dev should be able to leave the field as Date in the Grid and Store, but filter is as a LocalDate by setting FilterChooserModel.fieldSpecs.fieldType to localDate and it should work. We wouldn't need the valueParser or valueRenderer stuff as LocalDate already handles the text input & > & <= operators properly.

The tricky part would be safely and consistently parsing the Record's date into a localDate before handing off to filtering. This ability to 'cast' before filtering is a new feature, but one that I think makes sense for Date <> LocalDate, and maybe some other fields types.

What do you guys think? Is the ability to override and cast the field type via the FilterFieldSpec something that could be generally useful?

[lbwexler]

Tom --- that sounds very compelling to me. Wonder if that similar logic would apply for say, filtering an 'auto' as a 'number/string' or a 'date' as a string (for e.g. starts with matching, or like matching to catch months, etc.)

[amcclain]

I'm totally open to whatever we think can move this ticket forward and get us to a less-broken default state.

The idea of supporting a FilterFieldSpecConfig.fieldType value that does not correspond with the value of the same field within a bound store seems to make sense in the Date > LocalDate case, where we have two types where one can be considered to be a more general version of the other.

We could determine if there are any other such conversions we would want to support (I am not sure that there are), and I suppose we could validate the provided fieldType against a typed store field (if there was one) to determine if the config was valid - i.e. they either need to match, one or both needs to be unspecified, or it's this special store:date filter:localDate case.

I don't see any issue with converting an existing Date to a LocalDate - I don't think we would expect to handle any timezone other than the browser's current timezone, so LocalDate.from(date) should be all we need.

We will still need to decide if this is something we apply automatically for date fields - I'd like to avoid leaving the current default grid filter behavior in place as I don't think it's correct as is.

[saba-mo]

Closing in favor of #3362