Improve filtering dates for specific operators #3362

saba-mo · 2023-05-12T00:43:04Z

Resolves #3338

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.

saba-mo · 2023-05-12T01:04:01Z

Hi Tom, this PR is far from complete but I wanted you to have something to look at. The issue I'm still facing is that the converted values in the FilterChooserFieldSpec are not the values used for comparison when filtering. I am checking FieldFilter's getTestFn to log the actual filter value and record value being compared.

I think the getTestFn could be the place to implement this detection/conversion, and it would address both filterChooser and gridFilter. However, line 110 of FieldFilter sets the filter's fieldType based on the store's fieldType for that field, not taking into account what is set in the FieldSpec by the developer. I think if we could detect the correct fieldSpec, we could handle the implementation in FieldFilter by converting the value on line 175, but I'm not sure how big of a change it is/if we'd want to add fieldType when constructing a FieldFilter.

Another issue I anticipate, though am hopefully wrong, is that for GridFilter, in order to provide a fieldType that differs from the column's fieldType, the dev would need to specify the fieldSpec for all filterable columns by providing a filterModel config and no longer be able to simply set filterModel: true on the gridModel. Maybe an argument for making this the default behavior for date filtering.

saba-mo · 2023-05-19T22:36:19Z

data/filter/BaseFilterFieldSpec.ts

+        this.enableValues =
+            !this.isMismatchedDateFieldType &&
+            (this.hasExplicitValues || (enableValues ?? this.isEnumerableByDefault));


I believe we said we don't want to enable the Values tab in this situation, right? It could be the dev's responsibility to set enableValues to false on the fieldSpec, but I think the default behavior of not showing it for dates makes sense.

It would be great to enableValues if we could find a clean way to restrict the values to the unique local dates in the dataset, rather than the full date timestamp. If this proves too complex however perhaps its not worth it.

saba-mo · 2023-05-19T22:41:10Z

data/filter/BaseFilterFieldSpec.ts

@@ -165,6 +189,7 @@ export abstract class BaseFilterFieldSpec extends HoistBase {
    private getDefaultOperators(): FieldFilterOperator[] {
        if (this.isBoolFieldType) return ['='];
        if (this.isCollectionType) return ['includes', 'excludes'];
+        if (this.isMismatchedDateFieldType) return ['>', '>=', '<', '<='];


This ensures the FilterChooser and GridFilter will offer the same operators for this field, which is not currently the case in the ActivityTracking example - only the GridFilter offers equals/does not equal.

I don't think we should be using the ActivityTracking tab as gospel for the operators we want to support - it would be better if we could support the full range of operators normally available for localDates.

That said, to support e.g. date = 2023-05-31, we need to produce a compound "between" filter like date >= 2023-05-31 00:00:00 AND date < 2023-06-01 00:00:00. Honestly not sure its worth the complexity :|

saba-mo · 2023-05-19T22:45:02Z

desktop/cmp/grid/impl/filter/custom/CustomRow.ts

+            valueType: fieldSpec.isMismatchedDateFieldType
+                ? 'date'
+                : (fieldSpec.fieldType as 'localDate' | 'date')


I noticed that if you picked a date and then closed and reopened the popover, the dateInput wouldn't render the value, due to the difference in fieldTypes.

Makes sense - we need to input to be able to read dates since that is what the filter is actually using 👍

saba-mo · 2023-05-19T23:10:24Z

cmp/filter/impl/Option.ts

+        fieldType =
+            fieldType === 'tags'
+                ? 'string'
+                : fieldSpec.isMismatchedDateFieldType
+                ? 'date'
+                : fieldType;


If we use localDate as the fieldType, we end up passing a date to the localDate case in Field's parseFieldValue function and would then need to use LocalDate.from(val) instead of LocalDate.get(val).

saba-mo · 2023-05-19T23:14:20Z

@TomTirapani FilterChooser and GridFilter now provide the same behavior as long as the developer indicates 'localDate' as the fieldSpec's fieldType. If we move forward with this approach, I wonder how we can make it more obvious that it's necessary, since I imagine it's a rather common use case. I believe this approach also still allows for true datetime comparisons.

# Conflicts: # CHANGELOG.md

TomTirapani

This looks and works great.

It's a shame we can't support the full "local date experience"; namely, the Values tab and = and != operators. We'll need to assess the added complexity of supporting them and see if its worth it for this convenience feature - I hope we can find a way to support the former without too much work, but I suspect the latter is just not worth the complexity of producing compound filters.

TomTirapani · 2023-05-31T10:58:00Z

data/filter/BaseFilterFieldSpec.ts

@@ -165,6 +189,7 @@ export abstract class BaseFilterFieldSpec extends HoistBase {
    private getDefaultOperators(): FieldFilterOperator[] {
        if (this.isBoolFieldType) return ['='];
        if (this.isCollectionType) return ['includes', 'excludes'];
+        if (this.isMismatchedDateFieldType) return ['>', '>=', '<', '<='];


I don't think we should be using the ActivityTracking tab as gospel for the operators we want to support - it would be better if we could support the full range of operators normally available for localDates.

That said, to support e.g. date = 2023-05-31, we need to produce a compound "between" filter like date >= 2023-05-31 00:00:00 AND date < 2023-06-01 00:00:00. Honestly not sure its worth the complexity :|

TomTirapani · 2023-05-31T11:05:23Z

desktop/cmp/grid/impl/filter/custom/CustomRow.ts

+            valueType: fieldSpec.isMismatchedDateFieldType
+                ? 'date'
+                : (fieldSpec.fieldType as 'localDate' | 'date')


Makes sense - we need to input to be able to read dates since that is what the filter is actually using 👍

TomTirapani · 2023-05-31T11:09:41Z

data/filter/BaseFilterFieldSpec.ts

+        this.enableValues =
+            !this.isMismatchedDateFieldType &&
+            (this.hasExplicitValues || (enableValues ?? this.isEnumerableByDefault));


It would be great to enableValues if we could find a clean way to restrict the values to the unique local dates in the dataset, rather than the full date timestamp. If this proves too complex however perhaps its not worth it.

saba-mo · 2023-06-02T02:35:16Z

Regarding enabling the Values tab, I'm still investigating how to compare the correct values, but it looks like in order to get just the unique local dates to appear in the tab, we could check filterDateAsLocalDate in the ValuesTabModel's values getter and return the converted values there. We'd have to check it again in the value column renderer and override the date column's renderer which likely includes a timestamp (as is the case of the activity tracking grid). I can't think of a specific example right now, but it could be problematic to not allow the developer to provide a custom renderer.

saba-mo · 2023-06-13T16:20:40Z

See #3389 for details on why we are not pursuing enabling the Values tab and = and != operators.

lbwexler · 2023-06-26T21:40:06Z

@amcclain wanted to take a look at this as well -- adding as a reviewer

lbwexler · 2023-07-07T10:03:24Z

@amcclain -- do you want to take a look at this? Would love to include this one in the current release if possible

amcclain · 2023-07-07T14:06:43Z

@lbwexler yes, will do - caught it up with develop, will look further this morning / early afternoon.

saba-mo added 2 commits May 11, 2023 17:31

Detect if field types differ and convert record values

1cc4a83

Merge branch 'develop' into filterAsLocalDate

4a9573b

saba-mo requested a review from TomTirapani May 12, 2023 01:04

saba-mo added 8 commits May 11, 2023 20:51

Detect if field types differ and convert record values for gridFilter

6181ecc

Merge branch 'develop' into filterAsLocalDate

c2a9569

Apply default value parser and ops to filterChooser

d1ff23e

Apply end of day logic to GridFilter

f7c4c51

Changelog

b8fdbf5

Edit CL entry

7d9c58f

Edit comments

db74226

DRY

ac0b3fc

saba-mo commented May 19, 2023

View reviewed changes

Typo

6e63c83

saba-mo commented May 19, 2023

View reviewed changes

Use source field type of date to avoid converting to LocalDate

2419502

saba-mo commented May 19, 2023

View reviewed changes

saba-mo marked this pull request as ready for review May 19, 2023 23:14

Make same changes for ClientErrors tab

4caca2e

saba-mo mentioned this pull request May 19, 2023

Improve GridFilter's filtering of date fieldType for specific operators #3339

Closed

6 tasks

saba-mo changed the title ~~Handle filtering dates as localDates~~ Improve filtering dates for specific operators May 19, 2023

TomTirapani added 2 commits May 30, 2023 18:11

Merge branch 'develop' into filterAsLocalDate

a4e8795

# Conflicts: # CHANGELOG.md

Tweaks

d66a5c6

TomTirapani requested changes May 31, 2023

View reviewed changes

saba-mo added 2 commits May 31, 2023 16:02

Merge branch 'develop' into filterAsLocalDate

d598c23

Merge branch 'develop' into filterAsLocalDate

e9d63e4

Merge branch 'develop' into filterAsLocalDate

5c04b8b

TomTirapani added 2 commits June 20, 2023 18:51

Merge branch 'develop' into filterAsLocalDate

b3d2041

Comment and CHANGELOG tweaks

248ea65

TomTirapani self-requested a review June 20, 2023 17:12

TomTirapani approved these changes Jun 20, 2023

View reviewed changes

lbwexler self-requested a review June 26, 2023 21:37

lbwexler approved these changes Jun 26, 2023

View reviewed changes

lbwexler requested a review from amcclain June 26, 2023 21:39

amcclain added 2 commits July 7, 2023 06:41

Merge branch 'develop' into filterAsLocalDate

3c3b197

Move changelog entry to current release

cde383f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve filtering dates for specific operators #3362

Improve filtering dates for specific operators #3362

saba-mo commented May 12, 2023 •

edited

Loading

saba-mo commented May 12, 2023

saba-mo May 19, 2023

TomTirapani May 31, 2023

saba-mo May 19, 2023

TomTirapani May 31, 2023

saba-mo May 19, 2023

TomTirapani May 31, 2023

saba-mo May 19, 2023

saba-mo commented May 19, 2023

TomTirapani left a comment

TomTirapani May 31, 2023

TomTirapani May 31, 2023

TomTirapani May 31, 2023

saba-mo commented Jun 2, 2023

saba-mo commented Jun 13, 2023

lbwexler commented Jun 26, 2023

lbwexler commented Jul 7, 2023

amcclain commented Jul 7, 2023

Improve filtering dates for specific operators #3362

Are you sure you want to change the base?

Improve filtering dates for specific operators #3362

Conversation

saba-mo commented May 12, 2023 • edited Loading

Hoist P/R Checklist

saba-mo commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saba-mo commented May 19, 2023

TomTirapani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saba-mo commented Jun 2, 2023

saba-mo commented Jun 13, 2023

lbwexler commented Jun 26, 2023

lbwexler commented Jul 7, 2023

amcclain commented Jul 7, 2023

saba-mo commented May 12, 2023 •

edited

Loading