[MI-3082]: Fixed the issue 198 'Added the logic to refresh and store the token' #7
Conversation
server/remote/remote.go
Outdated
| @@ -15,7 +15,7 @@ import ( | |||
| ) | |||
|
|
|||
| type Remote interface { | |||
| MakeClient(context.Context, bot.Poster, store.Store, string, *oauth2.Token) Client | |||
| MakeClient(context.Context, *oauth2.Token, store.Store, string, func() bool, func(error)) Client | |||
There was a problem hiding this comment.
Ideally the remote package is not aware of the store package. We can pass in another closure here to avoid this.
Maybe it makes sense to group these 3 callbacks into a struct, like remote.UserTokenHelpers
server/mscalendar/oauth2.go
Outdated
| RefreshAndStoreToken: app.Store.RefreshAndStoreToken, | ||
| } | ||
|
|
||
| client := app.Remote.MakeClient(ctx, tok, mattermostUserID, app.Poster, tokenHelpers) |
There was a problem hiding this comment.
Should this be MakeUserClient instead?
There was a problem hiding this comment.
@mickmister I guess this gets run during the Oauth, so I thought this won't need the refresh token part here.
There was a problem hiding this comment.
so I thought this won't need the refresh token part here.
I would say that we should still call MakeUserClient, to differentiate that this is a client for a user, and not a superuser client
server/remote/msgraph/client.go
Outdated
| bot.Logger | ||
| mattermostUserID string | ||
| conf *config.Config | ||
| tokenHelpers *serializer.UserTokenHelpers |
There was a problem hiding this comment.
I'm not sure if serializier is the right package for UserTokenHelpers. It's only used in remote, and it's not ever serialized.
There was a problem hiding this comment.
@mickmister I have moved this to the remote package
There was a problem hiding this comment.
Mostly LGTM, just one remaining request to rename the functions UserTokenHelpers to match the names in the store. Approving preemptively for this.
Thanks @Kshitij-Katiyar
mattermost#256