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: annotation broken #20651

Merged
merged 3 commits into from
Jul 11, 2022
Merged

fix: annotation broken #20651

merged 3 commits into from
Jul 11, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Jul 8, 2022

SUMMARY

Currently, the annotation on the time series chart has been broken.

image

  1. This error is from the TypeScript type guard function.
  2. The annotation on the master branch doesn't apply annotation_data from the query response.

After these fixes it turns out that the Apply button is not working again, which I think should be fixed in a seprated PR. Also, the annnotation layer logic is very difficult to maintain, I would suggest investing more time in rewriting the current codes.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

annotation.mov

Before

image

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 marked this pull request as ready for review July 8, 2022 11:44
@pull-request-size pull-request-size bot added size/L and removed size/S labels Jul 8, 2022
@codecov
Copy link

codecov bot commented Jul 8, 2022

Codecov Report

Merging #20651 (df054bd) into master (c29261b) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##           master   #20651   +/-   ##
=======================================
  Coverage   66.85%   66.85%           
=======================================
  Files        1753     1753           
  Lines       65823    65827    +4     
  Branches     7007     7006    -1     
=======================================
+ Hits        44005    44010    +5     
+ Misses      20032    20031    -1     
  Partials     1786     1786           
Flag Coverage Δ
javascript 51.95% <85.71%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...art-controls/src/sections/annotationsAndLayers.tsx 100.00% <ø> (ø)
...hart-echarts/src/MixedTimeseries/transformProps.ts 0.00% <0.00%> (ø)
...uperset-ui-core/src/query/types/AnnotationLayer.ts 100.00% <100.00%> (ø)
...gin-chart-echarts/src/Timeseries/transformProps.ts 55.95% <100.00%> (+1.19%) ⬆️
...ugins/plugin-chart-echarts/src/utils/annotation.ts 85.10% <100.00%> (+1.38%) ⬆️

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 c29261b...df054bd. Read the comment docs.

@john-bodley
Copy link
Member

john-bodley commented Jul 9, 2022

Thanks @zhaoyongjie for the fix and adding unit tests. For context I'm running into the exact same issue whilst testing a prior Preset release internally at Airbnb.

I was wondering whether you knew what PR introduced said regression and/or whether it was possible to simply revert it?

@zhaoyongjie
Copy link
Member Author

Thanks @zhaoyongjie for the fix and adding unit tests. For context I'm running into the exact same issue whilst testing a prior Preset release internally at Airbnb.

I was wondering whether you knew what PR introduced said regression and/or whether it was possible to simply revert it?

thanks for the review, this issue from the PR

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM

@rusackas
Copy link
Member

@zhaoyongjie any idea why codecov is failing on this PR? There seem to be similarly confusing codecov failures on lots of PRs right now, but I'm not sure yet what to make of it.

@rusackas rusackas merged commit 7f918a4 into apache:master Jul 11, 2022
jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Jul 12, 2022
* fix: annotation broken

* fix UT

* add annotation data to mixed timeseries chart

(cherry picked from commit 7f918a4)
john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Jul 12, 2022
* fix: annotation broken

* fix UT

* add annotation data to mixed timeseries chart

(cherry picked from commit 7f918a4)
@zhaoyongjie
Copy link
Member Author

@rusackas the coverage issue didn't bring from this PR, the master branch has broken some days. I will look into / fix coverage issue in the separated pr.

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Jul 12, 2022
* fix: annotation broken

* fix UT

* add annotation data to mixed timeseries chart

(cherry picked from commit 7f918a4)
akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022
* fix: annotation broken

* fix UT

* add annotation data to mixed timeseries chart
zhaorui2022 pushed a commit to zhaorui2022/superset that referenced this pull request Sep 22, 2022
* fix: annotation broken

* fix UT

* add annotation data to mixed timeseries chart
eschutho pushed a commit to preset-io/superset that referenced this pull request Oct 18, 2022
* fix: annotation broken

* fix UT

* add annotation data to mixed timeseries chart

(cherry picked from commit 7f918a4)
Fahrenheit35 pushed a commit to Fahrenheit35/superset that referenced this pull request Nov 11, 2022
* fix: annotation broken

* fix UT

* add annotation data to mixed timeseries chart

(cherry picked from commit 7f918a4)
@slavanorm
Copy link

can we say that this issue is not complete?
in 2.* version annotations are broken:
#24374

@mistercrunch mistercrunch added 🍒 2.0.1 🏷️ 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 preset:2022.27 size/L v2.0 v2.0.1 🍒 2.0.1 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants