Sophtron follow-up: pending items from PR #1698 review

[jjmata]

Context

PR #1698 (Complete Sophtron account mapping) was merged with the following known issues deferred for follow-up. This issue tracks them as a single body of work for the next Sophtron PR.

---

Bugs

1. Wrong pluralization word in transactions_failed error message

File: app/models/sophtron_item/syncer.rb ~line 123

'account'.pluralize is used for a transactions_failed count, producing messages like "2 accounts failed to import transactions" instead of "2 transactions failed to import transactions".

# Current (wrong)
message: "#{import_result[:transactions_failed]} #{'account'.pluralize(import_result[:transactions_failed])} failed to import transactions"

# Fix
message: "#{import_result[:transactions_failed]} #{'transaction'.pluralize(import_result[:transactions_failed])} failed to import"

2. Unguarded update! inside rescue Provider::Sophtron::Error blocks

File: app/models/sophtron_item/importer.rb — fetch_and_store_transactions and refresh_account_before_transaction_fetch

Both rescue blocks call sophtron_item.update!(status: :requires_update) without an inner rescue. If update! raises ActiveRecord::RecordInvalid, it escapes the outer rescue and turns a recoverable auth failure into a hard job crash.

# Current (dangerous)
rescue Provider::Sophtron::Error => e
  requires_update = e.error_type.in?([ :unauthorized, :access_forbidden ])
  sophtron_item.update!(status: :requires_update) if requires_update
  { success: false, ... }

# Fix: use non-bang update, or add inner rescue
  sophtron_item.update(status: :requires_update) if requires_update

3. Behavioral edge case in initial_transaction_fetch? for newly-added accounts

File: app/models/sophtron_item/importer.rb

The current condition is:

sophtron_account.raw_transactions_payload.nil? && sophtron_item.last_synced_at.blank?

A newly-linked account added to an already-synced item has raw_transactions_payload.nil? but last_synced_at present, so it falls into the incremental path and gets only last_synced_at - buffer of history instead of a full historical load. The original code comment said "treat it as a first sync for this account even if the item has been synced before."

Action needed: Confirm whether this is intentional. If not, change to sophtron_account.raw_transactions_payload.nil? alone (account-scoped check only).

---

Security / Privacy

4. Raw Sophtron API response bodies exposed in browser flash alerts

Files: app/models/provider/sophtron.rb + app/controllers/sophtron_items_controller.rb

handle_response embeds the raw API response body in exception messages:

raise Error.new("Bad request to Sophtron API: #{body}", :bad_request, details: body)
raise Error.new("Sophtron API request failed: #{response.code} #{response.message} - #{body}", :fetch_failed, details: body)

Controller rescue blocks then forward e.message directly to user-visible flash alerts:

redirect_to ..., alert: t(".api_error", message: e.message)

This means raw Sophtron API bodies (which can include MFA challenge content, auth error payloads, and backend error details) reach the user's browser. The body is already available in details: for diagnostics — remove it from the human-readable message string.

# Fix
raise Error.new("Bad request to Sophtron API", :bad_request, details: body)
raise Error.new("Sophtron API request failed: #{response.code} #{response.message}", :fetch_failed, details: body)

5. normalize_base_url silently falls back to DEFAULT_BASE_URL on invalid URI

File: app/models/provider/sophtron.rb

A misconfigured base URL (invalid scheme, garbage value) is silently swallowed and replaced with the production Sophtron URL, causing live credentials to be sent to the wrong endpoint rather than surfacing a configuration error.

# Current
rescue URI::InvalidURIError
  DEFAULT_BASE_URL

# Fix
rescue URI::InvalidURIError => e
  raise Error.new("Invalid Sophtron base URL: #{e.message}", :configuration_error)

6. Rails.logger.debug logs full account data on every page render

File: app/views/sophtron_items/select_accounts.html.erb line ~18

<% Rails.logger.debug "Sophtron account data: #{account.inspect}" %>

account.inspect includes account numbers, balances, and institution names. This line predates PR #1698 but was not removed. Delete it or restrict to Rails.env.development?.

---

i18n

7. Hardcoded English strings in sophtron_item/syncer.rb import_errors_for

File: app/models/sophtron_item/syncer.rb ~lines 115, 123, 128

Three raw English strings violate the mandatory i18n convention:

message: "#{n} #{'account'.pluralize(n)} failed to import"
message: "#{n} #{'account'.pluralize(n)} failed to import transactions"
errors.presence || [ { message: "Sophtron import failed", category: "sync_error" } ]

Move to config/locales/en.yml under sophtron_items.syncer.* and use t() with Rails pluralization.

8. Hardcoded English strings in last_connection_error from background jobs

Files: app/jobs/sophtron_refresh_poll_job.rb, app/models/sophtron_item/importer.rb

Several places write hardcoded English directly to last_connection_error, which is rendered to users in the sync panel:

sophtron_item.update!(last_connection_error: "Sophtron refresh failed")
sophtron_item.update!(last_connection_error: "Sophtron refresh did not finish before the polling timeout")
last_connection_error: "Sophtron refresh requires MFA"

These should use locale keys, consistent with items 7 above.

---

Code Quality

9. first_present helper duplicated in two files

Files: app/models/provider/sophtron.rb, app/models/sophtron_account.rb

Identical private first_present(hash, *keys) method defined independently in both classes. Extract to a shared module (e.g. Provider::Sophtron::PayloadHelpers or a concern).

10. Fat controller — domain logic in private controller methods

File: app/controllers/sophtron_items_controller.rb

The controller has ~30 private methods including domain logic that belongs in the model layer per project conventions ("Skinny Controllers, Fat Models"):

• build_mfa_challenge → SophtronItem#mfa_challenge
• post_mfa_job_payload? / login_progress_job_payload? → SophtronItem predicates
• sophtron_connection_failure_message → SophtronItem#connection_failure_message
• fetch_remote_accounts → SophtronItem#remote_accounts(force:)
• persist_remote_sophtron_accounts → SophtronItem#persist_remote_accounts

This can be tackled incrementally alongside other model-layer work.

---

Priority

#	Item	Priority
1	Wrong pluralization word	High — produces incorrect UI copy
2	Unguarded update! in rescue blocks	High — turns auth failures into job crashes
4	Raw API body in flash alerts	High — privacy/security
7	Hardcoded strings in import_errors_for	Medium — i18n violation
8	Hardcoded strings in last_connection_error	Medium — i18n violation
5	normalize_base_url silent fallback	Medium — misconfiguration hazard
3	initial_transaction_fetch? edge case	Medium — confirm intent first
6	Rails.logger.debug in view	Low — pre-existing, debug level only
9	first_present duplication	Low — code smell
10	Fat controller	Low — can be done incrementally

Items 1, 2, and 4 are the most impactful and can be fixed in a small focused PR.

---

Tracked from code review of PR #1698.

[jjmata]

Other items for follow-up:

• Implement OAuth flow like Plaid
• Add correct links to singup flow
• Make 1/6 and 1/15 messages explain progress change
• Make sure "sophtron" is capitalized throughout
• Review these values:

  CONNECTION_STATUS_MAX_POLLS = 6
  LOGIN_PROGRESS_CONNECTION_STATUS_MAX_POLLS = 15
  POST_MFA_CONNECTION_STATUS_MAX_POLLS = 15
  CONNECTION_STATUS_POLL_INTERVAL_MS = 4_000
  MAX_SECURITY_ANSWERS = 10
  MAX_SECURITY_ANSWER_LENGTH = 256
  INITIAL_LOAD_LOOKBACK_DAYS = 120
  MAX_TRANSACTION_HISTORY_YEARS = 3

[danez]

Other items for follow-up:

* Implement OAuth flow like Plaid

* Add correct links to singup flow

* Make 1/6 and 1/15 messages explain progress change

* Make sure "sophtron" is capitalized throughout

* Review these values:

  CONNECTION_STATUS_MAX_POLLS = 6
  LOGIN_PROGRESS_CONNECTION_STATUS_MAX_POLLS = 15
  POST_MFA_CONNECTION_STATUS_MAX_POLLS = 15
  CONNECTION_STATUS_POLL_INTERVAL_MS = 4_000
  MAX_SECURITY_ANSWERS = 10
  MAX_SECURITY_ANSWER_LENGTH = 256
  INITIAL_LOAD_LOOKBACK_DAYS = 120
  MAX_TRANSACTION_HISTORY_YEARS = 3

I had to change *_MAX_POLLS to 24 to not timeout before Amex even asks for 2fa code.

But I couldn't get it working further, I receive a notification in my American Express App with an approval for the new login, I approve, but then sure asks for verification code.

So far i always hot patched my actual docker containers, how would i debug the sopthtron requests proper in the dev container?

[jjmata]

So far i always hot patched my actual docker containers, how would i debug the sopthtron requests proper in the dev container?

I spent a few hours copy pasting dev logs to Codex that was able to figure out failure states for me. Devcontainer logs, that is. It's still finicky ... and I sometimes get it to sync but it brings no transactions until I sync again. Did you try that? Maybe swing by the Discord and we can start a "Sophtron testing" thread, and I can put our OpenClaw to work on it.

I've seen transactions flow in from Bank of America and Apple Card, personally!