betodealmeida commented on pull request #16077: URL: https://github.com/apache/superset/pull/16077#issuecomment-897163100
> 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 ](https://en.wikipedia.org/wiki/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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
