You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/06/14 07:17:50 UTC
[GitHub] [superset] villebro opened a new pull request #15117: feat(native-filters): add optional time col to time range
villebro opened a new pull request #15117:
URL: https://github.com/apache/superset/pull/15117
### SUMMARY
Adds an optional time column option to the filter config modal to achieve full feature parity with Filter Box.
### BEFORE
Currently time ranges can only be applied to the default time column:
### AFTER
When no time range is specified, the time column control is hidden:
When a time range is specified, the optional time column control is exposed (when left unpopulated, the default time column is used):
In addition, the sort metric is moved to a row of it's own:
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #15117:
URL: https://github.com/apache/superset/pull/15117#issuecomment-859599166
# [Codecov](https://codecov.io/gh/apache/superset/pull/15117?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15117](https://codecov.io/gh/apache/superset/pull/15117?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1ff0498) into [master](https://codecov.io/gh/apache/superset/commit/5e825cf06302841bece9b0a3463298ee0e63750c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e825cf) will **decrease** coverage by `0.01%`.
> The diff coverage is `56.00%`.
> :exclamation: Current head 1ff0498 differs from pull request most recent head 8ebb589. Consider uploading reports for the commit 8ebb589 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/15117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #15117 +/- ##
==========================================
- Coverage 77.54% 77.52% -0.02%
==========================================
Files 967 967
Lines 49759 49798 +39
Branches 6352 6364 +12
==========================================
+ Hits 38585 38607 +22
- Misses 10972 10989 +17
Partials 202 202
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `72.42% <56.00%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15117?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/dashboard/actions/hydrate.js](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2h5ZHJhdGUuanM=) | `1.72% <ø> (ø)` | |
| [...mponents/nativeFilters/FiltersConfigModal/utils.ts](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL3V0aWxzLnRz) | `66.25% <ø> (ø)` | |
| [...nd/src/dashboard/components/nativeFilters/utils.ts](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvdXRpbHMudHM=) | `86.25% <ø> (ø)` | |
| [...nd/src/dashboard/containers/DashboardComponent.jsx](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZENvbXBvbmVudC5qc3g=) | `92.30% <ø> (ø)` | |
| [...-frontend/src/dashboard/reducers/dashboardState.js](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9yZWR1Y2Vycy9kYXNoYm9hcmRTdGF0ZS5qcw==) | `73.33% <0.00%> (ø)` | |
| [...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL0ZpbHRlcnNDb25maWdGb3JtLnRzeA==) | `68.83% <16.66%> (-1.24%)` | :arrow_down: |
| [...t-frontend/src/dashboard/actions/dashboardState.js](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL2Rhc2hib2FyZFN0YXRlLmpz) | `28.65% <33.33%> (ø)` | |
| [...nd/src/dashboard/components/nativeFilters/state.ts](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvc3RhdGUudHM=) | `75.00% <64.10%> (-25.00%)` | :arrow_down: |
| [...d/src/dashboard/components/gridComponents/Tabs.jsx](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL1RhYnMuanN4) | `86.79% <85.71%> (-0.71%)` | :arrow_down: |
| [...ilters/FilterBar/FilterControls/FilterControls.tsx](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlckNvbnRyb2xzLnRzeA==) | `80.00% <100.00%> (+2.41%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/apache/superset/pull/15117/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15117?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15117?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5e825cf...8ebb589](https://codecov.io/gh/apache/superset/pull/15117?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro merged pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #15117:
URL: https://github.com/apache/superset/pull/15117
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje commented on pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
kgabryje commented on pull request #15117:
URL: https://github.com/apache/superset/pull/15117#issuecomment-859597639
A couple of points to discuss:
1. Should the time range picker and time column be in 1 row? In my opinion that would strongly suggest to the user that those 2 fields are closely related. In my opinion, due to rther large vertical distance between time range and time column, they seem like separate values at first glance 
2. Should we prefill time column with the default value rather than leaving it empty? That would make it clearer which column is being used, especially when user doesn't know which temporal column is default
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #15117:
URL: https://github.com/apache/superset/pull/15117#issuecomment-859610899
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a change in pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #15117:
URL: https://github.com/apache/superset/pull/15117#discussion_r650916244
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.test.tsx
##########
@@ -94,3 +107,23 @@ test('Should call "getClientErrorObject" when api returns an error', async () =>
expect(spy).toBeCalled();
});
});
+
+test('Should filter results', async () => {
+ const props = createProps({
+ filterValues: (column: Column) => column.is_dttm,
+ });
+ const { container } = render(<ColumnSelect {...(props as any)} />, {
+ useRedux: true,
+ });
+ expect(container.children).toHaveLength(1);
+ userEvent.type(screen.getByRole('combobox'), 'column_name');
+ await waitFor(() => {
Review comment:
Same as previous comment
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.test.tsx
##########
@@ -37,34 +39,45 @@ fetchMock.get('http://localhost/api/v1/dataset/456', {
body: {
result: {
columns: [
- { column_name: 'column_name_04' },
- { column_name: 'column_name_05' },
- { column_name: 'column_name_06' },
+ { column_name: 'column_name_04', is_dttm: false },
+ { column_name: 'column_name_05', is_dttm: false },
+ { column_name: 'column_name_06', is_dttm: false },
],
},
},
});
fetchMock.get('http://localhost/api/v1/dataset/789', { status: 404 });
-const createProps = () => ({
+const createProps = (extraProps: JsonObject = {}) => ({
filterId: 'filterId',
form: { getFieldValue: jest.fn(), setFields: jest.fn() },
datasetId: 123,
value: 'column_name_01',
onChange: jest.fn(),
+ ...extraProps,
});
afterAll(() => {
fetchMock.restore();
});
-test('Should render', () => {
+test('Should render', async () => {
const props = createProps();
const { container } = render(<ColumnSelect {...(props as any)} />, {
useRedux: true,
});
expect(container.children).toHaveLength(1);
+ userEvent.type(screen.getByRole('combobox'), 'column_name');
+ await waitFor(() => {
Review comment:
You can join them all in one `waitFor`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] junlincc commented on pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #15117:
URL: https://github.com/apache/superset/pull/15117#issuecomment-859687271
change request: select show only when there are more than 1 time columns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a change in pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #15117:
URL: https://github.com/apache/superset/pull/15117#discussion_r649956083
##########
File path: superset-frontend/src/dashboard/components/nativeFilters/utils.ts
##########
@@ -74,7 +75,7 @@ export const getFormData = ({
adhoc_filters: adhoc_filters ?? [],
extra_filters: [],
extra_form_data: cascadingFilters,
- granularity_sqla: 'ds',
+ granularity_sqla,
Review comment:
😁
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #15117:
URL: https://github.com/apache/superset/pull/15117#issuecomment-860659082
One nit: the sort type and sort metric have different widths.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kgabryje edited a comment on pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
kgabryje edited a comment on pull request #15117:
URL: https://github.com/apache/superset/pull/15117#issuecomment-859597639
A couple of points to discuss:
1. Should the time range picker and time column be in 1 row? That would strongly suggest to the user that those 2 fields are closely related. In my opinion, due to a rather large vertical distance between time range and time column, they seem like separate values at first glance 
2. Should we prefill time column with the default value rather than leaving it empty? That would make it clearer which column is being used, especially when user doesn't know which temporal column is default
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro edited a comment on pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
villebro edited a comment on pull request #15117:
URL: https://github.com/apache/superset/pull/15117#issuecomment-859610899
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #15117: feat(native-filters): add optional time col to time range
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #15117:
URL: https://github.com/apache/superset/pull/15117#issuecomment-861382330
Merging to unblock work on a related issue (will address the test comment in a separate PR)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org