You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "villebro (via GitHub)" <gi...@apache.org> on 2023/06/26 08:20:49 UTC
[GitHub] [superset] villebro opened a new pull request, #24513: fix(sqllab): normalize changedOn timestamp
villebro opened a new pull request, #24513:
URL: https://github.com/apache/superset/pull/24513
### SUMMARY
#24422 introduced a regression, which caused SQL Lab results to not show up in SQL Lab for clients that had a negative TZ offset. This was due to the backend returning timestamps that don't include the Zulu prefix to indicate that the timestamp is in UTC TZ. Due to parsing this using `Date.parse()` in the frontend, this timestamp was assumed to be in local timezone, causing an inconsistency when requesting updated queries using the epoch.
This fixes the issue by using the `normalizeTimestamp` function from `@superset-ui/core`, and adds tests to ensure that `queriesLastUpdate` in Redux state are correctly normalized. This test would previously have failed on master branch when run in non-UTC TZ.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: fixes #24487
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] hughhhh merged pull request #24513: fix(sqllab): normalize changedOn timestamp
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh merged PR #24513:
URL: https://github.com/apache/superset/pull/24513
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #24513: fix(sqllab): normalize changedOn timestamp
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #24513:
URL: https://github.com/apache/superset/pull/24513#issuecomment-1609904175
@villebro could you expand on what you mean by,
> ... backend returning timestamps in naive format
The reason I ask is I was wondering whether the fix should be a backend rather than frontend fix, where the backend returns a non-ambiguous timestamp.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24513: fix(sqllab): normalize changedOn timestamp
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24513:
URL: https://github.com/apache/superset/pull/24513#issuecomment-1607192249
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24513?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24513](https://app.codecov.io/gh/apache/superset/pull/24513?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (81b429f) into [master](https://app.codecov.io/gh/apache/superset/commit/ba3bdc077cfe1a017105332dc688b9d7faef6795?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ba3bdc0) will **increase** coverage by `0.01%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 81b429f differs from pull request most recent head ccf3960. Consider uploading reports for the commit ccf3960 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24513 +/- ##
==========================================
+ Coverage 69.06% 69.07% +0.01%
==========================================
Files 1901 1902 +1
Lines 74019 74025 +6
Branches 8116 8117 +1
==========================================
+ Hits 51121 51135 +14
+ Misses 20787 20779 -8
Partials 2111 2111
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `55.74% <100.00%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24513?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/SqlLab/fixtures.ts](https://app.codecov.io/gh/apache/superset/pull/24513?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
| [...core/src/time-format/utils/denormalizeTimestamp.ts](https://app.codecov.io/gh/apache/superset/pull/24513?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdGltZS1mb3JtYXQvdXRpbHMvZGVub3JtYWxpemVUaW1lc3RhbXAudHM=) | `100.00% <100.00%> (ø)` | |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://app.codecov.io/gh/apache/superset/pull/24513?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `48.25% <100.00%> (+4.98%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #24513: fix(sqllab): normalize changedOn timestamp
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #24513:
URL: https://github.com/apache/superset/pull/24513#issuecomment-1610013293
> @villebro could you expand on what you mean by,
>
> > ... backend returning timestamps in naive format
>
> The reason I ask is I was wondering whether the fix should be a backend rather than frontend fix, where the backend returns a non-ambiguous timestamp.
@john-bodley yes, I think we should do that in the long run. However, at this point I felt it an unnecessarily risky change, as it might have far reaching implications for users that are relying on the current formatting behavior. Also, the fact that this functionality was broken in multiple ways on the frontend, including having either erroneous or missing tests (hence the regression), I felt it prudent to first get this area improved some more before proceeding to fixing the backend, which IMO has an even larger blast radius.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org