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

This resolves the timezone issue #13

Merged
merged 1 commit into from
Feb 24, 2015
Merged

This resolves the timezone issue #13

merged 1 commit into from
Feb 24, 2015

Conversation

thapar
Copy link
Contributor

@thapar thapar commented Feb 17, 2015

This resolves the timezone issue (issue #3) where some date/time entries miscalculated whether it was today or tomorrow.

The issue is that SugarJS doesn't take into account the timezone. This pull request forces the use of Google's Utilities.formatDate(), which takes into account the timezone. Previously, when when a date wasn't "converted", it simply used the SugarJS method to create the date (which didn't take into account the timezone). But with this change, the selected timezone will always be taken into account.

This resolves the timezone issue where some date/time entries miscalculated whether it was today or tomorrow.

The issue is that SugarJS doesn't take into account the timezone. This pull request forces the use of Google's `Utilities.formatDate()`, which takes into account the timezone. Previously, when when a date wasn't "converted", it simply used the SugarJS method to create the date (which didn't take into account the timezone). But with this change, the selected timezone will always be taken into account.
@webdigi
Copy link
Owner

webdigi commented Feb 17, 2015

Thanks. Can you please outline a few tests we can do verify the commit wont break anything.

@thapar
Copy link
Contributor Author

thapar commented Feb 17, 2015

Time is now 2:45pm (Eastern Standard Time, which is GMT -5) on Monday, February 17, 2015.
Before pull request: (Timezone is not taken into account)

"3pm" -> Tuesday, February 18, 2015 3:00:00pm
"4pm" -> Tuesday, February 18, 2015 4:00:00pm
"8pm" -> Monday, February 17, 2015 8:00:00pm

After pull request: (Timezone is taken into account)

"3pm" -> Monday, February 17, 2015 3:00:00pm
"4pm" -> Monday, February 17, 2015 4:00:00pm
"8pm" -> Monday, February 17, 2015 8:00:00pm

@webdigi
Copy link
Owner

webdigi commented Feb 18, 2015

we are reviewing and will get back

@webdigi
Copy link
Owner

webdigi commented Feb 18, 2015

how would this work if user enters +1 day
most users have 2 days later / + 3 days etc. removing the check might cause issues right?

@thapar
Copy link
Contributor Author

thapar commented Feb 23, 2015

You seem to still be apprehensive about this pull request. It's very clear that that using dateConversionRequired() was implemented before SugarJS was implemented. After implementing Date Sugar, dateConversionRequired() became useless. I'm not sure who put this code together, but they can definitely confirm this pull request as a fix for anyone not living at the GMT time zone.

@webdigi webdigi added the bug label Feb 23, 2015
@webdigi
Copy link
Owner

webdigi commented Feb 23, 2015

Thanks @thapar for your usual support. We do have several live users using the scheduler. So it might be best if we setup this in a separate copy to test? I am happy to commit this but we need to make sure it does not cause any errors.

The original developer will be back in a few weeks and we can investigate this further.

@thapar
Copy link
Contributor Author

thapar commented Feb 23, 2015

This will not break for ANY user, as I have already tested the change. In fact, after you merge this pull request, I can clean up some of the superfluous and unused lines of code as well. You probably don't notice the need for this pull request because you are in England (GMT +0). For everyone in the rest of the world using this script, it's tedious because of the timezone bug.

webdigi added a commit that referenced this pull request Feb 24, 2015
This resolves the timezone issue
@webdigi webdigi merged commit 88a14ad into webdigi:master Feb 24, 2015
@webdigi
Copy link
Owner

webdigi commented Feb 24, 2015

Please review and confirm if this works as expected

thapar added a commit to thapar/GmailScheduler that referenced this pull request Feb 26, 2015
Although webdigi#13 removed some troubling code, there still seems to be an issue where SugarJS is maintaining the timezone of the user and mixing it up with the time zone of the server (it seems). Forcing the GmailScheduler script to use UTC is the sane solution to dealing with people in different time zones.

HOWEVER, before merging this pull request, I highly recommend you create a new Google Apps Script project and transfer your current project there to use as a test project. Put this code in that new Google Apps Script project, which will serve as a sandbox. Then enable me (or anyone else in a different timezone as you) to test the script by checking the Log window in Google Apps' script editor.
@thapar thapar mentioned this pull request Feb 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants