You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2021/08/16 05:48:58 UTC
[superset] 08/34: fix: ensure that users viewing chart does not
automatically save edit data (#16077)
This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch 1.3
in repository https://gitbox.apache.org/repos/asf/superset.git
commit cf922e1dcf684aebc18e6012d418f853ceba8460
Author: Phillip Kelley-Dotson <pk...@yahoo.com>
AuthorDate: Tue Aug 10 09:29:49 2021 -0700
fix: ensure that users viewing chart does not automatically save edit data (#16077)
* add last_change_at migration
* add last_saved_by db migration
* finish rest of api migration
* run precommit
* fix name
* run precommitt
* remove unused mods
* merge migrations
* Update superset/migrations/versions/6d20ba9ecb33_add_last_saved_at_to_slice_model.py
Co-authored-by: Beto Dealmeida <ro...@dealmeida.net>
* Update superset/migrations/versions/6d20ba9ecb33_add_last_saved_at_to_slice_model.py
Co-authored-by: Beto Dealmeida <ro...@dealmeida.net>
* Update superset/migrations/versions/f6196627326f_update_chart_permissions.py
Co-authored-by: Beto Dealmeida <ro...@dealmeida.net>
* fix test
* precommit
* remove print
* fix test
* change test
* test commit
* test 2
* test 3
* third time the charm
* fix put req
Co-authored-by: Beto Dealmeida <ro...@dealmeida.net>
(cherry picked from commit f0e3b68cc2902483adf06e7a1dd6267745c44f64)
---
.../src/explore/components/ExploreChartPanel.jsx | 1 +
.../src/views/CRUD/chart/ChartList.tsx | 23 ++++++--
superset/charts/api.py | 12 ++++
superset/charts/commands/create.py | 3 +
superset/charts/commands/update.py | 4 ++
superset/charts/schemas.py | 11 ++++
...d20ba9ecb33_add_last_saved_at_to_slice_model.py | 66 ++++++++++++++++++++++
superset/models/slice.py | 9 ++-
superset/views/core.py | 4 +-
tests/integration_tests/charts/commands_tests.py | 26 +++++++++
10 files changed, 150 insertions(+), 9 deletions(-)
diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.jsx
index a8d07ed..bd2213f 100644
--- a/superset-frontend/src/explore/components/ExploreChartPanel.jsx
+++ b/superset-frontend/src/explore/components/ExploreChartPanel.jsx
@@ -147,6 +147,7 @@ const ExploreChartPanel = props => {
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
query_context: JSON.stringify(queryContext),
+ query_context_generation: true,
}),
});
}
diff --git a/superset-frontend/src/views/CRUD/chart/ChartList.tsx b/superset-frontend/src/views/CRUD/chart/ChartList.tsx
index 61b262d..d7b5bf1 100644
--- a/superset-frontend/src/views/CRUD/chart/ChartList.tsx
+++ b/superset-frontend/src/views/CRUD/chart/ChartList.tsx
@@ -25,6 +25,7 @@ import {
import React, { useMemo, useState } from 'react';
import rison from 'rison';
import { uniqBy } from 'lodash';
+import moment from 'moment';
import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
import {
createErrorHandler,
@@ -270,23 +271,33 @@ function ChartList(props: ChartListProps) {
Cell: ({
row: {
original: {
- changed_by_name: changedByName,
+ last_saved_by: lastSavedBy,
changed_by_url: changedByUrl,
},
},
- }: any) => <a href={changedByUrl}>{changedByName}</a>,
+ }: any) => (
+ <a href={changedByUrl}>
+ {lastSavedBy?.first_name
+ ? `${lastSavedBy?.first_name} ${lastSavedBy?.last_name}`
+ : null}
+ </a>
+ ),
Header: t('Modified by'),
- accessor: 'changed_by.first_name',
+ accessor: 'last_saved_by',
size: 'xl',
},
{
Cell: ({
row: {
- original: { changed_on_delta_humanized: changedOn },
+ original: { last_saved_at: lastSavedAt },
},
- }: any) => <span className="no-wrap">{changedOn}</span>,
+ }: any) => (
+ <span className="no-wrap">
+ {lastSavedAt ? moment.utc(lastSavedAt).fromNow() : null}
+ </span>
+ ),
Header: t('Last modified'),
- accessor: 'changed_on_delta_humanized',
+ accessor: 'last_saved_at',
size: 'xl',
},
{
diff --git a/superset/charts/api.py b/superset/charts/api.py
index 8de2e42..cb815dd 100644
--- a/superset/charts/api.py
+++ b/superset/charts/api.py
@@ -152,6 +152,10 @@ class ChartRestApi(BaseSupersetModelRestApi):
"description_markeddown",
"edit_url",
"id",
+ "last_saved_at",
+ "last_saved_by.id",
+ "last_saved_by.first_name",
+ "last_saved_by.last_name",
"owners.first_name",
"owners.id",
"owners.last_name",
@@ -170,12 +174,20 @@ class ChartRestApi(BaseSupersetModelRestApi):
"changed_on_delta_humanized",
"datasource_id",
"datasource_name",
+ "last_saved_at",
+ "last_saved_by.id",
+ "last_saved_by.first_name",
+ "last_saved_by.last_name",
"slice_name",
"viz_type",
]
search_columns = [
"created_by",
"changed_by",
+ "last_saved_at",
+ "last_saved_by.id",
+ "last_saved_by.first_name",
+ "last_saved_by.last_name",
"datasource_id",
"datasource_name",
"datasource_type",
diff --git a/superset/charts/commands/create.py b/superset/charts/commands/create.py
index 1425396..ff127cb 100644
--- a/superset/charts/commands/create.py
+++ b/superset/charts/commands/create.py
@@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from datetime import datetime
from typing import Any, Dict, List, Optional
from flask_appbuilder.models.sqla import Model
@@ -43,6 +44,8 @@ class CreateChartCommand(BaseCommand):
def run(self) -> Model:
self.validate()
try:
+ self._properties["last_saved_at"] = datetime.now()
+ self._properties["last_saved_by"] = self._actor
chart = ChartDAO.create(self._properties)
except DAOCreateFailedError as ex:
logger.exception(ex.exception)
diff --git a/superset/charts/commands/update.py b/superset/charts/commands/update.py
index dcbf8c3..7ac3c60 100644
--- a/superset/charts/commands/update.py
+++ b/superset/charts/commands/update.py
@@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from datetime import datetime
from typing import Any, Dict, List, Optional
from flask_appbuilder.models.sqla import Model
@@ -51,6 +52,9 @@ class UpdateChartCommand(BaseCommand):
def run(self) -> Model:
self.validate()
try:
+ if self._properties.get("query_context_generation") is None:
+ self._properties["last_saved_at"] = datetime.now()
+ self._properties["last_saved_by"] = self._actor
chart = ChartDAO.update(self._model, self._properties)
except DAOUpdateFailedError as ex:
logger.exception(ex.exception)
diff --git a/superset/charts/schemas.py b/superset/charts/schemas.py
index 9c086a7..cc9bb99 100644
--- a/superset/charts/schemas.py
+++ b/superset/charts/schemas.py
@@ -82,6 +82,11 @@ query_context_description = (
"in order to generate the data the visualization, and in what "
"format the data should be returned."
)
+query_context_generation_description = (
+ "The query context generation represents whether the query_context"
+ "is user generated or not so that it does not update user modfied"
+ "state."
+)
cache_timeout_description = (
"Duration (in seconds) of the caching timeout "
"for this chart. Note this defaults to the datasource/table"
@@ -177,6 +182,9 @@ class ChartPostSchema(Schema):
allow_none=True,
validate=utils.validate_json,
)
+ query_context_generation = fields.Boolean(
+ description=query_context_generation_description, allow_none=True
+ )
cache_timeout = fields.Integer(
description=cache_timeout_description, allow_none=True
)
@@ -212,6 +220,9 @@ class ChartPutSchema(Schema):
query_context = fields.String(
description=query_context_description, allow_none=True
)
+ query_context_generation = fields.Boolean(
+ description=query_context_generation_description, allow_none=True
+ )
cache_timeout = fields.Integer(
description=cache_timeout_description, allow_none=True
)
diff --git a/superset/migrations/versions/6d20ba9ecb33_add_last_saved_at_to_slice_model.py b/superset/migrations/versions/6d20ba9ecb33_add_last_saved_at_to_slice_model.py
new file mode 100644
index 0000000..9f863b0
--- /dev/null
+++ b/superset/migrations/versions/6d20ba9ecb33_add_last_saved_at_to_slice_model.py
@@ -0,0 +1,66 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_last_saved_at_to_slice_model
+
+Revision ID: 6d20ba9ecb33
+Revises: ('ae1ed299413b', 'f6196627326f')
+Create Date: 2021-08-02 21:14:58.200438
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = "6d20ba9ecb33"
+down_revision = ("ae1ed299413b", "f6196627326f")
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.dialects import postgresql
+
+
+def upgrade():
+ with op.batch_alter_table("slices") as batch_op:
+ batch_op.add_column(sa.Column("last_saved_at", sa.DateTime(), nullable=True))
+ batch_op.add_column(sa.Column("last_saved_by_fk", sa.Integer(), nullable=True))
+ batch_op.create_foreign_key(
+ "slices_last_saved_by_fk", "ab_user", ["last_saved_by_fk"], ["id"]
+ )
+
+ # now do data migration, copy values from changed_on and changed_by
+ slices_table = sa.Table(
+ "slices",
+ sa.MetaData(),
+ sa.Column("changed_on", sa.DateTime(), nullable=True),
+ sa.Column("changed_by_fk", sa.Integer(), nullable=True),
+ sa.Column("last_saved_at", sa.DateTime(), nullable=True),
+ sa.Column("last_saved_by_fk", sa.Integer(), nullable=True),
+ )
+ conn = op.get_bind()
+ conn.execute(
+ slices_table.update().values(
+ last_saved_at=slices_table.c.changed_on,
+ last_saved_by_fk=slices_table.c.changed_by_fk,
+ )
+ )
+ # ### end Alembic commands ###
+
+
+def downgrade():
+ with op.batch_alter_table("slices") as batch_op:
+ batch_op.drop_constraint("slices_last_saved_by_fk", type_="foreignkey")
+ batch_op.drop_column("last_saved_by_fk")
+ batch_op.drop_column("last_saved_at")
+ # ### end Alembic commands ###
diff --git a/superset/models/slice.py b/superset/models/slice.py
index 30badac..310343a 100644
--- a/superset/models/slice.py
+++ b/superset/models/slice.py
@@ -23,7 +23,7 @@ import sqlalchemy as sqla
from flask_appbuilder import Model
from flask_appbuilder.models.decorators import renders
from markupsafe import escape, Markup
-from sqlalchemy import Column, ForeignKey, Integer, String, Table, Text
+from sqlalchemy import Column, DateTime, ForeignKey, Integer, String, Table, Text
from sqlalchemy.engine.base import Connection
from sqlalchemy.orm import relationship
from sqlalchemy.orm.mapper import Mapper
@@ -71,6 +71,13 @@ class Slice( # pylint: disable=too-many-instance-attributes,too-many-public-met
cache_timeout = Column(Integer)
perm = Column(String(1000))
schema_perm = Column(String(1000))
+ # the last time a user has saved the chart, changed_on is referencing
+ # when the database row was last written
+ last_saved_at = Column(DateTime, nullable=True)
+ last_saved_by_fk = Column(Integer, ForeignKey("ab_user.id"), nullable=True,)
+ last_saved_by = relationship(
+ security_manager.user_model, foreign_keys=[last_saved_by_fk]
+ )
owners = relationship(security_manager.user_model, secondary=slice_user)
table = relationship(
"SqlaTable",
diff --git a/superset/views/core.py b/superset/views/core.py
index a0f8482..b46fa96 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -604,7 +604,6 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
)
form_data = get_form_data()[0]
-
try:
datasource_id, datasource_type = get_datasource_info(
datasource_id, datasource_type, form_data
@@ -719,7 +718,6 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
user_id = g.user.get_id() if g.user else None
form_data, slc = get_form_data(use_slice_data=True)
query_context = request.form.get("query_context")
-
# Flash the SIP-15 message if the slice is owned by the current user and has not
# been updated, i.e., is not using the [start, end) interval.
if (
@@ -948,6 +946,8 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods
slc.viz_type = form_data["viz_type"]
slc.datasource_type = datasource_type
slc.datasource_id = datasource_id
+ slc.last_saved_by = g.user
+ slc.last_saved_at = datetime.now()
slc.slice_name = slice_name
slc.query_context = query_context
diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py
index 2e1bbde..cd6e01f 100644
--- a/tests/integration_tests/charts/commands_tests.py
+++ b/tests/integration_tests/charts/commands_tests.py
@@ -17,15 +17,18 @@
# pylint: disable=no-self-use, invalid-name
import json
+from datetime import datetime
from unittest.mock import patch
import pytest
import yaml
+from flask import g
from superset import db, security_manager
from superset.charts.commands.exceptions import ChartNotFoundError
from superset.charts.commands.export import ExportChartsCommand
from superset.charts.commands.importers.v1 import ImportChartsCommand
+from superset.charts.commands.update import UpdateChartCommand
from superset.commands.exceptions import CommandInvalidError
from superset.commands.importers.exceptions import IncorrectVersionError
from superset.connectors.sqla.models import SqlaTable
@@ -275,3 +278,26 @@ class TestImportChartsCommand(SupersetTestCase):
"database_name": ["Missing data for required field."],
}
}
+
+
+class TestChartsUpdateCommand(SupersetTestCase):
+ @patch("superset.views.base.g")
+ @patch("superset.security.manager.g")
+ @pytest.mark.usefixtures("load_energy_table_with_slice")
+ def test_update_v1_response(self, mock_sm_g, mock_g):
+ """"Test that a chart command updates properties"""
+ pk = db.session.query(Slice).all()[0].id
+ actor = security_manager.find_user(username="admin")
+ mock_g.user = mock_sm_g.user = actor
+ model_id = pk
+ json_obj = {
+ "description": "test for update",
+ "cache_timeout": None,
+ "owners": [1],
+ }
+ command = UpdateChartCommand(actor, model_id, json_obj)
+ last_saved_before = db.session.query(Slice).get(pk).last_saved_at
+ command.run()
+ chart = db.session.query(Slice).get(pk)
+ assert chart.last_saved_at != last_saved_before
+ assert chart.last_saved_by == actor