john-bodley commented on issue #14383: URL: https://github.com/apache/superset/issues/14383#issuecomment-1295581036
#### Primer Per https://github.com/apache/superset/issues/14383#issuecomment-1123140073 although the work has been implemented, this SIP has not been voted on and thus I was hoping to share some recent observations combined with a proposed change as to how the migration logic could work, which will then serve as the seed for a formal discussion and potential vote. #### Observations The dashboard native filters and frontend filter-box migration features are controlled via the `DASHBOARD_NATIVE_FILTERS` (defaults to true) and `ENABLE_FILTER_BOX_MIGRATION` (defaults to false) feature flags respectively. The following observations relate to having the respective feature flag enabled. ##### DASHBOARD_NATIVE_FILTERS 1. The native filters don't always show up by default. This is most likely related to (2). 2. Simply viewing a dashboard without the feature enabled saves the dashboard which is undesirable and has the potential of polluting Superset's metadata. This is likely due to the addition of the `show_native_filters` JSON parameter. This key is likely superfluous—I hypothesize it’s leveraged by the migration workflow—and likely can be removed. 3. Though not ideal from a UX perspective, legacy filter scopes and coexist with the native filter scopes. Note if the dashboard does not have any filter-box charts then the “Set filter mapping” option is disabled. 4. One can still add a legacy filter-box chart to the dashboard. The only way to (partially) prevent this is to enable the `ENABLE_FILTER_BOX_MIGRATION` feature. 5. The GET `/api/v1/dataset/{pk}` RESTful API endpoint is sub-performant for very large datasets making the feature somewhat unusable. See [this](https://apache-superset.slack.com/archives/G01FST7CBNG/p1658865091229009) Slack thread in the #project-native-filters channel. ##### ENABLE_FILTER_BOX_MIGRATION 1. It’s not apparent what the review process is, i.e., how to accept or revert the change. Note I believe that the intended flow is that these native filters are automatically persisted without any course to revert—refer to (2) as to why this isn’t the case—which makes the term “review process” somewhat of a misnomer. 2. The feature seems to be broken in master as one is unable to save. Per the video [here](https://github.com/apache/superset/pull/16992) there’s no obvious way of how to persist the migration, i.e., there’s no save button or way to exit the workflow, though it’s worth adding that a pencil icon exists in the filter bar which is no longer present. 3. Post migration though the filter-box charts are grayed out they still remain functional (albeit being a duplicate). Furthermore the filter pill etc. is enabled and potentially misleading as they’re now deemed as a chart which is governed by a native filter i.e., the UX isn’t ideal given the confusion. 4. The feature is not sufficient in the sense that only the dashboards where the owners completed the review are updated. A one off CLI migration (which doesn’t currently exist) still needs to be added. 5. Even though you can’t add a filter-box chart to the dashboard via the dashboard edit flow you still can—via explore— either i) create a filter-box chart, or ii) add a filter-box chart to a dashboard on save. #### Recommendations 1. Add a GET `/api/v1/dataset/{pk}/column` RESTful API endpoint which merely fetches the columns (the existing RESTful API uses singular rather than plural when referring to multiple) of the specified dataset which is all that is required. This will address the performance issue. 2. Remove the `ENABLE_FILTER_BOX_MIGRATION` feature. Though a frontend migration workflow has merit there's no specific call to action and/or mechanism to revert the migration. Furthermore the rubric outlined in [apache/superset#16992](https://github.com/apache/superset/pull/16992) with the various scenarios is overly complex and can be greatly simplified. 3. Remove the superfluous `show_native_filters` JSON parameter given that the `DASHBOARD_NATIVE_FILTERS` feature flag should be suffice. This will prevent dashboards from being saved on view if a filter-box chart is present. 4. Disable creating or adding filter-box charts to a dashboard (via the various flows) when the `DASHBOARD_NATIVE_FILTERS` feature is enabled. 5. Perform a two-phase migration (to help ensure a smoother transition as well as providing a method of recourse): - Author a CLI script to migrate dashboards. Assuming that the `DASHBOARD_NATIVE_FILTERS` feature is enabled, the script will: - Migrate all embedded filter-box charts to native filters irrespective of whether there already exists native filters on a dashboard. One would preferably run the script soon after the feature is enabled to reduce the coexistence of both native and legacy filters which have their own scoping mechanism. Note per (2) only native filters can be added to a new or existing dashboard which means once migrated a dashboard cannot regress to having both native and legacy filters. - Uniquify the filter names (which is currently not the case) leveraging the chart title if the filterable element exists within multiple filter-box charts within the dashboards. - Keep the legacy filter scopes in the dashboard metadata which will allow for the migration to be reverted. - Use a markdown placeholder where the previous filter-box chart resided. This ensures that i) the layout of the dashboard is preserved, and ii) all filter-box charts are actually removed (as opposed to disabled) from the dashboard. The markdown will include the name and link to the deprecated filter-box chart for reference. Note these markdown components can only ever be removed by the dashboard owner(s). - Author a CLI script to delete (clean up) all filter-box charts and legacy filter scopes—which by definition of (4) and (5) are not associated with any dashboards. This should be run after (4) once it is decided sufficient time has elapsed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
