-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(chart): deprecate persisting url_params #18960
fix(chart): deprecate persisting url_params #18960
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18960 +/- ##
==========================================
- Coverage 66.40% 66.39% -0.01%
==========================================
Files 1641 1641
Lines 63518 63522 +4
Branches 6422 6422
==========================================
- Hits 42176 42174 -2
- Misses 19681 19687 +6
Partials 1661 1661
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
611307d
to
6fc5ef2
Compare
0687fc3
to
3654d25
Compare
3654d25
to
7074744
Compare
5367ef8
to
172a7b8
Compare
172a7b8
to
74f8537
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left a non-blocking suggestion.
@zhaoyongjie Since you know this feature better, can you also review it and make sure I didn't forget anything? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @villebro @michael-s-molina
Thanks for the fix. I tested all scenarios based on this PR, and it works fine!
- url_params was removed when I created a chart on the master branch and switch to the current PR.
- url_params was removed if I create a chart from the current branch.
But now the problem is, we can't find the original form_data_key anymore. This means that every time Chart is opened, a new form_data_key is generated. I would hear your thoughts.
4c54205
to
c02543b
Compare
🏷️ preset:2022.9 |
* fix(chart): deprecate peristing url_params * remove duplicated backend logic * use omitBy * simplify omit (cherry picked from commit bd63a1b)
* fix(chart): deprecate peristing url_params * remove duplicated backend logic * use omitBy * simplify omit (cherry picked from commit bd63a1b)
SUMMARY
Currently there are a few problems in Explore view:
url_params
are present in chart metadata, it's impossible to set new URL params to a chart in Explore viewThis PR changes how the hidden control
url_params
is handled by no longer be persisting it in chart metadata or cached chart form data. This is done by changing the following:form_data_key
is not present in the URL (sort of unrelated, but a regression nonetheless).url_params
property fromform_data
if present before saving chart metadataurl_params
back to the URL during saving in the frontend so that the reloaded Explore/Dashboard view will retain the URL params that were present before saving chart metadataCreateChartCommand
(bycatch - it was missing) and updates related unit testsNote that charts that currently rely on default URL params that are persisted in chart data will still work like before - however, when the chart is saved/overwritten, the
url_params
state will be lost. So when making changes to old charts that are using theurl_param
function in templating code and haven't set default value as the second argument, users will need to update theurl_param
call to include the default value.TESTING INSTRUCTIONS
ENABLE_TEMPLATE_PROCESSING
feature flagflights
datasetAIRLINE
, a metric forCOUNT(*)
and a filter forAIRLINE IN ('{{ url_param('test') }}')
test=AA
to the URLtest=AA
reappears (no longer happens after PR)ADDITIONAL INFORMATION