Account state machine

[timlawrenz]

Hi,

I suggest using state machines for columns that should have controlled workflows.
This PR also tries to ensure that the decision logic (whether an account can be synced) stays close to the model. A family should not know too much about whether an account is syncable.

Please let me know what you think.

[timlawrenz]

The controller shouldn't know too much about the model's internal behavior.

[timlawrenz]

This now also tests if the state change is allowed.

[timlawrenz]

moved to syncable

[timlawrenz]

moved to syncable

[timlawrenz]

Cleanup: the assigned value_node was never used.

[timlawrenz]

Coming from the model. It might be sensible to name this to sync_state. An Account may have other states (maybe related to the financial organization, such as closed, unavailable, etc.).

[timlawrenz]

The Family shouldn't know too much about internal states of Accounts.

[timlawrenz]

This is now tested earlier through the state machine.

[zachgoll]

Thanks for this contribution! Overall, I agree with the consolidation you've done to move everything into the model and within the Syncable module.

Before we get this merged, I just had a few questions below:

Naming

I agree that we should probably consider renaming a few things to make it clear that these state transitions are in the context of "Account Syncing".

What do you think of something like this?

(would require us to do a DB migration to change :status -> :sync_state and remove the enum)

aasm column: :sync_state do
  # implementation
end

Possible states

The account sync will soon have several steps within it and I'm wondering your thoughts on the best way to incorporate a state machine for this. For example, a "Sync" may include all of the following steps:

1. Sync data provider transactions
2. Sync data provider exchange rates
3. Sync data provider account data
4. Sync data provider stock securities prices
5. Sync balances (takes all 3rd party data and uses it to calculate balances)

Essentially, it's an ETL process that will be similar to the V1 implementation.

While I suppose we could keep these all combined into an event :sync, after: :start_sync, it may be nicer if we're able to see the exact step that failed in the DB.

My thought is that rather than including an :error state, we'd let all the steps run and "collect" an array of errors that we could later view in the sync_errors column on the accounts table. There may be steps that don't make sense to run after certain failures, but I'm thinking it might be too early to decide on that.

aasm column: :sync_state do 
  state :idle, initial: true
  state :syncing_exchange_rates
  state :syncing_balances

  event :sync_exchange_rates do
    after { |start_date| start_exchange_rate_sync(start_date) }
    error { |e| append_sync_error(e) }
    transitions from: :idle, to: :syncing_exchange_rates
  end

  event :sync_balances do
    after { |start_date| start_balance_sync(start_date) }
    error { |e| append_sync_error(e) }
    transitions from: :syncing_exchange_rates, to: :syncing_balances
  end

  event :complete_sync do 
     transitions from: :syncing_balances, to: :idle
  end

  def start_exchange_rate_sync(start_date = nil)
  end

  def start_balance_sync(start_date = nil)
  end

  def complete_sync 
  end

  def append_sync_error(e)
    # Updates the `sync_errors` column
  end
end

There may be a better way to represent this. Curious to hear your thoughts.

[timlawrenz]

Hi @zachgoll,

Thank you for your feedback!

I like your approach, which is to step through the individual stages and label them as states.

While I suppose we could keep these all combined into an event :sync, after: :start_sync, it may be nicer if we're able to see the exact step that failed in the DB.

To improve observability, it's not unreasonable to create a model Sync (or ProviderSync, ProviderSyncLog, or similar) that logs each individual sync step, together with account, and possible error message. If necessary we can clean that out after 30.days or so, but could provide a log with latest sync issues.

[zachgoll]

@timlawrenz yeah I'd agree with that. If we created an Account::Sync model, I'd assume sync_state (and state machine) stay on Account and then we'd possibly lift and shift some (or all) of the Syncable logic to the new model?

My guess is that 3rd party provider data could be stored directly on the Account::Sync model as JSONB columns, so we could do something like:

sync = Account::Sync.new(start_date: 10.days.ago)

sync.fetch_exchange_rates # stores in a JSONB column 
sync.fetch_other_provider_data

And as you mentioned, future improvements may include things like purging old syncs or maybe even setting up associations (rather than JSONB) as we become more familiar with exactly what data we need.

[timlawrenz]

@zachgoll I think that's the right next step. I don't yet have a deep enough understanding of which model should know about which part of the business logic. I assume that some parts of the Provider play a role in this.
I don't fully understand what you mean by storing the 3rd party provider data in the sync. Would you store the log-like data in there, or the actual results?

[zachgoll]

@timlawrenz I meant the actual results that were fetched from the provider. Each sync will follow some sort of ETL pattern, and it may not be possible to "transform + load" the data right away (i.e. we might need to combine transaction data with exchange rate data). My line of thinking is that we should be able to perform the "extract" step in parallel with retries, which could be challenging if we don't save the raw fetched data in some sort of interim "staging" area.

None of this needs to be implemented here in this PR, but wanted to add this as context in case it helps design the various states we'll need here.

[timlawrenz]

@zachgoll Thanks for clarifying. I'll update this PR to

• include a sync model that
  • has a relationship with an account
  • is responsible for the actual syncing
  • stores an error message if applicable

We should create a new sync object (row) for every sync step to keep the individual results/error messages around. The account state machine orchestrates the creation of those objects. What is now in the syncable concern should be moved to the sync class.

[zachgoll]

@timlawrenz yep, I like that approach. If you'd like to introduce a sync "row", that's fine. The one thing I think we should optimize most for right now is clarity and simplicity. 100% fine suffering on performance a little bit to get us to the clearest and most understandable flow possible.

[timlawrenz]

@zachgoll, that is very good guidance.

[zachgoll]

@timlawrenz do you plan on continuing work on this? I'll be updating some sync related logic soon, so just let me know.

No worries either way.