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 2022/11/18 15:02:10 UTC

[GitHub] [superset] cccs-RyanK opened a new pull request, #22168: Adhoc dashboard native filters

cccs-RyanK opened a new pull request, #22168:
URL: https://github.com/apache/superset/pull/22168

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   Our users really like the functionality of the adhoc filters in the explore view and were interested in something similar for for dashboard filtering. Anything that can be done with an adhoc filter native filter can be done with the other filter types and vice versa, but in order to filter by any given column the user has to either:
   - create a new filter (which can be a hassle to do on an adhoc basis),
   - or have a value filter for every column in the dashboard which is not feasible in every dashboard.
   
   In this PR we added a new native filter type that reuses the adhoc filter component from the explore view to allow for more complex filtering to easily be done in a dashboard.
   
   This solution provides the user with a lot of power and flexibility in dashboards, which is why it is behind a feature flag for now. 
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   https://user-images.githubusercontent.com/102618419/202734225-c44cfa32-40e5-4edd-8cab-83dafe94bb2a.mp4
   
   https://user-images.githubusercontent.com/102618419/202734246-d650a4e7-c964-4f07-8a31-ea6e0e907270.mp4
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   - Enable Feature Flag: ADHOC_DASHBOARD_NATIVE_FILTERS
   - Navigate to dashboard
   - Add/Edit Filters on the sidebar
   - A new filter type should appear called "Adhoc"
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [x] Required feature flags: ADHOC_DASHBOARD_NATIVE_FILTERS
   - [x] 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
   - [x] 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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] yousoph commented on pull request #22168: feat(native-filters): Adhoc dashboard native filters

Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1513845648

   Cool! Thanks for this feature! Two comments: 
   1. The + button feels out of place, I think we can hide it like it's hidden from the chart builder: 
   ![image](https://user-images.githubusercontent.com/10627051/232914816-77cf6196-ee53-4f10-b6a6-2485608fa63b.png)
   2. I don't think we use the terminology "ad hoc filters" in any user facing copy right now, are there any other names that might make it clear to the user about what the filter does? I'm having trouble naming it, would love any ideas! 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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 #22168: Adhoc dashboard native filters

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1320156289

   # [Codecov](https://codecov.io/gh/apache/superset/pull/22168?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 [#22168](https://codecov.io/gh/apache/superset/pull/22168?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b72fb28) into [master](https://codecov.io/gh/apache/superset/commit/e990690dde9d3a5dbc6eeacde651a06e3a8d1ce7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e990690) will **decrease** coverage by `11.27%`.
   > The diff coverage is `81.81%`.
   
   > :exclamation: Current head b72fb28 differs from pull request most recent head aaf0a9b. Consider uploading reports for the commit aaf0a9b to get more accurate results
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #22168       +/-   ##
   ===========================================
   - Coverage   67.00%   55.72%   -11.28%     
   ===========================================
     Files        1835     1835               
     Lines       69971    69971               
     Branches     7588     7588               
   ===========================================
   - Hits        46882    38990     -7892     
   - Misses      21123    29015     +7892     
     Partials     1966     1966               
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `52.60% <ø> (ø)` | |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `52.49% <ø> (ø)` | |
   | python | `57.80% <ø> (-23.59%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `50.89% <ø> (ø)` | |
   
   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/22168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ø> (ø)` | |
   | [...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx](https://codecov.io/gh/apache/superset/pull/22168/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==) | `59.50% <ø> (ø)` | |
   | [...d/src/filters/components/Time/TimeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9UaW1lL1RpbWVGaWx0ZXJQbHVnaW4udHN4) | `0.00% <ø> (ø)` | |
   | [superset-frontend/src/filters/components/common.ts](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9jb21tb24udHM=) | `83.33% <ø> (ø)` | |
   | [...-frontend/src/visualizations/presets/MainPreset.js](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL3ByZXNldHMvTWFpblByZXNldC5qcw==) | `100.00% <ø> (ø)` | |
   | [superset/config.py](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQvY29uZmlnLnB5) | `91.22% <ø> (-0.63%)` | :arrow_down: |
   | [superset-frontend/src/filters/utils.ts](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvdXRpbHMudHM=) | `92.10% <75.00%> (ø)` | |
   | [...ters/FiltersConfigModal/FiltersConfigForm/utils.ts](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdGb3JtL3V0aWxzLnRz) | `77.77% <100.00%> (ø)` | |
   | [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset/tags/core.py](https://codecov.io/gh/apache/superset/pull/22168/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-c3VwZXJzZXQvdGFncy9jb3JlLnB5) | `4.54% <0.00%> (-95.46%)` | :arrow_down: |
   | ... and [295 more](https://codecov.io/gh/apache/superset/pull/22168/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) | |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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


Re: [PR] feat(native-filters): Adhoc dashboard native filters [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1922202222

   Setting this to draft, but please make it ready for review when it's... ready for review. :) 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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


Re: [PR] feat(native-filters): Adhoc dashboard native filters [superset]

Posted by "michaelkovatt (via GitHub)" <gi...@apache.org>.
michaelkovatt commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1821579839

   > > @cccs-RyanK , @michael-s-molina Is there a permission to control this. I would want this available to Gamma roles and admins will have the normal add/edit filter.
   > 
   > Hey @michaelkovatt! I am not sure I understand all your question. For the first part no there was no permissions in place for this type of filter as of the last update, but for the second part what exactly is the behaviour you were thinking? You would like to configure it so that only certain roles have access to this type of filter?
   
   
   
   > > @cccs-RyanK , @michael-s-molina Is there a permission to control this. I would want this available to Gamma roles and admins will have the normal add/edit filter.
   > 
   > Hey @michaelkovatt! I am not sure I understand all your question. For the first part no there was no permissions in place for this type of filter as of the last update, but for the second part what exactly is the behaviour you were thinking? You would like to configure it so that only certain roles have access to this type of filter?
   
   @cccs-RyanK , yes you are right. Only need to enable this for some roles. Also, wanted to know if there is any timeline for the fix to go as part of next release? 


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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


Re: [PR] feat(native-filters): Adhoc dashboard native filters [superset]

Posted by "cccs-RyanK (via GitHub)" <gi...@apache.org>.
cccs-RyanK commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1823264418

   > @cccs-RyanK , yes you are right. Only need to enable this for some roles. Also, wanted to know if there is any timeline for the feature to go as part of next release?
   
   I have no timeline on finishing it at the moment since I have been using this feature in my fork of superset for quite a while. Your comment the other day brought my attention back to this and I would like to finally push to include it in a future release. One major wall that I ran into was the design of the component when in horizontal view. In the screenshot I have included below you can see that adding a few filters expands the height too much. If you added enough filters you could eventually expand the filterbar to take up the whole screen. I am looking for input/ideas on adjusting it 
   ![image](https://github.com/apache/superset/assets/102618419/ddcf334e-0a29-45c0-b58c-bee3d946776e)
   
   
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] rusackas commented on pull request #22168: feat(native-filters): Adhoc dashboard native filters

Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1513362600

   Pinging @kasiazjc @yousoph to make sure we're ok with the popover approach and other components being recycled in the filters area.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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


Re: [PR] feat(native-filters): Adhoc dashboard native filters [superset]

Posted by "michaelkovatt (via GitHub)" <gi...@apache.org>.
michaelkovatt commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1820164852

   Is there an update on when this will be merged and available ?


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] cccs-RyanK commented on pull request #22168: feat(native-filters): Adhoc dashboard native filters

Posted by "cccs-RyanK (via GitHub)" <gi...@apache.org>.
cccs-RyanK commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1514850642

   > Cool! Thanks for this feature! Two comments:
   > 
   >     1. The + button feels out of place, I think we can hide it like it's hidden from the chart builder:
   >        ![image](https://user-images.githubusercontent.com/10627051/232914816-77cf6196-ee53-4f10-b6a6-2485608fa63b.png)
   > 
   >     2. I don't think we use the terminology "ad hoc filters" in any user facing copy right now, are there any other names that might make it clear to the user about what the filter does? I'm having trouble naming it, would love any ideas!
   
   Thank you for the feedback @yousoph! I agree with the first point I can remove the plus button for sure. For the name maybe "advanced", "extra", or "custom" filters? Not sure if any of those fit.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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 #22168: feat(native-filters): Adhoc dashboard native filters

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1516693682

   > How will this filter behave considering inter filter features such as Values depend on other filters and Filter scopes? Specially in the case where we have a SQL value and it's hard to determine the referenced column/dataset.
   
   @cccs-RyanK Just to be more clear about the above point. There's a feature called **Values depend on other filters** (check [this](https://github.com/apache/superset/pull/23566) PR's video) which allows you to establish an hierarchy between filters. One example is when you have a State and City filters and you want to change the City's values depending on the selected value in State. This all works because there's a clear definition of the dataset and column when creating a filter. When you introduce a SQL filter, we don't know what's the column (or even if the SQL returns a column), so I'm wondering how all the features that depend on this definition will work. I think we need to at least disable these features when working with a SQL filter. If I defined the State filter as a SQL filter, then it shouldn't appear as one of the options in **Values depend on other filters** when configuring the City filter.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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 diff in pull request #22168: feat(native-filters): Adhoc dashboard native filters

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22168:
URL: https://github.com/apache/superset/pull/22168#discussion_r1033560751


##########
superset-frontend/src/filters/components/Adhoc/AdhocFilterPlugin.tsx:
##########
@@ -0,0 +1,254 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable no-param-reassign */
+import {
+  DataMask,
+  ensureIsArray,
+  ExtraFormData,
+  JsonObject,
+  JsonResponse,
+  smartDateDetailedFormatter,
+  SupersetApiError,
+  SupersetClient,
+  t,
+} from '@superset-ui/core';
+import React, { useCallback, useEffect, useState, useMemo } from 'react';
+import { useImmerReducer } from 'use-immer';
+import AdhocFilterControl from 'src/explore/components/controls/FilterControl/AdhocFilterControl';
+import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
+// eslint-disable-next-line import/no-unresolved
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+// eslint-disable-next-line import/no-unresolved
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+// eslint-disable-next-line import/no-unresolved
+import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+// eslint-disable-next-line import/no-unresolved
+import { useChangeEffect } from 'src/hooks/useChangeEffect';
+import { PluginFilterAdhocProps } from './types';
+import {
+  StyledFormItem,
+  FilterPluginStyle,
+  StatusMessage,
+  ControlContainer,
+} from '../common';
+import { getDataRecordFormatter, getAdhocExtraFormData } from '../../utils';
+
+type DataMaskAction =
+  | { type: 'ownState'; ownState: JsonObject }
+  | {
+      type: 'filterState';
+      __cache: JsonObject;
+      extraFormData: ExtraFormData;
+      filterState: {
+        label?: string;
+        filters?: AdhocFilter[];
+        value: AdhocFilter[];
+      };
+    };
+
+function reducer(
+  draft: DataMask & { __cache?: JsonObject },
+  action: DataMaskAction,
+) {
+  switch (action.type) {
+    case 'ownState':
+      draft.ownState = {
+        ...draft.ownState,
+        ...action.ownState,
+      };
+      return draft;
+    case 'filterState':
+      draft.extraFormData = action.extraFormData;
+      // eslint-disable-next-line no-underscore-dangle
+      draft.__cache = action.__cache;
+      draft.filterState = { ...draft.filterState, ...action.filterState };
+      return draft;
+    default:
+      return draft;
+  }
+}
+
+export default function PluginFilterAdhoc(props: PluginFilterAdhocProps) {
+  const {
+    filterState,
+    formData,
+    height,
+    width,
+    setDataMask,
+    setFocusedFilter,
+    unsetFocusedFilter,
+    appSection,
+  } = props;
+  const { enableEmptyFilter, inverseSelection, defaultToFirstItem } = formData;

Review Comment:
   I assume this is a leftover? This should also simplify downstream logic
   ```suggestion
     const { enableEmptyFilter } = formData;
   ```



##########
superset-frontend/src/filters/components/Adhoc/AdhocFilterPlugin.tsx:
##########
@@ -0,0 +1,254 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable no-param-reassign */
+import {
+  DataMask,
+  ensureIsArray,
+  ExtraFormData,
+  JsonObject,
+  JsonResponse,
+  smartDateDetailedFormatter,
+  SupersetApiError,
+  SupersetClient,
+  t,
+} from '@superset-ui/core';
+import React, { useCallback, useEffect, useState, useMemo } from 'react';
+import { useImmerReducer } from 'use-immer';
+import AdhocFilterControl from 'src/explore/components/controls/FilterControl/AdhocFilterControl';
+import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
+// eslint-disable-next-line import/no-unresolved
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+// eslint-disable-next-line import/no-unresolved
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+// eslint-disable-next-line import/no-unresolved
+import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+// eslint-disable-next-line import/no-unresolved
+import { useChangeEffect } from 'src/hooks/useChangeEffect';
+import { PluginFilterAdhocProps } from './types';
+import {
+  StyledFormItem,
+  FilterPluginStyle,
+  StatusMessage,
+  ControlContainer,
+} from '../common';
+import { getDataRecordFormatter, getAdhocExtraFormData } from '../../utils';
+
+type DataMaskAction =
+  | { type: 'ownState'; ownState: JsonObject }
+  | {
+      type: 'filterState';
+      __cache: JsonObject;
+      extraFormData: ExtraFormData;
+      filterState: {
+        label?: string;
+        filters?: AdhocFilter[];
+        value: AdhocFilter[];
+      };
+    };
+
+function reducer(
+  draft: DataMask & { __cache?: JsonObject },
+  action: DataMaskAction,
+) {
+  switch (action.type) {
+    case 'ownState':
+      draft.ownState = {
+        ...draft.ownState,
+        ...action.ownState,
+      };
+      return draft;
+    case 'filterState':
+      draft.extraFormData = action.extraFormData;
+      // eslint-disable-next-line no-underscore-dangle
+      draft.__cache = action.__cache;
+      draft.filterState = { ...draft.filterState, ...action.filterState };
+      return draft;
+    default:
+      return draft;
+  }
+}
+
+export default function PluginFilterAdhoc(props: PluginFilterAdhocProps) {
+  const {
+    filterState,
+    formData,
+    height,
+    width,
+    setDataMask,
+    setFocusedFilter,
+    unsetFocusedFilter,
+    appSection,
+  } = props;
+  const { enableEmptyFilter, inverseSelection, defaultToFirstItem } = formData;
+  const datasetId = useMemo(
+    () => formData.datasource.split('_')[0],
+    [formData.datasource],
+  );
+  const [datasetDetails, setDatasetDetails] = useState<Record<string, any>>();
+  const [columns, setColumns] = useState();
+  const [dataMask, dispatchDataMask] = useImmerReducer(reducer, {
+    extraFormData: {},
+    filterState,
+  });
+  const labelFormatter = useMemo(
+    () =>
+      getDataRecordFormatter({
+        timeFormatter: smartDateDetailedFormatter,
+      }),
+    [],
+  );
+
+  const localCache = new Map<string, any>();
+
+  const cachedSupersetGet = cacheWrapper(
+    SupersetClient.get,
+    localCache,
+    ({ endpoint }) => endpoint || '',
+  );
+
+  useChangeEffect(datasetId, () => {
+    if (datasetId) {
+      cachedSupersetGet({
+        endpoint: `/api/v1/dataset/${datasetId}`,
+      })
+        .then((response: JsonResponse) => {
+          const dataset = response.json?.result;
+          // modify the response to fit structure expected by AdhocFilterControl
+          dataset.type = dataset.datasource_type;
+          dataset.filter_select = true;
+          setDatasetDetails(dataset);
+        })
+        .catch((response: SupersetApiError) => {
+          addDangerToast(response.message);
+        });
+    }
+  });
+
+  useChangeEffect(datasetId, () => {
+    if (datasetId != null) {
+      cachedSupersetGet({
+        endpoint: `/api/v1/dataset/${datasetId}`,
+      }).then(
+        ({ json: { result } }) => {
+          setColumns(result.columns);
+        },
+        async badResponse => {
+          const { error, message } = await getClientErrorObject(badResponse);
+          let errorText = message || error || t('An error has occurred');
+          if (message === 'Forbidden') {
+            errorText = t('You do not have permission to edit this dashboard');
+          }
+          addDangerToast(errorText);
+        },
+      );
+    }
+  });
+
+  const labelString: (props: AdhocFilter) => string = (props: AdhocFilter) => {
+    if (ensureIsArray(props.comparator).length >= 2) {
+      return `${props.subject} ${props.operator} (${props.comparator.join(
+        ', ',
+      )})`;
+    }
+    return `${props.subject} ${props.operator} ${props.comparator}`;
+  };

Review Comment:
   Can we DRY this up by reusing the existing pill label formatter function?



##########
superset-frontend/src/filters/components/Adhoc/AdhocFilterPlugin.tsx:
##########
@@ -0,0 +1,254 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable no-param-reassign */
+import {
+  DataMask,
+  ensureIsArray,
+  ExtraFormData,
+  JsonObject,
+  JsonResponse,
+  smartDateDetailedFormatter,
+  SupersetApiError,
+  SupersetClient,
+  t,
+} from '@superset-ui/core';
+import React, { useCallback, useEffect, useState, useMemo } from 'react';
+import { useImmerReducer } from 'use-immer';
+import AdhocFilterControl from 'src/explore/components/controls/FilterControl/AdhocFilterControl';
+import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
+// eslint-disable-next-line import/no-unresolved
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+// eslint-disable-next-line import/no-unresolved
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+// eslint-disable-next-line import/no-unresolved
+import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+// eslint-disable-next-line import/no-unresolved
+import { useChangeEffect } from 'src/hooks/useChangeEffect';
+import { PluginFilterAdhocProps } from './types';
+import {
+  StyledFormItem,
+  FilterPluginStyle,
+  StatusMessage,
+  ControlContainer,
+} from '../common';
+import { getDataRecordFormatter, getAdhocExtraFormData } from '../../utils';
+
+type DataMaskAction =
+  | { type: 'ownState'; ownState: JsonObject }
+  | {
+      type: 'filterState';
+      __cache: JsonObject;
+      extraFormData: ExtraFormData;
+      filterState: {
+        label?: string;
+        filters?: AdhocFilter[];
+        value: AdhocFilter[];
+      };
+    };
+
+function reducer(
+  draft: DataMask & { __cache?: JsonObject },
+  action: DataMaskAction,
+) {
+  switch (action.type) {
+    case 'ownState':
+      draft.ownState = {
+        ...draft.ownState,
+        ...action.ownState,
+      };
+      return draft;
+    case 'filterState':
+      draft.extraFormData = action.extraFormData;
+      // eslint-disable-next-line no-underscore-dangle
+      draft.__cache = action.__cache;
+      draft.filterState = { ...draft.filterState, ...action.filterState };
+      return draft;
+    default:
+      return draft;
+  }
+}
+
+export default function PluginFilterAdhoc(props: PluginFilterAdhocProps) {
+  const {
+    filterState,
+    formData,
+    height,
+    width,
+    setDataMask,
+    setFocusedFilter,
+    unsetFocusedFilter,
+    appSection,
+  } = props;
+  const { enableEmptyFilter, inverseSelection, defaultToFirstItem } = formData;
+  const datasetId = useMemo(
+    () => formData.datasource.split('_')[0],
+    [formData.datasource],
+  );
+  const [datasetDetails, setDatasetDetails] = useState<Record<string, any>>();
+  const [columns, setColumns] = useState();
+  const [dataMask, dispatchDataMask] = useImmerReducer(reducer, {
+    extraFormData: {},
+    filterState,
+  });
+  const labelFormatter = useMemo(
+    () =>
+      getDataRecordFormatter({
+        timeFormatter: smartDateDetailedFormatter,
+      }),
+    [],
+  );
+
+  const localCache = new Map<string, any>();
+
+  const cachedSupersetGet = cacheWrapper(
+    SupersetClient.get,
+    localCache,
+    ({ endpoint }) => endpoint || '',
+  );
+
+  useChangeEffect(datasetId, () => {
+    if (datasetId) {
+      cachedSupersetGet({
+        endpoint: `/api/v1/dataset/${datasetId}`,
+      })
+        .then((response: JsonResponse) => {
+          const dataset = response.json?.result;
+          // modify the response to fit structure expected by AdhocFilterControl
+          dataset.type = dataset.datasource_type;
+          dataset.filter_select = true;
+          setDatasetDetails(dataset);
+        })
+        .catch((response: SupersetApiError) => {
+          addDangerToast(response.message);
+        });
+    }
+  });
+
+  useChangeEffect(datasetId, () => {
+    if (datasetId != null) {
+      cachedSupersetGet({
+        endpoint: `/api/v1/dataset/${datasetId}`,
+      }).then(
+        ({ json: { result } }) => {
+          setColumns(result.columns);
+        },
+        async badResponse => {
+          const { error, message } = await getClientErrorObject(badResponse);
+          let errorText = message || error || t('An error has occurred');
+          if (message === 'Forbidden') {
+            errorText = t('You do not have permission to edit this dashboard');

Review Comment:
   Isn't this going to happen if the user doesn't have permission to read the dataset?
   ```suggestion
               errorText = t('You do not have permission to access the dataset');
   ```



##########
superset-frontend/src/filters/components/Adhoc/AdhocFilterPlugin.tsx:
##########
@@ -0,0 +1,254 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable no-param-reassign */
+import {
+  DataMask,
+  ensureIsArray,
+  ExtraFormData,
+  JsonObject,
+  JsonResponse,
+  smartDateDetailedFormatter,
+  SupersetApiError,
+  SupersetClient,
+  t,
+} from '@superset-ui/core';
+import React, { useCallback, useEffect, useState, useMemo } from 'react';
+import { useImmerReducer } from 'use-immer';
+import AdhocFilterControl from 'src/explore/components/controls/FilterControl/AdhocFilterControl';
+import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
+// eslint-disable-next-line import/no-unresolved
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+// eslint-disable-next-line import/no-unresolved
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+// eslint-disable-next-line import/no-unresolved
+import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+// eslint-disable-next-line import/no-unresolved
+import { useChangeEffect } from 'src/hooks/useChangeEffect';
+import { PluginFilterAdhocProps } from './types';
+import {
+  StyledFormItem,
+  FilterPluginStyle,
+  StatusMessage,
+  ControlContainer,
+} from '../common';
+import { getDataRecordFormatter, getAdhocExtraFormData } from '../../utils';
+
+type DataMaskAction =
+  | { type: 'ownState'; ownState: JsonObject }
+  | {
+      type: 'filterState';
+      __cache: JsonObject;
+      extraFormData: ExtraFormData;
+      filterState: {
+        label?: string;
+        filters?: AdhocFilter[];
+        value: AdhocFilter[];

Review Comment:
   I know this is poorly documented, but the filter indicator shows the filter as being applied as long as `value` is not equal to `null`. So always remember to set `value` to `null` if the filter is unset.



##########
superset-frontend/src/filters/components/Adhoc/AdhocFilterPlugin.tsx:
##########
@@ -0,0 +1,254 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/* eslint-disable no-param-reassign */
+import {
+  DataMask,
+  ensureIsArray,
+  ExtraFormData,
+  JsonObject,
+  JsonResponse,
+  smartDateDetailedFormatter,
+  SupersetApiError,
+  SupersetClient,
+  t,
+} from '@superset-ui/core';
+import React, { useCallback, useEffect, useState, useMemo } from 'react';
+import { useImmerReducer } from 'use-immer';
+import AdhocFilterControl from 'src/explore/components/controls/FilterControl/AdhocFilterControl';
+import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
+// eslint-disable-next-line import/no-unresolved
+import { addDangerToast } from 'src/components/MessageToasts/actions';
+// eslint-disable-next-line import/no-unresolved
+import { cacheWrapper } from 'src/utils/cacheWrapper';
+// eslint-disable-next-line import/no-unresolved
+import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+// eslint-disable-next-line import/no-unresolved
+import { useChangeEffect } from 'src/hooks/useChangeEffect';
+import { PluginFilterAdhocProps } from './types';
+import {
+  StyledFormItem,
+  FilterPluginStyle,
+  StatusMessage,
+  ControlContainer,
+} from '../common';
+import { getDataRecordFormatter, getAdhocExtraFormData } from '../../utils';
+
+type DataMaskAction =
+  | { type: 'ownState'; ownState: JsonObject }
+  | {
+      type: 'filterState';
+      __cache: JsonObject;
+      extraFormData: ExtraFormData;
+      filterState: {
+        label?: string;
+        filters?: AdhocFilter[];
+        value: AdhocFilter[];
+      };
+    };
+
+function reducer(
+  draft: DataMask & { __cache?: JsonObject },
+  action: DataMaskAction,
+) {
+  switch (action.type) {
+    case 'ownState':
+      draft.ownState = {
+        ...draft.ownState,
+        ...action.ownState,
+      };
+      return draft;
+    case 'filterState':
+      draft.extraFormData = action.extraFormData;
+      // eslint-disable-next-line no-underscore-dangle
+      draft.__cache = action.__cache;
+      draft.filterState = { ...draft.filterState, ...action.filterState };
+      return draft;
+    default:
+      return draft;
+  }
+}
+
+export default function PluginFilterAdhoc(props: PluginFilterAdhocProps) {
+  const {
+    filterState,
+    formData,
+    height,
+    width,
+    setDataMask,
+    setFocusedFilter,
+    unsetFocusedFilter,
+    appSection,
+  } = props;
+  const { enableEmptyFilter, inverseSelection, defaultToFirstItem } = formData;
+  const datasetId = useMemo(
+    () => formData.datasource.split('_')[0],
+    [formData.datasource],
+  );
+  const [datasetDetails, setDatasetDetails] = useState<Record<string, any>>();
+  const [columns, setColumns] = useState();
+  const [dataMask, dispatchDataMask] = useImmerReducer(reducer, {
+    extraFormData: {},
+    filterState,
+  });
+  const labelFormatter = useMemo(
+    () =>
+      getDataRecordFormatter({
+        timeFormatter: smartDateDetailedFormatter,
+      }),
+    [],
+  );
+
+  const localCache = new Map<string, any>();
+
+  const cachedSupersetGet = cacheWrapper(
+    SupersetClient.get,
+    localCache,
+    ({ endpoint }) => endpoint || '',
+  );
+
+  useChangeEffect(datasetId, () => {
+    if (datasetId) {
+      cachedSupersetGet({
+        endpoint: `/api/v1/dataset/${datasetId}`,
+      })
+        .then((response: JsonResponse) => {
+          const dataset = response.json?.result;
+          // modify the response to fit structure expected by AdhocFilterControl
+          dataset.type = dataset.datasource_type;
+          dataset.filter_select = true;
+          setDatasetDetails(dataset);
+        })
+        .catch((response: SupersetApiError) => {
+          addDangerToast(response.message);
+        });
+    }
+  });
+
+  useChangeEffect(datasetId, () => {
+    if (datasetId != null) {
+      cachedSupersetGet({
+        endpoint: `/api/v1/dataset/${datasetId}`,
+      }).then(
+        ({ json: { result } }) => {
+          setColumns(result.columns);
+        },
+        async badResponse => {
+          const { error, message } = await getClientErrorObject(badResponse);
+          let errorText = message || error || t('An error has occurred');
+          if (message === 'Forbidden') {
+            errorText = t('You do not have permission to edit this dashboard');
+          }
+          addDangerToast(errorText);
+        },
+      );
+    }
+  });
+
+  const labelString: (props: AdhocFilter) => string = (props: AdhocFilter) => {
+    if (ensureIsArray(props.comparator).length >= 2) {
+      return `${props.subject} ${props.operator} (${props.comparator.join(
+        ', ',
+      )})`;
+    }
+    return `${props.subject} ${props.operator} ${props.comparator}`;
+  };
+
+  const updateDataMask = useCallback(
+    (adhoc_filters: AdhocFilter[]) => {
+      const emptyFilter =
+        enableEmptyFilter && !inverseSelection && !adhoc_filters?.length;
+
+      dispatchDataMask({
+        type: 'filterState',
+        __cache: filterState,
+        extraFormData: getAdhocExtraFormData(
+          adhoc_filters,
+          emptyFilter,
+          inverseSelection,
+        ),
+        filterState: {
+          ...filterState,
+          label: (adhoc_filters || [])
+            .map(f =>
+              f.sqlExpression ? String(f.sqlExpression) : labelString(f),
+            )
+            .join(', '),
+          value: adhoc_filters,

Review Comment:
   So here we should make sure that `value` is set to `null` if it's unset:
   ```suggestion
             value: adhoc_filters?.length ? adhoc_filters | null,
   ```



##########
superset-frontend/src/filters/utils.ts:
##########
@@ -24,8 +24,29 @@ import {
   TimeFormatter,
   ExtraFormData,
 } from '@superset-ui/core';
+import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
 import { FALSE_STRING, NULL_STRING, TRUE_STRING } from 'src/utils/common';
 
+export const getAdhocExtraFormData = (
+  adhoc_filters: AdhocFilter[] = [],
+  emptyFilter = false,
+  inverseSelection = false,

Review Comment:
   I assume this is also a leftover
   ```suggestion
     inverseSelection = false,
   ```



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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 #22168: feat(native-filters): Adhoc dashboard native filters

Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1514655272

   Thank you for the PR @cccs-RyanK!
   
   How will this filter behave considering inter filter features such as `Values depend on other filters` and `Filter scopes`? Specially in the case where we have a SQL value and it's hard to determine the referenced column/dataset.
   
   Is the popup arrow pointing to the correct direction when the filter bar is horizontal? We need to consider the scenario where the filter is displayed in the bar in which case the arrow should point up and the case where the filter is displayed in the dropdown in which case the arrow should point to the right.
   
   I agree with @yousoph's points.
   


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] hushaoqing commented on pull request #22168: feat(native-filters): Adhoc dashboard native filters

Posted by "hushaoqing (via GitHub)" <gi...@apache.org>.
hushaoqing commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1510580430

   looking forward to it being released.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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] cccs-RyanK commented on pull request #22168: feat(native-filters): Adhoc dashboard native filters

Posted by "cccs-RyanK (via GitHub)" <gi...@apache.org>.
cccs-RyanK commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1516705497

   
   
   
   > > How will this filter behave considering inter filter features such as Values depend on other filters and Filter scopes? Specially in the case where we have a SQL value and it's hard to determine the referenced column/dataset.
   > 
   > @cccs-RyanK Just to be more clear about the above point. There's a feature called **Values depend on other filters** (check [this](https://github.com/apache/superset/pull/23566) PR's video) which allows you to establish an hierarchy between filters. One example is when you have a State and City filters and you want to change the City's values depending on the selected value in State. This all works because there's a clear definition of the dataset and column when creating a filter. When you introduce a SQL filter, we don't know what's the column (or even if the SQL returns a column), so I'm wondering how all the features that depend on this definition will work. I think we need to at least disable these features when working with a SQL filter. If I defined the State filter as a SQL filter, then it shouldn't appear as one of the options in **Values depend on other filters** when configuring the City filter.
   
   @michael-s-molina thank you for the clarification! Yes I think that feature will need to be disabled on the SQL filter based on the reasons you mentioned. As for scoping, the SQL filter should work the same as other filters such as Time column filter which only specifies the dataset and not a specific column. 
   
   Lastly, I also agree about the horizontal filter bar I will test and adjust the popover so that it pops below instead of to the right in that case. Aiming to work on this soon.


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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


Re: [PR] feat(native-filters): Adhoc dashboard native filters [superset]

Posted by "cccs-RyanK (via GitHub)" <gi...@apache.org>.
cccs-RyanK commented on PR #22168:
URL: https://github.com/apache/superset/pull/22168#issuecomment-1821129342

   > @cccs-RyanK , @michael-s-molina Is there a permission to control this. I would want this available to Gamma roles and admins will have the normal add/edit filter.
   
   Hey @michaelkovatt! I am not sure I understand all your question. For the first part no there was no permissions in place for this type of filter as of the last update, but for the second part what exactly is the behaviour you were thinking? You would like to configure it so that only certain roles have access to this type of filter?


-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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