Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion app/jobs/salesforce/contact_sync_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def perform(school_id:)
sf_contact = Salesforce::Contact.find_by(pi_accounts_unique_id__c: school.creator_id)
raise SalesforceRecordNotFound, "Contact not found for creator_id: #{school.creator_id}" unless sf_contact

sf_contact.editoragreetouxcontact__c = school.creator_agree_to_ux_contact
sf_contact.editor_consent_to_ux_contact__c = school.creator_agree_to_ux_contact
sf_contact.save!
end

Expand Down
7 changes: 7 additions & 0 deletions app/jobs/salesforce/role_sync_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ def perform(role_id:)

sf_role = Salesforce::Role.find_or_initialize_by(affiliation_id__c: role_id)
sf_role.attributes = sf_role_attributes(role:)

# We also have a field on the Contact_Editor_Affiliation in Salesforce
# called Editor_Type__c - this is mapped to the value of role.school.user_origin
# If, for any reason, we can't get that, we fall back to the School model's default
# value for user_origin. ::School.new never persists to the DB.
sf_role.editor_type__c = role.school&.user_origin || ::School.new.user_origin

sf_role.save!
end

Expand Down
8 changes: 5 additions & 3 deletions app/jobs/salesforce/school_sync_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,34 @@ class SchoolSyncJob < SalesforceSyncJob
addressline2__c: :address_line_2,
editormunicipality__c: :municipality,
editoradministrativearea__c: :administrative_area,
editortype__c: :user_origin,
postcode__c: :postal_code,
countrycode__c: :country_code,
verifiedat__c: :verified_at,
createdat__c: :created_at,
updatedat__c: :updated_at,
rejectedat__c: :rejected_at,
website__c: :website,
userorigin__c: :user_origin,
districtnamesupplied__c: :district_name,
ncesid__c: :district_nces_id,
schoolrollnumber__c: :school_roll_number
}.freeze

def perform(school_id:)
def perform(school_id:, is_create: false)
school = ::School.find(school_id)

sf_school = Salesforce::School.find_or_initialize_by(editoruuid__c: school_id)
sf_school.attributes = sf_school_attributes(school:)
sf_school.status__c = 'New' if is_create

sf_school.save!
end

private

def sf_school_attributes(school:)
mapped_attributes(school:).to_h do |sf_field, value|
value = 'for_education' if sf_field == :userorigin__c && value.nil?
value = ::School.new.user_origin if sf_field == :editortype__c && value.nil?
value = truncate_value(sf_field:, value:) if value.is_a?(String)

[sf_field, value]
Expand Down
7 changes: 4 additions & 3 deletions app/models/school.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ class School < ApplicationRecord

after_create :generate_code!

after_commit :do_salesforce_sync, on: %i[create update], if: -> { FeatureFlags.salesforce_sync? }
after_commit -> { do_salesforce_sync(is_create: true) }, on: :create, if: -> { FeatureFlags.salesforce_sync? }
after_commit -> { do_salesforce_sync(is_create: false) }, on: :update, if: -> { FeatureFlags.salesforce_sync? }
Comment on lines 56 to +59
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

after_create :generate_code! performs an update!, so creating a School will include both :create and :update actions in the same transaction. With separate after_commit callbacks for on: :create and on: :update, this can enqueue Salesforce sync jobs twice on initial creation (once with is_create: true and again with is_create: false). Consider consolidating to a single callback that computes is_create (or adding a guard so the update callback doesn’t run for the initial code-generation update).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't believe this is correct. I have a passing test in school_spec.rb named "enqueues Salesforce::SchoolSyncJob exactly once on create, with is_create: true'.


def self.find_for_user!(user)
school = Role.find_by(user_id: user.id)&.school || find_by(creator_id: user.id, rejected_at: nil)
Expand Down Expand Up @@ -169,8 +170,8 @@ def format_uk_postal_code
self.postal_code = "#{cleaned_postal_code[0..-4]} #{cleaned_postal_code[-3..]}"
end

def do_salesforce_sync
Salesforce::SchoolSyncJob.perform_later(school_id: id)
def do_salesforce_sync(is_create:)
Salesforce::SchoolSyncJob.perform_later(school_id: id, is_create:)
Salesforce::ContactSyncJob.perform_later(school_id: id)
end
end
10 changes: 5 additions & 5 deletions spec/jobs/salesforce/contact_sync_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
ClimateControl.modify(SALESFORCE_ENABLED: 'true') { example.run }
end

it 'sets editoragreetouxcontact__c from school.creator_agree_to_ux_contact' do
it 'sets editor_consent_to_ux_contact__c from school.creator_agree_to_ux_contact' do
perform_job
expect(sf_contact.reload.editoragreetouxcontact__c).to be(true)
expect(sf_contact.reload.editor_consent_to_ux_contact__c).to be(true)
end

it 'saves the contact' do
Expand All @@ -36,7 +36,7 @@
allow(Salesforce::Contact).to receive(:find_by)
.with(pi_accounts_unique_id__c: school.creator_id)
.and_return(sf_contact_double)
allow(sf_contact_double).to receive(:editoragreetouxcontact__c=)
allow(sf_contact_double).to receive(:editor_consent_to_ux_contact__c=)
allow(sf_contact_double).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
end

Expand All @@ -53,9 +53,9 @@
end

it 'discards the job without syncing' do
sf_contact.update!(editoragreetouxcontact__c: false)
sf_contact.update!(editor_consent_to_ux_contact__c: false)
perform_job
expect(sf_contact.reload.editoragreetouxcontact__c).to be(false)
expect(sf_contact.reload.editor_consent_to_ux_contact__c).to be(false)
end
end

Expand Down
6 changes: 6 additions & 0 deletions spec/jobs/salesforce/role_sync_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
"Expected #{sf_field} to equal role.#{role_field}"
end
end

it 'sets editor_type__c from the school user_origin' do
sf_role = Salesforce::Role.find_by(affiliation_id__c: role.id)
expect(sf_role.editor_type__c).to eq(role.school.user_origin)
end
end

context 'when the Salesforce role fails to save' do
Expand All @@ -30,6 +35,7 @@
before do
allow(Salesforce::Role).to receive(:find_or_initialize_by).with(affiliation_id__c: role.id).and_return(sf_role)
allow(sf_role).to receive(:attributes=)
allow(sf_role).to receive(:editor_type__c=)
allow(sf_role).to receive(:save!).and_raise(ActiveRecord::RecordInvalid)
end

Expand Down
15 changes: 15 additions & 0 deletions spec/jobs/salesforce/school_sync_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
subject(:perform_job) { described_class.perform_now(school_id: school.id) }

let(:school) { create(:school) }
let(:perform_job_as_create) { described_class.perform_now(school_id: school.id, is_create: true) }

around do |example|
ClimateControl.modify(SALESFORCE_ENABLED: 'true') { example.run }
Expand All @@ -23,6 +24,11 @@
end
end

it 'does not set status__c when is_create is false' do
sf_school = Salesforce::School.find_by(editoruuid__c: school.id)
expect(sf_school.status__c).to be_nil
end

context 'when an address field is very long' do
let(:school) { create(:school, address_line_1: '❌' * 300) }

Expand Down Expand Up @@ -52,6 +58,15 @@
end
end

context 'when is_create is true' do
before { perform_job_as_create }

it 'sets status__c to "New"' do
sf_school = Salesforce::School.find_by(editoruuid__c: school.id)
expect(sf_school.status__c).to eq('New')
end
end

context 'when the Salesforce school fails to save' do
let(:sf_school) { instance_double(Salesforce::School) }

Expand Down
9 changes: 6 additions & 3 deletions spec/models/school_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,11 @@
ClimateControl.modify(SALESFORCE_ENABLED: 'true') { example.run }
end

it 'enqueues Salesforce::SchoolSyncJob on create' do
expect { create(:school) }.to have_enqueued_job(Salesforce::SchoolSyncJob)
it 'enqueues Salesforce::SchoolSyncJob exactly once on create, with is_create: true' do
expect { create(:school) }
.to have_enqueued_job(Salesforce::SchoolSyncJob)
.exactly(1).times
.with(hash_including(is_create: true))
end

it 'enqueues Salesforce::ContactSyncJob on create' do
Expand All @@ -629,7 +632,7 @@

it 'enqueues Salesforce::SchoolSyncJob on update' do
school = create(:school)
expect { school.update!(name: 'Updated Name') }.to have_enqueued_job(Salesforce::SchoolSyncJob)
expect { school.update!(name: 'Updated Name') }.to have_enqueued_job(Salesforce::SchoolSyncJob).with(hash_including(is_create: false))
end

it 'enqueues Salesforce::ContactSyncJob on update' do
Expand Down
Loading