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

o365 calendar sync #8044

Merged
merged 15 commits into from
Nov 7, 2024
Merged

o365 calendar sync #8044

merged 15 commits into from
Nov 7, 2024

Conversation

brendanlaschke
Copy link
Contributor

@brendanlaschke brendanlaschke commented Oct 24, 2024

Implemented:

  • Account Connect
  • Calendar sync via delta ids then requesting single events

I think I would split the messaging part into a second pr - that's a step more complex then the calendar :)

@BOHEUS
Copy link
Contributor

BOHEUS commented Oct 25, 2024

Related to #7228

@FelixMalfait
Copy link
Member

Wow @brendanlaschke is back 😍

Copy link
Contributor

@bosiraphael bosiraphael left a comment

Choose a reason for hiding this comment

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

Great job @brendanlaschke ! Thank you very much for your contribution. I've left a couple of comments.
I can add an account but my calendar is stuck in the CALENDAR_EVENT_LIST_FETCH_ONGOING syncStage.
Also, we should put this feature behind a feature flag for now. I can assist you with it if you have any questions.

@FelixMalfait
Copy link
Member

/award 3500 massive contribution thanks!

Copy link

oss-gg bot commented Oct 27, 2024

Awarding brendanlaschke: 3500 points 🕹️ Well done! Check out your new contribution on oss.gg/brendanlaschke

@brendanlaschke brendanlaschke marked this pull request as ready for review October 27, 2024 17:58
@brendanlaschke brendanlaschke changed the title o365 sync o365 calendar sync Oct 27, 2024
Copy link
Contributor

@bosiraphael bosiraphael left a comment

Choose a reason for hiding this comment

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

Great job @brendanlaschke ! Amazing contribution. I left some comments to address before we can merge the PR :)


const response: PageCollection = await microsoftClient
.api(syncCursor || '/me/calendar/events/delta')
.version('beta')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use the beta api and not the v1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v1 hasn't got the delta queries for events sadly - should still be fine as they are only used to retrieve ids (where the behavior shouldn't change)?

Comment on lines +34 to +37
const response: PageCollection = await microsoftClient
.api(syncCursor || '/me/calendar/events/delta')
.version('beta')
.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

I got the following error using a free exchange account. I wasn't aware of this, but to use the microsoft graph api you have to have a valid exchange online subscription.

code = 'NOT_FOUND'
message = 'The mailbox is either inactive, soft-deleted, or is hosted on-premise.'

When this error happens, maybe we should put the channel in FAILED_INSUFFICIENT_PERMISSIONS for now. Later we might also want to add a User Var as it is done inside accounts-to-reconnect.service. We store some information inside a key value pair table about the users and the workspace, and we use that to display a banner to alert the user that his account needs to be reconnected. We can do the same here by adding a key UNSUPPORTED_MICROSOFT_ACCOUNTS and store inside an array the list of these accounts. This will allow us to display a banner to alert the user that he needs an a valid exchange online license to continue.

Copy link

github-actions bot commented Nov 4, 2024

Warnings
⚠️ Changes were made to the environment variables, but not to the documentation - Please review your changes and check if a change needs to be documented!

Generated by 🚫 dangerJS against e6ae2f9

Copy link
Contributor

@bosiraphael bosiraphael left a comment

Choose a reason for hiding this comment

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

LGTM :)

@bosiraphael bosiraphael merged commit f9c076d into twentyhq:main Nov 7, 2024
17 checks passed
@brendanlaschke brendanlaschke deleted the o365sync branch November 7, 2024 20:26
@zeynepmetin11

This comment was marked as spam.

@FelixMalfait FelixMalfait mentioned this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants