-
Notifications
You must be signed in to change notification settings - Fork 5
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
Mailchimp syncer checks if user has completed course #1365
Conversation
WalkthroughThe pull request modifies a SQL query in the headless LMS service to enhance the logic for fetching unsynced user marketing consents. The change adds an additional condition to check the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/headless-lms/models/src/marketing_consents.rs (1)
Line range hint
119-119
: Update function documentation to mention course completion tracking.The function's documentation should be updated to mention that it also tracks course completion updates.
-/// Fetches all user marketing consents with detailed user information for a specific course language group, if they haven't been synced to Mailchimp or if there have been updates since the last sync. +/// Fetches all user marketing consents with detailed user information for a specific course language group, if they haven't been synced to Mailchimp or if there have been updates (including course completions) since the last sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/headless-lms/models/.sqlx/query-9085a18a90d505c905af9b0bbd078bf3b88a6ea28f869d5a106b6456752361f0.json
(2 hunks)services/headless-lms/models/src/marketing_consents.rs
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: headless_lms
- GitHub Check: build
- GitHub Check: build-and-deploy
🔇 Additional comments (4)
services/headless-lms/models/.sqlx/query-9085a18a90d505c905af9b0bbd078bf3b88a6ea28f869d5a106b6456752361f0.json (2)
3-3
: LGTM! Query enhancement properly tracks course completion updates.The added condition
cmc.updated_at > umc.synced_to_mailchimp_at
correctly ensures that Mailchimp is updated when a user completes a course after their last sync.
121-121
: LGTM! Query hash updated correctly.The hash value has been properly updated to reflect the query changes.
services/headless-lms/models/src/marketing_consents.rs (2)
161-161
: LGTM! Condition added to sync course completion updates.The added condition
OR cmc.updated_at > umc.synced_to_mailchimp_at
properly ensures that marketing consents are resynced when course completion status changes.
Line range hint
119-164
: Verify Mailchimp API rate limits.The addition of course completion as a sync trigger might increase the frequency of Mailchimp API calls. Please ensure this aligns with your Mailchimp API rate limits and consider implementing rate limiting if necessary.
Summary by CodeRabbit