Add exchange and currency fields to trade imports #1822

Merged
onyedikachi-david merged 20 commits from feature/add-trade-import into main 2025-02-24 23:00:25 +08:00
11 changed files with 204 additions and 28 deletions

View File

@@ -29,6 +29,7 @@ class Import::ConfigurationsController < ApplicationController
:account_col_label,
zachgoll commented 2025-02-19 00:08:06 +08:00 (Migrated from github.com)
Review

I think this needs updating

I think this needs updating
:qty_col_label,
:ticker_col_label,
:exchange_operating_mic_col_label,
:price_col_label,
:entity_type_col_label,
:notes_col_label,

View File

@@ -20,6 +20,7 @@ module ImportsHelper
notes: "Notes",
qty: "Quantity",
ticker: "Ticker",
exchange: "Exchange",
price: "Price",
entity_type: "Type"
}[key]

View File

@@ -111,6 +111,7 @@ class Import < ApplicationRecord
date: row[date_col_label].to_s,
qty: sanitize_number(row[qty_col_label]).to_s,
ticker: row[ticker_col_label].to_s,
exchange_operating_mic: row[exchange_operating_mic_col_label].to_s,
price: sanitize_number(row[price_col_label]).to_s,
amount: sanitize_number(row[amount_col_label]).to_s,
currency: (row[currency_col_label] || default_currency).to_s,

View File

@@ -128,11 +128,12 @@ class Provider::Synth
raw_response: error
end
def search_securities(query:, dataset: "limited", country_code:)
def search_securities(query:, dataset: "limited", country_code: nil, exchange_operating_mic: nil)
response = client.get("#{base_url}/tickers/search") do |req|
req.params["name"] = query
req.params["dataset"] = dataset
req.params["country_code"] = country_code
req.params["country_code"] = country_code if country_code.present?
req.params["exchange_operating_mic"] = exchange_operating_mic if exchange_operating_mic.present?
req.params["limit"] = 25
end

View File

@@ -13,7 +13,8 @@ class Security < ApplicationRecord
security_prices_provider.search_securities(
query: query[:search],
dataset: "limited",
country_code: query[:country]
country_code: query[:country],
exchange_operating_mic: query[:exchange_operating_mic]
).securities.map { |attrs| new(**attrs) }
end
end

View File

@@ -5,14 +5,24 @@ class TradeImport < Import
rows.each do |row|
account = mappings.accounts.mappable_for(row.account)
security = Security.find_or_create_by(ticker: row.ticker)
# Try to find or create security with ticker only
security = find_or_create_security(
ticker: row.ticker,
exchange_operating_mic: row.exchange_operating_mic
)
entry = account.entries.build \
date: row.date_iso,
amount: row.signed_amount,
name: row.name,
currency: row.currency,
entryable: Account::Trade.new(security: security, qty: row.qty, currency: row.currency, price: row.price),
currency: row.currency.presence || account.currency,
entryable: Account::Trade.new(
security: security,
qty: row.qty,
currency: row.currency.presence || account.currency,
price: row.price
),
import: self
entry.save!
@@ -29,7 +39,7 @@ class TradeImport < Import
end
def column_keys
%i[date ticker qty price currency account name]
%i[date ticker exchange_operating_mic currency qty price account name]
end
def dry_run
@@ -41,12 +51,63 @@ class TradeImport < Import
zachgoll commented 2025-02-20 05:08:10 +08:00 (Migrated from github.com)
Review

We'll want to remove the exchange_operating_mic from this so that our Security model properly identifies it as "missing prices":

79e1a2c0ff/app/models/security.rb (L38-L40)

We'll want to remove the `exchange_operating_mic` from this so that our `Security` model properly identifies it as "missing prices": https://github.com/maybe-finance/maybe/blob/79e1a2c0ffd5191fe6c040da68a311e1c1e6beb4/app/models/security.rb#L38-L40
zachgoll commented 2025-02-21 23:37:09 +08:00 (Migrated from github.com)
Review

We need to explicitly set exchange_operating_mic: nil here as I had in my snippet below to prevent invalid exchange mics from being persisted to the DB.

We need to explicitly set `exchange_operating_mic: nil` here as I had in my snippet below to prevent invalid exchange mics from being persisted to the DB.
zachgoll commented 2025-02-21 23:48:22 +08:00 (Migrated from github.com)
Review
      # If no exact match and no exchange_operating_mic was provided, try to find any security with the same ticker
      if exchange_operating_mic.nil?
        internal_security = Security.where(ticker:).first
        if internal_security.present?
          internal_security.update!(exchange_operating_mic: nil)
          return internal_security
        end
      end

      # If we couldn't find the security locally and the provider isn't available, create with provided info
      return Security.create!(ticker: ticker, exchange_operating_mic: exchange_operating_mic) unless Security.security_prices_provider.present?

The issue with adding in this logic is in the case where the user has provided a valid ticker + exchange mic in their CSV that doesn't exist in our current DB. For example, let's say they provide AAPL + XNAS (valid in Synth) and it is not currently in our DB, but there is a Security in our DB of [AAPL, nil]. This logic will pick that existing security up even though what we want to do is go fetch from Synth to find the new security.

I think given that scenario, we should remove this and go with something like the example code I provided: https://github.com/maybe-finance/maybe/pull/1822/files#r1963740617

```rb # If no exact match and no exchange_operating_mic was provided, try to find any security with the same ticker if exchange_operating_mic.nil? internal_security = Security.where(ticker:).first if internal_security.present? internal_security.update!(exchange_operating_mic: nil) return internal_security end end # If we couldn't find the security locally and the provider isn't available, create with provided info return Security.create!(ticker: ticker, exchange_operating_mic: exchange_operating_mic) unless Security.security_prices_provider.present? ``` The issue with adding in this logic is in the case where the user has provided a valid ticker + exchange mic in their CSV that doesn't exist in our current DB. For example, let's say they provide `AAPL` + `XNAS` (valid in Synth) and it is not currently in our DB, but there _is_ a `Security` in our DB of `[`AAPL`, `nil`]`. This logic will pick that existing security up even though what we want to do is go fetch from Synth to find the new security. I think given that scenario, we should remove this and go with something like the example code I provided: https://github.com/maybe-finance/maybe/pull/1822/files#r1963740617
def csv_template
template = <<-CSV
date*,ticker*,qty*,price*,currency,account,name
05/15/2024,AAPL,10,150.00,USD,Trading Account,Apple Inc. Purchase
05/16/2024,GOOGL,-5,2500.00,USD,Investment Account,Alphabet Inc. Sale
05/17/2024,TSLA,2,700.50,USD,Retirement Account,Tesla Inc. Purchase
date*,ticker*,exchange_operating_mic,currency,qty*,price*,account,name
05/15/2024,AAPL,XNAS,USD,10,150.00,Trading Account,Apple Inc. Purchase
05/16/2024,GOOGL,XNAS,USD,-5,2500.00,Investment Account,Alphabet Inc. Sale
05/17/2024,TSLA,XNAS,USD,2,700.50,Retirement Account,Tesla Inc. Purchase
CSV
CSV.parse(template, headers: true)
end
private
def find_or_create_security(ticker:, exchange_operating_mic:)
# Normalize empty string to nil for consistency
exchange_operating_mic = nil if exchange_operating_mic.blank?
# First try to find an exact match in our DB, or if no exchange_operating_mic is provided, find by ticker only
internal_security = if exchange_operating_mic.present?
Security.find_by(ticker:, exchange_operating_mic:)
else
Security.find_by(ticker:)
end
return internal_security if internal_security.present?
# If security prices provider isn't properly configured or available, create with nil exchange_operating_mic
provider = Security.security_prices_provider
unless provider.present? && provider.respond_to?(:search_securities)
return Security.find_or_create_by!(ticker: ticker, exchange_operating_mic: nil)
end
# Cache provider responses so that when we're looping through rows and importing,
# we only hit our provider for the unique combinations of ticker / exchange_operating_mic
cache_key = [ ticker, exchange_operating_mic ]
@provider_securities_cache ||= {}
provider_security = @provider_securities_cache[cache_key] ||= begin
response = provider.search_securities(
query: ticker,
exchange_operating_mic: exchange_operating_mic
)
if !response || !response.success? || !response.securities || response.securities.empty?
nil
else
response.securities.first
end
rescue => e
nil
end
return Security.find_or_create_by!(ticker: ticker, exchange_operating_mic: nil) if provider_security.nil?
Security.find_or_create_by!(ticker: provider_security[:ticker], exchange_operating_mic: provider_security[:exchange_operating_mic]) do |security|
security.name = provider_security[:name]
security.country_code = provider_security[:country_code]
security.logo_url = provider_security[:logo_url]
security.exchange_acronym = provider_security[:exchange_acronym]
security.exchange_mic = provider_security[:exchange_mic]
end
end
end

View File

@@ -1,25 +1,37 @@
<%# locals: (import:) %>
<%= styled_form_with model: @import, url: import_configuration_path(@import), scope: :import, method: :patch, class: "space-y-2" do |form| %>
<div class="flex items-center gap-2">
<%= form.select :date_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Date" }, required: true %>
<%= form.select :date_format, Family::DATE_FORMATS, { label: t(".date_format_label")}, label: true, required: true %>
</div>
<div class="space-y-4">
<div class="flex items-center gap-2">
<%= form.select :date_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Date" }, required: true %>
<%= form.select :date_format, Family::DATE_FORMATS, { label: t(".date_format_label")}, label: true, required: true %>
</div>
<div class="flex items-center gap-2">
<%= form.select :qty_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Quantity" } %>
<%= form.select :signage_convention, [["Buys are positive qty", "inflows_positive"], ["Buys are negative qty", "inflows_negative"]], label: true %>
</div>
<div class="flex items-center gap-2">
<%= form.select :qty_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Quantity" } %>
<%= form.select :signage_convention, [["Buys are positive qty", "inflows_positive"], ["Buys are negative qty", "inflows_negative"]], label: true %>
</div>
<div class="flex items-center gap-2">
<%= form.select :currency_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Currency" } %>
<%= form.select :number_format, Import::NUMBER_FORMATS.keys, { label: "Format", prompt: "Select format" }, required: true %>
</div>
<div class="flex items-center gap-2">
<%= form.select :currency_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Currency" } %>
<%= form.select :number_format, Import::NUMBER_FORMATS.keys, { label: "Format", prompt: "Select format" }, required: true %>
</div>
<%= form.select :ticker_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Ticker" } %>
<%= form.select :price_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Price" } %>
<%= form.select :account_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Account (optional)" } %>
<%= form.select :name_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Name (optional)" } %>
<%= form.select :ticker_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Ticker" } %>
<%= form.select :exchange_operating_mic_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Exchange Operating MIC" } %>
<%= form.select :price_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Price" } %>
<%= form.select :account_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Account (optional)" } %>
<%= form.select :name_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Name (optional)" } %>
<% if Security.security_prices_provider.nil? %>
<div class="alert alert-warning">
<p>
<strong>Note:</strong> The Synth provider is not configured. Exchange validation is disabled.
Securities will be created without exchange validation, and price history will not be available.
</p>
</div>
<% end %>
</div>
<%= form.submit "Apply configuration", class: "w-full btn btn--primary", disabled: import.complete? %>
<% end %>

View File

@@ -0,0 +1,6 @@
class UpdateImportsForOperatingMicV2 < ActiveRecord::Migration[7.2]
def change
add_column :import_rows, :exchange_operating_mic, :string
add_column :imports, :exchange_operating_mic_col_label, :string
end
end
zachgoll commented 2025-02-21 21:03:18 +08:00 (Migrated from github.com)
Review

With rails we should always prefer a change migration as it's much easier to read and reason about. Here's what this migration should be:

def change
  add_column :import_rows, :exchange_operating_mic, :string
  add_column :imports, :exchange_operating_mic_col_label, :string
end
With rails we should always prefer a `change` migration as it's much easier to read and reason about. Here's what this migration should be: ```rb def change add_column :import_rows, :exchange_operating_mic, :string add_column :imports, :exchange_operating_mic_col_label, :string end ```

4
db/schema.rb generated
View File

@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema[7.2].define(version: 2025_02_12_213301) do
ActiveRecord::Schema[7.2].define(version: 2025_02_20_153958) do
# These are extensions that must be enabled in order to support this database
enable_extension "pgcrypto"
enable_extension "plpgsql"
@@ -385,6 +385,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_02_12_213301) do
t.text "notes"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.string "exchange_operating_mic"
t.index ["import_id"], name: "index_import_rows_on_import_id"
end
@@ -415,6 +416,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_02_12_213301) do
t.string "signage_convention", default: "inflows_positive"
t.string "error"
t.string "number_format"
t.string "exchange_operating_mic_col_label"
t.index ["family_id"], name: "index_imports_on_family_id"
end

View File

@@ -2,3 +2,8 @@ transaction:
family: dylan_family
type: TransactionImport
status: pending
trade:
family: dylan_family
type: TradeImport
status: pending

View File

@@ -0,0 +1,85 @@
require "test_helper"
require "ostruct"
class TradeImportTest < ActiveSupport::TestCase
include ActiveJob::TestHelper, ImportInterfaceTest
setup do
@subject = @import = imports(:trade)
end
test "imports trades and accounts" do
# Create an existing AAPL security with no exchange_operating_mic
aapl = Security.create!(ticker: "AAPL", exchange_operating_mic: nil)
provider = mock
# We should only hit the provider for GOOGL since AAPL already exists
provider.expects(:search_securities).with(
query: "GOOGL",
exchange_operating_mic: "XNAS"
).returns(
OpenStruct.new(
securities: [
{
ticker: "GOOGL",
name: "Google Inc.",
country_code: "US",
exchange_mic: "XNGS",
exchange_operating_mic: "XNAS",
exchange_acronym: "NGS"
}
],
success?: true,
raw_response: nil
)
).once
Security.stubs(:security_prices_provider).returns(provider)
import = <<~CSV
date,ticker,qty,price,currency,account,name,exchange_operating_mic
01/01/2024,AAPL,10,150.00,USD,TestAccount1,Apple Purchase,
01/02/2024,GOOGL,5,2500.00,USD,TestAccount1,Google Purchase,XNAS
CSV
@import.update!(
raw_file_str: import,
date_col_label: "date",
ticker_col_label: "ticker",
qty_col_label: "qty",
price_col_label: "price",
exchange_operating_mic_col_label: "exchange_operating_mic",
date_format: "%m/%d/%Y",
signage_convention: "inflows_positive"
)
@import.generate_rows_from_csv
@import.mappings.create! key: "TestAccount1", create_when_empty: true, type: "Import::AccountMapping"
@import.reload
assert_difference [
-> { Account::Entry.count },
-> { Account::Trade.count }
], 2 do
assert_difference [
-> { Security.count },
-> { Account.count }
], 1 do
@import.publish
end
end
assert_equal "complete", @import.status
# Verify the securities were created/updated correctly
aapl.reload
assert_nil aapl.exchange_operating_mic
googl = Security.find_by(ticker: "GOOGL")
assert_equal "XNAS", googl.exchange_operating_mic
assert_equal "XNGS", googl.exchange_mic
end
end