Skip to content

Commit bd7e298

Browse files
mwvoloCopilotCopilot
authored
Fix salesforce sync (#1686)
* create a new lead if one is not found for the user * do not overwrite faculty status if confirmed * Update app/routines/update_user_contact_info.rb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update app/routines/update_user_contact_info.rb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Protect rejected_faculty status from Salesforce sync overwrites (#1690) * Initial plan * Add protection for rejected_faculty status from being overwritten Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Fix indentation in create_or_update_salesforce_lead.rb (#1689) * Initial plan * Fix indentation in create_or_update_salesforce_lead.rb Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Use salesforce_lead_id directly for lead lookup with email fallback (#1687) * Initial plan * Use salesforce_lead_id directly when it exists with fallback to email search Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Improve exception handling and error logging Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Add salesforce_lead_not_found_by_id event type to SecurityLog Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Fix formatting: remove trailing whitespace Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Remove additional trailing whitespace Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Fix type mismatch in rejected_faculty status protection logic and update VCR cassette (#1691) * Initial plan * Fix type mismatch in rejected_faculty status comparison Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Update VCR cassette for Lead.find_by email query Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Add test coverage and fix symbol/string bug in UpdateUserContactInfo faculty status preservation (#1688) * Initial plan * Add comprehensive test coverage for UpdateUserContactInfo faculty status preservation logic Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Simplify faculty status validation using VALID_FACULTY_STATUSES constant Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> * Fix SecurityLog test by setting salesforce_contact_id on test user Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com> Co-authored-by: Michael Volo <volo@rice.edu> * remove extra lead assignment/search calls * user User model for VALID_FACULTY_STATUS * rejected faculty can't go to incomplete or no_info * move salesforce spec helpers to spec/support * clean up some error-prone specs * search leads by email and uuid, then verify no contact before creating the lead * test lead search by email and uuid * remove database cache * improved migration check login, fix flaky test on redirect page --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: mwvolo <3905516+mwvolo@users.noreply.github.com>
1 parent 609f0c2 commit bd7e298

File tree

13 files changed

+662
-149
lines changed

13 files changed

+662
-149
lines changed

‎.github/workflows/migrations.yml‎

Lines changed: 23 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -2,25 +2,19 @@ name: Migrations
22

33
on:
44
pull_request:
5-
types:
6-
- opened
7-
- edited
8-
- synchronize
9-
- reopened
5+
types: [opened, synchronize, reopened]
6+
paths:
7+
- 'db/migrate/**'
8+
- 'db/schema.rb'
109

11-
# Add concurrency to cancel outdated workflow runs
1210
concurrency:
1311
group: ${{ github.workflow }}-${{ github.ref }}
1412
cancel-in-progress: true
1513

1614
jobs:
1715
migrations:
18-
timeout-minutes: 30
16+
timeout-minutes: 15
1917
runs-on: ubuntu-latest
20-
permissions:
21-
contents: read
22-
pull-requests: write
23-
issues: write
2418
services:
2519
postgres:
2620
image: postgres:11
@@ -30,93 +24,42 @@ jobs:
3024
env:
3125
POSTGRES_USER: postgres
3226
POSTGRES_PASSWORD: postgres
33-
redis:
34-
image: redis
35-
ports:
36-
- 6379:6379
37-
# Set health checks to wait until redis has started
38-
options: >-
39-
--health-cmd "redis-cli ping"
40-
--health-interval 10s
41-
--health-timeout 5s
42-
--health-retries 5
4327

4428
steps:
45-
# Clone repo and checkout merge commit parent (PR target commit)
4629
- uses: actions/checkout@v6
4730
with:
48-
fetch-depth: 2
49-
50-
- run: git checkout HEAD^
31+
fetch-depth: 0
5132

52-
# Install base commit ruby version
5333
- name: Set up Ruby
5434
uses: ruby/setup-ruby@v1
5535
with:
5636
bundler-cache: true
5737

58-
# Create data to be migrated and revert to PR merge commit
59-
- name: Cache database setup
60-
uses: actions/cache@v5
61-
with:
62-
path: |
63-
tmp/cache
64-
db/schema.rb
65-
key: db-setup-${{ github.sha }}
66-
restore-keys: |
67-
db-setup-
68-
69-
- name: Create data to be migrated
38+
# Load the base branch schema to simulate an existing production database
39+
- name: Load base schema
7040
env:
7141
OXA_DB_USER: postgres
7242
OXA_DB_PASS: postgres
73-
PGPASSWORD: postgres
7443
RAILS_ENV: test
7544
run: |
76-
bin/rake db:create db:schema:load db:seed --trace
77-
bin/rails runner '3.times { FactoryBot.create :user }'
78-
# Export data for integrity check
79-
pg_dump -U postgres -h localhost -F c -f pre_migration.dump ox_accounts_test
80-
git checkout --force -
45+
git show origin/${{ github.base_ref }}:db/schema.rb > db/schema_base.rb
46+
cp db/schema_base.rb db/schema.rb
47+
bin/rake db:create db:schema:load
48+
git checkout -- db/schema.rb
8149
82-
# Install PR ruby version
83-
- name: Set up Ruby (PR version)
84-
uses: ruby/setup-ruby@v1
85-
with:
86-
bundler-cache: true
87-
88-
# Migrate the data
89-
- name: Migrate and verify
50+
# Run only the new migrations from this PR
51+
- name: Run migrations
9052
env:
9153
OXA_DB_USER: postgres
9254
OXA_DB_PASS: postgres
93-
PGPASSWORD: postgres
9455
RAILS_ENV: test
95-
run: |
96-
# Run migrations
97-
bin/rake db:migrate
98-
99-
# Verify data integrity
100-
bin/rails runner '
101-
begin
102-
User.count == 3 or raise "User count mismatch"
103-
puts "✅ Data integrity check passed"
104-
rescue => e
105-
puts "❌ Data integrity check failed: #{e.message}"
106-
exit 1
107-
end
108-
'
109-
110-
# Export post-migration data
111-
pg_dump -U postgres -h localhost -F c -f post_migration.dump ox_accounts_test
56+
run: bin/rake db:migrate
11257

113-
- name: Upload migration artifacts
114-
if: always()
115-
uses: actions/upload-artifact@v6
116-
with:
117-
name: migration-artifacts
118-
path: |
119-
pre_migration.dump
120-
post_migration.dump
121-
log/development.log
122-
retention-days: 7
58+
# Verify the resulting schema matches what's committed
59+
- name: Verify schema
60+
run: |
61+
if ! git diff --exit-code db/schema.rb; then
62+
echo "❌ db/schema.rb is out of date. Run migrations locally and commit the updated schema."
63+
exit 1
64+
fi
65+
echo "✅ Schema is up to date"

‎app/models/security_log.rb‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,11 @@ class SecurityLog < ApplicationRecord
107107
attempted_to_add_school_not_cached_yet
108108
school_added_to_user_from_sheerid_webhook
109109
user_lead_id_updated_from_salesforce
110+
salesforce_lead_not_found_by_id
111+
salesforce_lead_found_by_uuid
112+
salesforce_lead_found_by_email
113+
user_already_has_contact_not_creating_lead
114+
creating_new_salesforce_lead
110115
]
111116

112117
json_serialize :event_data, Hash

‎app/routines/newflow/create_or_update_salesforce_lead.rb‎

Lines changed: 100 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -60,46 +60,113 @@ def exec(user:)
6060
end
6161

6262

63+
lead = nil
6364
if user.salesforce_lead_id
65+
begin
66+
lead = OpenStax::Salesforce::Remote::Lead.find(user.salesforce_lead_id)
67+
rescue StandardError => e
68+
# Log when the stored lead ID doesn't correspond to an existing lead or find fails
69+
SecurityLog.create!(
70+
user: user,
71+
event_type: :salesforce_lead_not_found_by_id,
72+
event_data: {
73+
salesforce_lead_id: user.salesforce_lead_id,
74+
error: e.class.name,
75+
error_message: e.message
76+
}
77+
)
78+
Sentry.capture_message(
79+
"Salesforce lead ID #{user.salesforce_lead_id} not found for user #{user.id}, will search by UUID and email. Error: #{e.class.name}: #{e.message}"
80+
)
81+
end
82+
end
83+
84+
# If no lead found by stored ID, search for existing lead by UUID
85+
if lead.nil?
86+
lead = OpenStax::Salesforce::Remote::Lead.find_by(accounts_uuid: user.uuid)
87+
if lead
88+
SecurityLog.create!(
89+
user: user,
90+
event_type: :salesforce_lead_found_by_uuid,
91+
event_data: { lead_id: lead.id }
92+
)
93+
end
94+
end
95+
96+
# If still no lead found, search by email
97+
if lead.nil?
6498
lead = OpenStax::Salesforce::Remote::Lead.find_by(email: user.best_email_address_for_salesforce)
65-
else
66-
lead = OpenStax::Salesforce::Remote::Lead.new(email: user.best_email_address_for_salesforce)
99+
if lead
100+
SecurityLog.create!(
101+
user: user,
102+
event_type: :salesforce_lead_found_by_email,
103+
event_data: { lead_id: lead.id, email: user.best_email_address_for_salesforce }
104+
)
105+
end
67106
end
68107

108+
# If user has a contact (already converted from lead), don't create a new lead
109+
if lead.nil? && user.salesforce_contact_id.present?
110+
begin
111+
contact = OpenStax::Salesforce::Remote::Contact.find(user.salesforce_contact_id)
112+
if contact
113+
SecurityLog.create!(
114+
user: user,
115+
event_type: :user_already_has_contact_not_creating_lead,
116+
event_data: { contact_id: user.salesforce_contact_id }
117+
)
118+
# User already has a contact, return without creating a lead
119+
outputs.lead = nil
120+
outputs.user = user
121+
return
122+
end
123+
rescue StandardError => e
124+
# Contact not found, proceed with lead creation
125+
Sentry.capture_message(
126+
"Salesforce contact ID #{user.salesforce_contact_id} not found for user #{user.id}, will create lead. Error: #{e.class.name}: #{e.message}"
127+
)
128+
end
129+
end
130+
131+
# Only create a new lead if none exists
69132
if lead.nil?
70-
Sentry.capture_message("Lead for user not found #{user.uuid} not found", level: :error)
71-
return
133+
lead = OpenStax::Salesforce::Remote::Lead.new(email: user.best_email_address_for_salesforce)
134+
SecurityLog.create!(
135+
user: user,
136+
event_type: :creating_new_salesforce_lead,
137+
event_data: { email: user.best_email_address_for_salesforce, uuid: user.uuid }
138+
)
72139
end
73140

74-
lead.first_name = user.first_name
75-
lead.last_name = user.last_name
76-
lead.phone = user.phone_number
77-
lead.source = LEAD_SOURCE
78-
lead.application_source = DEFAULT_REFERRING_APP_NAME
79-
lead.role = sf_role
80-
lead.position = sf_position
81-
lead.title = user.other_role_name
82-
lead.who_chooses_books = user.who_chooses_books
83-
lead.subject_interest = user.which_books
84-
lead.num_students = user.how_many_students
85-
lead.adoption_status = ADOPTION_STATUS_FROM_USER[user.using_openstax_how]
86-
lead.adoption_json = adoption_json
87-
lead.os_accounts_id = user.id
88-
lead.accounts_uuid = user.uuid
89-
lead.school = user.most_accurate_school_name
90-
lead.city = user.most_accurate_school_city
91-
lead.country = user.most_accurate_school_country
92-
lead.verification_status = user.faculty_status == User::NO_FACULTY_INFO ? nil : user.faculty_status
93-
lead.b_r_i_marketing = user.is_b_r_i_user?
94-
lead.title_1_school = user.title_1_school?
95-
lead.newsletter = user.receive_newsletter?
96-
lead.newsletter_opt_in = user.receive_newsletter?
97-
lead.self_reported_school = user.self_reported_school
98-
lead.sheerid_school_name = user.sheerid_reported_school
99-
lead.account_id = sf_school_id
100-
lead.school_id = sf_school_id
101-
lead.signup_date = user.created_at.strftime("%Y-%m-%dT%T.%L%z")
102-
lead.tracking_parameters = "#{Rails.application.secrets.openstax_url}/accounts/i/signup/"
141+
lead.first_name = user.first_name
142+
lead.last_name = user.last_name
143+
lead.phone = user.phone_number
144+
lead.source = LEAD_SOURCE
145+
lead.application_source = DEFAULT_REFERRING_APP_NAME
146+
lead.role = sf_role
147+
lead.position = sf_position
148+
lead.title = user.other_role_name
149+
lead.who_chooses_books = user.who_chooses_books
150+
lead.subject_interest = user.which_books
151+
lead.num_students = user.how_many_students
152+
lead.adoption_status = ADOPTION_STATUS_FROM_USER[user.using_openstax_how]
153+
lead.adoption_json = adoption_json
154+
lead.os_accounts_id = user.id
155+
lead.accounts_uuid = user.uuid
156+
lead.school = user.most_accurate_school_name
157+
lead.city = user.most_accurate_school_city
158+
lead.country = user.most_accurate_school_country
159+
lead.verification_status = user.faculty_status == User::NO_FACULTY_INFO ? nil : user.faculty_status
160+
lead.b_r_i_marketing = user.is_b_r_i_user?
161+
lead.title_1_school = user.title_1_school?
162+
lead.newsletter = user.receive_newsletter?
163+
lead.newsletter_opt_in = user.receive_newsletter?
164+
lead.self_reported_school = user.self_reported_school
165+
lead.sheerid_school_name = user.sheerid_reported_school
166+
lead.account_id = sf_school_id
167+
lead.school_id = sf_school_id
168+
lead.signup_date = user.created_at.strftime("%Y-%m-%dT%T.%L%z")
169+
lead.tracking_parameters = "#{Rails.application.secrets.openstax_url}/accounts/i/signup/"
103170

104171
state = user.most_accurate_school_state
105172
unless state.blank?

‎app/routines/update_user_contact_info.rb‎

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
class UpdateUserContactInfo
2+
class UnknownFacultyVerifiedError < StandardError; end
3+
24
COLLEGE_TYPES = [
35
'College/University (4)',
46
'Technical/Community College (2)',
@@ -52,30 +54,40 @@ def call
5254
SecurityLog.create!(
5355
user: user,
5456
event_type: :user_contact_id_updated_from_salesforce,
55-
event_data: { previous_contact_id: previous_contact_id,new_contact_id: sf_contact.id }
57+
event_data: { previous_contact_id: previous_contact_id, new_contact_id: sf_contact.id }
5658
)
5759
end
5860

5961
old_fv_status = user.faculty_status
60-
user.faculty_status = case sf_contact.faculty_verified
61-
when "confirmed_faculty"
62-
:confirmed_faculty
63-
when "pending_faculty"
64-
:pending_faculty
65-
when "rejected_faculty"
66-
:rejected_faculty
67-
when "rejected_by_sheerid"
68-
:rejected_by_sheerid
69-
when "incomplete_signup"
70-
:incomplete_signup
71-
when "no_faculty_info"
72-
:no_faculty_info
73-
when NilClass
74-
:no_faculty_info
75-
else
76-
Sentry.capture_message("Unknown faculty_verified field: '#{
77-
sf_contact.faculty_verified}'' on contact #{sf_contact.id}")
78-
end
62+
# Map Salesforce faculty_verified values to our string-based enum values
63+
# nil maps to no_faculty_info; unknown values raise an error
64+
faculty_verified = sf_contact.faculty_verified
65+
new_status = if faculty_verified.nil?
66+
"no_faculty_info"
67+
elsif User::VALID_FACULTY_STATUSES.include?(faculty_verified)
68+
faculty_verified
69+
else
70+
message = "Unknown faculty_verified field: '#{faculty_verified}' on contact #{sf_contact.id}"
71+
Sentry.capture_message(message)
72+
raise UnknownFacultyVerifiedError, message
73+
end
74+
75+
# Don't overwrite confirmed or pending faculty status with incomplete/no_info
76+
# Don't overwrite confirmed with pending
77+
# Don't overwrite rejected_faculty with incomplete/no_info
78+
should_update_status = true
79+
if user.faculty_status == "confirmed_faculty" &&
80+
["pending_faculty", "incomplete_signup", "no_faculty_info"].include?(new_status)
81+
should_update_status = false
82+
elsif user.faculty_status == "pending_faculty" &&
83+
["incomplete_signup", "no_faculty_info"].include?(new_status)
84+
should_update_status = false
85+
elsif user.faculty_status == "rejected_faculty" &&
86+
["incomplete_signup", "no_faculty_info"].include?(new_status)
87+
should_update_status = false
88+
end
89+
90+
user.faculty_status = new_status if should_update_status
7991

8092
if user.faculty_status_changed?
8193
users_fv_status_changed += 1

‎lib/contracts_not_required.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ def arriving_from_app_that_skips_terms?(client_id)
4242
end
4343

4444
def all_user_apps_skip_terms?
45+
return false if current_user.nil?
4546
current_user.applications.any? &&
4647
current_user.applications.all?{|app| app.skip_terms? }
4748
end

‎lib/user_session_management.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ def sign_out!(options={})
6969
end
7070

7171
def signed_in?
72+
return false if current_user.nil?
7273
!current_user.is_anonymous?
7374
end
7475

0 commit comments

Comments
 (0)