-
Notifications
You must be signed in to change notification settings - Fork 21
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
[GCAL] Enable notifications and reminders when a superuser token is not supported #272
Changes from 5 commits
4d3a2bb
fe6fe75
7634f2c
9dc21a2
a436769
18767a3
8de4223
af611a9
24c2c64
0ecad3f
165948c
2e6c571
cc88b2e
0def44a
c5f0e1c
0b7bce2
100497a
d90fb5e
4345953
57b30f9
92de736
3e3f43d
5c05a74
5dbc701
19abc70
c3336e9
e4f73fd
9951416
a92d545
c090410
ecca739
6f2b1d7
351730c
7c3b63d
d8745f6
58e7b9f
9aac20f
c9378ce
b139ac5
08b039e
b240b85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,7 @@ func withRemoteUser(user *User) func(m *mscalendar) error { | |
func withActingUser(mattermostUserID string) func(m *mscalendar) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope this is not cheating 🙃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not cheating per se, but maybe error-prone. This is being called in a service account context, so we are changing the state of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only way I imagined of leaving the architecture as it is, without moving other parts of the logic outside of the main "calendar" logic, but I do not know how this could affect elsewhere, if anything. Only running under GCal at the moment. Open to suggestions! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general this seems fine, though just concerned about potential pitfalls of mutating the state. Really that's just an artifact of the I think it's fine how this PR implements it, though we will need to rethink this if we want to perform these requests concurrently across different users. Speaking of which, what are your thoughts on the performance of the current approach, versus fanning out the requests in some fashion? The timing of reminders etc. depends on the speed of this operation, so I'm thinking we'll need some performance optimization here in the case of sending individual requests for each user. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In order to parallelize work we can do a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me 👍 |
||
return func(m *mscalendar) error { | ||
m.actingUser = NewUser(mattermostUserID) | ||
m.client = nil | ||
return nil | ||
} | ||
} | ||
|
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.
Do you see an opportunity to make this code less error-prone? It concerns me that it's easy to make an error based on where this call is placed
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.
That was the intention when I removed the
withActingUser
from this method. Now we can call this one without problems from other places since the newwithActingUser
only gets called from the fenced code in the "main" calls.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.
Right, but what can we do to avoid needing to have
withActingUser
in the fenced code? Just want to make sure we don't run into something similar later, especially if it's something not caught by a unit test