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/07/12 14:29:02 UTC
[GitHub] [superset] codyml opened a new pull request, #20684: Fix missing metadata.
codyml opened a new pull request, #20684:
URL: https://github.com/apache/superset/pull/20684
<!---
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 -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] 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] zhaoyongjie commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1185574979
This PR should wait merging [PR](https://github.com/apache/superset/pull/20673)
--
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] github-actions[bot] commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1190127044
Ephemeral environment shutdown and build artifacts deleted.
--
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 #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1185519227
I did some digging and this is the [PR](https://github.com/apache/superset/pull/11336) that introduced that filter. Maybe @nytai has some context on it?
--
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] john-bodley commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
john-bodley commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1307873917
@codyml in reference to your comment,
> Note: I'm opening this PR under the assumption that this filter was present for no good reason. If there's a reason that you should only be able to add charts that you own to a dashboard, let me know and I'll find another solution.
I thought there was merit in sharing some user feedback—for context at Airbnb where we have a significant number of charts, i.e., in excess of 100k—I received:
> To me the experience is degraded, because almost all charts I add are created by me, and there’s a clear candidate for the next chart to add, i.e., it’s the chart most recently created by me.
>
> Of course it’s good to be able to add charts created by others, but I would think that maybe this could be visible through a toggle (default is to show charts created by me, with toggle in “on” stage for “filter to my charts”).
>
> This change of removing the filter might be an improvement in a small company, but in a big place it’s very unlikely that charts by others are useful to me, there’s just too many.
Based on this feedback I was wondering whether:
1. There should be a toggle button (or similar) to filter between your charts (default) and all charts to help streamline the user workflow—especially in the case where exists thousands and thousands of charts.
2. The placeholder text should be updated given that "Filter your charts" is no longer accurate.
Thoughts?
--
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] codyml commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1270566304
> QA team found an issue that when user add a chart (not owner) to the dashboard and then delete, user will automatically become owner of the chart. This issue has been filed in ticket and will be fixed in another pr. Other than that, LGTM~
Linking for future documentation: https://github.com/apache/superset/pull/21720
--
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] jinghua-qa commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1185219056
/testenv up
--
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] jinghua-qa commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1186008015
QA team found an issue that when user add a chart (not owner) to the dashboard and then delete, user will automatically become owner of the chart. This issue has been filed in ticket and will be fixed in another pr. Other than that, LGTM~
--
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] github-actions[bot] commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1185220386
@jinghua-qa Ephemeral environment spinning up at http://35.166.98.73:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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] codyml commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1185653640
> I did some digging and this is the [PR](https://github.com/apache/superset/pull/11336) that introduced that filter. Maybe @nytai has some context on it?
@michael-s-molina Looks like that PR transitioned from another endpoint that had a similar restriction – do you know if there's any spec for what the permissions are supposed to be around adding charts to a dashboard? My thought would be that if you have view permissions on a chart, you should be able to add it to a dashboard that you have edit permissions for, but that's just off the top of my head, I don't have a great understanding of the permissions model.
--
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 #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1182596620
# [Codecov](https://codecov.io/gh/apache/superset/pull/20684?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 [#20684](https://codecov.io/gh/apache/superset/pull/20684?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a19d3a0) into [master](https://codecov.io/gh/apache/superset/commit/7f918a4ec0e162be13bf3fc0e2f15aaaa5450cec?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7f918a4) will **decrease** coverage by `0.00%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #20684 +/- ##
==========================================
- Coverage 66.85% 66.85% -0.01%
==========================================
Files 1753 1753
Lines 65827 65833 +6
Branches 7006 7007 +1
==========================================
+ Hits 44010 44011 +1
- Misses 20031 20036 +5
Partials 1786 1786
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.95% <ø> (-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/20684?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...et-frontend/src/dashboard/actions/sliceEntities.js](https://codecov.io/gh/apache/superset/pull/20684/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL3NsaWNlRW50aXRpZXMuanM=) | `74.28% <ø> (ø)` | |
| [...perset-frontend/src/addSlice/AddSliceContainer.tsx](https://codecov.io/gh/apache/superset/pull/20684/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FkZFNsaWNlQ29udGFpbmVyLnRzeA==) | `59.61% <0.00%> (-7.06%)` | :arrow_down: |
| [superset-frontend/src/addSlice/App.tsx](https://codecov.io/gh/apache/superset/pull/20684/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2FkZFNsaWNlL0FwcC50c3g=) | `0.00% <0.00%> (ø)` | |
| [...nd/src/views/CRUD/data/dataset/AddDatasetModal.tsx](https://codecov.io/gh/apache/superset/pull/20684/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXRNb2RhbC50c3g=) | `42.50% <0.00%> (ø)` | |
| [...kages/superset-ui-core/src/query/processFilters.ts](https://codecov.io/gh/apache/superset/pull/20684/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-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvcHJvY2Vzc0ZpbHRlcnMudHM=) | `100.00% <0.00%> (+8.33%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20684?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/20684?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 [7f918a4...a19d3a0](https://codecov.io/gh/apache/superset/pull/20684?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] michael-s-molina merged pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
michael-s-molina merged PR #20684:
URL: https://github.com/apache/superset/pull/20684
--
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] codyml commented on pull request #20684: fix(dashboard): Fix missing metadata on draggable dashboard edit chart cards
Posted by GitBox <gi...@apache.org>.
codyml commented on PR #20684:
URL: https://github.com/apache/superset/pull/20684#issuecomment-1309483293
@john-bodley Thanks for the follow-up! Those points make a lot of sense to me and it sounds like there's definitely room for improvement.
From what I remember, no one at Preset was clamoring for being able to add charts that you don't own to a dashboard (cc @lauderbaugh in case I'm wrong). I think I chose this approach just because it seemed like the easiest way to resolve the missing metadata bug caused by the list trying to display charts that you don't own that were already added to the dashboard.
I like the idea of a toggle – that sounds like a best of both worlds solution. A bandaid solution could also be to revert this change and solve the missing metadata issue by just not trying to show charts added to the dashboard that you don't own in the list at all. The downside of that would be that if you remove a chart you don't own from a dashboard there would be no way to get it back. But, that was already sort of a problem before this PR: if you removed a chart you didn't own and then saved the dashboard, you wouldn't be able to add it back from the dashboard edit list.
Is changing this behavior something that you'd like to work on? Happy to discuss these or any other ideas further or do any PR reviews as helpful – let me know!
--
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