-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Migrate copy_dash endpoint to api v1 #23112
chore: Migrate copy_dash endpoint to api v1 #23112
Conversation
Codecov Report
@@ Coverage Diff @@
## master #23112 +/- ##
==========================================
+ Coverage 67.71% 67.74% +0.02%
==========================================
Files 1916 1916
Lines 74014 74100 +86
Branches 8039 8039
==========================================
+ Hits 50122 50202 +80
- Misses 21843 21850 +7
+ Partials 2049 2048 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -1225,3 +1228,68 @@ def delete_embedded(self, dashboard: Dashboard) -> Response: | |||
for embedded in dashboard.embedded: | |||
DashboardDAO.delete(embedded) | |||
return self.response(200, message="OK") | |||
|
|||
@expose("/<id_or_slug>/copy/", methods=["POST"]) | |||
@protect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make this endpoint permission be can write on Dashboard
add copy_dash to https://github.com/apache/superset/blob/master/superset/constants.py#L151
@@ -299,3 +293,38 @@ def favorited_ids(dashboards: List[Dashboard]) -> List[FavStar]: | |||
) | |||
.all() | |||
] | |||
|
|||
@classmethod | |||
def copy_dashboard( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: how about just def copy(
we know it's a dashboard from the class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to be explicit here
…/migrate-copy-dash-to-api-v1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but let's do what @dpgaspar recommended and add the method to constants.py
so we don't add a new unneeded permission.
I added it just now, but I'm curious if the Edit: The test that failed after I made the change seems to confirm that is the case |
SUMMARY
This PR deprecates
superset/copy_dash/<dash_id>/
and introducesapi/v1/dashboard/<dash_id>/copy/
to replace it.The new endpoint behaves nearly identically to the old one; the only functional differences are that the endpoint accepts a json body instead of a form, and it only responds with the data used by the frontend.
This PR also marks the
superset/save_dash/<dash_id>/
endpoint as deprecated as it is not called from client code anywhere.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
[x] Add tests
[x] Test copying a dashboard from the frontend with "Save as"
ADDITIONAL INFORMATION