As I was implementing #888 which introduces a "delegated type" for an InvestmentTransaction, I noticed some model namespacing that will soon become confusing as we add these new transaction types.
A slight re-org of these model namespaces would significantly speed up future development and allow for a better understanding of the codebase for new contributors.
This issue is a preview of the namespace changes (and additions) I am considering, which will hopefully serve 2 purposes:
Get some early feedback / ideas on my proposed changes
Serve as documentation for future contributors to understand the domain model better
None of these changes should affect existing functionality, and I will be adding tests where needed prior to refactoring.
Below is a rough draft of the domain model changes:
Account Namespace
Since we originally introduced delegated Account types, we've introduced several new domain models that also relate to an Account and belong in the Account namespace. For example, "account valuations", "account balances", "account transactions", etc.
After examining the existing accountable types, I believe that adding these to the global namespace could make the relationships a bit clearer.
For example, moving Account::Depository => Depository, Account::Loan => Loan, etc.
This aligns with the delegated types docs and generally thins out DB tables, class names, etc. Furthermore, since we'll have another delegated type (event.rb) that is only valid in the context of an Account, this helps avoid a hierarchy of Account::Event::Transaction / Account::Event::Valuation, which is a bit verbose in my opinion.
As I was thinking through the delegated "Transaction" type for #888, I came up with the following:
class Account::Entry < ApplicationRecord
belongs_to :account
delegated_type :entryable, types: %w[ Valuation Transaction Trade CryptoTrade ]
end
module Account::Entryable
extend ActiveSupport::Concern
included do
has_one :entry, as: :entryable, touch: true
end
end
# Point-in-time valuation of an account
class Valuation < ApplicationRecord
include Account::Entryable
end
class Transaction < ApplicationRecord
include Account::Entryable
end
# i.e. buy/sell of AAPL
class Trade < ApplicationRecord
include Account::Entryable
end
class CryptoTrade < ApplicationRecord
include Account::Entryable
end
For more details about each of these entry types, see #888
Benefits
Will make it easier to display a "history" for the account (see #838)
Will make it easier to "sync" an account and avoid logic like this that has to check both transactions and valuations to determine how to sync the account
Categories, Tags, Merchants
Currently, we have both categories and tags under the Transaction namespace—Transaction::Category and Transaction::Merchant
Given the changes proposed above, this hierarchy gets pretty messy—do we put them under Account::Event? Account::Transaction?
These 3 concepts make sense on their own and I don't see an issue moving them up the hierarchy and letting the domain models like Account::Transaction decide whether a belongs_to relationship is valid rather than trying to communicate this idea via namespacing.
As I was implementing #888 which introduces a "delegated type" for an
InvestmentTransaction
, I noticed some model namespacing that will soon become confusing as we add these new transaction types.A slight re-org of these model namespaces would significantly speed up future development and allow for a better understanding of the codebase for new contributors.
This issue is a preview of the namespace changes (and additions) I am considering, which will hopefully serve 2 purposes:
None of these changes should affect existing functionality, and I will be adding tests where needed prior to refactoring.
Below is a rough draft of the domain model changes:
Account Namespace
Since we originally introduced delegated Account types, we've introduced several new domain models that also relate to an
Account
and belong in the Account namespace. For example, "account valuations", "account balances", "account transactions", etc.After examining the existing accountable types, I believe that adding these to the global namespace could make the relationships a bit clearer.
For example, moving
Account::Depository
=>Depository
,Account::Loan
=>Loan
, etc.This aligns with the delegated types docs and generally thins out DB tables, class names, etc. Furthermore, since we'll have another delegated type (
event.rb
) that is only valid in the context of anAccount
, this helps avoid a hierarchy ofAccount::Event::Transaction
/Account::Event::Valuation
, which is a bit verbose in my opinion.Account "Entries"
As I was thinking through the delegated "Transaction" type for #888, I came up with the following:
For more details about each of these entry types, see #888
Benefits
Categories, Tags, Merchants
Currently, we have both categories and tags under the
Transaction
namespace—Transaction::Category
andTransaction::Merchant
Given the changes proposed above, this hierarchy gets pretty messy—do we put them under
Account::Event
?Account::Transaction
?These 3 concepts make sense on their own and I don't see an issue moving them up the hierarchy and letting the domain models like
Account::Transaction
decide whether abelongs_to
relationship is valid rather than trying to communicate this idea via namespacing.Proposed namespace
In
app/models
: