Refactor event sync to pass account into constructors#459
Conversation
squeaky-pl
changed the title
| The token | ||
| """ | ||
| with session_scope(self.namespace_id) as db_session: | ||
| acc = db_session.query(Account).get(self.account_id) |
There was a problem hiding this comment.
Is there any possible reason why reloading the account information here might've been important? Or is the freshest possible state of the account a) irrelevant, or b) already ensured through different logic somehow?
| BaseSyncMonitor.__init__( | ||
| self, | ||
| account_id, | ||
| namespace_id, | ||
| email_address, | ||
| account.id, | ||
| account.namespace.id, | ||
| account.email_address, | ||
| EVENT_SYNC_FOLDER_ID, | ||
| EVENT_SYNC_FOLDER_NAME, | ||
| provider_name, | ||
| account.verbose_provider, | ||
| poll_frequency=poll_frequency, | ||
| scope="calendar", | ||
| ) |
There was a problem hiding this comment.
This is a side note and not something that needs to happen in this PR, but I feel like this number of arguments could warrant using * to force all of them to be passed as kwargs. That way at least it's a bit clearer what the role/expectation is for each variable. For example, I think provider_name=account.verbose_provider is clearer than just passing account.verbose_provider into this constructor.
| if self.provider.webhook_notifications_enabled(): | ||
| # Run the sync loop more frequently if push notifications are | ||
| # enabled. Note that we'll only update the calendar if a | ||
| # Webhook was receicved recently, or if we haven't synced for |
| namespace_id, | ||
| provider_class=GoogleEventsProvider, | ||
| ) | ||
| event_sync = EventSync(generic_account, provider_class=GoogleEventsProvider,) |
There was a problem hiding this comment.
Nit: Probably don't need that trailing comma.
2 participants
This is a follow-up on #458,
This refactors Google and Microsoft event sync to accept
Accountobjects in constructor and not in methods, as each ofEventSync/WebhookEventSyncgreenlet andEventsProviderwork always on a single account.