-
-
Notifications
You must be signed in to change notification settings - Fork 395
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
zulip-integrations: Update google oauth and reminders for google calendar integration. #850
base: main
Are you sure you want to change the base?
Conversation
2249012
to
af1090e
Compare
@timabbott could you please review this PR? |
@Niloth-p It would be great to get your review on this one. |
@Niloth-p any updates? |
@timabbott @Niloth-p anything pending in this pr? |
Hi, @theofficialvedantjoshi, apologies for the delay in getting back. Thank you for the PR! If you're interested in working more on this integration, it would be helpful if you could help improve the message being generated, by adding separate commit(s) on top of this PR. Here are some ways we can improve the message:
|
Hi @Niloth-p, thanks for the review. I am interested in working more on this integration. I'll set things up locally and add the features mentioned above. I am also interested in participating in GSoC 2025 with zulip. Will you be mentoring this time? Will contributions to this repository add value to my proposal? |
Glad to hear that. Contributions to this repo do count as much as any other Zulip repo. Though I anticipate being involved with any GSoC contributors working on Zulip integrations, and the set of integrations here in this repo could do with a lot of improvements, the priorities of GSoC contributions and project proposals is not my domain, the GSoC channel is a much better place to get info regarding that. |
Hello @theofficialvedantjoshi, it seems like you have referenced #847 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage. Please run An example of a correctly-formatted commit:
To learn how to write a great commit message, please refer to our guide. |
3626384
to
50a9579
Compare
Will be working on features related to |
Fixed reminder logic to respect Google Calendar's
|
@Niloth-p Tagging for your review, but I'm not following the details here, so please feel free to change the status on this PR if needed. |
@Niloth-p any updates? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've successfully absorbed the first 3 commits into my branch. Thanks for the work!
I haven't had the time to implement the last commit. So, I'm leaving some review comments regarding the implementation idea I have in mind.
I haven't checked for edge cases, so you'll have to build upon the suggested idea, and check for them.
@@ -40,13 +40,14 @@ class Event(TypedDict): | |||
description: str | |||
organizer: str | |||
hangout_link: str | |||
reminder: int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment explaining this field here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
# Unique keys for events we've already sent, so we don't remind twice. | ||
sent: Set[Tuple[int, datetime.datetime]] = set() | ||
# Unique keys for reminders we've already sent, so we don't remind twice. | ||
sent: Set[Tuple[int, datetime.datetime, int]] = set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about instead of adding a separate reminder field, we actually switch the start_time field in the tuple to instead use the reminder_time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better will make this change
@@ -73,12 +74,11 @@ google-calendar --calendar [email protected] | |||
|
|||
|
|||
parser.add_argument( | |||
"--interval", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not change this.
This is not meant to be an override, this is the default value for the interval we're going to be using.
This shouldn't be a required argument, but an optional one with the default value as 30 mins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this argument properly? What is it supposed to signify?
timeMin=now, | ||
maxResults=5, | ||
timeMin=datetime.datetime.now(pytz.utc).isoformat(), | ||
timeMax=datetime.datetime.now(pytz.utc).isoformat().split("T")[0] + "T23:59:59Z", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxResults=5
is definitely restricting and needs to be updated for the new algorithm, but I'm not sure we want to pick only events for the same day.
For example, what if the user wants to send reminders like "7 days to go!", "3 days to go", etc. We should be able to support those too.
So, I'm not yet sure what we can do here, to avoid fetching too many events every single call, especially since we do events.clear()
each time.
There are other options like not clearing our cache, and getting notifications from Google, on the calendar being updated instead. But again, I haven't looked into the feasibility of it, and whether that's necessary or optimal. And that would warrant more commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can deal with this later I guess. Will try to think of a feasible solution.
@@ -162,6 +161,9 @@ def populate_events() -> Optional[None]: | |||
# of the tzinfo base class. | |||
start = calendar_timezone.localize(start_naive) | |||
end = calendar_timezone.localize(end_naive) | |||
now = datetime.datetime.now(tz=start.tzinfo) | |||
if start < now: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this is required.
We're using timeMin = now
, so why would start be < now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fetching events with timeMin = now also fetches events that are ongoing for some reason and we want to ignore those events since they have already started. Hence a small check to ignore these events.
Example:
@@ -176,7 +178,25 @@ def populate_events() -> Optional[None]: | |||
else event["organizer"].get("displayName", event["organizer"]["email"]) | |||
) | |||
hangout_link = event.get("hangoutLink", "") | |||
events.append( | |||
reminders = event["reminders"] | |||
# If the user has specified an override, we use that for all events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's actually do this instead.
Let's ignore the calendar's default reminders value.
The only value we'll use is event["reminders"]["overrides"]["minutes"]
.
We'll use 30 mins as the default interval.
If the user wants something else, they can pass it in with --interval.
And for particular events with reminders set for x intervals, we override using the interval.
So, something like the below.
# If the user has specified an override, we use that for all events. | |
if event.get("reminders", {}).get("overrides"): | |
for override in event["reminders"]["overrides"]: | |
reminder_interval = override["minutes"] | |
event["reminder"] = event["start"] - datetime.timedelta(minutes=reminder_interval) | |
# Call events.append() for each override |
Note that I'm suggesting setting event["reminder"]
not to event["reminders"]["overrides"]["minutes"]
, but actually the time it should be called, as a replacement for event["start"]
.
And I know I had previously mentioned not sending notifications if it's turned off for particular events, after taking a look at the API. But, event["reminders"]["useDefault"]
seems to return False by default, and I haven't yet figured out how to set it to True through the Google Calendar UI. So, unless there's an easy way to do that, and its commonly used (and you can figure it out), I think we can drop that, at least for now.
# repeating events. | ||
key = (event["id"], event["start"]) | ||
# Sort events by the time of the reminder. | ||
events.sort( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching to using "reminder" instead of "start" can now simplify this to:
events.sort( | |
events.sort(key=lambda event: (event["reminder"])) |
) | ||
# Iterate through the events and send reminders for those whose reminder time has come or passed and remove them from the list. | ||
# The instant a reminder's time is greater than the current time, we stop sending reminders and break out of the loop. | ||
while len(events): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching to using "reminder" instead of "start" can now simplify this to using the previous logic itself, so this section of changes wouldn't be needed.
Just "start" -> "reminder".
while len(events): | |
dt = event["reminder"] - now | |
if dt.days == 0 and dt.seconds > 0: | |
# The unique key includes the reminder time due to repeating events, and reminder overrides. | |
key = (event["id"], event["reminder"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes makes sense will do.
Hey, thanks for adding that RefreshError commit addressing that, but that's actually not the fix for that issue. It's not the client-secret key that needs the refresh. I've added the fix for this in #856. Also, if you're interested, could you help me add automated tests for the google calendar integration? |
Alright, I am interested in adding automated tests Will do these once the above issues are addressed. |
Reviewing my fix for |
Both answers are already mentioned. I've added more details now.
Pull the branch from #856 using the command above.
It's part of this commit.
|
I have added the reminder logic to #856 # in my branch keeping in mind the above resolutions. To clarify
There is a way to change the default by going into calendar settings and changing the Event notifications. In my original commit I had changed this default and tested this scenario. Please review my changes and let me know what else needs to added. Also I still do not understand the point of |
The
oauth2client
library used in the google-calendar integration scripts has become deprecated. This PR updates the script to replaceoauth2client
with other libraries that Google is now using, specificallygoogle-auth-httplib2
andgoogle-auth-oauthlib
.Fixes: #847
How did you test this PR?
client_secret.json
file from a test Google Cloud application that uses the calendar API.google-credentials.json
in the home directory.Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: