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 <[email protected]> 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 <[email protected]> * Update superset/migrations/versions/6d20ba9ecb33_add_last_saved_at_to_slice_model.py Co-authored-by: Beto Dealmeida <[email protected]> * Update superset/migrations/versions/f6196627326f_update_chart_permissions.py Co-authored-by: Beto Dealmeida <[email protected]> * 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 <[email protected]> (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
