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

[pkdotson]

SUMMARY

This pr fixes an issue where user would view charts and run queries without saving but the api would return that user had modified the chart. This was caused because of a change in the api using query_context where accessing the slice table would do an automatic saved changed_on_delta_humanize. This pr add two new cols to the Slice table so that the user only see's actually modified charts.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before:

Screen.Recording.2021-08-05.at.9.11.25.AM.mov

after

Screen.Recording.2021-08-05.at.9.40.46.AM.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• [x ] Has associated issue: fixes [chart list] Simply explore a chart makes that chart shows as modified on chart list #15990
• Changes UI
• [ x] Includes DB Migration (follow approval process in SIP-59)
  • [x ] Migration is atomic, supports rollback & is backwards-compatible
  • [x ] Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[github-actions]

⚠️ @pkdotson Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

[codecov]

Codecov Report

Merging #16077 (80f4abe) into master (2bfc1c2) will decrease coverage by 0.16%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master   #16077      +/-   ##
==========================================
- Coverage   76.76%   76.60%   -0.17%     
==========================================
  Files         995      996       +1     
  Lines       52881    52960      +79     
  Branches     6721     6736      +15     
==========================================
- Hits        40596    40571      -25     
- Misses      12059    12163     +104     
  Partials      226      226              

Flag	Coverage Δ
hive	?
javascript	71.20% <85.71%> (-0.02%)	⬇️
mysql	81.53% <100.00%> (-0.06%)	⬇️
postgres	81.59% <100.00%> (-0.02%)	⬇️
python	81.68% <100.00%> (-0.31%)	⬇️
sqlite	81.20% <100.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ntend/src/explore/components/ExploreChartPanel.jsx	15.00% <ø> (ø)
superset/charts/api.py	85.82% <ø> (+0.11%)	⬆️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx	72.90% <85.71%> (+0.23%)	⬆️
superset/charts/commands/create.py	92.15% <100.00%> (+0.49%)	⬆️
superset/charts/commands/update.py	88.40% <100.00%> (+0.71%)	⬆️
superset/charts/schemas.py	100.00% <100.00%> (ø)
superset/models/slice.py	86.41% <100.00%> (+0.22%)	⬆️
superset/views/core.py	75.25% <100.00%> (+0.03%)	⬆️
superset/db_engines/hive.py	0.00% <0.00%> (-82.15%)	⬇️
superset/db_engine_specs/hive.py	69.80% <0.00%> (-16.87%)	⬇️
... and 32 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bfc1c2...80f4abe. Read the comment docs.

[betodealmeida]

Comments on the DB migration.

[zhaoyongjie]

@pkdotson Maybe I missed some details. I'm curious as to why a get method would modify the metadata? Should we avoid this operation? I searched all of the codebases seems that only save_or_overwrite_slice can change slice object.

[github-actions]

⚠️ @pkdotson Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

[suddjian]

@zhaoyongjie I'm not sure exactly where it happens either, but one of @betodealmeida's changes started saving query_context. It's not unheard of for GET requests to involve db writes, when the server wants to save some aspect of a request or lazily calculate and then save a value, so I think it's okay.

The main thing here is that the value being used for "last modified" in the UI should not be something that auto-updates when a database row is written. This PR makes it more explicit.

[pkdotson]

/testenv up

[github-actions]

@pkdotson Ephemeral environment spinning up at http://52.38.130.113:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[villebro]

Nice! LGTM

[betodealmeida]

We probably don't need this here, but it does no harm.

[betodealmeida]

Awesome, thanks for fixing this!

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.

[rosemarie-chiu]

🏷 2021.31

[john-bodley]

I'm a little perplexed by this PR,

This pr fixes an issue where user would view charts and run queries without saving but the api would return that user had modified the chart.

Shouldn't we remedy the root cause, i.e., prevent viewing of a slice from augmenting the database record, rather than adding additional columns to the table schema? This feels somewhat akin to the There Was an Old Lady Who Swallowed a Fly
rhyme.

[betodealmeida]

I'm a little perplexed by this PR,

This pr fixes an issue where user would view charts and run queries without saving but the api would return that user had modified the chart.

Shouldn't we remedy the root cause, i.e., prevent viewing of a slice from augmenting the database record, rather than adding additional columns to the table schema? This feels somewhat akin to the There Was an Old Lady Who Swallowed a Fly rhyme.

@john-bodley I added a change where the first time the user views a saved chart it does a PUT request storing the associated query_context in a new column (slices.query_context). This is needed because:

1. To get the data for a given chart, we need to POST the query_context to /api/v1/chart/.
2. To obtain query_context from form_data we need to run Javascript code unique to each visualization.

This means that, programmatically, it was impossible to get the data for a given saved chart, something we needed for reports.

We can't populate the new column slices.query_context with the migration script that added, since it requires Javascript code, so the charts now check to see if the column is null, and if so, they will do the PUT request to populate the column.

The side-effect of this is that charts are being shown as modified, even if they're not. The problem here is that the changed_on column is auto-populated by FAB every time the model changes, and we can't control that from Superset (please correct me if I'm wrong). Which is why @pkdotson added a custom column to track actual modifications by the user, and not by background processes.

Personally I don't see this as an anti-pattern. FAB offers a column changed_on for convenience, and if we need finer control I think it's ok to add a new column and track the updates ourselves. This has the potential to be useful for other use cases in the future, like migrations that modify the chart data.

I hope this clarifies things, and makes the solution more acceptable.

[john-bodley]

@betodealmeida I don't have much context on the query context change, but when I read, the following,

To get the data for a given chart, we need to POST the query_context to /api/v1/chart/

i.e., a GET request first requires a POST (to I gather the same table) I just wanted to check that we're confident in the pattern that's being used. Per this PR it feels like that change is snowballing and resulted in unforeseen behavior and thus thought there was merit in confirming that collectively the community was onboard with this approach.