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

Changed the timezone used for saving meetups and scheduling reminders #44

Merged
merged 8 commits into from
Oct 27, 2020

Conversation

starkah
Copy link
Contributor

@starkah starkah commented Oct 17, 2020

Fixes #41

Copy link
Member

@daemon1024 daemon1024 left a comment

Choose a reason for hiding this comment

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

@starkah Thanks for the PR 😄 . Requested some changes. Rest LGTM

@sidntrivedi012 @DelusionalOptimist What do you think?

telegram-bot/meetup.go Outdated Show resolved Hide resolved
telegram-bot/meetup.go Outdated Show resolved Hide resolved
telegram-bot/meetup.go Outdated Show resolved Hide resolved
Copy link
Contributor

@DelusionalOptimist DelusionalOptimist left a comment

Choose a reason for hiding this comment

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

I changed the timezone of my system to UTC to test the changes. The reminder works fine. Great job 👍 .

Just one thing, when sending a reminder the bot still uses its local timezone. So you'll need to modify reminder and nextmeetup functions too.
starkah_bot_test

@starkah
Copy link
Contributor Author

starkah commented Oct 18, 2020

@daemon1024 @DelusionalOptimist thanks for pointing out the necessary changes. I'm on it.

@starkah
Copy link
Contributor Author

starkah commented Oct 18, 2020

@DelusionalOptimist Wouldn't it be more easier for the user if the reminder was in it's local time zone?
I suggest that in the first line of the reminder we could change it from "MEETUP REMINDER!" to " REMINDER - MEETUP IN 2 HOURS" or something similar. What do you think?

I changed the timezone of my system to UTC to test the changes. The reminder works fine. Great job +1 .

Just one thing, when sending a reminder the bot still uses its local timezone. So you'll need to modify reminder and nextmeetup functions too.
starkah_bot_test

@daemon1024
Copy link
Member

@starkah Note that the users are the members of OSDC and IST would be the local timezone for em :) But the bot can be deployed anywhere and the timezone of the developer's local system or any environment shouldn't be a dependency.

@starkah
Copy link
Contributor Author

starkah commented Oct 18, 2020

@daemon1024 Makes sense. I'll try to do that.

Copy link
Member

@daemon1024 daemon1024 left a comment

Choose a reason for hiding this comment

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

Some small nits rest LGTM.

telegram-bot/meetup.go Outdated Show resolved Hide resolved
telegram-bot/meetup.go Outdated Show resolved Hide resolved
telegram-bot/meetup.go Outdated Show resolved Hide resolved
Copy link
Contributor

@DelusionalOptimist DelusionalOptimist left a comment

Choose a reason for hiding this comment

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

LGTM. Great work @starkah. 🚀

@daemon1024 daemon1024 merged commit 9e19052 into osdc:master Oct 27, 2020
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.

Change timezone used for saving meetups and scheduling reminders
3 participants