fix: ensure that users viewing chart does not automatically save edit…

… data (apache#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 <roberto@dealmeida.net>

* Update superset/migrations/versions/6d20ba9ecb33_add_last_saved_at_to_slice_model.py

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

* Update superset/migrations/versions/f6196627326f_update_chart_permissions.py

Co-authored-by: Beto Dealmeida <roberto@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 <roberto@dealmeida.net>

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,
           }),
         });
       }

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',
       },
       {

superset/charts/api.py

@@ -152,6 +152,10 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
         "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 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]:
         "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",

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 @@ def __init__(self, user: User, data: Dict[str, Any]):
     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)

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 @@ def __init__(self, user: User, model_id: int, data: Dict[str, Any]):
     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)

superset/charts/schemas.py

@@ -82,6 +82,11 @@
     "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"
@@ -176,6 +181,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
     )
@@ -211,6 +219,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
     )

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 ###

superset/models/slice.py

@@ -23,7 +23,7 @@
 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",

superset/views/core.py

@@ -604,7 +604,6 @@ def explore_json(
             )
 
         form_data = get_form_data()[0]
-
         try:
             datasource_id, datasource_type = get_datasource_info(
                 datasource_id, datasource_type, form_data
@@ -719,7 +718,6 @@ def explore(  # pylint: disable=too-many-locals,too-many-return-statements,too-m
         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 @@ def save_or_overwrite_slice(
         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
 

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 @@ def test_import_v1_chart_validation(self):
                 "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