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/09/07 14:28:01 UTC
[GitHub] [superset] kgabryje opened a new pull request #16621: fix(dashboard): label colors included in explore url
kgabryje opened a new pull request #16621:
URL: https://github.com/apache/superset/pull/16621
### SUMMARY
Due to changes from https://github.com/apache/superset/pull/16444, label_colors from dashboard were being included in chart's URL when user visited explore from dashboard. This PR partially reverts the changes from https://github.com/apache/superset/pull/16444, while still preserving the perf improvement introduced in that PR
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before - example link of Proportion of Monthly Revenue by Product Line chart from Sales Dashboard (2142 characters):
http://localhost:9000/superset/explore/?form_data=%7B%22viz_type%22%3A%22area%22%2C%22datasource%22%3A%2220__table%22%2C%22slice_id%22%3A96%2C%22url_params%22%3A%7B%7D%2C%22time_range_endpoints%22%3A%5B%22inclusive%22%2C%22exclusive%22%5D%2C%22granularity_sqla%22%3A%22order_date%22%2C%22time_grain_sqla%22%3A%22P1M%22%2C%22time_range%22%3A%22No+filter%22%2C%22metrics%22%3A%5B%7B%22aggregate%22%3A%22SUM%22%2C%22column%22%3A%7B%22column_name%22%3A%22sales%22%2C%22description%22%3Anull%2C%22expression%22%3Anull%2C%22filterable%22%3Atrue%2C%22groupby%22%3Atrue%2C%22id%22%3A917%2C%22is_dttm%22%3Afalse%2C%22optionName%22%3A%22_col_Sales%22%2C%22python_date_format%22%3Anull%2C%22type%22%3A%22DOUBLE+PRECISION%22%2C%22verbose_name%22%3Anull%7D%2C%22expressionType%22%3A%22SIMPLE%22%2C%22hasCustomLabel%22%3Afalse%2C%22isNew%22%3Afalse%2C%22label%22%3A%22%28Sales%29%22%2C%22optionName%22%3A%22metric_3is69ofceho_6d0ezok7ry6%22%2C%22sqlExpression%22%3Anull%7D%5D%2C%22adhoc_filters%22%3A%5B%5D%2C
%22groupby%22%3A%5B%22product_line%22%5D%2C%22limit%22%3A100%2C%22order_desc%22%3Atrue%2C%22contribution%22%3Atrue%2C%22row_limit%22%3Anull%2C%22show_brush%22%3A%22auto%22%2C%22show_legend%22%3Atrue%2C%22line_interpolation%22%3A%22linear%22%2C%22stacked_style%22%3A%22stack%22%2C%22color_scheme%22%3A%22bnbColors%22%2C%22label_colors%22%3A%7B%22SUM%28Sales%29%22%3A%22%23ff5a5f%22%2C%22Medium%22%3A%22%237b0051%22%2C%22Small%22%3A%22%23007A87%22%2C%22Large%22%3A%22%2300d1c1%22%2C%22SUM%28SALES%29%22%3A%22%238ce071%22%2C%22Classic+Cars%22%3A%22%23ffb400%22%2C%22Vintage+Cars%22%3A%22%23b4a76c%22%2C%22Motorcycles%22%3A%22%23ff8083%22%2C%22Trucks+and+Buses%22%3A%22%23cc0086%22%2C%22Planes%22%3A%22%2300a1b3%22%2C%22Ships%22%3A%22%2300ffeb%22%2C%22Trains%22%3A%22%23bbedab%22%7D%2C%22rich_tooltip%22%3Atrue%2C%22bottom_margin%22%3A%22auto%22%2C%22x_ticks_layout%22%3A%22auto%22%2C%22x_axis_format%22%3A%22smart_date%22%2C%22y_axis_format%22%3A%22SMART_NUMBER%22%2C%22y_axis_bounds%22%3A%5Bnull%2Cn
ull%5D%2C%22rolling_type%22%3A%22None%22%2C%22comparison_type%22%3A%22values%22%2C%22annotation_layers%22%3A%5B%5D%2C%22extra_form_data%22%3A%7B%7D%7D
After (1709 characters):
http://localhost:9000/superset/explore/?form_data=%7B%22viz_type%22%3A%22area%22%2C%22datasource%22%3A%2220__table%22%2C%22slice_id%22%3A96%2C%22url_params%22%3A%7B%7D%2C%22time_range_endpoints%22%3A%5B%22inclusive%22%2C%22exclusive%22%5D%2C%22granularity_sqla%22%3A%22order_date%22%2C%22time_grain_sqla%22%3A%22P1M%22%2C%22time_range%22%3A%22No+filter%22%2C%22metrics%22%3A%5B%7B%22aggregate%22%3A%22SUM%22%2C%22column%22%3A%7B%22column_name%22%3A%22sales%22%2C%22description%22%3Anull%2C%22expression%22%3Anull%2C%22filterable%22%3Atrue%2C%22groupby%22%3Atrue%2C%22id%22%3A917%2C%22is_dttm%22%3Afalse%2C%22optionName%22%3A%22_col_Sales%22%2C%22python_date_format%22%3Anull%2C%22type%22%3A%22DOUBLE+PRECISION%22%2C%22verbose_name%22%3Anull%7D%2C%22expressionType%22%3A%22SIMPLE%22%2C%22hasCustomLabel%22%3Afalse%2C%22isNew%22%3Afalse%2C%22label%22%3A%22%28Sales%29%22%2C%22optionName%22%3A%22metric_3is69ofceho_6d0ezok7ry6%22%2C%22sqlExpression%22%3Anull%7D%5D%2C%22adhoc_filters%22%3A%5B%5D%2C
%22groupby%22%3A%5B%22product_line%22%5D%2C%22limit%22%3A100%2C%22order_desc%22%3Atrue%2C%22contribution%22%3Atrue%2C%22row_limit%22%3Anull%2C%22show_brush%22%3A%22auto%22%2C%22show_legend%22%3Atrue%2C%22line_interpolation%22%3A%22linear%22%2C%22stacked_style%22%3A%22stack%22%2C%22color_scheme%22%3A%22bnbColors%22%2C%22label_colors%22%3A%7B%7D%2C%22rich_tooltip%22%3Atrue%2C%22bottom_margin%22%3A%22auto%22%2C%22x_ticks_layout%22%3A%22auto%22%2C%22x_axis_format%22%3A%22smart_date%22%2C%22y_axis_format%22%3A%22SMART_NUMBER%22%2C%22y_axis_bounds%22%3A%5Bnull%2Cnull%5D%2C%22rolling_type%22%3A%22None%22%2C%22comparison_type%22%3A%22values%22%2C%22annotation_layers%22%3A%5B%5D%2C%22extra_form_data%22%3A%7B%7D%7D
### TESTING INSTRUCTIONS
1. Go to a dashboard
2. Click "View chart in Explore"
3. Verify that label colors are not included in url
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] 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
CC @junlincc @serenajiang
--
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 #16621: fix(dashboard): label colors included in explore url
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #16621:
URL: https://github.com/apache/superset/pull/16621#issuecomment-914377258
# [Codecov](https://codecov.io/gh/apache/superset/pull/16621?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 [#16621](https://codecov.io/gh/apache/superset/pull/16621?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8559380) into [master](https://codecov.io/gh/apache/superset/commit/effcf3b50f347f1c3c3ddc82406046c392205ebd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (effcf3b) will **decrease** coverage by `0.00%`.
> The diff coverage is `33.33%`.
[](https://codecov.io/gh/apache/superset/pull/16621?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 #16621 +/- ##
==========================================
- Coverage 76.69% 76.69% -0.01%
==========================================
Files 1003 1003
Lines 53950 53961 +11
Branches 7324 7332 +8
==========================================
+ Hits 41379 41386 +7
- Misses 12332 12336 +4
Partials 239 239
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `71.08% <33.33%> (-0.01%)` | :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/16621?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/16621/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.69% <0.00%> (-0.03%)` | :arrow_down: |
| [...shboard/util/charts/getFormDataWithExtraFilters.ts](https://codecov.io/gh/apache/superset/pull/16621/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2NoYXJ0cy9nZXRGb3JtRGF0YVdpdGhFeHRyYUZpbHRlcnMudHM=) | `83.87% <50.00%> (ø)` | |
| [superset-frontend/src/components/Select/Select.tsx](https://codecov.io/gh/apache/superset/pull/16621/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvU2VsZWN0L1NlbGVjdC50c3g=) | `72.65% <0.00%> (+0.19%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/16621?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/16621?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 [effcf3b...8559380](https://codecov.io/gh/apache/superset/pull/16621?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.
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] graceguo-supercat edited a comment on pull request #16621: fix(dashboard): label colors included in explore url
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #16621:
URL: https://github.com/apache/superset/pull/16621#issuecomment-915427458
The issue for "explore URL" is **_not_** really caused by #16444. [This line](https://github.com/apache/superset/blob/3fe2e6ec731529ae228904b9b5e39baec073ae8d/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx#L286)
```
<a href={this.props.exploreUrl} rel="noopener noreferrer">
```
will generate a GET request when user click on it, but browser (and many companies using nginx) has limitation for length of GET request. In airbnb we had a lot of charts with very long parameters, these charts couldn't be open by GET request. #16444 added label colors into url makes things worse, but it is no the root cause.
Note, originally, there was an onClick event handler for this `View chart in explore` menu item, but it was lost since #11554. In the event handler, it generate a POST request to open the chart since GET won't work for long url.
Ideally we should **avoid** opening chart by GET request.
--
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] kgabryje merged pull request #16621: fix(dashboard): label colors included in explore url
Posted by GitBox <gi...@apache.org>.
kgabryje merged pull request #16621:
URL: https://github.com/apache/superset/pull/16621
--
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] graceguo-supercat edited a comment on pull request #16621: fix(dashboard): label colors included in explore url
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #16621:
URL: https://github.com/apache/superset/pull/16621#issuecomment-915427458
--
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] graceguo-supercat edited a comment on pull request #16621: fix(dashboard): label colors included in explore url
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #16621:
URL: https://github.com/apache/superset/pull/16621#issuecomment-915427458
The issue for "explore URL" is **_not_** really caused by #16444. [This line](https://github.com/apache/superset/blob/3fe2e6ec731529ae228904b9b5e39baec073ae8d/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx#L286)
```
<a href={this.props.exploreUrl} rel="noopener noreferrer">
```
will generate a GET request when user click on it, but browser (and many companies using nginx) has limitation for length of GET request. In airbnb we had a lot of charts with very long parameters, these charts couldn't be open by GET request. #16444 added label colors into url makes things worse, but it is no the root cause.
Note, originally, there was an onClick event handler for this `View chart in explore` menu item, but it was lost since #11554. In the event handler, it generate a POST request to open the chart.
Ideally we should **avoid** opening chart by GET request.
--
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] graceguo-supercat commented on pull request #16621: fix(dashboard): label colors included in explore url
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #16621:
URL: https://github.com/apache/superset/pull/16621#issuecomment-915427458
The issue for "explore URL" is **_not_** really caused by #16444. [This line](https://github.com/apache/superset/blob/3fe2e6ec731529ae228904b9b5e39baec073ae8d/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx#L286)
```
<a href={this.props.exploreUrl} rel="noopener noreferrer">
```
will generate a GET request when user click on it, but browser (and many companies using nginx) has limitation for length of GET request. In airbnb we had a lot of charts with very long parameters, these charts couldn't be open by GET request. #16444 added label colors into url makes things worse, but it is no the root cause.
Note, originally, there was an onClick event handler for this `View chart in explore` menu item, but it was lost since #11554. In the event handler, it generate a POST request to open the chart since GET won't work for long url.
Ideally we should avoid open chart using GET request.
--
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