From 4f4367edf37cbd70fdeac276d066ae5c0bf95ed0 Mon Sep 17 00:00:00 2001 From: Grace Guo Date: Mon, 12 Oct 2020 17:58:32 -0700 Subject: [PATCH] feat: prevent co-edit dashboard collision (#11220) * feat: prevent co-edit dashboard collision * fix comments --- .../src/dashboard/components/Header.jsx | 1 + .../src/dashboard/components/SaveModal.jsx | 1 + .../src/dashboard/reducers/getInitialState.js | 1 + superset/models/dashboard.py | 1 + superset/views/core.py | 20 +++++++++++++++++++ tests/dashboard_tests.py | 15 ++++++++++++++ 6 files changed, 39 insertions(+) diff --git a/superset-frontend/src/dashboard/components/Header.jsx b/superset-frontend/src/dashboard/components/Header.jsx index c986a314a..2dba65551 100644 --- a/superset-frontend/src/dashboard/components/Header.jsx +++ b/superset-frontend/src/dashboard/components/Header.jsx @@ -290,6 +290,7 @@ class Header extends React.PureComponent { label_colors: labelColors, dashboard_title: dashboardTitle, refresh_frequency: refreshFrequency, + last_modified_time: dashboardInfo.lastModifiedTime, }; // make sure positions data less than DB storage limitation: diff --git a/superset-frontend/src/dashboard/components/SaveModal.jsx b/superset-frontend/src/dashboard/components/SaveModal.jsx index 3e69d5383..03dafb5cb 100644 --- a/superset-frontend/src/dashboard/components/SaveModal.jsx +++ b/superset-frontend/src/dashboard/components/SaveModal.jsx @@ -129,6 +129,7 @@ class SaveModal extends React.PureComponent { saveType === SAVE_TYPE_NEWDASHBOARD ? newDashName : dashboardTitle, duplicate_slices: this.state.duplicateSlices, refresh_frequency: refreshFrequency, + last_modified_time: dashboardInfo.lastModifiedTime, }; if (saveType === SAVE_TYPE_NEWDASHBOARD && !newDashName) { diff --git a/superset-frontend/src/dashboard/reducers/getInitialState.js b/superset-frontend/src/dashboard/reducers/getInitialState.js index d9ddc59cd..3e5b6ea75 100644 --- a/superset-frontend/src/dashboard/reducers/getInitialState.js +++ b/superset-frontend/src/dashboard/reducers/getInitialState.js @@ -266,6 +266,7 @@ export default function getInitialState(bootstrapData) { id: dashboard.id, slug: dashboard.slug, metadata: dashboard.metadata, + lastModifiedTime: dashboard.last_modified_time, userId: user_id, dash_edit_perm: dashboard.dash_edit_perm, dash_save_perm: dashboard.dash_save_perm, diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index 86484791f..56ca1ba6a 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -237,6 +237,7 @@ class Dashboard( # pylint: disable=too-many-instance-attributes "slug": self.slug, "slices": [slc.data for slc in self.slices], "position_json": positions, + "last_modified_time": self.changed_on.replace(microsecond=0).timestamp(), } @property # type: ignore diff --git a/superset/views/core.py b/superset/views/core.py index fd62a0e01..c6847b713 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -1019,6 +1019,11 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods """Copy dashboard""" session = db.session() data = json.loads(request.form["data"]) + # client-side send back last_modified_time which was set when + # the dashboard was open. it was use to avoid mid-air collision. + # remove it to avoid confusion. + data.pop("last_modified_time", None) + dash = models.Dashboard() original_dash = session.query(Dashboard).get(dashboard_id) @@ -1065,6 +1070,21 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods dash = session.query(Dashboard).get(dashboard_id) check_ownership(dash, raise_if_false=True) data = json.loads(request.form["data"]) + # client-side send back last_modified_time which was set when + # the dashboard was open. it was use to avoid mid-air collision. + remote_last_modified_time = data.get("last_modified_time") + current_last_modified_time = dash.changed_on.replace(microsecond=0).timestamp() + if remote_last_modified_time < current_last_modified_time: + return json_error_response( + __( + "This dashboard was changed recently. " + "Please reload dashboard to get latest version." + ), + 412, + ) + # remove to avoid confusion. + data.pop("last_modified_time", None) + DashboardDAO.set_dash_metadata(dash, data) session.merge(dash) session.commit() diff --git a/tests/dashboard_tests.py b/tests/dashboard_tests.py index 27926e4f6..3888cf359 100644 --- a/tests/dashboard_tests.py +++ b/tests/dashboard_tests.py @@ -16,6 +16,7 @@ # under the License. # isort:skip_file """Unit tests for Superset""" +from datetime import datetime import json import unittest from random import random @@ -89,6 +90,8 @@ class TestDashboard(SupersetTestCase): "expanded_slices": {}, "positions": positions, "dashboard_title": dash.dashboard_title, + # set a further modified_time for unit test + "last_modified_time": datetime.now().timestamp() + 1000, } url = "/superset/save_dash/{}/".format(dash.id) resp = self.get_resp(url, data=dict(data=json.dumps(data))) @@ -107,6 +110,8 @@ class TestDashboard(SupersetTestCase): "positions": positions, "dashboard_title": dash.dashboard_title, "default_filters": default_filters, + # set a further modified_time for unit test + "last_modified_time": datetime.now().timestamp() + 1000, } url = "/superset/save_dash/{}/".format(dash.id) @@ -134,6 +139,8 @@ class TestDashboard(SupersetTestCase): "positions": positions, "dashboard_title": dash.dashboard_title, "default_filters": default_filters, + # set a further modified_time for unit test + "last_modified_time": datetime.now().timestamp() + 1000, } url = "/superset/save_dash/{}/".format(dash.id) @@ -154,6 +161,8 @@ class TestDashboard(SupersetTestCase): "expanded_slices": {}, "positions": positions, "dashboard_title": "new title", + # set a further modified_time for unit test + "last_modified_time": datetime.now().timestamp() + 1000, } url = "/superset/save_dash/{}/".format(dash.id) self.get_resp(url, data=dict(data=json.dumps(data))) @@ -176,6 +185,8 @@ class TestDashboard(SupersetTestCase): "color_namespace": "Color Namespace Test", "color_scheme": "Color Scheme Test", "label_colors": new_label_colors, + # set a further modified_time for unit test + "last_modified_time": datetime.now().timestamp() + 1000, } url = "/superset/save_dash/{}/".format(dash.id) self.get_resp(url, data=dict(data=json.dumps(data))) @@ -203,6 +214,8 @@ class TestDashboard(SupersetTestCase): "color_namespace": "Color Namespace Test", "color_scheme": "Color Scheme Test", "label_colors": new_label_colors, + # set a further modified_time for unit test + "last_modified_time": datetime.now().timestamp() + 1000, } # Save changes to Births dashboard and retrieve updated dash @@ -271,6 +284,8 @@ class TestDashboard(SupersetTestCase): "expanded_slices": {}, "positions": positions, "dashboard_title": dash.dashboard_title, + # set a further modified_time for unit test + "last_modified_time": datetime.now().timestamp() + 1000, } # save dash