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: add fallback and validation for report and cron timezones #17338

Merged
merged 3 commits into from
Nov 12, 2021

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Nov 3, 2021

SUMMARY

It was possible for short period of time to save an invalid timezone value (GMT Standard Time) in the reports schedule, which was causing errors and failures when running the cron job. With this change, I am catching any cron errors with regard to the timezone function and defaulting to UTC. I also added timezone validation when creating a report.

TESTING INSTRUCTIONS

unit and integration tests have been updated. To manually test, you should be able to create and run a report with a timezone. You should not be able to create a report with a bad timezone.

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

@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #17338 (580c15b) into master (7cdd58b) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17338      +/-   ##
==========================================
- Coverage   77.01%   76.96%   -0.05%     
==========================================
  Files        1040     1041       +1     
  Lines       56074    56070       -4     
  Branches     7735     7735              
==========================================
- Hits        43188    43157      -31     
- Misses      12628    12655      +27     
  Partials      258      258              
Flag Coverage Δ
hive 81.51% <100.00%> (+0.04%) ⬆️
mysql 81.94% <100.00%> (+0.04%) ⬆️
postgres 81.95% <100.00%> (+0.04%) ⬆️
presto ?
python 82.30% <100.00%> (-0.10%) ⬇️
sqlite 81.62% <100.00%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
superset/reports/schemas.py 98.78% <100.00%> (+0.01%) ⬆️
superset/tasks/cron_util.py 100.00% <100.00%> (ø)
superset/db_engine_specs/presto.py 84.30% <0.00%> (-5.65%) ⬇️
superset/connectors/sqla/models.py 86.58% <0.00%> (-1.39%) ⬇️
superset/models/core.py 89.26% <0.00%> (-0.74%) ⬇️
superset/charts/data/query_context_cache_loader.py 90.00% <0.00%> (ø)
superset/utils/date_parser.py 96.63% <0.00%> (+0.04%) ⬆️
superset/charts/data/api.py 88.48% <0.00%> (+0.25%) ⬆️
superset/charts/commands/data.py 97.95% <0.00%> (+1.59%) ⬆️
superset/charts/api.py 85.22% <0.00%> (+4.17%) ⬆️

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 7cdd58b...580c15b. Read the comment docs.

@eschutho eschutho force-pushed the elizabeth/timezone-error branch from a185067 to b208ded Compare November 3, 2021 23:20
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

This is great!

timezone = fields.String(
description=timezone_description,
default="UTC",
validate=validate.OneOf(choices=tuple(all_timezones)),
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

superset/tasks/cron_util.py Show resolved Hide resolved
Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

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

LGTM!

@eschutho eschutho force-pushed the elizabeth/timezone-error branch from df6f550 to 6c58a3b Compare November 12, 2021 00:58
@eschutho eschutho force-pushed the elizabeth/timezone-error branch from 6c58a3b to 07e11fd Compare November 12, 2021 17:39
@eschutho eschutho merged commit f10bc6d into apache:master Nov 12, 2021
@eschutho eschutho deleted the elizabeth/timezone-error branch November 12, 2021 20:28
@eschutho eschutho added the v1.4 label Nov 22, 2021
eschutho added a commit that referenced this pull request Nov 22, 2021
* add fallback and validation for report and cron timezones

* add logging to exception catch

* Run black

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
(cherry picked from commit f10bc6d)
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
* add fallback and validation for report and cron timezones

* add logging to exception catch

* Run black

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
@mistercrunch mistercrunch added 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 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-io size/L v1.4 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants