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

feat: migration to add timezone to report schedule #15747

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Jul 16, 2021

SUMMARY

Adds timezone to report schedule db in order to allow us to add a UI on the reports for users to select a timezone for their schedule. Timezones are strings of locations, such as "America/Los_Angeles" or "UTC".

The default value of UTC as an example will be GMT, which is the current +0 offset time that we are using.

In the future UI, the plan is to "guess" the user's timezone and default the selection to their current timezone.

TimezoneSelector_-_Interactive_Timezone_Selector_⋅_Storybook

For context, the full PR is https://github.com/apache/superset/pull/15743/files

TESTING INSTRUCTIONS

results of db migration script:

Current: 0.33 s
10+: 0.34 s
100+: 0.32 s
1000+: 0.33 s

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@eschutho eschutho requested a review from a team as a code owner July 16, 2021 18:54
@eschutho eschutho changed the title add timezone to report schedule feat: migration to add timezone to report schedule Jul 16, 2021
@eschutho eschutho added the risk:db-migration PRs that require a DB migration label Jul 16, 2021
@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #15747 (9ad8a01) into master (fefcea7) will decrease coverage by 0.22%.
The diff coverage is n/a.

❗ Current head 9ad8a01 differs from pull request most recent head f081ddd. Consider uploading reports for the commit f081ddd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15747      +/-   ##
==========================================
- Coverage   77.13%   76.90%   -0.23%     
==========================================
  Files         984      984              
  Lines       51677    51677              
  Branches     6995     6995              
==========================================
- Hits        39860    39742     -118     
- Misses      11593    11711     +118     
  Partials      224      224              
Flag Coverage Δ
hive ?
mysql 81.54% <ø> (ø)
postgres 81.56% <ø> (+<0.01%) ⬆️
presto ?
python 81.65% <ø> (-0.45%) ⬇️
sqlite 81.19% <ø> (ø)

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

Impacted Files Coverage Δ
superset/db_engines/hive.py 0.00% <0.00%> (-82.15%) ⬇️
superset/db_engine_specs/hive.py 69.44% <0.00%> (-17.07%) ⬇️
superset/db_engine_specs/presto.py 83.36% <0.00%> (-6.95%) ⬇️
superset/views/database/mixins.py 81.03% <0.00%> (-1.73%) ⬇️
superset/connectors/sqla/models.py 88.31% <0.00%> (-1.64%) ⬇️
superset/db_engine_specs/base.py 88.28% <0.00%> (-0.40%) ⬇️
superset/models/core.py 89.61% <0.00%> (-0.26%) ⬇️
superset/utils/core.py 88.12% <0.00%> (-0.13%) ⬇️

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 fefcea7...f081ddd. Read the comment docs.

@eschutho
Copy link
Member Author

thanks @betodealmeida

@eschutho
Copy link
Member Author

The failing test is at ERROR at setup of TestImportExport.test_import_2_slices_for_same_table ____

@eschutho eschutho added the need:merge The PR is ready to be merged label Jul 19, 2021
@github-actions
Copy link
Contributor

⚠️ @eschutho Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

@eschutho eschutho force-pushed the elizabeth/timezone-migration branch from c1f4d06 to baf4c11 Compare July 21, 2021 00:15
@github-actions
Copy link
Contributor

⚠️ @eschutho Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

@eschutho eschutho force-pushed the elizabeth/timezone-migration branch from baf4c11 to 4165cf4 Compare July 21, 2021 00:28
@github-actions
Copy link
Contributor

⚠️ @eschutho Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

1 similar comment
@github-actions
Copy link
Contributor

⚠️ @eschutho Your base branch master has just also updated superset/migrations.

Please consider rebasing your branch to avoid db migration conflicts.

@eschutho eschutho force-pushed the elizabeth/timezone-migration branch 3 times, most recently from 0b1057e to 9ecd2ac Compare July 21, 2021 23:09
@eschutho eschutho merged commit c1eb9ce into apache:master Jul 22, 2021
@eschutho eschutho deleted the elizabeth/timezone-migration branch July 22, 2021 22:36
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 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 need:merge The PR is ready to be merged preset-io risk:db-migration PRs that require a DB migration 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants