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/10/18 16:57:10 UTC
[GitHub] [superset] suddjian opened a new pull request #17145: refactor: small changes to dashboard->explore code
suddjian opened a new pull request #17145:
URL: https://github.com/apache/superset/pull/17145
<!---
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 -->
Just a small refactor that I didn't get a chance to push before #17123 was merged. This separates the dashboard-specific function from the function used by Explore. Extending rather than modifying to minimize the footprint of this change.
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Go to a dashboard and Explore Chart on one of the charts.
### 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
--
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 #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-948098889
- this is a quick summary that compares GET vs POST:
[https://www.w3schools.com/tags/ref_httpmethods.asp
](https://www.w3schools.com/tags/ref_httpmethods.asp
)[https://www.guru99.com/difference-get-post-http.html](https://www.guru99.com/difference-get-post-http.html)
they all mentioned about the length limit of the URL
- <a> is html tag, it can trigger POST request, just add your own onClick event handler, and check if it is simple click or context menu click. in the click handler you can decide open in the same window or open another window.
- Superset use POST to handle click event in many other places, and they don't cause any issue. (surprised?) check this menu:
<img width="545" alt="Screen Shot 2021-10-20 at 3 59 21 PM" src="https://user-images.githubusercontent.com/27990562/138183992-0660a82a-bcf0-4d51-98ad-262979e66a09.png">
(you can image that chart's query could be very long very complicated right? by GET method there is no way to handle this 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 edited a comment on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-946056824
Hi @suddjian thanks for the work. But I do not think from dashboard -> explore chart should use GET request with full form params in the url. Superset used to handle this issue very well, but was broken since #15668. I do not think this solution, reducing a couple of extra parameters from dashboard, can fix the root cause: browser has limits for the url length, and many companies with Nginx also limits the total length of GET request url.
May i know why not use POST request for this problem, as Superset did before?
--
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 #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-948098889
- this is a quick summary that compares GET vs POST:
[https://www.w3schools.com/tags/ref_httpmethods.asp
](https://www.w3schools.com/tags/ref_httpmethods.asp
)[https://www.guru99.com/difference-get-post-http.html](https://www.guru99.com/difference-get-post-http.html)
they all mentioned about the length limit of the URL
- <a> is html tag, it can trigger POST request, just add your own onClick event handler, and check if it is simple click or context menu click. in the click handler you can decide open in the same window or open another window.
- Superset use POST to handle click event in many other places, and they don't cause any issue. (surprised?) check this menu: <img width="545" alt="Screen Shot 2021-10-20 at 3 59 21 PM" src="https://user-images.githubusercontent.com/27990562/138183992-0660a82a-bcf0-4d51-98ad-262979e66a09.png">
--
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 #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-948098889
- this is a quick summary that compares GET vs POST:
[https://www.w3schools.com/tags/ref_httpmethods.asp
](https://www.w3schools.com/tags/ref_httpmethods.asp
)[https://www.guru99.com/difference-get-post-http.html](https://www.guru99.com/difference-get-post-http.html)
they all mentioned about the length limit of the URL
- `<a>` is html tag, it can trigger POST request, just add your own onClick event handler, and check if it is simple click or context menu click. in the click handler you can decide open in the same window or open another window.
- Superset use POST to handle click event in many other places, and they don't cause any issue. (surprised?) check this menu:
<img width="545" alt="Screen Shot 2021-10-20 at 3 59 21 PM" src="https://user-images.githubusercontent.com/27990562/138183992-0660a82a-bcf0-4d51-98ad-262979e66a09.png">
(you can image that chart's query could be very long very complicated right? by GET method there is no way to handle this 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] codecov[bot] commented on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-945988561
# [Codecov](https://codecov.io/gh/apache/superset/pull/17145?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 [#17145](https://codecov.io/gh/apache/superset/pull/17145?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6cdc8ff) into [master](https://codecov.io/gh/apache/superset/commit/ae4ced8da6933cdc657452d1f11415c49c6c68b8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae4ced8) will **not change** coverage.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/17145?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 #17145 +/- ##
=======================================
Coverage 76.68% 76.68%
=======================================
Files 1038 1038
Lines 55516 55516
Branches 7564 7564
=======================================
Hits 42570 42570
Misses 12696 12696
Partials 250 250
```
| Flag | Coverage Δ | |
|---|---|---|
| mysql | `81.90% <ø> (ø)` | |
| postgres | `81.91% <ø> (ø)` | |
| python | `81.99% <ø> (ø)` | |
| sqlite | `81.58% <ø> (ø)` | |
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/17145?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `80.37% <100.00%> (ø)` | |
| [...uperset-frontend/src/explore/exploreUtils/index.js](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZXhwbG9yZVV0aWxzL2luZGV4Lmpz) | `66.66% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/17145?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/17145?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 [ae4ced8...6cdc8ff](https://codecov.io/gh/apache/superset/pull/17145?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 #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-948098889
- this is a quick summary that compares GET vs POST:
[https://www.w3schools.com/tags/ref_httpmethods.asp
](https://www.w3schools.com/tags/ref_httpmethods.asp
)[https://www.guru99.com/difference-get-post-http.html](https://www.guru99.com/difference-get-post-http.html)
they all mentioned about the length limit of the URL
- `<a>` is html tag, it can trigger POST request, just add your own onClick event handler, and check if it is simple click or context menu click. in the click handler you can decide open in the same window or open another window.
- Superset use POST to handle click event in many other places, and they didn't cause any issue. (surprised?) check this menu:
<img width="545" alt="Screen Shot 2021-10-20 at 3 59 21 PM" src="https://user-images.githubusercontent.com/27990562/138183992-0660a82a-bcf0-4d51-98ad-262979e66a09.png">
(you can image that chart's query could be very long very complicated right? by GET method there is no way to handle this 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] codecov[bot] edited a comment on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-945988561
# [Codecov](https://codecov.io/gh/apache/superset/pull/17145?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 [#17145](https://codecov.io/gh/apache/superset/pull/17145?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6cdc8ff) into [master](https://codecov.io/gh/apache/superset/commit/ae4ced8da6933cdc657452d1f11415c49c6c68b8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae4ced8) will **increase** coverage by `0.22%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/17145?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 #17145 +/- ##
==========================================
+ Coverage 76.68% 76.90% +0.22%
==========================================
Files 1038 1038
Lines 55516 55544 +28
Branches 7564 7564
==========================================
+ Hits 42570 42717 +147
+ Misses 12696 12577 -119
Partials 250 250
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `81.44% <ø> (?)` | |
| javascript | `70.93% <100.00%> (+0.02%)` | :arrow_up: |
| mysql | `81.90% <ø> (ø)` | |
| postgres | `81.91% <ø> (ø)` | |
| presto | `81.77% <ø> (?)` | |
| python | `82.41% <ø> (+0.41%)` | :arrow_up: |
| sqlite | `81.58% <ø> (ø)` | |
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/17145?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `80.37% <100.00%> (ø)` | |
| [...uperset-frontend/src/explore/exploreUtils/index.js](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZXhwbG9yZVV0aWxzL2luZGV4Lmpz) | `67.07% <100.00%> (+0.40%)` | :arrow_up: |
| [...src/dashboard/components/DashboardBuilder/state.ts](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZEJ1aWxkZXIvc3RhdGUudHM=) | `71.42% <0.00%> (-1.75%)` | :arrow_down: |
| [...c/filters/components/Select/SelectFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvU2VsZWN0RmlsdGVyUGx1Z2luLnRzeA==) | `80.64% <0.00%> (-0.47%)` | :arrow_down: |
| [...board/components/nativeFilters/FilterBar/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL2luZGV4LnRzeA==) | `88.09% <0.00%> (-0.10%)` | :arrow_down: |
| [...Filters/FilterBar/FilterControls/FilterControl.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlckNvbnRyb2wudHN4) | `100.00% <0.00%> (ø)` | |
| [.../FilterBar/CascadeFilters/CascadePopover/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0Nhc2NhZGVGaWx0ZXJzL0Nhc2NhZGVQb3BvdmVyL2luZGV4LnRzeA==) | `63.23% <0.00%> (ø)` | |
| [...frontend/src/components/TimezoneSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGltZXpvbmVTZWxlY3Rvci9pbmRleC50c3g=) | `97.77% <0.00%> (+0.05%)` | :arrow_up: |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.09% <0.00%> (+0.11%)` | :arrow_up: |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `88.57% <0.00%> (+0.33%)` | :arrow_up: |
| ... and [13 more](https://codecov.io/gh/apache/superset/pull/17145/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/17145?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/17145?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 [ae4ced8...6cdc8ff](https://codecov.io/gh/apache/superset/pull/17145?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] suddjian merged pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
suddjian merged pull request #17145:
URL: https://github.com/apache/superset/pull/17145
--
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] edited a comment on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-945988561
# [Codecov](https://codecov.io/gh/apache/superset/pull/17145?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 [#17145](https://codecov.io/gh/apache/superset/pull/17145?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6cdc8ff) into [master](https://codecov.io/gh/apache/superset/commit/ae4ced8da6933cdc657452d1f11415c49c6c68b8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae4ced8) will **increase** coverage by `0.22%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/17145?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 #17145 +/- ##
==========================================
+ Coverage 76.68% 76.90% +0.22%
==========================================
Files 1038 1038
Lines 55516 55544 +28
Branches 7564 7564
==========================================
+ Hits 42570 42717 +147
+ Misses 12696 12577 -119
Partials 250 250
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `81.44% <ø> (?)` | |
| javascript | `70.93% <100.00%> (+0.02%)` | :arrow_up: |
| mysql | `81.90% <ø> (ø)` | |
| postgres | `81.91% <ø> (ø)` | |
| presto | `81.77% <ø> (?)` | |
| python | `82.41% <ø> (+0.41%)` | :arrow_up: |
| sqlite | `81.58% <ø> (ø)` | |
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/17145?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `80.37% <100.00%> (ø)` | |
| [...uperset-frontend/src/explore/exploreUtils/index.js](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZXhwbG9yZVV0aWxzL2luZGV4Lmpz) | `67.07% <100.00%> (+0.40%)` | :arrow_up: |
| [...src/dashboard/components/DashboardBuilder/state.ts](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZEJ1aWxkZXIvc3RhdGUudHM=) | `71.42% <0.00%> (-1.75%)` | :arrow_down: |
| [...c/filters/components/Select/SelectFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvU2VsZWN0RmlsdGVyUGx1Z2luLnRzeA==) | `80.64% <0.00%> (-0.47%)` | :arrow_down: |
| [...board/components/nativeFilters/FilterBar/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL2luZGV4LnRzeA==) | `88.09% <0.00%> (-0.10%)` | :arrow_down: |
| [...Filters/FilterBar/FilterControls/FilterControl.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlckNvbnRyb2wudHN4) | `100.00% <0.00%> (ø)` | |
| [.../FilterBar/CascadeFilters/CascadePopover/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0Nhc2NhZGVGaWx0ZXJzL0Nhc2NhZGVQb3BvdmVyL2luZGV4LnRzeA==) | `63.23% <0.00%> (ø)` | |
| [...frontend/src/components/TimezoneSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGltZXpvbmVTZWxlY3Rvci9pbmRleC50c3g=) | `97.77% <0.00%> (+0.05%)` | :arrow_up: |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.09% <0.00%> (+0.11%)` | :arrow_up: |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `88.57% <0.00%> (+0.33%)` | :arrow_up: |
| ... and [13 more](https://codecov.io/gh/apache/superset/pull/17145/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/17145?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/17145?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 [ae4ced8...6cdc8ff](https://codecov.io/gh/apache/superset/pull/17145?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] codecov[bot] edited a comment on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-945988561
# [Codecov](https://codecov.io/gh/apache/superset/pull/17145?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 [#17145](https://codecov.io/gh/apache/superset/pull/17145?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6cdc8ff) into [master](https://codecov.io/gh/apache/superset/commit/ae4ced8da6933cdc657452d1f11415c49c6c68b8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae4ced8) will **increase** coverage by `0.14%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/17145?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 #17145 +/- ##
==========================================
+ Coverage 76.68% 76.82% +0.14%
==========================================
Files 1038 1038
Lines 55516 55544 +28
Branches 7564 7564
==========================================
+ Hits 42570 42673 +103
+ Misses 12696 12621 -75
Partials 250 250
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `81.44% <ø> (?)` | |
| javascript | `70.93% <100.00%> (+0.02%)` | :arrow_up: |
| mysql | `81.90% <ø> (ø)` | |
| postgres | `81.91% <ø> (ø)` | |
| python | `82.26% <ø> (+0.26%)` | :arrow_up: |
| sqlite | `81.58% <ø> (ø)` | |
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/17145?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `80.37% <100.00%> (ø)` | |
| [...uperset-frontend/src/explore/exploreUtils/index.js](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZXhwbG9yZVV0aWxzL2luZGV4Lmpz) | `67.07% <100.00%> (+0.40%)` | :arrow_up: |
| [...src/dashboard/components/DashboardBuilder/state.ts](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZEJ1aWxkZXIvc3RhdGUudHM=) | `71.42% <0.00%> (-1.75%)` | :arrow_down: |
| [...c/filters/components/Select/SelectFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvU2VsZWN0RmlsdGVyUGx1Z2luLnRzeA==) | `80.64% <0.00%> (-0.47%)` | :arrow_down: |
| [...board/components/nativeFilters/FilterBar/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL2luZGV4LnRzeA==) | `88.09% <0.00%> (-0.10%)` | :arrow_down: |
| [...Filters/FilterBar/FilterControls/FilterControl.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlckNvbnRyb2wudHN4) | `100.00% <0.00%> (ø)` | |
| [.../FilterBar/CascadeFilters/CascadePopover/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0Nhc2NhZGVGaWx0ZXJzL0Nhc2NhZGVQb3BvdmVyL2luZGV4LnRzeA==) | `63.23% <0.00%> (ø)` | |
| [...frontend/src/components/TimezoneSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGltZXpvbmVTZWxlY3Rvci9pbmRleC50c3g=) | `97.77% <0.00%> (+0.05%)` | :arrow_up: |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.09% <0.00%> (+0.11%)` | :arrow_up: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `85.84% <0.00%> (+0.23%)` | :arrow_up: |
| ... and [12 more](https://codecov.io/gh/apache/superset/pull/17145/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/17145?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/17145?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 [ae4ced8...6cdc8ff](https://codecov.io/gh/apache/superset/pull/17145?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 #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-946056824
Hi @suddjian thanks for the work. But I do not think from dashboard -> explore chart should use GET request with full form params in the url. Superset used to handle this issue very well, but was broken since #15668. I do not think this solution, reducing a couple of extra parameters from dashboard, can fix the root cause: browser has limits for the url length, and many companies with Nginx also limits the total length of GET request url.
May i know why not use POST request to resolve this problem, as Superset did before?
--
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 #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-949264954
> * in the click handler you can decide open in the same window or open another window
I think this is the center of the issue. With the click handler, _the developer_ must decide to open in the same window or in a new tab/window. With an old-school `href` link, the _user_ has the choice of opening in the same window or opening in a new tab. Giving the user the choice was the reason for going that route.
An onClick handler _might_ be able to capture modifier keys to emulate this behavior, but it would not have the same level of support (no right click menu, accessibility concerns).
The longer term idea we're stewing on would be a combination... opening the context menu could do a POST to push all the context of the dashboard (or other superset object, potentially) into a database row, and return a URL with a slug. That short URL could be used _in a conventional link_ to take you to Explore (or other superset area) which would look up all of the state/context needed to load as desired.
Approving this PR since it improves the existing feature. Going back to a simple onClick/POST will just rekindle the usability discussions we've had before. I think if we build a system like I'm (only briefly) attempting to describe above, it will solve the long URL problem AND improve accessibility/usability for this instance, and perhaps several other areas of Superset.
--
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] suddjian edited a comment on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-946083688
The goal with moving from `POST` to an `<a>` link was to improve UX by allowing proper browser controls (such as "open in a new tab" vs in-tab navigation). It's usually good UX practice to navigate using links.
I agree that this won't completely solve the long url issue, but it will help in certain cases. I'd really like to understand better what the differences are between the POST solution and this one, to be able to solve your issues. The `getExploreLongUrl` function is also used by Explore to set the url, so presumably any issues with long urls would also apply there.
I think a workable plan to get rid of the long url issue permanently could be to set up some kind of "exploration context" object in the db that has all this information and can be referenced by id.
--
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] suddjian commented on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
suddjian commented on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-946083688
The goal with moving from `POST` to an `<a>` link was to improve UX by allowing proper browser controls (such as "open in a new tab" vs in-tab navigation). It's usually good UX practice to navigate using links.
I'd really like to understand better what the differences are between the POST solution and this one, to be able to solve your issues. The `getExploreLongUrl` function is also used by Explore to set the url, so presumably any issues with long urls would also apply there.
--
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 #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-948098889
- this is a quick summary that compares GET vs POST:
[https://www.w3schools.com/tags/ref_httpmethods.asp
](https://www.w3schools.com/tags/ref_httpmethods.asp
)[https://www.guru99.com/difference-get-post-http.html](https://www.guru99.com/difference-get-post-http.html)
they all mentioned about the length limit of the URL
- <a> is html tag, it can trigger POST request, just add your own onClick event handler, and check if it is simple click or context menu click. in the click handler you can decide open in the same window or open another window.
- Superset use POST to handle click event in many other places, and they don't cause any issue. (surprised?) check this menu: <img width="545" alt="Screen Shot 2021-10-20 at 3 59 21 PM" src="https://user-images.githubusercontent.com/27990562/138183992-0660a82a-bcf0-4d51-98ad-262979e66a09.png">
(you can image that chart's query could be very long very complicated right? by GET method there is no way to handle this 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] codecov[bot] edited a comment on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-945988561
# [Codecov](https://codecov.io/gh/apache/superset/pull/17145?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 [#17145](https://codecov.io/gh/apache/superset/pull/17145?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6cdc8ff) into [master](https://codecov.io/gh/apache/superset/commit/ae4ced8da6933cdc657452d1f11415c49c6c68b8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae4ced8) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/17145?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 #17145 +/- ##
=======================================
Coverage 76.68% 76.68%
=======================================
Files 1038 1038
Lines 55516 55544 +28
Branches 7564 7564
=======================================
+ Hits 42570 42596 +26
- Misses 12696 12698 +2
Partials 250 250
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `70.93% <100.00%> (+0.02%)` | :arrow_up: |
| mysql | `81.90% <ø> (ø)` | |
| postgres | `81.91% <ø> (ø)` | |
| python | `81.99% <ø> (ø)` | |
| sqlite | `81.58% <ø> (ø)` | |
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/17145?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `80.37% <100.00%> (ø)` | |
| [...uperset-frontend/src/explore/exploreUtils/index.js](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZXhwbG9yZVV0aWxzL2luZGV4Lmpz) | `67.07% <100.00%> (+0.40%)` | :arrow_up: |
| [...src/dashboard/components/DashboardBuilder/state.ts](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZEJ1aWxkZXIvc3RhdGUudHM=) | `71.42% <0.00%> (-1.75%)` | :arrow_down: |
| [...c/filters/components/Select/SelectFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9TZWxlY3QvU2VsZWN0RmlsdGVyUGx1Z2luLnRzeA==) | `80.64% <0.00%> (-0.47%)` | :arrow_down: |
| [...board/components/nativeFilters/FilterBar/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL2luZGV4LnRzeA==) | `88.09% <0.00%> (-0.10%)` | :arrow_down: |
| [...Filters/FilterBar/FilterControls/FilterControl.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0ZpbHRlckNvbnRyb2xzL0ZpbHRlckNvbnRyb2wudHN4) | `100.00% <0.00%> (ø)` | |
| [.../FilterBar/CascadeFilters/CascadePopover/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQmFyL0Nhc2NhZGVGaWx0ZXJzL0Nhc2NhZGVQb3BvdmVyL2luZGV4LnRzeA==) | `63.23% <0.00%> (ø)` | |
| [...frontend/src/components/TimezoneSelector/index.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGltZXpvbmVTZWxlY3Rvci9pbmRleC50c3g=) | `97.77% <0.00%> (+0.05%)` | :arrow_up: |
| [...src/filters/components/Range/RangeFilterPlugin.tsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZpbHRlcnMvY29tcG9uZW50cy9SYW5nZS9SYW5nZUZpbHRlclBsdWdpbi50c3g=) | `88.57% <0.00%> (+0.33%)` | :arrow_up: |
| [...ilters/FilterBar/FilterControls/FilterControls.tsx](https://codecov.io/gh/apache/superset/pull/17145/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.85% <0.00%> (+0.85%)` | :arrow_up: |
| ... and [5 more](https://codecov.io/gh/apache/superset/pull/17145/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/17145?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/17145?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 [ae4ced8...6cdc8ff](https://codecov.io/gh/apache/superset/pull/17145?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] codecov[bot] edited a comment on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-945988561
# [Codecov](https://codecov.io/gh/apache/superset/pull/17145?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 [#17145](https://codecov.io/gh/apache/superset/pull/17145?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6cdc8ff) into [master](https://codecov.io/gh/apache/superset/commit/ae4ced8da6933cdc657452d1f11415c49c6c68b8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae4ced8) will **not change** coverage.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/17145?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 #17145 +/- ##
=======================================
Coverage 76.68% 76.68%
=======================================
Files 1038 1038
Lines 55516 55516
Branches 7564 7564
=======================================
Hits 42570 42570
Misses 12696 12696
Partials 250 250
```
| Flag | Coverage Δ | |
|---|---|---|
| mysql | `81.90% <ø> (ø)` | |
| postgres | `81.91% <ø> (ø)` | |
| python | `81.99% <ø> (ø)` | |
| sqlite | `81.58% <ø> (ø)` | |
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/17145?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../src/dashboard/components/gridComponents/Chart.jsx](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0NoYXJ0LmpzeA==) | `80.37% <100.00%> (ø)` | |
| [...uperset-frontend/src/explore/exploreUtils/index.js](https://codecov.io/gh/apache/superset/pull/17145/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvZXhwbG9yZVV0aWxzL2luZGV4Lmpz) | `66.66% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/17145?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/17145?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 [ae4ced8...6cdc8ff](https://codecov.io/gh/apache/superset/pull/17145?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 commented on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-946056824
Hi @suddjian thanks for the work. But I do not think from dashboard -> explore chart should use GET request with full form params in the url. Superset used to handle this issue very well, but was broken since #15668. I do not think this solution, that reduces a couple of extra parameters from dashboard, can fix the root cause: browser has limits for the url length, and many companies with Nginx also limits the total length of GET request url.
May i know why not use POST request for this problem, as Superset did before?
--
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] suddjian edited a comment on pull request #17145: refactor: dashboard->explore url generation
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on pull request #17145:
URL: https://github.com/apache/superset/pull/17145#issuecomment-946083688
The goal with moving from `POST` to an `<a>` link was to improve UX by allowing proper browser controls (such as "open in a new tab" vs in-tab navigation). It's usually good UX practice to navigate using links.
I'd really like to understand better what the differences are between the POST solution and this one, to be able to solve your issues. The `getExploreLongUrl` function is also used by Explore to set the url, so presumably any issues with long urls would also apply there.
I think a workable plan to get rid of the long url issue permanently could be to set up some kind of "exploration context" object in the db that has all this information and can be referenced by id.
--
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