Skip to content
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: time grain can't be removed in explore #21644

Merged

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Sep 29, 2022

SUMMARY

There is a long-term issue in the Time Grain control that it can't be cleared time grain value even though remove it or select Original value.

This PR intends to fix it.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

time.grain.-.before.mov

After

time.grain.after.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@zhaoyongjie zhaoyongjie changed the title fix: time grain can't remove on explore and dashbaord fix: time grain can't remove in explore Sep 29, 2022
@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #21644 (6f9edc8) into master (9dd102f) will not change coverage.
The diff coverage is 25.00%.

@@           Coverage Diff           @@
##           master   #21644   +/-   ##
=======================================
  Coverage   66.83%   66.83%           
=======================================
  Files        1798     1798           
  Lines       68823    68823           
  Branches     7333     7333           
=======================================
  Hits        45996    45996           
+ Misses      20949    20948    -1     
- Partials     1878     1879    +1     
Flag Coverage Δ
javascript 53.16% <25.00%> (ø)

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

Impacted Files Coverage Δ
...et-ui-chart-controls/src/shared-controls/index.tsx 54.23% <25.00%> (-2.91%) ⬇️
...ugins/plugin-chart-echarts/src/Gauge/buildQuery.ts 66.66% <0.00%> (-8.34%) ⬇️
...s/plugin-chart-handlebars/src/plugin/buildQuery.ts 100.00% <0.00%> (ø)
...ontend/src/explore/controlUtils/getControlState.ts 90.16% <0.00%> (+4.91%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zhaoyongjie zhaoyongjie changed the title fix: time grain can't remove in explore fix: time grain can't be removed in explore Sep 29, 2022
@zhaoyongjie zhaoyongjie force-pushed the fix/fix_time_grain_default_value branch from 1b10fc8 to 61eaa87 Compare September 30, 2022 02:33
@zhaoyongjie zhaoyongjie force-pushed the fix/fix_time_grain_default_value branch from 61eaa87 to d9c2575 Compare September 30, 2022 05:28
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this annoying but pretty major bug - minor nit, LGTM

Comment on lines +192 to +195
// If a chart is a new one that isn't saved, the 'time_grain_sqla' isn't in the form_data.
return 'time_grain_sqla' in (state?.form_data ?? {})
? state.form_data?.time_grain_sqla
: 'P1D';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some reason we're not just doing state.form_data?.time_grain_sqla || 'P1D' here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When Time Grain control be set to "Original value" or just removal, the time_grain_sqla in the form_data will be null. If time_grain_sqla get a null value, the 'P1D' will always be set to control, in this case, the chart will never use an empty time grain value.
For instance:

  1. make a Line Chart and set Original value to Time Grain, and then save it; next, go to chart list; then, reopen the chart.
  2. if use state.form_data?.time_grain_sqla || 'P1D' the Time Grain will return P1D

@zhaoyongjie zhaoyongjie merged commit 4c17f0e into apache:master Sep 30, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants