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 2020/01/15 18:12:30 UTC
[GitHub] [incubator-superset] dpgaspar opened a new pull request #8972:
[dashboards] New, API for Bulk delete
dpgaspar opened a new pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972
### CATEGORY
Choose one
- [ ] Bug Fix
- [X] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Dashboards bulk delete
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [X] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] robdiciuccio commented on a change in pull
request #8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
robdiciuccio commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r368191125
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
Review comment:
+1 on checking ownership before deletion. Currently, if the user attempts to delete a mix of dashboards that do and do not own, they will be left in an unclear state. If the user requests to delete something they don't have permission to delete, the request should fail without any dashboards being modified/deleted, 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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] robdiciuccio commented on a change in pull
request #8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
robdiciuccio commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r368730639
##########
File path: superset/views/dashboard/api.py
##########
@@ -346,71 +346,68 @@ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
type: integer
responses:
200:
- description: Dashboard multiple delete
+ description: Dashboard bulk delete
content:
application/json:
schema:
type: object
properties:
message:
type: string
- count:
- description: Number of deleted dashboards
- type: integer
401:
$ref: '#/components/responses/401'
403:
- description: Dashboard multiple delete
- content:
- application/json:
- schema:
- type: object
- properties:
- message:
- type: string
- count:
- description: Number of deleted dashboards
- type: integer
+ $ref: '#/components/responses/401'
404:
$ref: '#/components/responses/404'
422:
$ref: '#/components/responses/422'
500:
$ref: '#/components/responses/500'
"""
+ item_ids = kwargs["rison"]
query = self.datamodel.session.query(Dashboard).filter(
- Dashboard.id.in_(kwargs["rison"])
+ Dashboard.id.in_(item_ids)
)
items = self._base_filters.apply_all(query).all()
if not items:
return self.response_404()
- delete_count = 0
- status_code = 200
for item in items:
try:
check_ownership(item)
- self.datamodel.delete(item, raise_exception=True)
- delete_count += 1
except SupersetSecurityException as e:
logger.warning(
f"Dashboard {item} was not deleted, "
f"because the user ({g.user}) does not own it"
)
- status_code = 403
+ return self.response(403, message=_("No dashboards deleted"))
except SQLAlchemyError as e:
return self.response_422(message=str(e))
- if delete_count == 0:
- return self.response(
- status_code, message=_("No dashboards deleted"), count=delete_count
- )
+ # bulk delete, first delete related data
+ for item in items:
+ try:
+ item.slices = []
+ item.owners = []
+ self.datamodel.session.merge(item)
+ except SQLAlchemyError as e:
Review comment:
Log this error?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8972:
[dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#issuecomment-575249609
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=h1) Report
> Merging [#8972](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/575c4d47a1dcee730e47668e8c8402edf251b5c4?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `67.62%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8972 +/- ##
==========================================
+ Coverage 59.15% 59.16% +0.01%
==========================================
Files 367 367
Lines 11681 11679 -2
Branches 2863 2862 -1
==========================================
Hits 6910 6910
+ Misses 4592 4590 -2
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/assets/src/welcome/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy93ZWxjb21lL0FwcC5qc3g=) | `0% <ø> (ø)` | :arrow_up: |
| [superset/assets/src/dashboard/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvaW5kZXguanM=) | `100% <ø> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `55.69% <ø> (ø)` | :arrow_up: |
| [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
| [...t/assets/src/dashboard/reducers/dashboardLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvZGFzaGJvYXJkTGF5b3V0Lmpz) | `93% <0%> (ø)` | :arrow_up: |
| [.../src/explore/components/AdhocMetricEditPopover.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3Zlci5qc3g=) | `62.35% <100%> (ø)` | :arrow_up: |
| [...explore/components/AdhocMetricEditPopoverTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3ZlclRpdGxlLmpzeA==) | `70% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/constants.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29uc3RhbnRzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/utils/sqlKeywords.ts](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvdXRpbHMvc3FsS2V5d29yZHMudHM=) | `100% <100%> (ø)` | :arrow_up: |
| [...components/AdhocFilterEditPopoverSqlTabContent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NGaWx0ZXJFZGl0UG9wb3ZlclNxbFRhYkNvbnRlbnQuanN4) | `64.86% <100%> (ø)` | :arrow_up: |
| ... and [25 more](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?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/incubator-superset/pull/8972?src=pr&el=footer). Last update [575c4d4...6211451](https://codecov.io/gh/apache/incubator-superset/pull/8972?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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r368220752
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
Review comment:
ok, makes sense
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on a change in pull request
#8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r368193387
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
Review comment:
+1 on all or nothing
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8972:
[dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#issuecomment-575249609
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=h1) Report
> Merging [#8972](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/575c4d47a1dcee730e47668e8c8402edf251b5c4?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `67.62%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8972 +/- ##
=======================================
Coverage 59.15% 59.15%
=======================================
Files 367 367
Lines 11681 11681
Branches 2863 2863
=======================================
Hits 6910 6910
Misses 4592 4592
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/assets/src/welcome/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy93ZWxjb21lL0FwcC5qc3g=) | `0% <ø> (ø)` | :arrow_up: |
| [superset/assets/src/dashboard/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvaW5kZXguanM=) | `100% <ø> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `55.69% <ø> (ø)` | :arrow_up: |
| [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
| [...t/assets/src/dashboard/reducers/dashboardLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvZGFzaGJvYXJkTGF5b3V0Lmpz) | `93% <0%> (ø)` | :arrow_up: |
| [.../src/explore/components/AdhocMetricEditPopover.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3Zlci5qc3g=) | `62.35% <100%> (ø)` | :arrow_up: |
| [...explore/components/AdhocMetricEditPopoverTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3ZlclRpdGxlLmpzeA==) | `70% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/constants.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29uc3RhbnRzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/utils/sqlKeywords.ts](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvdXRpbHMvc3FsS2V5d29yZHMudHM=) | `100% <100%> (ø)` | :arrow_up: |
| [...components/AdhocFilterEditPopoverSqlTabContent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NGaWx0ZXJFZGl0UG9wb3ZlclNxbFRhYkNvbnRlbnQuanN4) | `64.86% <100%> (ø)` | :arrow_up: |
| ... and [24 more](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?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/incubator-superset/pull/8972?src=pr&el=footer). Last update [575c4d4...0c09b4e](https://codecov.io/gh/apache/incubator-superset/pull/8972?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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r368075901
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
Review comment:
Irrespective of performance, I'd still lean on collecting/checking then deleting as opposed to deleting while things are checked. This allows us to perform the delete operation in a single unit of work, as opposed to breaking things up in which case the delete operation will leave things hanging.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r367890642
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
Review comment:
My goal is to make this method generic (charts also have ownership), when using bulk delete on models with relationships we have to explicitly cleanup, on this case `owners` and `slices`. My take is that the performance gain on this kind of operation may not compensate the necessary specific code and complexity increase. Thoughts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8972:
[dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#issuecomment-575249609
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=h1) Report
> Merging [#8972](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/575c4d47a1dcee730e47668e8c8402edf251b5c4?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `67.62%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8972 +/- ##
==========================================
+ Coverage 59.15% 59.16% +0.01%
==========================================
Files 367 367
Lines 11681 11679 -2
Branches 2863 2862 -1
==========================================
Hits 6910 6910
+ Misses 4592 4590 -2
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/assets/src/welcome/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy93ZWxjb21lL0FwcC5qc3g=) | `0% <ø> (ø)` | :arrow_up: |
| [superset/assets/src/dashboard/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvaW5kZXguanM=) | `100% <ø> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `55.69% <ø> (ø)` | :arrow_up: |
| [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
| [...t/assets/src/dashboard/reducers/dashboardLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvZGFzaGJvYXJkTGF5b3V0Lmpz) | `93% <0%> (ø)` | :arrow_up: |
| [.../src/explore/components/AdhocMetricEditPopover.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3Zlci5qc3g=) | `62.35% <100%> (ø)` | :arrow_up: |
| [...explore/components/AdhocMetricEditPopoverTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3ZlclRpdGxlLmpzeA==) | `70% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/constants.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29uc3RhbnRzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/utils/sqlKeywords.ts](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvdXRpbHMvc3FsS2V5d29yZHMudHM=) | `100% <100%> (ø)` | :arrow_up: |
| [...components/AdhocFilterEditPopoverSqlTabContent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NGaWx0ZXJFZGl0UG9wb3ZlclNxbFRhYkNvbnRlbnQuanN4) | `64.86% <100%> (ø)` | :arrow_up: |
| ... and [25 more](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?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/incubator-superset/pull/8972?src=pr&el=footer). Last update [575c4d4...f830ee6](https://codecov.io/gh/apache/incubator-superset/pull/8972?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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] robdiciuccio commented on a change in pull
request #8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
robdiciuccio commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r368730677
##########
File path: superset/views/dashboard/api.py
##########
@@ -346,71 +346,68 @@ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
type: integer
responses:
200:
- description: Dashboard multiple delete
+ description: Dashboard bulk delete
content:
application/json:
schema:
type: object
properties:
message:
type: string
- count:
- description: Number of deleted dashboards
- type: integer
401:
$ref: '#/components/responses/401'
403:
- description: Dashboard multiple delete
- content:
- application/json:
- schema:
- type: object
- properties:
- message:
- type: string
- count:
- description: Number of deleted dashboards
- type: integer
+ $ref: '#/components/responses/401'
404:
$ref: '#/components/responses/404'
422:
$ref: '#/components/responses/422'
500:
$ref: '#/components/responses/500'
"""
+ item_ids = kwargs["rison"]
query = self.datamodel.session.query(Dashboard).filter(
- Dashboard.id.in_(kwargs["rison"])
+ Dashboard.id.in_(item_ids)
)
items = self._base_filters.apply_all(query).all()
if not items:
return self.response_404()
- delete_count = 0
- status_code = 200
for item in items:
try:
check_ownership(item)
- self.datamodel.delete(item, raise_exception=True)
- delete_count += 1
except SupersetSecurityException as e:
logger.warning(
f"Dashboard {item} was not deleted, "
f"because the user ({g.user}) does not own it"
)
- status_code = 403
+ return self.response(403, message=_("No dashboards deleted"))
except SQLAlchemyError as e:
return self.response_422(message=str(e))
- if delete_count == 0:
- return self.response(
- status_code, message=_("No dashboards deleted"), count=delete_count
- )
+ # bulk delete, first delete related data
+ for item in items:
+ try:
+ item.slices = []
+ item.owners = []
+ self.datamodel.session.merge(item)
+ except SQLAlchemyError as e:
+ self.datamodel.session.rollback()
+ return self.response_422(message=str(e))
+ # bulk delete itself
+ try:
+ self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(item_ids)
+ ).delete(synchronize_session="fetch")
+ except SQLAlchemyError as e:
Review comment:
Log this error?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8972:
[dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#issuecomment-575249609
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=h1) Report
> Merging [#8972](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/575c4d47a1dcee730e47668e8c8402edf251b5c4?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `67.62%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8972 +/- ##
=======================================
Coverage 59.15% 59.15%
=======================================
Files 367 367
Lines 11681 11681
Branches 2863 2863
=======================================
Hits 6910 6910
Misses 4592 4592
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/assets/src/welcome/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy93ZWxjb21lL0FwcC5qc3g=) | `0% <ø> (ø)` | :arrow_up: |
| [superset/assets/src/dashboard/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvaW5kZXguanM=) | `100% <ø> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `55.69% <ø> (ø)` | :arrow_up: |
| [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
| [...t/assets/src/dashboard/reducers/dashboardLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvZGFzaGJvYXJkTGF5b3V0Lmpz) | `93% <0%> (ø)` | :arrow_up: |
| [.../src/explore/components/AdhocMetricEditPopover.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3Zlci5qc3g=) | `62.35% <100%> (ø)` | :arrow_up: |
| [...explore/components/AdhocMetricEditPopoverTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3ZlclRpdGxlLmpzeA==) | `70% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/constants.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29uc3RhbnRzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/utils/sqlKeywords.ts](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvdXRpbHMvc3FsS2V5d29yZHMudHM=) | `100% <100%> (ø)` | :arrow_up: |
| [...components/AdhocFilterEditPopoverSqlTabContent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NGaWx0ZXJFZGl0UG9wb3ZlclNxbFRhYkNvbnRlbnQuanN4) | `64.86% <100%> (ø)` | :arrow_up: |
| ... and [24 more](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?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/incubator-superset/pull/8972?src=pr&el=footer). Last update [575c4d4...000d747](https://codecov.io/gh/apache/incubator-superset/pull/8972?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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8972:
[dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#issuecomment-575249609
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=h1) Report
> Merging [#8972](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/6211451baf533d3fa927209e87fc96d4669640a9?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8972 +/- ##
=======================================
Coverage 59.16% 59.16%
=======================================
Files 367 367
Lines 11679 11679
Branches 2862 2862
=======================================
Hits 6910 6910
Misses 4590 4590
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../explore/components/controls/DateFilterControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvY29udHJvbHMvRGF0ZUZpbHRlckNvbnRyb2wuanN4) | `38.78% <100%> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?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/incubator-superset/pull/8972?src=pr&el=footer). Last update [6211451...c8175b9](https://codecov.io/gh/apache/incubator-superset/pull/8972?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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8972:
[dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#issuecomment-575249609
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=h1) Report
> Merging [#8972](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/575c4d47a1dcee730e47668e8c8402edf251b5c4?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `67.62%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8972 +/- ##
=======================================
Coverage 59.15% 59.15%
=======================================
Files 367 367
Lines 11681 11681
Branches 2863 2863
=======================================
Hits 6910 6910
Misses 4592 4592
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/assets/src/welcome/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy93ZWxjb21lL0FwcC5qc3g=) | `0% <ø> (ø)` | :arrow_up: |
| [superset/assets/src/dashboard/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvaW5kZXguanM=) | `100% <ø> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `55.69% <ø> (ø)` | :arrow_up: |
| [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
| [...t/assets/src/dashboard/reducers/dashboardLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvZGFzaGJvYXJkTGF5b3V0Lmpz) | `93% <0%> (ø)` | :arrow_up: |
| [.../src/explore/components/AdhocMetricEditPopover.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3Zlci5qc3g=) | `62.35% <100%> (ø)` | :arrow_up: |
| [...explore/components/AdhocMetricEditPopoverTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3ZlclRpdGxlLmpzeA==) | `70% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/constants.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29uc3RhbnRzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/utils/sqlKeywords.ts](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvdXRpbHMvc3FsS2V5d29yZHMudHM=) | `100% <100%> (ø)` | :arrow_up: |
| [...components/AdhocFilterEditPopoverSqlTabContent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NGaWx0ZXJFZGl0UG9wb3ZlclNxbFRhYkNvbnRlbnQuanN4) | `64.86% <100%> (ø)` | :arrow_up: |
| ... and [24 more](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?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/incubator-superset/pull/8972?src=pr&el=footer). Last update [575c4d4...000d747](https://codecov.io/gh/apache/incubator-superset/pull/8972?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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #8972:
[dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#issuecomment-575249609
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=h1) Report
> Merging [#8972](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/575c4d47a1dcee730e47668e8c8402edf251b5c4?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `67.62%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8972 +/- ##
=======================================
Coverage 59.15% 59.15%
=======================================
Files 367 367
Lines 11681 11681
Branches 2863 2863
=======================================
Hits 6910 6910
Misses 4592 4592
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/8972?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/assets/src/welcome/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy93ZWxjb21lL0FwcC5qc3g=) | `0% <ø> (ø)` | :arrow_up: |
| [superset/assets/src/dashboard/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvaW5kZXguanM=) | `100% <ø> (ø)` | :arrow_up: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `55.69% <ø> (ø)` | :arrow_up: |
| [...ssets/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29udGFpbmVycy9EYXNoYm9hcmRIZWFkZXIuanN4) | `100% <ø> (ø)` | :arrow_up: |
| [...t/assets/src/dashboard/reducers/dashboardLayout.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvcmVkdWNlcnMvZGFzaGJvYXJkTGF5b3V0Lmpz) | `93% <0%> (ø)` | :arrow_up: |
| [.../src/explore/components/AdhocMetricEditPopover.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3Zlci5qc3g=) | `62.35% <100%> (ø)` | :arrow_up: |
| [...explore/components/AdhocMetricEditPopoverTitle.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NNZXRyaWNFZGl0UG9wb3ZlclRpdGxlLmpzeA==) | `70% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/constants.js](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29uc3RhbnRzLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [superset/assets/src/SqlLab/utils/sqlKeywords.ts](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvdXRpbHMvc3FsS2V5d29yZHMudHM=) | `100% <100%> (ø)` | :arrow_up: |
| [...components/AdhocFilterEditPopoverSqlTabContent.jsx](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvQWRob2NGaWx0ZXJFZGl0UG9wb3ZlclNxbFRhYkNvbnRlbnQuanN4) | `64.86% <100%> (ø)` | :arrow_up: |
| ... and [24 more](https://codecov.io/gh/apache/incubator-superset/pull/8972/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8972?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/incubator-superset/pull/8972?src=pr&el=footer). Last update [575c4d4...5d31ac6](https://codecov.io/gh/apache/incubator-superset/pull/8972?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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r367595401
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
Review comment:
Instead of deleting as you go along and checking for ownership, it would probably be better to collect the items that are "to be deleted", checking ownership first. After everything checks out, then iterate over the list, or map the list to a collection of ids and do a bulk `delete where id in()`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r368497478
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
Review comment:
Updated, it's all or nothing now
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch merged pull request #8972:
[dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
mistercrunch merged pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r367709402
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
+ delete_count += 1
+ except SupersetSecurityException as e:
+ logger.warning(
+ f"Dashboard {item} was not deleted, "
+ f"because the user ({g.user}) does not own it"
+ )
+ status_code = 403
+ except SQLAlchemyError as e:
+ return self.response_422(message=str(e))
+ if len(items) != delete_count and delete_count > 0:
+ return self.response(
+ status_code, message="Some dashboards deleted", count=delete_count
Review comment:
Also i18n
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8972: [dashboards] New, API for Bulk delete
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8972: [dashboards] New, API for Bulk delete
URL: https://github.com/apache/incubator-superset/pull/8972#discussion_r367595987
##########
File path: superset/views/dashboard/api.py
##########
@@ -320,6 +326,88 @@ def post(self):
except SQLAlchemyError as e:
return self.response_422(message=str(e))
+ @expose("/", methods=["DELETE"])
+ @protect()
+ @safe
+ @rison(get_delete_ids_schema)
+ def multiple_delete(self, **kwargs): # pylint: disable=arguments-differ
+ """Delete multiple Dashboards
+ ---
+ delete:
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ type: array
+ items:
+ type: integer
+ responses:
+ 200:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ description: Dashboard multiple delete
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
+ count:
+ description: Number of deleted dashboards
+ type: integer
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ query = self.datamodel.session.query(Dashboard).filter(
+ Dashboard.id.in_(kwargs["rison"])
+ )
+ items = self._base_filters.apply_all(query).all()
+ if not items:
+ return self.response_404()
+ delete_count = 0
+ status_code = 200
+ for item in items:
+ try:
+ check_ownership(item)
+ self.datamodel.delete(item, raise_exception=True)
+ delete_count += 1
+ except SupersetSecurityException as e:
+ logger.warning(
+ f"Dashboard {item} was not deleted, "
+ f"because the user ({g.user}) does not own it"
+ )
+ status_code = 403
+ except SQLAlchemyError as e:
+ return self.response_422(message=str(e))
+ if len(items) != delete_count and delete_count > 0:
+ return self.response(
+ status_code, message="Some dashboards deleted", count=delete_count
Review comment:
This seems weird. As a user, a message like this might scare me a bit :). If you bulk delete instead, you can perform the operation in the context of a single transaction, thereby making this operation atomic.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org