Skip to content

Add batching parameter to salesforce_sync:* tasks #797

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.

Sign up for GitHub

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

Draft
fspeirs wants to merge 1 commit into
base: main
Choose a base branch
from
+54 −3
Draft

Add batching parameter to salesforce_sync:* tasks #797

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 54 additions & 3 deletions lib/tasks/salesforce_sync.rake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,75 @@
# frozen_string_literal: true

namespace :salesforce_sync do
# Sync Schools to Salesforce that don't already exist there.
# Usage:
# rake salesforce_sync:school # Default: sync up to 25 records
# LIMIT=100 rake salesforce_sync:school # Sync up to 100 records
# LIMIT=0 rake salesforce_sync:school # Sync all missing records (unlimited)
desc 'Sync all Schools to Salesforce'
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

The usage comments indicate this task syncs only missing schools and is LIMITed, but the desc still reads “Sync all Schools to Salesforce”. Consider updating the desc/wording to reflect the new behavior so operators don’t assume it will update existing Salesforce records.

Suggested change
desc 'Sync all Schools to Salesforce'
desc 'Sync missing Schools to Salesforce'

Copilot uses AI. Check for mistakes.
task school: :environment do
limit = ENV['LIMIT'] ? ENV['LIMIT'].to_i : 25
limit = nil if limit.zero?
enqueued = 0
Comment on lines +11 to +13
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

ENV['LIMIT'].to_i treats non-numeric values as 0, which then becomes nil (unlimited) here. That makes typos like LIMIT=abc potentially enqueue an unbounded number of jobs; consider validating LIMIT explicitly (e.g., parse with Integer and abort unless it’s >= 0) before proceeding.

Copilot uses AI. Check for mistakes.
checked = 0
Comment on lines 10 to +14
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

These tasks now have non-trivial behavior (LIMIT parsing, existence gating, early break) but there are no rake task specs covering them. Since the repo already tests tasks under spec/lib/tasks, consider adding specs for default LIMIT, LIMIT=0, and that existing Salesforce records are skipped.

Copilot uses AI. Check for mistakes.

School.find_each do |school|
Salesforce::SchoolSyncJob.perform_later(school_id: school.id)
checked += 1
unless Salesforce::School.exists?(editoruuid__c: school.id)
Salesforce::SchoolSyncJob.perform_later(school_id: school.id)
enqueued += 1
break if limit && enqueued >= limit
end
Comment on lines +18 to +22
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

This uses Salesforce::School.exists? inside the find_each, which results in one Salesforce DB query per School (N+1). For large backfills, consider batching existence checks per find_each batch (load existing editoruuid__c values for the batch into a Set) to reduce query volume.

Copilot uses AI. Check for mistakes.
end

puts "Checked #{checked} schools, enqueued #{enqueued} jobs"
end

# Sync non-student Roles to Salesforce that don't already exist there.
# Usage:
# rake salesforce_sync:role # Default: sync up to 25 records
# LIMIT=100 rake salesforce_sync:role # Sync up to 100 records
# LIMIT=0 rake salesforce_sync:role # Sync all missing records (unlimited)
desc 'Sync all non-student Roles to Salesforce'
Comment on lines +28 to 33
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

The docs/desc here still read like the task syncs “all” non-student roles, but the implementation only enqueues roles missing in Salesforce and stops after LIMIT. Consider updating the wording so it’s clear this is a “missing records backfill” task rather than a full resync.

Suggested change
# Sync non-student Roles to Salesforce that don't already exist there.
# Usage:
# rake salesforce_sync:role # Default: sync up to 25 records
# LIMIT=100 rake salesforce_sync:role # Sync up to 100 records
# LIMIT=0 rake salesforce_sync:role # Sync all missing records (unlimited)
desc 'Sync all non-student Roles to Salesforce'
# Backfill non-student Roles to Salesforce when they don't already exist there.
# Usage:
# rake salesforce_sync:role # Default: sync up to 25 missing records
# LIMIT=100 rake salesforce_sync:role # Sync up to 100 missing records
# LIMIT=0 rake salesforce_sync:role # Sync all missing records (unlimited)
desc 'Backfill missing non-student Roles to Salesforce'

Copilot uses AI. Check for mistakes.
task role: :environment do
limit = ENV['LIMIT'] ? ENV['LIMIT'].to_i : 25
limit = nil if limit.zero?
enqueued = 0
Comment on lines +35 to +37
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

ENV['LIMIT'].to_i has the same “invalid string becomes 0 => unlimited” issue here as in the school task. Please validate that LIMIT is numeric and >= 0, and fail fast on invalid values to avoid accidentally enqueueing an unbounded backfill.

Copilot uses AI. Check for mistakes.
checked = 0

Role.where.not(role: Role.roles[:student]).find_each do |role|
Salesforce::RoleSyncJob.perform_later(role_id: role.id)
checked += 1
unless Salesforce::Role.exists?(affiliation_id__c: role.id)
Salesforce::RoleSyncJob.perform_later(role_id: role.id)
enqueued += 1
break if limit && enqueued >= limit
end
Comment on lines +42 to +46
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

This loop performs Salesforce::Role.exists? per Role, creating an N+1 query pattern against the Salesforce DB. Consider batching existence checks for each find_each batch (query existing affiliation_id__c for the batch’s role IDs) before enqueueing jobs.

Copilot uses AI. Check for mistakes.
end

puts "Checked #{checked} roles, enqueued #{enqueued} jobs"
end

# Sync creator_agree_to_ux_contact for Schools where the Contact exists in Salesforce.
# Usage:
# rake salesforce_sync:contact # Default: sync up to 25 records
# LIMIT=100 rake salesforce_sync:contact # Sync up to 100 records
# LIMIT=0 rake salesforce_sync:contact # Sync all records (unlimited)
desc 'Sync creator_agree_to_ux_contact for all Schools to Salesforce Contact'
Comment on lines +52 to 57
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

The usage comment says LIMIT=0 will “Sync all records (unlimited)”, but the code only enqueues jobs for Schools whose Contact already exists in Salesforce. Consider updating this wording (and potentially the desc) so the scope/behavior is clear.

Suggested change
# Sync creator_agree_to_ux_contact for Schools where the Contact exists in Salesforce.
# Usage:
# rake salesforce_sync:contact # Default: sync up to 25 records
# LIMIT=100 rake salesforce_sync:contact # Sync up to 100 records
# LIMIT=0 rake salesforce_sync:contact # Sync all records (unlimited)
desc 'Sync creator_agree_to_ux_contact for all Schools to Salesforce Contact'
# Sync creator_agree_to_ux_contact for Schools whose Contact already exists in Salesforce.
# Usage:
# rake salesforce_sync:contact # Default: enqueue up to 25 eligible schools
# LIMIT=100 rake salesforce_sync:contact # Enqueue up to 100 eligible schools
# LIMIT=0 rake salesforce_sync:contact # Enqueue all eligible schools (unlimited)
desc 'Sync creator_agree_to_ux_contact to Salesforce Contact for Schools whose Contact already exists'

Copilot uses AI. Check for mistakes.
task contact: :environment do
limit = ENV['LIMIT'] ? ENV['LIMIT'].to_i : 25
limit = nil if limit.zero?
enqueued = 0
Comment on lines +59 to +61
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

ENV['LIMIT'].to_i again turns invalid values into 0 (unlimited). Since this task can enqueue many jobs, it’s safer to validate LIMIT (numeric, >= 0) and abort with a clear message when it’s invalid.

Copilot uses AI. Check for mistakes.
checked = 0

School.find_each do |school|
Salesforce::ContactSyncJob.perform_later(school_id: school.id)
checked += 1
if Salesforce::Contact.exists?(pi_accounts_unique_id__c: school.creator_id)
Salesforce::ContactSyncJob.perform_later(school_id: school.id)
enqueued += 1
break if limit && enqueued >= limit
end
Comment on lines +66 to +70
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

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

This does Salesforce::Contact.exists? for every School, which is an N+1 query pattern. Consider batching by collecting creator_ids for each find_each batch and querying existing Contacts in one call, then enqueue only for those creator_ids that exist.

Copilot uses AI. Check for mistakes.
end

puts "Checked #{checked} schools, enqueued #{enqueued} contact sync jobs"
end
end
Loading