show edit modal on dashboards list view#9211
Conversation
nytai
left a comment
nytai
left a comment
There was a problem hiding this comment.
a couple questions, none are blocking
| ] | ||
| order_columns = ["dashboard_title", "changed_on", "published", "changed_by_fk"] | ||
| list_columns = [ | ||
| "json_metadata", |
There was a problem hiding this comment.
Is this property consumed in the listview? Seems like we're fetching the full dashboard show payload on edit.
There was a problem hiding this comment.
That's a good point, we could use the metadata from the modal's fetch call.
There was a problem hiding this comment.
I ended up simplifying the contract for PropertiesModal, now it just takes a dashboard id and fetches everything it needs on its own. Not ideal, it'd be nicer to pass a dashboard object as a prop, but we don't have a super consistent data model for a dashboard in every place that it's used, so it's probably the best option for now.
| handleDashboardEdit = (edits: any, original: Dashboard) => { | ||
| this.setState({ loading: true }); | ||
| return SupersetClient.get({ | ||
| endpoint: `/api/v1/dashboard/${original.id}`, |
There was a problem hiding this comment.
Is there a reason we're making this call instead of just updating from edits?
There was a problem hiding this comment.
I think there was some field that wasn't returned from the PUT endpoint. I wasn't sure how to add that field and eventually just did this to make it work. If you think it's worth the effort I'll take another pass at it, otherwise I can add a comment documenting why it's there.
There was a problem hiding this comment.
happy to merge this, this shouldn't affect perf much. However, we should figure out how to configure the PUT response
There was a problem hiding this comment.
I remember this now. The API endpoint uses the edit_model_schema Marshmallow schema when returning the edited object, which is the same as the one used to parse incoming edited fields from the client. IMO a better design would be to return the entire object as it would from the GET endpoint, but that's a larger scale API design decision.
There was a problem hiding this comment.
In this particular case, the impact is that slug can be changed, which changes the url, but only slug is returned from the endpoint.
Codecov Report
@@ Coverage Diff @@
## master #9211 +/- ##
==========================================
+ Coverage 58.92% 59.14% +0.21%
==========================================
Files 372 373 +1
Lines 11999 12057 +58
Branches 2940 2961 +21
==========================================
+ Hits 7071 7131 +60
+ Misses 4750 4747 -3
- Partials 178 179 +1
Continue to review full report at Codecov.
|
mistercrunch
added
🏷️ bot
* show edit modal on dashboards list view * lint * fix test * simplify PropertiesModal interface * lint * comply with method ordering * fix type issue
5 participants
CATEGORY
Choose one
SUMMARY
This changes the dashboard react list view to use the react-based edit modal, instead of linking to the CRUD-based edit page.
A detail of this implementation is that after editing a dashboard, it will stay in the list even if it is no longer applicable based on the chosen filters. This is intended, as removing a just-edited dashboard from the list could be annoying.
I had to remove the "owners" accessor column as it doesn't actually work correctly when a dashboard has
ownerspopulated. React complains about trying to render an object. Owners will need a component rather than just an accessor. This bug was invisible before, since the list endpoint for dashboards doesn't actually return owners at all.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After clicking the edit button
After saving the dashboard
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS
@nytai