gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

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
Conversation 13 Commits 20 Files Changed 11 +204 -28
onyedikachi-david commented 2025-02-07 17:33:33 +08:00 (Migrated from github.com)
Copy Link

Related Issues

Fixes #1718
/claim #1718

https://github.com/user-attachments/assets/e306e7b6-0c87-45dc-a10c-b456ed39b2cc

## Related Issues Fixes #1718 /claim #1718 https://github.com/user-attachments/assets/e306e7b6-0c87-45dc-a10c-b456ed39b2cc
zachgoll commented 2025-02-11 02:14:16 +08:00 (Migrated from github.com)
Copy Link

Hey @onyedikachi-david thanks for the submission! Just an update—we're currently working on some related data issues regarding the Security model over in #1826, so we'll need to put this one on hold for the moment as I believe the fields required for this will need to change / be updated based on where we land with that.

I'll post here when we're ready to resume work and add some comments to the PR!

Hey @onyedikachi-david thanks for the submission! Just an update—we're currently working on some related data issues regarding the `Security` model over in #1826, so we'll need to put this one on hold for the moment as I believe the fields required for this will need to change / be updated based on where we land with that. I'll post here when we're ready to resume work and add some comments to the PR!
👍 1
zachgoll commented 2025-02-14 00:54:31 +08:00 (Migrated from github.com)
Copy Link

Hey @onyedikachi-david, I think we're able to resume work on this now that the securities updates were pushed to production.

I would check out #1826 for some context to what changed, but in summary:

  • We're now considering Security tickers unique by [ :ticker, :exchange_operating_mic ]
  • There are 2 spots in the app where the user can input these:
    • This import
    • Trade form - in this form, we search the Synth provider for the security prior to submitting the form. If a security is not found, exchange_operating_mic is set to nil and the Security's has_prices? method returns false.

In order to properly import these trades, we will now need the following information from the user's CSV:

  • currency - what currency is the price in? (we already should have this field)
  • ticker - the security ticker (we already have this)
  • exchange_operating_mic - the code that identifies the market exchange in which the security is traded on. For example, AAPL trades on XNAS (operating mic), which is the broad name for "Nasdaq"

For this PR, I think the updates we need to make are:

  • Allow user
  • If exchange_operating_mic is NOT provided by the user, we should set it to nil when we find_or_create_by the Security
  • If exchange_operating_mic is present, we need to make a call to Synth to check if this ticker/exchange_operating_mic combination is present with a valid price.
    • If Synth provider is not setup, we show the user a warning and "ignore" the exchange (set it to nil) regardless of whether it could be valid
    • If Synth provider is set up and the ticker/exchange combo is valid, we proceed
    • If Synth provider is set up and the ticker/exchange is not found, we need to show a validation error within the row that it happened to alert the user that prices couldn't be found and that we won't be able to show history for this ticker
Hey @onyedikachi-david, I think we're able to resume work on this now that the securities updates were pushed to production. I would check out #1826 for some context to what changed, but in summary: - We're now considering `Security` tickers unique by `[ :ticker, :exchange_operating_mic ]` - There are 2 spots in the app where the user can input these: - This import - Trade form - in this form, we search the Synth provider for the security prior to submitting the form. If a security is not found, `exchange_operating_mic` is set to `nil` and the Security's `has_prices?` method returns false. In order to properly import these trades, we will now need the following information from the user's CSV: - `currency` - what currency is the _price_ in? (we already should have this field) - `ticker` - the security ticker (we already have this) - exchange_operating_mic - the code that identifies the market exchange in which the security is traded on. For example, `AAPL` trades on `XNAS` (operating mic), which is the broad name for "Nasdaq" For this PR, I think the updates we need to make are: - Allow user - If `exchange_operating_mic` is NOT provided by the user, we should set it to `nil` when we `find_or_create_by` the Security - If `exchange_operating_mic` is present, we need to make a call to Synth to check if this `ticker`/`exchange_operating_mic` combination is present with a valid price. - If Synth provider is not setup, we show the user a warning and "ignore" the exchange (set it to `nil`) regardless of whether it could be valid - If Synth provider is set up and the ticker/exchange combo is valid, we proceed - If Synth provider is set up and the ticker/exchange is not found, we need to show a validation error within the row that it happened to alert the user that prices couldn't be found and that we won't be able to show history for this ticker
onyedikachi-david commented 2025-02-16 00:36:48 +08:00 (Migrated from github.com)
Copy Link

https://github.com/user-attachments/assets/f642bbb6-bd40-491c-81ee-9a9ebbacdd14

@zachgoll I just pushed some changes addressing the updates.

https://github.com/user-attachments/assets/f642bbb6-bd40-491c-81ee-9a9ebbacdd14 @zachgoll I just pushed some changes addressing the updates.
zachgoll (Migrated from github.com) requested changes 2025-02-19 00:43:18 +08:00
app/controllers/import/configurations_controller.rb
@@ -29,6 +29,7 @@ class Import::ConfigurationsController < ApplicationController
:account_col_label,
zachgoll (Migrated from github.com) commented 2025-02-19 00:08:06 +08:00
Copy Link

I think this needs updating

I think this needs updating
app/models/import/row.rb
zachgoll (Migrated from github.com) commented 2025-02-19 00:12:25 +08:00
Copy Link

I'm second guessing this. I know I said in the requirements that we need a per-cell validation, but I think our best bet is to remove this and check for tickers directly during the import operation.

This will make a Synth call for every single row in the import, which could quickly exhaust a user's Synth credits. I think I'd rather just remove this for now.

I'm second guessing this. I know I said in the requirements that we need a per-cell validation, but I think our best bet is to remove this and check for tickers directly during the import operation. This will make a Synth call for every single row in the import, which could quickly exhaust a user's Synth credits. I think I'd rather just remove this for now.
app/models/trade_import.rb
zachgoll (Migrated from github.com) commented 2025-02-19 00:40:02 +08:00
Copy Link

As I mentioned above in another comment, I think we need to remove the row-level validation, which means by the time we get to this import! operation, we're not 100% certain whether the tickers are correct.

I think we need to rework the flow of this method a little bit to something like this:

def find_or_create_security(ticker:, exchange_operating_mic:)
  # 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
    Security.security_prices_provider.search_securities(
      query: ticker,
      exchange_operating_mic: exchange_operating_mic
    ).securities.first
  end

  if provider_security&[ :ticker ] && provider_security&[ :exchange_operating_mic ]
    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
  else
    Security.find_or_create_by!(ticker: ticker)
  end
end

We'll need to update the search_securities method on the Synth provider:

  • Make the country_code param optional
  • Add the exchange_operating_mic param (also optional)
As I mentioned above in another comment, I think we need to remove the row-level validation, which means by the time we get to this `import!` operation, we're not 100% certain whether the tickers are correct. I think we need to rework the flow of this method a little bit to something like this: ```rb def find_or_create_security(ticker:, exchange_operating_mic:) # 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 Security.security_prices_provider.search_securities( query: ticker, exchange_operating_mic: exchange_operating_mic ).securities.first end if provider_security&[ :ticker ] && provider_security&[ :exchange_operating_mic ] 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 else Security.find_or_create_by!(ticker: ticker) end end ``` We'll need to update the `search_securities` method on the Synth provider: - Make the country_code param optional - Add the `exchange_operating_mic` param (also optional)
db/migrate/20250207061552_add_exchange_and_currency_to_securities.rb
zachgoll (Migrated from github.com) commented 2025-02-19 00:04:11 +08:00
Copy Link

We'll want to remove this field and index. When we search our provider for a specific security, it will return a currency value that we then load directly into the Security::Price model.

This way, we have the price and currency sitting on the same table where it is needed to construct Money objects.

We'll want to remove this field and index. When we search our provider for a specific security, it will return a `currency` value that we then load directly into the `Security::Price` model. This way, we have the `price` and `currency` sitting on the same table where it is needed to construct `Money` objects.
db/migrate/20250207062248_add_exchange_to_import_rows.rb
zachgoll (Migrated from github.com) commented 2025-02-19 00:01:48 +08:00
Copy Link

This is outdated I believe. I think we can remove this column and all related code now?

This is outdated I believe. I think we can remove this column and all related code now?
db/migrate/20250215153930_add_exchange_operating_mic_col_label_to_imports.rb
zachgoll (Migrated from github.com) commented 2025-02-19 00:00:17 +08:00
Copy Link

I think we can skip this step of migrating old imports and just let the user fix it manually. Most users will likely need to reconfigure their import data anyways.

I think we can skip this step of migrating old imports and just let the user fix it manually. Most users will likely need to reconfigure their import data anyways.
zachgoll (Migrated from github.com) reviewed 2025-02-20 05:12:21 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Overall looking good! Once changes are addressed we'll do a final sweep + testing and get it merged.

Overall looking good! Once changes are addressed we'll do a final sweep + testing and get it merged.
app/models/import/row.rb
zachgoll (Migrated from github.com) commented 2025-02-20 05:05:14 +08:00
Copy Link

Let's remove this so that users can import a CSV of trades where some have exchange mic codes provided (will have historical prices) while others don't (custom, or unknown tickers).

Our backend logic during our "sync" process will handle this okay.

Let's remove this so that users can import a CSV of trades where _some_ have exchange mic codes provided (will have historical prices) while others don't (custom, or unknown tickers). Our backend logic during our "sync" process will handle this okay.
app/models/trade_import.rb
@@ -41,12 +51,63 @@ class TradeImport < Import
zachgoll (Migrated from github.com) commented 2025-02-20 05:08:10 +08:00
Copy Link

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
db/migrate/20250219085043_remove_currency_from_securities.rb
zachgoll (Migrated from github.com) commented 2025-02-20 05:09:54 +08:00
Copy Link

Would you mind consolidating all of these migrations into one? Should be able to run bin/rails db:rollback STEP=8, delete all but one, consolidate, then run bin/rails db:migrate again.

If you want a clean slate, bin/rails db:migrate:reset also works.

Would you mind consolidating all of these migrations into one? Should be able to run `bin/rails db:rollback STEP=8`, delete all but one, consolidate, then run `bin/rails db:migrate` again. If you want a clean slate, `bin/rails db:migrate:reset` also works.
db/schema.rb
zachgoll (Migrated from github.com) commented 2025-02-20 05:11:50 +08:00
Copy Link

I think the currency and number_format might be stale from a prior PR correct? I'm guessing once you clean up migrations this should be resolved.

I _think_ the `currency` and `number_format` might be stale from a prior PR correct? I'm guessing once you clean up migrations this should be resolved.
zachgoll (Migrated from github.com) reviewed 2025-02-20 23:08:44 +08:00
app/models/trade_import.rb
zachgoll (Migrated from github.com) commented 2025-02-20 23:01:56 +08:00
Copy Link

Did some local testing of this and I think we may need to bring back a portion of your original solution where we check to see if the security exists in the Maybe DB first prior to hitting Synth. Here's the final flow I'm thinking:

def find_or_create_security(ticker:, exchange_operating_mic:)
  internal_security = exchange_operating_mic.present? ? Security.find_by(ticker:, exchange_operating_mic:) : Security.find_by(ticker:)
  
  return internal_security if internal_security.present?

  # 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 = Security.security_prices_provider.search_securities(
      query: ticker,
      exchange_operating_mic: exchange_operating_mic
    )

    return nil unless response.success?

    response.securities.first
  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.dig(:ticker), exchange_operating_mic: provider_security.dig(:exchange_operating_mic)) do |security|
    security.name = provider_security.dig(:name)
    security.country_code = provider_security.dig(:country_code)
    security.logo_url = provider_security.dig(:logo_url)
    security.exchange_acronym = provider_security.dig(:exchange_acronym)
    security.exchange_mic = provider_security.dig(:exchange_mic)
  end
end
Did some local testing of this and I think we may need to bring back a portion of your original solution where we check to see if the security exists in the Maybe DB first prior to hitting Synth. Here's the final flow I'm thinking: ```rb def find_or_create_security(ticker:, exchange_operating_mic:) internal_security = exchange_operating_mic.present? ? Security.find_by(ticker:, exchange_operating_mic:) : Security.find_by(ticker:) return internal_security if internal_security.present? # 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 = Security.security_prices_provider.search_securities( query: ticker, exchange_operating_mic: exchange_operating_mic ) return nil unless response.success? response.securities.first 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.dig(:ticker), exchange_operating_mic: provider_security.dig(:exchange_operating_mic)) do |security| security.name = provider_security.dig(:name) security.country_code = provider_security.dig(:country_code) security.logo_url = provider_security.dig(:logo_url) security.exchange_acronym = provider_security.dig(:exchange_acronym) security.exchange_mic = provider_security.dig(:exchange_mic) end end ```
zachgoll (Migrated from github.com) commented 2025-02-20 23:08:33 +08:00
Copy Link

We should also add test/models/trade_import_test.rb with a basic test to validate all of this. Here's a starter test that will need to be adapted to this new code:

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
    provider = mock

    # TODO: Update for changes
    provider.expects(:search_securities).with(query: "GOOGL", country_code: "US").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
      )
    )

    Security.stubs(:security_prices_provider).returns(provider)

    Security.security_prices_provider.search_securities(query: "GOOGL", country_code: "US")

    import = <<~CSV
      date,ticker,qty,price,currency,account,name
      01/01/2024,AAPL,10,150.00,USD,TestAccount1,Apple Purchase
      01/02/2024,GOOGL,5,2500.00,USD,TestAccount1,Google Purchase
    CSV

    @import.update!(
      raw_file_str: import,
      date_col_label: "date",
      ticker_col_label: "ticker",
      qty_col_label: "qty",
      price_col_label: "price",
      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 } => 2,
                      -> { Account::Trade.count } => 2,
                      -> { Security.count } => 1,
                      -> { Account.count } => 1 do
      @import.publish
    end

    assert_equal "complete", @import.status
  end
end
We should also add `test/models/trade_import_test.rb` with a basic test to validate all of this. Here's a starter test that will need to be adapted to this new code: ```rb 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 provider = mock # TODO: Update for changes provider.expects(:search_securities).with(query: "GOOGL", country_code: "US").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 ) ) Security.stubs(:security_prices_provider).returns(provider) Security.security_prices_provider.search_securities(query: "GOOGL", country_code: "US") import = <<~CSV date,ticker,qty,price,currency,account,name 01/01/2024,AAPL,10,150.00,USD,TestAccount1,Apple Purchase 01/02/2024,GOOGL,5,2500.00,USD,TestAccount1,Google Purchase CSV @import.update!( raw_file_str: import, date_col_label: "date", ticker_col_label: "ticker", qty_col_label: "qty", price_col_label: "price", 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 } => 2, -> { Account::Trade.count } => 2, -> { Security.count } => 1, -> { Account.count } => 1 do @import.publish end assert_equal "complete", @import.status end end ```
db/migrate/20250207011850_add_exchange_operating_mic_to_securities.rb
zachgoll (Migrated from github.com) commented 2025-02-20 22:14:44 +08:00
Copy Link

There are quite a few changes here that are not needed. Let's keep this PR's migration solely to deal with:

  • Remove exchange from import_rows
  • Remove exchange_col_label from imports
  • Add exchange_operating_mic to import_rows
  • Add exchange_operating_mic_col_label to imports
There are quite a few changes here that are not needed. Let's keep this PR's migration solely to deal with: - Remove `exchange` from `import_rows` - Remove `exchange_col_label` from `imports` - Add `exchange_operating_mic` to `import_rows` - Add `exchange_operating_mic_col_label` to `imports`
db/migrate/20250207194638_adjust_securities_indexes.rb
zachgoll (Migrated from github.com) commented 2025-02-20 22:06:16 +08:00
Copy Link

We'll need to add this one back in as it was a migration from a prior commit

We'll need to add this one back in as it was a migration from a prior commit
db/migrate/20250220013452_fix_imports_schema_duplicates.rb
zachgoll (Migrated from github.com) commented 2025-02-20 22:11:29 +08:00
Copy Link

I don't think this migration is necessary. Given we aren't updating number_format in this PR there shouldn't be any issues with the data.

I don't think this migration is necessary. Given we aren't updating `number_format` in this PR there shouldn't be any issues with the data.
onyedikachi-david commented 2025-02-21 06:44:19 +08:00 (Migrated from github.com)
Copy Link

addressed all, with the test.

addressed all, with the test.
zachgoll (Migrated from github.com) requested changes 2025-02-21 22:26:04 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

I pushed #1876 to address some general issues with imports causing this trade import to fail. You'll need to accept a "combination merge" to resolve conflicts when pulling in the main branch.

I pushed #1876 to address some general issues with imports causing this trade import to fail. You'll need to accept a "combination merge" to resolve conflicts when pulling in the `main` branch.
app/helpers/imports_helper.rb
zachgoll (Migrated from github.com) commented 2025-02-21 22:25:16 +08:00
Copy Link

Is this being used anywhere? I think we already have a variation of this in our helpers module.

Is this being used anywhere? I think we already have a variation of this in our helpers module.
db/migrate/20250207011850_add_exchange_operating_mic_to_securities.rb
zachgoll (Migrated from github.com) commented 2025-02-21 21:04:43 +08:00
Copy Link

This file should not be deleted as it is a prior migration on the main branch. You can add back with:

git checkout main db/migrate/20250207011850_add_exchange_operating_mic_to_securities.rb
This file should not be deleted as it is a prior migration on the `main` branch. You can add back with: ```git git checkout main db/migrate/20250207011850_add_exchange_operating_mic_to_securities.rb ```
db/migrate/20250207194638_adjust_securities_indexes.rb
zachgoll (Migrated from github.com) commented 2025-02-21 21:01:38 +08:00
Copy Link

So what I meant here is that we need to get rid of this migration from this pull request entirely.

We should not be adjusting prior migrations that are already committed to the main branch:

https://github.com/maybe-finance/maybe/blob/main/db/migrate/20250207194638_adjust_securities_indexes.rb

You can revert this and get back to a clean state with this:

git checkout main db/migrate/20250207194638_adjust_securities_indexes.rb
So what I meant here is that we need to get rid of this migration from this pull request entirely. We should not be adjusting prior migrations that are already committed to the `main` branch: https://github.com/maybe-finance/maybe/blob/main/db/migrate/20250207194638_adjust_securities_indexes.rb You can revert this and get back to a clean state with this: ```git git checkout main db/migrate/20250207194638_adjust_securities_indexes.rb ```
db/migrate/20250220153958_update_imports_for_operating_mic_v2.rb
@@ -0,0 +3,4 @@
add_column :import_rows, :exchange_operating_mic, :string
add_column :imports, :exchange_operating_mic_col_label, :string
end
end
zachgoll (Migrated from github.com) commented 2025-02-21 21:03:18 +08:00
Copy Link

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 ```
onyedikachi-david (Migrated from github.com) reviewed 2025-02-21 22:41:28 +08:00
db/migrate/20250207194638_adjust_securities_indexes.rb
onyedikachi-david (Migrated from github.com) commented 2025-02-21 22:41:28 +08:00
Copy Link

okay

okay
zachgoll (Migrated from github.com) reviewed 2025-02-21 23:52:49 +08:00
app/models/trade_import.rb
@@ -41,12 +51,63 @@ class TradeImport < Import
zachgoll (Migrated from github.com) commented 2025-02-21 23:37:09 +08:00
Copy Link

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 (Migrated from github.com) commented 2025-02-21 23:48:22 +08:00
Copy Link
      # 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
db/schema.rb
zachgoll (Migrated from github.com) commented 2025-02-21 23:49:47 +08:00
Copy Link

Migrations are looking good now, but it appears that this is still from a stale migration. Running bin/rails db:migrate:reset should fix and remove this. If it doesn't let me know.

As you can see in production:

8539ac7dec/db/schema.rb (L417)

This default doesn't exist. And we didn't alter this at all in this PR, so it shouldn't be showing up in the output schema.

Migrations are looking good now, but it appears that this is still from a stale migration. Running `bin/rails db:migrate:reset` should fix and remove this. If it doesn't let me know. As you can see in production: https://github.com/maybe-finance/maybe/blob/8539ac7dec4c713fc6f3b92abf05108a261bd194/db/schema.rb#L417 This default doesn't exist. And we didn't alter this at all in this PR, so it shouldn't be showing up in the output schema.
zachgoll (Migrated from github.com) approved these changes 2025-02-22 04:51:43 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Looks good now, nice work!

Looks good now, nice work!
onyedikachi-david commented 2025-02-22 07:31:33 +08:00 (Migrated from github.com)
Copy Link

The import was failing in the UI due to issues with security provider responses.

The test suite continues to pass.

The import was failing in the UI due to issues with security provider responses. The test suite continues to pass.
👍 1
zachgoll commented 2025-02-24 23:00:50 +08:00 (Migrated from github.com)
Copy Link

@onyedikachi-david nice work! If you have any issues with the bounty payment be sure to let me know.

@onyedikachi-david nice work! If you have any issues with the bounty payment be sure to let me know.
🎉 2
Sign in to join this conversation.
Reviewers
No Reviewers
onyedikachi-david
zachgoll
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label 🙋 Bounty claim
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#1822
Delete Branch "feature/add-trade-import"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?