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(android, timezones): timezone offset already millis, do not adjust it #4055

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

mikehardy
Copy link
Collaborator

Description

Timezone.getOffset() provides result in milliseconds, but was being adjusted as if it were in seconds

Related issues

Fixes #4053 with issue noticed and fix provided by @AidanHost

Release Summary

fix(android, timezones): timezone offset already in millis, do not adjust it

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

I am literally making this change directly in the github web UI and relying on the automated test to handle it.

The documentation linked appears very clear cut and the change is so small so that seems reasonably unreasonable


Think react-native-firebase is great? Please consider supporting the project with any of the below:

Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

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

LGTM!

LGTM GIF

@mikehardy
Copy link
Collaborator Author

@Salakar CI flaked again but I re-ran it and it passed, but needs 2 reviews from not-me :)

@Salakar
Copy link
Member

Salakar commented Aug 5, 2020

Lowered it to 1 review :)

@mikehardy mikehardy merged commit 8b0e189 into master Aug 5, 2020
@mikehardy mikehardy deleted the mikehardy-sharedutils-tz-offset branch August 5, 2020 19:06
@mikehardy
Copy link
Collaborator Author

I do so love hitting merge buttons 😄 🚀

tally-ho

@AidanHost
Copy link
Contributor

AidanHost commented Aug 6, 2020

Hi, so I checked the commit, but it's still not 100%. timestamp is in seconds, so it should be converted to millis.

something like this:

long millisTimestamp = timestamp * 1000;
Date date = new Date(millisTimestamp + calendar.getTimeZone().getOffset(millisTimestamp));

@mikehardy please see above

@mikehardy
Copy link
Collaborator Author

@AidanHost thank you for reviewing it! As the interested party, you can just use the Github web UI and hit the edit pencil on the file then post a PR with the change? I can do it if not - let me know - but that's all I'm doing (poorly apparently, sorry!)

@AidanHost
Copy link
Contributor

@mikehardy thanks, that's where the edit button is. I couldn't find it before.

here is a PR for this issue: #4057

androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect date-time on android
3 participants