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/02/05 22:59:48 UTC
[GitHub] [superset] suddjian opened a new pull request #12978: feat(dashboard): API to provide a dashboard's charts
suddjian opened a new pull request #12978:
URL: https://github.com/apache/superset/pull/12978
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Adds a new API to get the chart definitions for a specific dashboard. This is going to support future efforts to turn Superset's frontend into a single page app.
**Alternative considered:** Using `GET` `dashboard/:id/related/slices`, and iterating over the results to fetch all the slices from `/charts/:id`.
I rejected this approach because it would require changing the `/related/slices` endpoint (currently the endpoint returns the names of charts rather than ids, which is pretty useless for this purpose), and also because it would result in a large number of API requests. The chosen method of a single convenient API endpoint is much simpler.
Thanks to @pkdotson for pairing ❤️
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
Wrote tests
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (2d3b001) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `13.67%`.
> The diff coverage is `36.99%`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
===========================================
+ Coverage 53.06% 66.73% +13.67%
===========================================
Files 489 491 +2
Lines 17314 28970 +11656
Branches 4482 0 -4482
===========================================
+ Hits 9187 19334 +10147
- Misses 8127 9636 +1509
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| python | `66.73% <36.99%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/constants.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uc3RhbnRzLnB5) | `100.00% <ø> (ø)` | |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `73.19% <ø> (ø)` | |
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [...s/260bf0649a77\_migrate\_x\_dateunit\_in\_time\_range.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8yNjBiZjA2NDlhNzdfbWlncmF0ZV94X2RhdGV1bml0X2luX3RpbWVfcmFuZ2UucHk=) | `0.00% <0.00%> (ø)` | |
| [...ons/versions/41ce8799acc3\_rename\_pie\_label\_type.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy80MWNlODc5OWFjYzNfcmVuYW1lX3BpZV9sYWJlbF90eXBlLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/connectors/sqla/views.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3ZpZXdzLnB5) | `62.43% <4.34%> (ø)` | |
| [superset/views/datasource.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS5weQ==) | `88.70% <16.66%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `87.63% <66.66%> (ø)` | |
| [superset/charts/commands/exceptions.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2V4Y2VwdGlvbnMucHk=) | `92.85% <77.77%> (ø)` | |
| [superset/db\_engine\_specs/elasticsearch.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | `89.74% <93.75%> (ø)` | |
| ... and [958 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [86807e4...2d3b001](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (6de6192) into [master](https://codecov.io/gh/apache/superset/commit/89b6a2cfcb087979c420534b24ceaa1ed5d08374?el=desc) (89b6a2c) will **decrease** coverage by `14.15%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
===========================================
- Coverage 66.75% 52.60% -14.16%
===========================================
Files 1015 480 -535
Lines 49634 17315 -32319
Branches 4839 4485 -354
===========================================
- Hits 33134 9108 -24026
+ Misses 16377 8207 -8170
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `52.60% <ø> (+1.61%)` | :arrow_up: |
| javascript | `?` | |
| python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...rc/dashboard/components/gridComponents/Divider.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0RpdmlkZXIuanN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...end/src/SqlLab/components/ExploreResultsButton.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0V4cGxvcmVSZXN1bHRzQnV0dG9uLmpzeA==) | `8.00% <0.00%> (-84.00%)` | :arrow_down: |
| [...nd/src/views/CRUD/data/query/QueryPreviewModal.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9xdWVyeS9RdWVyeVByZXZpZXdNb2RhbC50c3g=) | `14.70% <0.00%> (-82.97%)` | :arrow_down: |
| ... and [909 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [89b6a2c...f152f56](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (6de6192) into [master](https://codecov.io/gh/apache/superset/commit/89b6a2cfcb087979c420534b24ceaa1ed5d08374?el=desc) (89b6a2c) will **decrease** coverage by `14.55%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
===========================================
- Coverage 66.75% 52.20% -14.56%
===========================================
Files 1015 441 -574
Lines 49634 14825 -34809
Branches 4839 3959 -880
===========================================
- Hits 33134 7739 -25395
+ Misses 16377 7086 -9291
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `52.20% <ø> (+1.22%)` | :arrow_up: |
| javascript | `?` | |
| python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...t-frontend/src/dashboard/containers/SliceAdder.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL1NsaWNlQWRkZXIuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ontend/src/dashboard/util/serializeFilterScopes.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL3NlcmlhbGl6ZUZpbHRlclNjb3Blcy5qcw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rc/dashboard/util/getLayoutComponentFromChartId.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldExheW91dENvbXBvbmVudEZyb21DaGFydElkLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...d/src/dashboard/util/updateComponentParentsList.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL3VwZGF0ZUNvbXBvbmVudFBhcmVudHNMaXN0Lmpz) | `0.00% <0.00%> (-87.50%)` | :arrow_down: |
| [...erset-frontend/src/components/DatabaseSelector.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRGF0YWJhc2VTZWxlY3Rvci50c3g=) | `3.40% <0.00%> (-87.02%)` | :arrow_down: |
| ... and [938 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [89b6a2c...f152f56](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r576005866
##########
File path: tests/dashboards/api_tests.py
##########
@@ -109,21 +112,35 @@ def create_dashboards(self):
with self.create_app().app_context():
dashboards = []
admin = self.get_user("admin")
- for cx in range(DASHBOARDS_FIXTURE_COUNT - 1):
- dashboards.append(
- self.insert_dashboard(f"title{cx}", f"slug{cx}", [admin.id])
+ charts = []
+ half_dash_count = round(DASHBOARDS_FIXTURE_COUNT / 2)
+ for cx in range(DASHBOARDS_FIXTURE_COUNT):
+ dashboard = self.insert_dashboard(
+ f"title{cx}",
+ f"slug{cx}",
+ [admin.id],
+ slices=charts if cx < half_dash_count else [],
)
+ if cx < half_dash_count:
+ chart = self.insert_chart(f"slice{cx}", [admin.id], 1, params="{}")
Review comment:
Non blocking. Having one or a bunch of dashboards with more then 1 chart could make a better GET test
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (2d3b001) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `13.67%`.
> The diff coverage is `36.99%`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
===========================================
+ Coverage 53.06% 66.74% +13.67%
===========================================
Files 489 491 +2
Lines 17314 28972 +11658
Branches 4482 0 -4482
===========================================
+ Hits 9187 19336 +10149
- Misses 8127 9636 +1509
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| python | `66.74% <36.99%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/constants.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uc3RhbnRzLnB5) | `100.00% <ø> (ø)` | |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `73.19% <ø> (ø)` | |
| [...43f2fdb\_add\_granularity\_to\_charts\_where\_missing.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8wNzBjMDQzZjJmZGJfYWRkX2dyYW51bGFyaXR5X3RvX2NoYXJ0c193aGVyZV9taXNzaW5nLnB5) | `0.00% <0.00%> (ø)` | |
| [...s/260bf0649a77\_migrate\_x\_dateunit\_in\_time\_range.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy8yNjBiZjA2NDlhNzdfbWlncmF0ZV94X2RhdGV1bml0X2luX3RpbWVfcmFuZ2UucHk=) | `0.00% <0.00%> (ø)` | |
| [...ons/versions/41ce8799acc3\_rename\_pie\_label\_type.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbWlncmF0aW9ucy92ZXJzaW9ucy80MWNlODc5OWFjYzNfcmVuYW1lX3BpZV9sYWJlbF90eXBlLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/connectors/sqla/views.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3ZpZXdzLnB5) | `62.43% <4.34%> (ø)` | |
| [superset/views/datasource.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YXNvdXJjZS5weQ==) | `88.70% <16.66%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `87.63% <66.66%> (ø)` | |
| [superset/charts/commands/exceptions.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2NvbW1hbmRzL2V4Y2VwdGlvbnMucHk=) | `92.85% <77.77%> (ø)` | |
| [superset/db\_engine\_specs/elasticsearch.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | `89.74% <93.75%> (ø)` | |
| ... and [958 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [86807e4...2d3b001](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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 #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774335288
Requesting review from @dpgaspar to make sure I got the security related code and doc strings correct :)
----------------------------------------------------------------
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.
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 #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774335288
Requesting review from @dpgaspar to make sure I got the security related code and doc strings correct :)
edit: CI says there's something wrong with my doc string but I'm not sure how to fix 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.
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 #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774580650
Thanks for the QA @junlincc, but I don't think that error could possibly have anything to do with this PR. No existing functionality has been changed here.
----------------------------------------------------------------
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.
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] junlincc commented on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774580875
got it, that's strange. i also checked on master and other branches which don't have this issue. please disregard if it's not related.
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (f152f56) into [master](https://codecov.io/gh/apache/superset/commit/89b6a2cfcb087979c420534b24ceaa1ed5d08374?el=desc) (89b6a2c) will **decrease** coverage by `6.42%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
==========================================
- Coverage 66.75% 60.33% -6.43%
==========================================
Files 1015 969 -46
Lines 49634 46050 -3584
Branches 4839 4485 -354
==========================================
- Hits 33134 27783 -5351
- Misses 16377 18267 +1890
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `48.68% <ø> (-2.30%)` | :arrow_down: |
| javascript | `?` | |
| python | `67.34% <100.00%> (+3.38%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/constants.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uc3RhbnRzLnB5) | `100.00% <ø> (ø)` | |
| [superset/dashboards/api.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9hcGkucHk=) | `87.24% <100.00%> (+0.60%)` | :arrow_up: |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `95.09% <100.00%> (+0.41%)` | :arrow_up: |
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...tend/src/visualizations/FilterBox/controlPanel.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL0ZpbHRlckJveC9jb250cm9sUGFuZWwuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| ... and [494 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [89b6a2c...f152f56](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] amitmiran137 commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
amitmiran137 commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571431126
##########
File path: tests/dashboards/api_tests.py
##########
@@ -218,9 +270,9 @@ def test_get_dashboard_not_found(self):
"""
Dashboard API: Test get dashboard not found
"""
- max_id = db.session.query(func.max(Dashboard.id)).scalar()
+ bad_id = self.get_nonexistent_dashboard_id()
Review comment:
thank you for much for fixing this!!
----------------------------------------------------------------
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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r573003576
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
+ query = db.session.query(Dashboard).filter(Dashboard.id == dashboard_id)
+ dashboard = query.scalar()
+ if not dashboard:
+ raise DashboardNotFoundError()
+ return [slice.data for slice in dashboard.slices]
Review comment:
The extra queries were on both, while iterating the relation and when `Slice.data` accesses owners. SQLAlchemy lazy loads by default.
If you mean generically, no, `slice.data` is a specific custom Slice model method/property.
If you mean the accepted way of getting a `Dict` from a slice model on superset, yes, but since we are laying the ground here for a future SPA, I prefer using marshmallow to serialise SQLA objects.
What do you think?
----------------------------------------------------------------
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.
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-io commented on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (f152f56) into [master](https://codecov.io/gh/apache/superset/commit/89b6a2cfcb087979c420534b24ceaa1ed5d08374?el=desc) (89b6a2c) will **increase** coverage by `0.18%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
==========================================
+ Coverage 66.75% 66.94% +0.18%
==========================================
Files 1015 489 -526
Lines 49634 28707 -20927
Branches 4839 0 -4839
==========================================
- Hits 33134 19217 -13917
+ Misses 16377 9490 -6887
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `66.94% <100.00%> (+2.97%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/constants.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uc3RhbnRzLnB5) | `100.00% <ø> (ø)` | |
| [superset/dashboards/api.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9hcGkucHk=) | `87.24% <100.00%> (+0.60%)` | :arrow_up: |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `95.09% <100.00%> (+0.41%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `32.65% <0.00%> (-59.19%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `59.64% <0.00%> (-22.81%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `89.65% <0.00%> (-10.35%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-10.00%)` | :arrow_down: |
| [superset/utils/cache.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `76.34% <0.00%> (-8.77%)` | :arrow_down: |
| ... and [594 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [89b6a2c...f152f56](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (f152f56) into [master](https://codecov.io/gh/apache/superset/commit/89b6a2cfcb087979c420534b24ceaa1ed5d08374?el=desc) (89b6a2c) will **decrease** coverage by `4.95%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
==========================================
- Coverage 66.75% 61.80% -4.96%
==========================================
Files 1015 969 -46
Lines 49634 46050 -3584
Branches 4839 4485 -354
==========================================
- Hits 33134 28461 -4673
- Misses 16377 17589 +1212
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `52.60% <ø> (+1.61%)` | :arrow_up: |
| javascript | `?` | |
| python | `67.34% <100.00%> (+3.38%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/constants.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uc3RhbnRzLnB5) | `100.00% <ø> (ø)` | |
| [superset/dashboards/api.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9hcGkucHk=) | `87.24% <100.00%> (+0.60%)` | :arrow_up: |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `95.09% <100.00%> (+0.41%)` | :arrow_up: |
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| ... and [484 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [89b6a2c...f152f56](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] junlincc commented on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
junlincc commented on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774559748
Congrats on the first step and thanks for the collaboration! @suddjian @pkdotson
seems to me that Multiline chart is not passing through the new API.
<img width="1723" alt="Screen Shot 2021-02-06 at 3 28 05 PM" src="https://user-images.githubusercontent.com/67837651/107131962-0d246380-6890-11eb-9324-2219e25049f5.png">
<img width="927" alt="Screen Shot 2021-02-06 at 3 27 43 PM" src="https://user-images.githubusercontent.com/67837651/107131967-1a415280-6890-11eb-8674-21c301ee4a40.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.
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] junlincc edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
junlincc edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774559748
Congrats on the first step and thanks for the collaboration! @suddjian @pkdotson
seems to me that Multiline chart is not passing through the new API.
<img width="927" alt="Screen Shot 2021-02-06 at 3 27 43 PM" src="https://user-images.githubusercontent.com/67837651/107131967-1a415280-6890-11eb-8674-21c301ee4a40.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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571591468
##########
File path: superset/dashboards/api.py
##########
@@ -210,6 +211,52 @@ def __init__(self) -> None:
self.include_route_methods = self.include_route_methods | {"thumbnail"}
super().__init__()
+ @expose("/<pk>/charts", methods=["GET"])
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get_charts",
+ log_to_statsd=False,
+ )
+ def get_charts(self, pk: int) -> Response:
+ """Gets the chart definitions for a given dashboard
+ ---
+ get:
+ description: >-
+ Get the chart definitions for a given dashboard
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ responses:
+ 200:
+ description: Dashboard chart definitions
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: array
+ items:
+ $ref: '#/components/schemas/ChartRestApi.post'
Review comment:
`ChartRestApi.post` schema is not equal to this endpoint response schema
----------------------------------------------------------------
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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r573809040
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
+ query = db.session.query(Dashboard).filter(Dashboard.id == dashboard_id)
+ dashboard = query.scalar()
+ if not dashboard:
+ raise DashboardNotFoundError()
+ return [slice.data for slice in dashboard.slices]
Review comment:
For now it's just a personal preference. I don't want to block this PR. Probably for now it's better to just solve the N+1 problem and keep the using slice.data.
----------------------------------------------------------------
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.
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 #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian merged pull request #12978:
URL: https://github.com/apache/superset/pull/12978
----------------------------------------------------------------
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.
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] ktmud commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r573294603
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
+ query = db.session.query(Dashboard).filter(Dashboard.id == dashboard_id)
+ dashboard = query.scalar()
+ if not dashboard:
+ raise DashboardNotFoundError()
+ return [slice.data for slice in dashboard.slices]
Review comment:
If we defaults to marshmallow for object serialization, does that mean `x.data` will be eventually deprecated someday?
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (f152f56) into [master](https://codecov.io/gh/apache/superset/commit/89b6a2cfcb087979c420534b24ceaa1ed5d08374?el=desc) (89b6a2c) will **decrease** coverage by `4.81%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
==========================================
- Coverage 66.75% 61.94% -4.82%
==========================================
Files 1015 969 -46
Lines 49634 46050 -3584
Branches 4839 4485 -354
==========================================
- Hits 33134 28525 -4609
- Misses 16377 17525 +1148
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `52.97% <ø> (+1.98%)` | :arrow_up: |
| javascript | `?` | |
| python | `67.34% <100.00%> (+3.38%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/constants.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uc3RhbnRzLnB5) | `100.00% <ø> (ø)` | |
| [superset/dashboards/api.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9hcGkucHk=) | `87.24% <100.00%> (+0.60%)` | :arrow_up: |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `95.09% <100.00%> (+0.41%)` | :arrow_up: |
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| ... and [484 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [89b6a2c...f152f56](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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 a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571519507
##########
File path: tests/dashboards/api_tests.py
##########
@@ -146,6 +151,53 @@ def create_dashboard_with_report(self):
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboard_charts(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ slice = self.insert_chart("slice1", [admin.id], 1, params="{}")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[slice]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
+ self.assertEqual(data["result"], [slice.data])
+ db.session.delete(dashboard)
+ db.session.delete(slice)
+ db.session.commit()
Review comment:
I agree. I was modeling this after the other tests around it, but have since learned about pytest fixtures. Will change.
----------------------------------------------------------------
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.
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] amitmiran137 commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
amitmiran137 commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571430585
##########
File path: tests/dashboards/api_tests.py
##########
@@ -146,6 +151,53 @@ def create_dashboard_with_report(self):
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboard_charts(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ slice = self.insert_chart("slice1", [admin.id], 1, params="{}")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[slice]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
+ self.assertEqual(data["result"], [slice.data])
+ db.session.delete(dashboard)
+ db.session.delete(slice)
+ db.session.commit()
Review comment:
I think you should handle dashboard and slice lifecycle(creation and destruction ) in the teardown after each test
##########
File path: tests/dashboards/api_tests.py
##########
@@ -146,6 +151,53 @@ def create_dashboard_with_report(self):
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboard_charts(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ slice = self.insert_chart("slice1", [admin.id], 1, params="{}")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[slice]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
+ self.assertEqual(data["result"], [slice.data])
+ db.session.delete(dashboard)
+ db.session.delete(slice)
+ db.session.commit()
+
+ def test_get_dashboard_charts_not_found(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard that does not exist
+ """
+ self.login(username="admin")
+ bad_id = self.get_nonexistent_dashboard_id()
+ uri = f"api/v1/dashboard/{bad_id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 404)
+ db.session.commit()
+
+ def test_get_dashboard_charts_empty(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard without any charts
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
Review comment:
extract method for a reuse in all other tests
----------------------------------------------------------------
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.
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 a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r574970205
##########
File path: superset/dashboards/api.py
##########
@@ -210,6 +211,52 @@ def __init__(self) -> None:
self.include_route_methods = self.include_route_methods | {"thumbnail"}
super().__init__()
+ @expose("/<pk>/charts", methods=["GET"])
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get_charts",
+ log_to_statsd=False,
+ )
+ def get_charts(self, pk: int) -> Response:
+ """Gets the chart definitions for a given dashboard
+ ---
+ get:
+ description: >-
+ Get the chart definitions for a given dashboard
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ responses:
+ 200:
+ description: Dashboard chart definitions
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: array
+ items:
+ $ref: '#/components/schemas/ChartRestApi.post'
Review comment:
fixed, thanks 🙂
----------------------------------------------------------------
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.
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 a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571520555
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
+ query = db.session.query(Dashboard).filter(Dashboard.id == dashboard_id)
+ dashboard = query.scalar()
+ if not dashboard:
+ raise DashboardNotFoundError()
+ return [slice.data for slice in dashboard.slices]
Review comment:
Interesting. Is it calling `dashboard.slices` that triggers many queries, or `slice.data` in a loop?
I called `slice.data` to get it into a JSON serializable format. Is that not the accepted way to do that with a SQLAlchemy 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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r573003576
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
+ query = db.session.query(Dashboard).filter(Dashboard.id == dashboard_id)
+ dashboard = query.scalar()
+ if not dashboard:
+ raise DashboardNotFoundError()
+ return [slice.data for slice in dashboard.slices]
Review comment:
The extra queries were on both, while iterating the relation and when `Slice.data` accesses owners. SQLAlchemy lazy loads by default.
I you mean generically no, `slice.data` is a specific custom Slice model method/property. If you mean the accepted way of getting a `Dict` from a slice model on superset yes, but since we are laying the ground here for a future SPA, I prefer using marshmallow to serialise SQLA objects.
Do do you think?
----------------------------------------------------------------
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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r576031971
##########
File path: superset/dashboards/api.py
##########
@@ -216,6 +219,53 @@ def __init__(self) -> None:
self.include_route_methods = self.include_route_methods | {"thumbnail"}
super().__init__()
+ @expose("/<pk>/charts", methods=["GET"])
+ @protect()
+ @safe
Review comment:
right! good point @amitmiran137, we can use `dashboard = self.datamodel.get(pk, self._base_filters)`
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (f152f56) into [master](https://codecov.io/gh/apache/superset/commit/89b6a2cfcb087979c420534b24ceaa1ed5d08374?el=desc) (89b6a2c) will **increase** coverage by `0.55%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
==========================================
+ Coverage 66.75% 67.31% +0.55%
==========================================
Files 1015 489 -526
Lines 49634 28735 -20899
Branches 4839 0 -4839
==========================================
- Hits 33134 19342 -13792
+ Misses 16377 9393 -6984
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `?` | |
| javascript | `?` | |
| python | `67.31% <100.00%> (+3.34%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/constants.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uc3RhbnRzLnB5) | `100.00% <ø> (ø)` | |
| [superset/dashboards/api.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9hcGkucHk=) | `87.24% <100.00%> (+0.60%)` | :arrow_up: |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `95.09% <100.00%> (+0.41%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `89.65% <0.00%> (-10.35%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `73.84% <0.00%> (-10.00%)` | :arrow_down: |
| [superset/utils/cache.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `76.34% <0.00%> (-8.77%)` | :arrow_down: |
| [superset/db\_engine\_specs/elasticsearch.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | `86.95% <0.00%> (-7.49%)` | :arrow_down: |
| [superset/db\_engine\_specs/clickhouse.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2NsaWNraG91c2UucHk=) | `87.09% <0.00%> (-7.35%)` | :arrow_down: |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| ... and [578 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [89b6a2c...f152f56](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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] amitmiran137 commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
amitmiran137 commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r575673552
##########
File path: superset/dashboards/api.py
##########
@@ -216,6 +219,53 @@ def __init__(self) -> None:
self.include_route_methods = self.include_route_methods | {"thumbnail"}
super().__init__()
+ @expose("/<pk>/charts", methods=["GET"])
+ @protect()
+ @safe
Review comment:
Should additionally use `has_dashboard access` decorator to enforce access for the dashboard
----------------------------------------------------------------
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.
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] nytai commented on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774339753
@suddjian You can probably get some logs if you visit `/swagger/v1` which the result of compiling these docstrings.
----------------------------------------------------------------
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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571496843
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
+ query = db.session.query(Dashboard).filter(Dashboard.id == dashboard_id)
+ dashboard = query.scalar()
+ if not dashboard:
+ raise DashboardNotFoundError()
+ return [slice.data for slice in dashboard.slices]
Review comment:
This will trigger N+1 queries on the DB. A possible solution can be `dashboard = db.session.query(Dashboard).join(Slice, Dashboard.slices).join(Slice.table).filter(Dashboard.id == 1).options(contains_eager(Dashboard.slices).contains_eager(Slice.owners)).one_or_none()`
I would avoid using `slice.data` unless it's really handy on this case
##########
File path: tests/dashboards/api_tests.py
##########
@@ -146,6 +151,53 @@ def create_dashboard_with_report(self):
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboard_charts(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ slice = self.insert_chart("slice1", [admin.id], 1, params="{}")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[slice]
+ )
Review comment:
can you change this to use a pytest.fixture we already have one `create_dashboards`
##########
File path: tests/dashboards/api_tests.py
##########
@@ -1302,7 +1354,7 @@ def test_import_dashboard_overwrite(self):
def test_import_dashboard_invalid(self):
"""
- Dataset API: Test import invalid dashboard
+ Dashboard API: Test import invalid dashboard
Review comment:
copy pasta ;)
##########
File path: tests/dashboards/api_tests.py
##########
@@ -98,6 +100,9 @@ def insert_dashboard(
db.session.commit()
return dashboard
+ def get_nonexistent_dashboard_id(self):
+ return (db.session.query(func.max(Dashboard.id)).scalar() or 0) + 1
Review comment:
nice!
##########
File path: tests/dashboards/api_tests.py
##########
@@ -146,6 +151,53 @@ def create_dashboard_with_report(self):
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboard_charts(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ slice = self.insert_chart("slice1", [admin.id], 1, params="{}")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[slice]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
+ self.assertEqual(data["result"], [slice.data])
+ db.session.delete(dashboard)
+ db.session.delete(slice)
+ db.session.commit()
+
+ def test_get_dashboard_charts_not_found(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard that does not exist
+ """
+ self.login(username="admin")
+ bad_id = self.get_nonexistent_dashboard_id()
+ uri = f"api/v1/dashboard/{bad_id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 404)
+ db.session.commit()
+
+ def test_get_dashboard_charts_empty(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard without any charts
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard(
Review comment:
pytest.fixture?
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
Review comment:
This method does not return `List[Slice]` but `List[Dict[str, Any]]`
----------------------------------------------------------------
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.
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 a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r573266011
##########
File path: tests/dashboards/api_tests.py
##########
@@ -146,6 +151,53 @@ def create_dashboard_with_report(self):
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboard_charts(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ slice = self.insert_chart("slice1", [admin.id], 1, params="{}")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[slice]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
+ self.assertEqual(data["result"], [slice.data])
+ db.session.delete(dashboard)
+ db.session.delete(slice)
+ db.session.commit()
+
+ def test_get_dashboard_charts_not_found(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard that does not exist
+ """
+ self.login(username="admin")
+ bad_id = self.get_nonexistent_dashboard_id()
+ uri = f"api/v1/dashboard/{bad_id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 404)
+ db.session.commit()
+
+ def test_get_dashboard_charts_empty(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard without any charts
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
Review comment:
I think there is value in keeping this explicit instead of adding a layer of indirection.
----------------------------------------------------------------
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.
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 a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571519285
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
Review comment:
Oh yes, thanks
----------------------------------------------------------------
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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571497685
##########
File path: tests/dashboards/api_tests.py
##########
@@ -1302,7 +1354,7 @@ def test_import_dashboard_overwrite(self):
def test_import_dashboard_invalid(self):
"""
- Dataset API: Test import invalid dashboard
+ Dashboard API: Test import invalid dashboard
Review comment:
copy pasta ;) (Thanks for the fix)
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
----------------------------------------------------------------
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.
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 a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r574970051
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
+ query = db.session.query(Dashboard).filter(Dashboard.id == dashboard_id)
+ dashboard = query.scalar()
+ if not dashboard:
+ raise DashboardNotFoundError()
+ return [slice.data for slice in dashboard.slices]
Review comment:
I've addressed this by using something similar to the suggested query, as well as adding a Marshmallow schema that is somewhat different from the existing `bootstrap_data` schema to remove a few less-REST-ful fields.
----------------------------------------------------------------
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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571592139
##########
File path: tests/dashboards/api_tests.py
##########
@@ -146,6 +151,53 @@ def create_dashboard_with_report(self):
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboard_charts(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ slice = self.insert_chart("slice1", [admin.id], 1, params="{}")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[slice]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
+ self.assertEqual(data["result"], [slice.data])
+ db.session.delete(dashboard)
+ db.session.delete(slice)
+ db.session.commit()
+
+ def test_get_dashboard_charts_not_found(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard that does not exist
+ """
+ self.login(username="admin")
+ bad_id = self.get_nonexistent_dashboard_id()
+ uri = f"api/v1/dashboard/{bad_id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 404)
+ db.session.commit()
+
+ def test_get_dashboard_charts_empty(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard without any charts
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
Review comment:
I think that @amitmiran137 is referring, is to create a method out of:
```
self.assertEqual(response.status_code, 200)
data = json.loads(response.data.decode("utf-8"))
```
and reuse it, that is certainly something we should do, but on all API tests, there's lot's DRYing opportunities. If you want to take the initial shot on this one. But the entire test module should be done on a different PR, IMO
----------------------------------------------------------------
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.
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 a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r571519785
##########
File path: tests/dashboards/api_tests.py
##########
@@ -146,6 +151,53 @@ def create_dashboard_with_report(self):
db.session.delete(dashboard)
db.session.commit()
+ def test_get_dashboard_charts(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ slice = self.insert_chart("slice1", [admin.id], 1, params="{}")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[slice]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
+ self.assertEqual(data["result"], [slice.data])
+ db.session.delete(dashboard)
+ db.session.delete(slice)
+ db.session.commit()
+
+ def test_get_dashboard_charts_not_found(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard that does not exist
+ """
+ self.login(username="admin")
+ bad_id = self.get_nonexistent_dashboard_id()
+ uri = f"api/v1/dashboard/{bad_id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 404)
+ db.session.commit()
+
+ def test_get_dashboard_charts_empty(self):
+ """
+ Dashboard API: Test getting charts belonging to a dashboard without any charts
+ """
+ self.login(username="admin")
+ admin = self.get_user("admin")
+ dashboard = self.insert_dashboard(
+ "title", "charted", [admin.id], admin, slices=[]
+ )
+ uri = f"api/v1/dashboard/{dashboard.id}/charts"
+ response = self.get_assert_metric(uri, "get_charts")
+ self.assertEqual(response.status_code, 200)
+ data = json.loads(response.data.decode("utf-8"))
Review comment:
I'm not sure what you want me to extract here
----------------------------------------------------------------
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.
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] dpgaspar commented on a change in pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #12978:
URL: https://github.com/apache/superset/pull/12978#discussion_r573809040
##########
File path: superset/dashboards/dao.py
##########
@@ -35,6 +36,14 @@ class DashboardDAO(BaseDAO):
model_cls = Dashboard
base_filter = DashboardFilter
+ @staticmethod
+ def get_charts_for_dashboard(dashboard_id: int) -> List[Slice]:
+ query = db.session.query(Dashboard).filter(Dashboard.id == dashboard_id)
+ dashboard = query.scalar()
+ if not dashboard:
+ raise DashboardNotFoundError()
+ return [slice.data for slice in dashboard.slices]
Review comment:
For now it's just a personal preference. I don't want to block this PR. Probably for now it's better to just solve the N+1 problem and keep using slice.data.
----------------------------------------------------------------
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.
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-io edited a comment on pull request #12978: feat(dashboard): API to get a dashboard's charts
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #12978:
URL: https://github.com/apache/superset/pull/12978#issuecomment-774349038
# [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=h1) Report
> Merging [#12978](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=desc) (6de6192) into [master](https://codecov.io/gh/apache/superset/commit/89b6a2cfcb087979c420534b24ceaa1ed5d08374?el=desc) (89b6a2c) will **decrease** coverage by `13.77%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #12978 +/- ##
===========================================
- Coverage 66.75% 52.97% -13.78%
===========================================
Files 1015 480 -535
Lines 49634 17315 -32319
Branches 4839 4485 -354
===========================================
- Hits 33134 9173 -23961
+ Misses 16377 8142 -8235
+ Partials 123 0 -123
```
| Flag | Coverage Δ | |
|---|---|---|
| cypress | `52.97% <ø> (+1.99%)` | :arrow_up: |
| javascript | `?` | |
| python | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...uperset-frontend/src/dashboard/util/dnd-reorder.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2RuZC1yZW9yZGVyLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...rset-frontend/src/dashboard/util/getEmptyLayout.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEVtcHR5TGF5b3V0Lmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...dashboard/components/resizable/ResizableHandle.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL3Jlc2l6YWJsZS9SZXNpemFibGVIYW5kbGUuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../src/dashboard/util/getFilterScopeFromNodesTree.js](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC91dGlsL2dldEZpbHRlclNjb3BlRnJvbU5vZGVzVHJlZS5qcw==) | `0.00% <0.00%> (-93.48%)` | :arrow_down: |
| [...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvRXhlY3V0aW9uTG9nLnRzeA==) | `11.76% <0.00%> (-88.24%)` | :arrow_down: |
| [...src/dashboard/components/gridComponents/Header.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0hlYWRlci5qc3g=) | `10.52% <0.00%> (-86.85%)` | :arrow_down: |
| [superset-frontend/src/components/IconTooltip.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvSWNvblRvb2x0aXAudHN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...rc/dashboard/components/gridComponents/Divider.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL0RpdmlkZXIuanN4) | `13.33% <0.00%> (-86.67%)` | :arrow_down: |
| [...end/src/SqlLab/components/ExploreResultsButton.jsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0V4cGxvcmVSZXN1bHRzQnV0dG9uLmpzeA==) | `8.00% <0.00%> (-84.00%)` | :arrow_down: |
| [...nd/src/views/CRUD/data/query/QueryPreviewModal.tsx](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9xdWVyeS9RdWVyeVByZXZpZXdNb2RhbC50c3g=) | `14.70% <0.00%> (-82.97%)` | :arrow_down: |
| ... and [909 more](https://codecov.io/gh/apache/superset/pull/12978/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=footer). Last update [89b6a2c...f152f56](https://codecov.io/gh/apache/superset/pull/12978?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
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