Salesforce sync redesign#1714
Open
mwvolo wants to merge 34 commits into
Open
Conversation
…Task 6) Drops the initializer and the rails_helper require/include that pointed at the now-removed openstax_salesforce gem. Required so the app boots and specs load while the new Salesforce module is being introduced.
Reopens ActiveForce::SObject with the find_or_initialize_by and save_if_changed helpers from the openstax_salesforce gem we removed, and aliases Salesforce::Records::Base to it. An intermediate Base subclass fought SObject's `inherited` hook (which auto-adds field :id and would double-register on grandchildren), so the alias approach mirrors how the original gem layered helpers on SObject. ActiveForce.sfdc_client is patched to lazily build Salesforce::Client on first use, keeping Rails boot and migrations safe when SF secrets aren't configured. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Field mappings absorbed from openstax_salesforce, with the new fields the redesign needs: - Lead: is_converted, converted_contact_id (for tracking lead-to-contact conversion in Reconcile Pass 2) - Contact: master_record_id, is_deleted (for skipping merged/deleted contacts in SyncContacts) The unused remote model classes from the gem (Book, Opportunity, Campaign, CampaignMember, AccountContactRelation, OpenstaxAccount, RecordType, TermYear, TutorCoursePeriod) are intentionally not ported — this app only uses Lead, Contact, and School. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the old openstax_salesforce initializer (deleted in Task 2) with one that calls Salesforce.configure from app secrets. The OpenStax inflection acronym used to be registered by the gem's engine; register it here now that the gem is gone, so any remaining OpenStax::-namespaced constants in the app continue to resolve cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cords::*
Mechanical rename across app/, lib/, and spec/ now that the new
Salesforce::Records::{Lead,Contact,School} are in place. The four
salesforce-touching specs that exercise these (create_or_update_salesforce_lead,
update_user_contact_info, update_school_salesforce_info,
update_salesforce_assignable_fields) all pass — 46 examples, 0 failures.
spec/features/admin/change_salesforce_contact_manually_spec.rb fails
with `uninitialized constant SalesforceProxy`, but that was already
failing on the baseline (verified by stashing the rename and re-running);
unrelated to this PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Appended (not inserted — Rails enums are positional) at the end of the event_type enum. Covers the audit taxonomy used by Salesforce::Audit, Salesforce::Lookup, Salesforce::UpsertLead, Salesforce::SyncContacts, and Salesforce::Reconcile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Audit.record(user, :event_name, **details) prepends "salesforce_" to form the SecurityLog#event_type and validates against the enum so a typo at a call site fails loudly at test time, not silently in production. event_data carries the details hash as-is. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds 12 new Settings::Db.store fields covering: - SyncContacts cursor + lookback overlap - Reconcile budget + per-pass cursors - Per-run alert thresholds (lead save failure, swap/conflict rates, unknown UUIDs, drift open total, cron drift) - SF-admin notification toggle - Reconcile self-heal feature flag Wrappers on Settings::Salesforce and Settings::FeatureFlags keep call sites readable and isolate the underlying rails-settings-cached storage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-run counter bag used by SyncContacts, SyncSchools, Reconcile, and UpsertLead. Integer counters and labeled sub-counters with a :total. #emit writes a tagged JSON log line and (when a slug is configured) closes a Sentry check-in. #alert! produces a Sentry message tagged salesforce-alert=<name> so existing tag-based alert rules can subscribe. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lead_owns_user? — true when UUID matches or is blank (adoptable). contact_owns_user? — same, but a contact never owns when merged or deleted. contact_can_be_replaced? — returns :gone, :merged, :uuid_cleared, or false. Used by SyncContacts to gate every contact_id swap on evidence rather than blindly trusting the incremental sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves a Lead (or Contact) for a User by trying the stored salesforce_lead_id first (best signal), then accounts_uuid (strongest match), then email with a UUID-collision guard so we don't claim a lead that belongs to another user. Returns a Result struct carrying :lead, :matched_by, and :rejected reasons for downstream audit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulls the faculty-status decision logic out of CreateOrUpdateSalesforceLead (lines 52-63) and UpdateUserContactInfo (lines 75-88) into one place, with two entry points: - from_signup(user): sets status based on profile completion + SheerID; persists the user. - from_contact(user, sf_contact): applies the SF-side faculty_verified value, respecting the protection rules — confirmed/pending/rejected can't be downgraded to incomplete/no_info, and confirmed can't be rolled back to pending. Doesn't persist; caller decides. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Side-effect-free mapping pulled out of CreateOrUpdateSalesforceLead so it can be unit-tested table-driven. Always writes accounts_uuid before return, which is the invariant that makes UpsertLead's job-retry path idempotent: a retried job that previously got as far as lead.save (then died) finds the just-created Lead via the UUID branch of Lookup, instead of creating a duplicate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single entry point for lead create-or-update. Sequence: 1. Audit begin. 2. Ensure user.school is set (falls back to "Find Me A Home"). 3. ResolveFacultyStatus.from_signup updates the user. 4. Lookup.lead_for resolves any existing lead via stored_id, uuid, email. 5. If no lead AND the user already has a contact that owns them (Lookup.contact_for verifies), return early — they've been converted. If the stored contact id is stale (no longer owns them), clear it and audit before proceeding. 6. Build new lead (with accounts_uuid set for retry idempotency) or update found lead via BuildLead.apply. 7. Save SF. On success, persist_lead_id retries user.save up to 3 times, with each retry audited. After 3 failures, log to Sentry; the next nightly Reconcile pass picks up the orphan via accounts_uuid. Replaces the inline lookup/build/save logic in the previous routine, which silently overwrote user.salesforce_contact_id and didn't retry on local persist failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the 259-line routine with a ~15-line lev_routine that
delegates to Salesforce::UpsertLead. The lev_routine wrapper preserves
the active_job_enqueue_options: { queue: :salesforce } so callers
(EducatorSignup::CompleteProfile, EducatorSignup::SheeridWebhook,
Admin::UsersController#force_update_lead) don't change.
Updates the existing spec to match the new event taxonomy:
- :creating_new_salesforce_lead -> :salesforce_upsert_lead_saved
- :salesforce_lead_found_by_uuid -> :salesforce_lookup_matched_by_uuid
- :salesforce_lead_found_by_email -> :salesforce_lookup_matched_by_email
- :user_already_has_contact_not_creating_lead -> :salesforce_upsert_lead_skipped_user_has_contact
- :salesforce_lead_save_failed -> :salesforce_upsert_lead_save_failed
Adds a stale-contact-id-cleared case that the old code couldn't handle.
All 13 examples pass; the 184-example sweep across all Salesforce-touching
specs is green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces UpdateUserContactInfo's body. Key behavior changes vs the old routine: - Cursor (Settings::Salesforce.contacts_synced_through) instead of fixed N-day lookback, with a configurable hour-of-overlap so a skipped cron run doesn't lose modifications. - Skips Contacts where master_record_id.present? or is_deleted at fetch time, instead of blindly attaching them to users. - Gates every salesforce_contact_id swap on Verify.contact_can_be_replaced? — evidence-based (:gone, :merged, :uuid_cleared) or no swap. Two-live-contact conflicts are logged and flagged for human review. - Faculty status flows through Salesforce::ResolveFacultyStatus (preserving the existing protection rules). - Per-run threshold alerts (cron drift, conflict count, swap rate, unknown UUID count) fire as tagged Sentry messages. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the 177-line routine with a 12-line shim that delegates to Salesforce::SyncContacts. The cron task (lib/tasks/cron/5-past-half-hour.rake) still calls UpdateUserContactInfo.call, so no cron changes required. UnknownFacultyVerifiedError is re-exported from Salesforce::ResolveFacultyStatus for any caller that referenced it. Update stub_salesforce_contacts (spec/support) to stub Salesforce::SyncContacts#fetch_contacts instead of the now-removed UpdateUserContactInfo#salesforce_contacts. The existing 29-example spec, which exercises faculty-status protection rules end-to-end, passes unchanged against the new flow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the body of UpdateSchoolSalesforceInfo verbatim into Salesforce::SyncSchools and wrap with Salesforce::Metrics so the run shows up as a Sentry check-in alongside the other sync routines. UpdateSchoolSalesforceInfo becomes a shim that re-exports BATCH_SIZE and SF_TO_DB_CACHE_COLUMNS_MAP so the existing 4-example spec keeps passing unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sforce_lead_id salesforce_drift_findings persists Reconcile findings — accounts-side issues we self-heal (or surface), and SF-side issues we can only report. The (category, resolved_at) and (user_id, category) indexes support the admin filter and the per-user lookup. last_seen_at is indexed so finalize_findings (Task 26) can close findings not seen in the current run. The concurrent index on users.salesforce_lead_id covers Reconcile Pass 2 (WHERE id IN (...)) and the stored-id lookup path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Backs salesforce_drift_findings. Open/resolved scopes; upsert_finding! bumps last_seen_at on an existing open match or creates a new row; resolve! sets resolved_at. The "create new when prior was resolved" behavior lets a finding reopen if it returns after being marked resolved (useful for cases where an SF-side fix didn't actually fix it). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Nightly drift detection + Accounts-side self-heal. Combines what the
plan separates into Tasks 23-26 into one cohesive file because the
passes share helpers (finding/upsert, heal_*, budget tracking,
metrics, query counting):
- run_pass_1: anchor on users with stored salesforce_contact_id.
Verify the SF Contact is alive and owns this user; heal merges
(follow MasterRecordId), deletes (clear + re-resolve via Lookup),
and disowned UUIDs. Open findings for SF-side issues.
- run_pass_2: anchor on users with stored salesforce_lead_id (no
contact). Attach the converted Contact when the Lead has
IsConverted=true and the resulting Contact owns the user. Heal
disowned/missing Leads.
- run_pass_3: discover missing links for profile-complete instructors
with no stored ids, by looking them up in SF by accounts_uuid.
Prefer Contact over Lead. Open user_unlinked_eligible findings when
nothing matches.
- sweep_sf_orphans: scan SF Leads/Contacts modified in last 90 days
whose accounts_uuid we don't recognize. Open sf_orphan_{contact,lead}
findings.
- finalize_findings: close findings not refreshed during this run
(last_seen_at < cutoff), prune resolved findings older than 60 days,
fire drift_findings_total_open alert when over threshold.
All self-heal writes gated by
Settings::FeatureFlags.salesforce_reconcile_self_heal (default false)
so the first production deploy is read-only.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Adds Salesforce::Reconcile.call to lib/tasks/cron/day.rake, alongside the existing UpdateSalesforceAssignableFields call. - Removes app/routines/update_user_lead_info.rb. It was unscheduled (not in any cron task), and its only reference was a comment in create_leads_for_instructors_not_sent_to_sf.rake which I've now pointed at Reconcile instead. After this commit the new sync architecture is fully wired: - Every 30 min: SyncSchools, SyncContacts (via shims) - Nightly: Salesforce::Reconcile + the existing daily jobs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Admin surface for the salesforce_drift_findings table: - /admin/salesforce_drift_findings lists open findings sorted by last_seen_at desc, with filter by category and (when linked from the user admin page) by user_id. - Each row links to the user admin and to the Salesforce record URL (built from Rails.application.secrets.salesforce.instance_url when configured). - "Mark resolved" button sets resolved_at; the next nightly Reconcile will reopen it if the underlying drift is still present. No bulk actions, no SF mutations — admins fix in SF, Reconcile picks it up next night. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When admins open /admin/users/:id/edit they now see a Salesforce
timeline section listing every SecurityLog entry with event_type
LIKE 'salesforce_%' for that user, oldest first (so the order matches
the actual chronology of what happened). 500-row cap to keep the page
bounded.
Also adds a link to /admin/salesforce_drift_findings?user_id=:id so
anyone investigating a single user can jump directly to that user's
open drift findings.
This is the per-user audit trail surface called out in the design
spec ("reconstruct the SF state of one user from logs"). Existing
3-example users_controller_spec still passes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds two helpers to the existing SalesforceSpecHelpers module: - stub_salesforce_records!: replaces ActiveForce.sfdc_client with an in-memory no-op so unit specs never accidentally talk to the SF sandbox. - limit_salesforce_queries / limit_salesforce_queries_by_token: absorbed from the openstax_salesforce gem's spec helpers; useful in sandbox-backed VCR specs where you want to ignore unrelated rows. Also adds Sentry.capture_exception to stub_sentry so the new routines' rescue StandardError => e; Sentry.capture_exception(e) paths don't trip on a missing stub. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes the Accounts ↔ Salesforce integration by replacing the legacy openstax_salesforce gem with a restforce + openstax_active_force-based architecture, introducing new Salesforce service objects (lookup/upsert/sync/reconcile), and adding drift-finding tracking + admin tooling for visibility and remediation.
Changes:
- Replace legacy Salesforce remote record access with
Salesforce::Records::*(ActiveForce) +Salesforce::Client(Restforce) and new service classes (UpsertLead,SyncContacts,SyncSchools,Reconcile). - Add
SalesforceDriftFindingmodel + admin UI/controller for listing/filtering/resolving drift findings; add a Salesforce-specific audit timeline on the admin user edit page. - Expand Salesforce-related
SecurityLogevent types, add Salesforce settings/feature flags, and add/adjust cron + rake tasks and test coverage.
Reviewed changes
Copilot reviewed 68 out of 69 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/salesforce_spec_helpers.rb | Updates/extends Salesforce test helpers for the new ActiveForce-based records and sync services. |
| spec/services/salesforce/verify_spec.rb | Adds unit tests for Lead/Contact ownership and replacement rules. |
| spec/services/salesforce/upsert_lead_spec.rb | Adds unit tests for the new Lead upsert orchestration and auditing behavior. |
| spec/services/salesforce/sync_contacts_spec.rb | Adds unit tests for incremental Contact → User sync behavior. |
| spec/services/salesforce/resolve_faculty_status_spec.rb | Adds unit tests for faculty_status resolution rules from signup/contact. |
| spec/services/salesforce/records/school_spec.rb | Adds mapping/field-availability tests for the School SObject wrapper. |
| spec/services/salesforce/records/lead_spec.rb | Adds mapping/field-availability tests for the Lead SObject wrapper. |
| spec/services/salesforce/records/contact_spec.rb | Adds mapping/field-availability tests for the Contact SObject wrapper. |
| spec/services/salesforce/records/base_spec.rb | Tests the Base aliasing/utility methods added to ActiveForce::SObject. |
| spec/services/salesforce/reconcile_spec.rb | Adds unit tests for nightly reconcile/self-heal + drift-finding logic. |
| spec/services/salesforce/metrics_spec.rb | Adds tests for per-run metrics counting, check-ins, and alert emission. |
| spec/services/salesforce/lookup_spec.rb | Adds unit tests for Lead/Contact lookup strategy + audit events. |
| spec/services/salesforce/client_spec.rb | Adds tests ensuring credentials/config are passed into Restforce client init. |
| spec/services/salesforce/build_lead_spec.rb | Adds unit tests for pure User → Lead attribute mapping. |
| spec/services/salesforce/audit_spec.rb | Adds tests for standardized Salesforce audit event recording. |
| spec/services/salesforce_spec.rb | Adds tests for Salesforce configuration defaults/validation. |
| spec/routines/update_school_salesforce_info_spec.rb | Updates routine specs to reference Salesforce::Records::School. |
| spec/routines/update_salesforce_assignable_fields_spec.rb | Updates routine specs to reference Salesforce::Records::Contact. |
| spec/routines/newflow/create_or_update_salesforce_lead_spec.rb | Updates routine specs for the shim delegating to new services and new audit events. |
| spec/rails_helper.rb | Removes legacy openstax_salesforce spec helper include/require. |
| spec/models/security_log_spec.rb | Verifies newly added Salesforce event types exist in the enum. |
| spec/models/salesforce_drift_finding_spec.rb | Adds model tests for drift finding upsert/resolve/scopes. |
| spec/lib/settings_salesforce_spec.rb | Adds tests for new Salesforce Settings accessors/defaults/feature flag. |
| spec/factories/salesforce_drift_findings.rb | Adds FactoryBot factory for drift findings. |
| spec/controllers/admin/salesforce_drift_findings_controller_spec.rb | Adds controller specs for the admin drift findings UI. |
| lib/tasks/cron/day.rake | Adds nightly Salesforce::Reconcile.call to daily cron. |
| lib/tasks/accounts/update_adopter_status.rake | Updates rake task to use Salesforce::Records::Contact. |
| lib/tasks/accounts/create_leads_for_instructors_not_sent_to_sf.rake | Updates rake task to use Salesforce::Records::Lead and clarifies reconcile behavior. |
| lib/settings/salesforce.rb | Adds new Settings accessors for cursors, budgets, thresholds, and notifications. |
| lib/settings/feature_flags.rb | Adds feature flag accessor for reconcile self-heal. |
| lib/settings.rb | Adds persistent settings fields for Salesforce sync redesign knobs. |
| Gemfile.lock | Removes openstax_salesforce; adds restforce and openstax_active_force dependencies. |
| Gemfile | Switches Salesforce gems from openstax_salesforce to restforce + openstax_active_force. |
| db/schema.rb | Adds salesforce_drift_findings table + users.salesforce_lead_id index + FK. |
| db/migrate/20260522060438_add_index_to_users_salesforce_lead_id.rb | Adds concurrent index on users.salesforce_lead_id. |
| db/migrate/20260522060416_create_salesforce_drift_findings.rb | Adds drift findings table + indexes. |
| config/routes.rb | Adds admin routes for drift findings index/update. |
| config/initializers/salesforce.rb | Adds new Salesforce configuration initializer for the new client. |
| config/initializers/openstax_salesforce.rb | Removes legacy OpenStax::Salesforce initializer. |
| config/initializers/inflections.rb | Adds OpenStax acronym inflection. |
| app/views/admin/users/edit.html.erb | Adds Salesforce timeline section to admin user edit page. |
| app/views/admin/users/_salesforce_timeline.html.erb | New partial rendering Salesforce SecurityLog timeline + drift link. |
| app/views/admin/salesforce_drift_findings/index.html.erb | New admin view for drift findings listing/filtering/resolution. |
| app/services/salesforce/verify.rb | New ownership/replacement verification helpers. |
| app/services/salesforce/upsert_lead.rb | New orchestrator replacing most of the legacy lead routine logic. |
| app/services/salesforce/sync_schools.rb | New school cache sync service with metrics/check-ins. |
| app/services/salesforce/sync_contacts.rb | New incremental contact sync service with cursor/overlap and swap safety checks. |
| app/services/salesforce/resolve_faculty_status.rb | New centralized faculty_status resolution logic. |
| app/services/salesforce/records/school.rb | New ActiveForce wrapper mapping Account → School fields. |
| app/services/salesforce/records/lead.rb | New ActiveForce wrapper mapping Lead fields. |
| app/services/salesforce/records/contact.rb | New ActiveForce wrapper mapping Contact fields + school association. |
| app/services/salesforce/records/base.rb | ActiveForce utilities + lazy client wiring. |
| app/services/salesforce/reconcile.rb | New nightly reconcile/self-heal + drift finding orchestration. |
| app/services/salesforce/metrics.rb | New per-run metrics + Sentry check-ins/alerts helper. |
| app/services/salesforce/lookup.rb | New lookup strategy for finding Leads/Contacts with auditing. |
| app/services/salesforce/client.rb | New Restforce client wrapper using validated configuration. |
| app/services/salesforce/build_lead.rb | New pure User → Lead mapping module. |
| app/services/salesforce/audit.rb | New standardized audit event writer into SecurityLog. |
| app/services/salesforce.rb | New configuration container + validation for Salesforce integration. |
| app/routines/update_user_lead_info.rb | Removes legacy routine (functionality moved to reconcile/other services). |
| app/routines/update_user_contact_info.rb | Replaces legacy routine body with a shim to Salesforce::SyncContacts. |
| app/routines/update_school_salesforce_info.rb | Replaces legacy routine body with a shim to Salesforce::SyncSchools. |
| app/routines/update_salesforce_assignable_fields.rb | Updates routine to use Salesforce::Records::Contact. |
| app/routines/newflow/create_or_update_salesforce_lead.rb | Replaces legacy routine body with a shim to Salesforce::UpsertLead. |
| app/models/user.rb | Updates helper accessors to use Salesforce::Records::*. |
| app/models/security_log.rb | Adds many new Salesforce-related enum event types for auditing. |
| app/models/salesforce_drift_finding.rb | New model for tracking and resolving drift findings. |
| app/controllers/admin/users_controller.rb | Adds Salesforce timeline query in edit action; updates contact lookup to new records class. |
| app/controllers/admin/salesforce_drift_findings_controller.rb | New admin controller for listing/filtering and resolving drift findings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Six fixes for spec failures the first full CI run surfaced:
1. Admin user edit page 500'd. SecurityLog#event_type is an integer-backed
enum, so `where("event_type LIKE 'salesforce_%'")` raised
"operator does not exist: integer ~~ unknown" from PostgreSQL.
Switched to translating the prefix to integer enum values and
filtering by IN. (Caught independently by Copilot's PR review.)
2. spec/whenever_spec.rb stubbed `expect_any_instance_of(UpdateUserContactInfo).to receive(:call)`
but the shim exposes .call as a class method now. Switched to
expect(UpdateUserContactInfo).to receive(:call).
3. spec/support/salesforce_spec_helpers.rb's limit_salesforce_queries
helper called remote_class.original_query (which the openstax_salesforce
gem aliased on SObject but our absorption didn't carry over) and never
actually applied its like_conditions. Removed both helpers since nothing
in the app calls them. (Also Copilot.)
4. SalesforceProxy was nested inside the now-gone OpenStax::Salesforce::SpecHelpers
module that was included at the top of spec/rails_helper.rb. A handful of
feature/handler specs (verify_email_by_code, change_salesforce_contact_manually,
newflow/student_signup_flow) depended on SalesforceProxy being a top-level
constant. Ported a minimal SalesforceProxy into spec/support/ — only the
methods this app actually uses (new, setup_cassette, new_contact, new_lead,
ensure_schools_exist, school helpers). Skip the gem's Book/Campaign/
CampaignMember helpers since nothing references them. PLACEHOLDER_CREDENTIALS
let Salesforce::Client#initialize's validate! pass in CI without real
SF env vars; the actual auth POST is intercepted by VCR cassettes.
5. Adding `master_record_id` and `is_deleted` to Salesforce::Records::Contact
changed the SOQL SELECT clause, so the three change_salesforce_contact_manually
cassettes had URIs that no longer matched. Patched the URIs to include the
two new fields. The recorded response bodies are unchanged.
6. Salesforce.configure in the initializer triggered Zeitwerk's "autoloaded
during initialization" deprecation — Rails then unloaded the Salesforce
constant and our config along with it, so the api_version, login_domain,
etc. fell back to the Configuration class defaults at runtime. Wrapped
the configure block in `Rails.application.reloader.to_prepare` per the
deprecation warning's recommendation. While I was there, switched the
default api_version from 61.0 to 51.0 to match the version VCR cassettes
were recorded against (the old openstax_salesforce initializer also
defaulted to 51.0; the 61.0 default I'd put in was a regression).
Salesforce spec sweep: 229 examples, 0 failures.
CI-failing specs (whenever, admin users, change_salesforce_contact,
verify_email_by_code, newflow/student_signup_flow): 21 examples,
0 failures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n SF fallback isn't cached yet Bug Copilot caught: if the local schools cache doesn't yet have the "Find Me A Home" SF Account (e.g. brand-new env, or SyncSchools hasn't run recently enough to pick it up), the previous version of ensure_school_or_fallback left @user.school as nil after looking up the SF Account. BuildLead.apply then read user.school&.salesforce_id as nil and the saved Lead landed with no account_id / school_id link to SF — either failing the upsert or creating an unlinked Lead. The original 259-line Newflow::CreateOrUpdateSalesforceLead routine sidestepped this by tracking sf_school_id in a local var and writing it to lead.account_id / lead.school_id directly, independent of user.school. The refactor delegated the field mapping to BuildLead, which reads from user.school — so the user.school association MUST be populated. Fix: when the SF fallback exists but isn't cached locally, create a stub School row with salesforce_id, name, is_kip, is_child_of_kip (the NOT NULL columns). The next SyncSchools run fills in the rest. Added spec for the cache-miss path. Full Salesforce sweep: 244 examples, 0 failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… of stubs
The change_salesforce_contact_manually feature spec kept failing in CI
because adding fields to Salesforce::Records::Contact (master_record_id,
is_deleted) changes the SOQL SELECT, which makes the recorded cassettes
go stale. The fix kept being "patch the cassettes," but cassettes are
the wrong tool here: the test is about admin UI behavior, not SF HTTP
semantics. Stubs are simpler, faster, and don't rot when we add fields.
Changes:
- spec/features/admin/change_salesforce_contact_manually_spec.rb: full
rewrite. Each example stubs Salesforce::Records::Contact.find with the
exact behavior the controller branches on (return record / return nil
/ raise). Removes the SalesforceProxy boilerplate and VCR setup.
- spec/features/newflow/student_signup_flow_spec.rb and
spec/handlers/verify_email_by_code_spec.rb: drop the
before(:all) { SalesforceProxy.new + setup_cassette } blocks. They
never referenced @Proxy in any test — purely defensive setup for SF
calls that don't actually happen (CreateOrUpdateSalesforceLead is
perform_later'd, and the test queue adapter doesn't execute the job).
- Delete now-orphan cassettes:
spec/cassettes/Change_Salesforce_contact_manually/
spec/cassettes/Newflow_CreateOrUpdateSalesforceLead/
spec/cassettes/Newflow_VerifyEmailByCode/sf_setup.yml
spec/cassettes/Newflow/Students/student_signup_flow/sf_setup.yml
- Delete spec/support/salesforce_proxy.rb — only remaining reference is
a commented-out line in sheerid_webhook_spec.rb, harmless.
Sweep across Salesforce-touching specs: 234 examples, 0 failures.
The three converted specs: 17 examples, 0 failures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tags The '51.0' default in config/initializers/salesforce.rb existed only to match cassettes we just deleted (commit c086b37). Bumping everywhere the version is defined to v66.0, the current GA Salesforce release (Spring '26, released Feb 2026): - config/initializers/salesforce.rb (was 51.0) - app/services/salesforce.rb Configuration default (was 61.0) - spec/services/salesforce_spec.rb default assertion - spec/services/salesforce/client_spec.rb passthrough test Also drops `require 'vcr_helper'` and `vcr: VCR_OPTS` from the two specs whose only VCR usage was the now-removed SalesforceProxy setup: - spec/features/newflow/student_signup_flow_spec.rb - spec/handlers/verify_email_by_code_spec.rb Neither spec makes external HTTP after the proxy removal — Salesforce calls go through perform_later/the :test queue adapter, which doesn't execute jobs. The VCR tag was harmless but misleading. PR audit for VCR-removal collateral: - upsert_lead_spec.rb's school stubs still work (the user has a school set, so ensure_school_or_fallback early-returns; the new cache-miss case is exercised by its own dedicated test). - No other specs reference SalesforceProxy or the deleted cassettes (sheerid_webhook_spec has a commented-out reference only). - The non-SF VCR specs (SheeridAPI, FetchBookData, SetGdprData, verify_email_by_pin) are untouched and still pass. Full Salesforce-touching sweep: 244 examples, 0 failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous version of enableOnChecked deferred the initial enable/
disable check by 500ms via setTimeout. That timer ran independently
of the click handler that was attached to the checkbox at the same
moment, creating a race for feature specs that `check 'agreement_i_agree'`
immediately after page load:
1. $(document).ready fires, click handler attaches,
setTimeout(enable_disable_continue, 500) queues.
2. Spec calls `check 'agreement_i_agree'` — click handler fires,
`checkCheckedButton` sees checkbox checked, enables button.
3. 500ms passes; the queued enable_disable_continue runs.
On a slow worker, sometimes this runs AFTER the spec's button
assertion saw enabled=true, then disables the button because
it re-reads the checkbox state but the spec has already moved on.
Worse, on slower CI workers the timer could fire before the click
handler attached at all, leaving the button stuck disabled.
The legacy `application/ui.js.coffee` version (lines 34-39) already
does it the right way: synchronous initial check inside
$(document).ready, then attach the click handler. Aligning the
newflow version to that pattern.
Verified by running spec/features/pose_terms_spec.rb 5 times in a
row — passes deterministically; signup-flow specs unaffected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This looks big, because it is... but I'm taking the openstax_salesforce gem code out and bringing it here for easier ongoing maintence. A lot of the additions are for the drift detection, which has caused almost 5k leads to not get processed properly over the last 2 years. This is the largest impetus behind this change.
This pull request introduces a major refactor and modernization of the Salesforce integration, moving from the legacy
openstax_salesforcegem to a new architecture based onrestforceandopenstax_active_force(which can go away when we upgrade to Rails 7+). It also introduces a new model and admin interface for tracking Salesforce drift findings, expands logging for Salesforce-related user events, and simplifies several routines by delegating to new service classes.Key changes include:
Salesforce Integration Refactor:
openstax_salesforcegem withrestforceandopenstax_active_forcein theGemfile, updating all model and routine references to use the newSalesforce::Recordsclasses instead of the oldOpenStax::Salesforce::Remoteclasses. [1] [2] [3] [4]Salesforce Drift Findings:
SalesforceDriftFindingmodel with upsert and resolve logic, and adds a correspondingAdmin::SalesforceDriftFindingsControllerfor listing, filtering, and resolving drift findings in the admin interface. [1] [2]Logging and Event Tracking:
SecurityLogmodel'sevent_typeenum to include many new Salesforce-related events, improving auditability and traceability of Salesforce sync and lookup operations.Routine Simplification and Shims:
CreateOrUpdateSalesforceLeadandUpdateSchoolSalesforceInforoutines to act as thin shims that delegate to new service classes (Salesforce::UpsertLeadandSalesforce::SyncSchools), preserving their interfaces for compatibility while removing legacy logic. [1] [2]These changes collectively modernize Salesforce integration, improve maintainability, and add new admin tools for monitoring data consistency.