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

connected account access token refresh cron #5706

Closed

Conversation

rostaklein
Copy link
Contributor

@rostaklein rostaklein commented Jun 1, 2024

solves #5632

This is a wild guess how it could look like, tested ant it seem to work :) Looking forward for the comments.

Comment on lines +167 to +171

await this.connectedAccountRepository.updateAccessTokenNeedsRefresh(
messageChannel.connectedAccountId,
workspaceId,
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not the correct place to put this into, but couldnt find a better place so its not repeated in every single sync job. Let me know if you have any other idea.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

  • Added RefreshAccessTokenCronCommandsModule to command.module.ts
  • Introduced accessTokenNeedsRefresh and accessTokenRefreshedAt fields in seed data
  • Integrated RefreshAccessTokenJobModule into jobs.module.ts
  • Added new constants for access token fields in standard-field-ids.ts
  • Implemented access token refresh logic in ConnectedAccountRepository and related services

@@ -0,0 +1,33 @@
import { Inject } from '@nestjs/common';
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a more descriptive comment about the purpose of this import.

super();
}

async run(): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle potential errors from the messageQueueService.addCron call.

@@ -59,6 +59,14 @@ export class GmailMessageListFetchJob
return;
}

if (connectedAccount.accessTokenNeedsRefresh) {
this.logger.log(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that the log message provides enough context for debugging, such as including the timestamp.

@@ -70,6 +70,14 @@ export class GmailFullMessageListFetchService {
return;
}

if (connectedAccount.accessTokenNeedsRefresh) {
this.logger.log(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure that the log message provides enough context for debugging, such as including the current state of the token.

@charlesBochet
Copy link
Member

Sorry @rostaklein, just saw your PR but we have this huge refactoring: #5710
Ping us on Discord, we can collaborate on messaging there is still things to do!

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.

2 participants