From c5da8ea5502f2732d01fcafebe43c0502fc22614 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Mon, 3 Mar 2025 12:47:30 -0500 Subject: [PATCH] Allow CSV imports to be configured with single or multi-account mode (#1943) * Allow CSV imports to be configured to a single account or multiple accounts * Initialize import directly from accounts page * Fix brakeman warnings * Fix schema * Fix Synth check --- app/controllers/import/confirms_controller.rb | 4 ++ app/controllers/import/uploads_controller.rb | 1 + app/controllers/imports_controller.rb | 3 +- app/models/family.rb | 2 +- app/models/import.rb | 13 ++++-- app/models/import/account_mapping.rb | 7 +++- app/models/trade_import.rb | 33 ++++++++++----- app/models/transaction_import.rb | 21 +++++++--- app/views/account/trades/_form.html.erb | 8 ++-- app/views/accounts/show/_menu.html.erb | 4 +- .../configurations/_mint_import.html.erb | 7 +++- .../configurations/_trade_import.html.erb | 6 ++- .../_transaction_import.html.erb | 5 ++- app/views/import/confirms/_mappings.html.erb | 2 +- app/views/import/uploads/show.html.erb | 42 +++++++++---------- app/views/imports/_import.html.erb | 4 ++ app/views/imports/_nav.html.erb | 2 +- ...3141007_add_optional_account_for_import.rb | 5 +++ db/schema.rb | 4 +- .../import/rows_controller_test.rb | 2 +- 20 files changed, 118 insertions(+), 57 deletions(-) create mode 100644 db/migrate/20250303141007_add_optional_account_for_import.rb diff --git a/app/controllers/import/confirms_controller.rb b/app/controllers/import/confirms_controller.rb index 0c1a8872..1a687d4b 100644 --- a/app/controllers/import/confirms_controller.rb +++ b/app/controllers/import/confirms_controller.rb @@ -4,6 +4,10 @@ class Import::ConfirmsController < ApplicationController before_action :set_import def show + if @import.mapping_steps.empty? + return redirect_to import_path(@import) + end + redirect_to import_clean_path(@import), alert: "You have invalid data, please edit until all errors are resolved" unless @import.cleaned? end diff --git a/app/controllers/import/uploads_controller.rb b/app/controllers/import/uploads_controller.rb index 8efc7c75..42e6c975 100644 --- a/app/controllers/import/uploads_controller.rb +++ b/app/controllers/import/uploads_controller.rb @@ -8,6 +8,7 @@ class Import::UploadsController < ApplicationController def update if csv_valid?(csv_str) + @import.account = Current.family.accounts.find_by(id: params.dig(:import, :account_id)) @import.assign_attributes(raw_file_str: csv_str, col_sep: upload_params[:col_sep]) @import.save!(validate: false) diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index 3fb704fe..e95e6e11 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -18,7 +18,8 @@ class ImportsController < ApplicationController end def create - import = Current.family.imports.create! import_params + account = Current.family.accounts.find_by(id: params.dig(:import, :account_id)) + import = Current.family.imports.create!(type: import_params[:type], account: account) redirect_to import_upload_path(import) end diff --git a/app/models/family.rb b/app/models/family.rb index ffb14a7d..0f71731f 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -106,7 +106,7 @@ class Family < ApplicationRecord # If family has any entries in different currencies, they need a provider for historical exchange rates uniq_currencies = entries.pluck(:currency).uniq return true if uniq_currencies.count > 1 - return true if uniq_currencies.first != self.currency + return true if uniq_currencies.count > 0 && uniq_currencies.first != self.currency false end diff --git a/app/models/import.rb b/app/models/import.rb index 9a21d6bd..600f30a7 100644 --- a/app/models/import.rb +++ b/app/models/import.rb @@ -1,6 +1,7 @@ class Import < ApplicationRecord TYPES = %w[TransactionImport TradeImport AccountImport MintImport].freeze SIGNAGE_CONVENTIONS = %w[inflows_positive inflows_negative] + SEPARATORS = [ [ "Comma (,)", "," ], [ "Semicolon (;)", ";" ] ].freeze NUMBER_FORMATS = { "1,234.56" => { separator: ".", delimiter: "," }, # US/UK/Asia @@ -10,6 +11,7 @@ class Import < ApplicationRecord }.freeze belongs_to :family + belongs_to :account, optional: true before_validation :set_default_number_format @@ -25,7 +27,7 @@ class Import < ApplicationRecord }, validate: true, default: "pending" validates :type, inclusion: { in: TYPES } - validates :col_sep, inclusion: { in: [ ",", ";" ] } + validates :col_sep, inclusion: { in: SEPARATORS.map(&:last) } validates :signage_convention, inclusion: { in: SIGNAGE_CONVENTIONS } validates :number_format, presence: true, inclusion: { in: NUMBER_FORMATS.keys } @@ -98,12 +100,17 @@ class Import < ApplicationRecord end def dry_run - { + mappings = { transactions: rows.count, - accounts: Import::AccountMapping.for_import(self).creational.count, categories: Import::CategoryMapping.for_import(self).creational.count, tags: Import::TagMapping.for_import(self).creational.count } + + mappings.merge( + accounts: Import::AccountMapping.for_import(self).creational.count, + ) if account.nil? + + mappings end def required_column_keys diff --git a/app/models/import/account_mapping.rb b/app/models/import/account_mapping.rb index c4c00414..9cf43f7a 100644 --- a/app/models/import/account_mapping.rb +++ b/app/models/import/account_mapping.rb @@ -1,5 +1,5 @@ class Import::AccountMapping < Import::Mapping - validates :mappable, presence: true, if: -> { key.blank? || !create_when_empty } + validates :mappable, presence: true, if: :requires_mapping? class << self def mapping_values(import) @@ -42,4 +42,9 @@ class Import::AccountMapping < Import::Mapping self.mappable = account save! end + + private + def requires_mapping? + (key.blank? || !create_when_empty) && import.account.nil? + end end diff --git a/app/models/trade_import.rb b/app/models/trade_import.rb index 549c9093..4bbffdaa 100644 --- a/app/models/trade_import.rb +++ b/app/models/trade_import.rb @@ -4,7 +4,11 @@ class TradeImport < Import mappings.each(&:create_mappable!) rows.each do |row| - account = mappings.accounts.mappable_for(row.account) + mapped_account = if account + account + else + mappings.accounts.mappable_for(row.account) + end # Try to find or create security with ticker only security = find_or_create_security( @@ -12,15 +16,15 @@ class TradeImport < Import exchange_operating_mic: row.exchange_operating_mic ) - entry = account.entries.build \ + entry = mapped_account.entries.build \ date: row.date_iso, amount: row.signed_amount, name: row.name, - currency: row.currency.presence || account.currency, + currency: row.currency.presence || mapped_account.currency, entryable: Account::Trade.new( security: security, qty: row.qty, - currency: row.currency.presence || account.currency, + currency: row.currency.presence || mapped_account.currency, price: row.price ), import: self @@ -31,7 +35,9 @@ class TradeImport < Import end def mapping_steps - [ Import::AccountMapping ] + base = [] + base << Import::AccountMapping if account.nil? + base end def required_column_keys @@ -39,14 +45,19 @@ class TradeImport < Import end def column_keys - %i[date ticker exchange_operating_mic currency qty price account name] + base = %i[date ticker exchange_operating_mic currency qty price name] + base.unshift(:account) if account.nil? + base end def dry_run - { - transactions: rows.count, + mappings = { transactions: rows.count } + + mappings.merge( accounts: Import::AccountMapping.for_import(self).creational.count - } + ) if account.nil? + + mappings end def csv_template @@ -57,7 +68,9 @@ class TradeImport < Import 05/17/2024,TSLA,XNAS,USD,2,700.50,Retirement Account,Tesla Inc. Purchase CSV - CSV.parse(template, headers: true) + csv = CSV.parse(template, headers: true) + csv.delete("account") if account.present? + csv end private diff --git a/app/models/transaction_import.rb b/app/models/transaction_import.rb index e2b1c3d6..2bb9d4d5 100644 --- a/app/models/transaction_import.rb +++ b/app/models/transaction_import.rb @@ -4,11 +4,16 @@ class TransactionImport < Import mappings.each(&:create_mappable!) rows.each do |row| - account = mappings.accounts.mappable_for(row.account) + mapped_account = if account + account + else + mappings.accounts.mappable_for(row.account) + end + category = mappings.categories.mappable_for(row.category) tags = row.tags_list.map { |tag| mappings.tags.mappable_for(tag) }.compact - entry = account.entries.build \ + entry = mapped_account.entries.build \ date: row.date_iso, amount: row.signed_amount, name: row.name, @@ -27,11 +32,15 @@ class TransactionImport < Import end def column_keys - %i[date amount name currency category tags account notes] + base = %i[date amount name currency category tags notes] + base.unshift(:account) if account.nil? + base end def mapping_steps - [ Import::CategoryMapping, Import::TagMapping, Import::AccountMapping ] + base = [ Import::CategoryMapping, Import::TagMapping ] + base << Import::AccountMapping if account.nil? + base end def csv_template @@ -42,6 +51,8 @@ class TransactionImport < Import 05/17/2024,-12.50,Coffee Shop,,,coffee,, CSV - CSV.parse(template, headers: true) + csv = CSV.parse(template, headers: true) + csv.delete("account") if account.present? + csv end end diff --git a/app/views/account/trades/_form.html.erb b/app/views/account/trades/_form.html.erb index a74d401f..89c8c151 100644 --- a/app/views/account/trades/_form.html.erb +++ b/app/views/account/trades/_form.html.erb @@ -29,11 +29,11 @@ <% if %w[buy sell].include?(type) %> <% if Security.provider.present? %>
- <%= form.combobox :ticker, - securities_path(country_code: Current.family.country), + <%= form.combobox :ticker, + securities_path(country_code: Current.family.country), name_when_new: "account_entry[manual_ticker]", - label: t(".holding"), - placeholder: t(".ticker_placeholder"), + label: t(".holding"), + placeholder: t(".ticker_placeholder"), required: true %>
<% else %> diff --git a/app/views/accounts/show/_menu.html.erb b/app/views/accounts/show/_menu.html.erb index f41723a6..57241528 100644 --- a/app/views/accounts/show/_menu.html.erb +++ b/app/views/accounts/show/_menu.html.erb @@ -20,8 +20,8 @@ <% end %> <% unless account.crypto? %> - <%= link_to new_import_path, - data: { turbo_frame: :modal }, + <%= button_to imports_path({ import: { type: account.investment? ? "TradeImport" : "TransactionImport", account_id: account.id } }), + data: { turbo_frame: :_top }, class: "block w-full py-2 px-3 space-x-2 text-primary hover:bg-gray-50 flex items-center rounded-lg" do %> <%= lucide_icon "download", class: "w-5 h-5 text-secondary" %> diff --git a/app/views/import/configurations/_mint_import.html.erb b/app/views/import/configurations/_mint_import.html.erb index 1aabce75..987f2fed 100644 --- a/app/views/import/configurations/_mint_import.html.erb +++ b/app/views/import/configurations/_mint_import.html.erb @@ -1,6 +1,6 @@ <%# locals: (import:) %> -
+
<%= lucide_icon("check-circle", class: "w-5 h-5 shrink-0 text-green-500") %>

We have pre-configured your Mint import for you. Please proceed to the next step.

@@ -21,7 +21,10 @@ <%= form.select :number_format, Import::NUMBER_FORMATS.keys, { label: "Format", prompt: "Select format" }, required: true %>
- <%= form.select :account_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Account (optional)" }, disabled: import.complete? %> + <% unless import.account.present? %> + <%= form.select :account_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Account (optional)" }, disabled: import.complete? %> + <% end %> + <%= form.select :name_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Name (optional)" }, disabled: import.complete? %> <%= form.select :category_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Category (optional)" }, disabled: import.complete? %> <%= form.select :tags_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Tags (optional)" }, disabled: import.complete? %> diff --git a/app/views/import/configurations/_trade_import.html.erb b/app/views/import/configurations/_trade_import.html.erb index f923940c..7db04ba9 100644 --- a/app/views/import/configurations/_trade_import.html.erb +++ b/app/views/import/configurations/_trade_import.html.erb @@ -20,7 +20,11 @@ <%= 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)" } %> + + <% unless import.account.present? %> + <%= form.select :account_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Account (optional)" } %> + <% end %> + <%= form.select :name_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Name (optional)" } %> <% unless Security.provider %> diff --git a/app/views/import/configurations/_transaction_import.html.erb b/app/views/import/configurations/_transaction_import.html.erb index 65c9f5f4..a75b1681 100644 --- a/app/views/import/configurations/_transaction_import.html.erb +++ b/app/views/import/configurations/_transaction_import.html.erb @@ -16,7 +16,10 @@ <%= form.select :number_format, Import::NUMBER_FORMATS.keys, { label: "Format", prompt: "Select format" }, required: true %> - <%= form.select :account_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Account (optional)" } %> + <% unless import.account.present? %> + <%= form.select :account_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Account (optional)" } %> + <% end %> + <%= form.select :name_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Name (optional)" } %> <%= form.select :category_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Category (optional)" } %> <%= form.select :tags_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Tags (optional)" } %> diff --git a/app/views/import/confirms/_mappings.html.erb b/app/views/import/confirms/_mappings.html.erb index 2880ec6d..0f07eb2f 100644 --- a/app/views/import/confirms/_mappings.html.erb +++ b/app/views/import/confirms/_mappings.html.erb @@ -3,7 +3,7 @@ <% mappings = mapping_class.for_import(import) %> <% is_last_step = step_idx == import.mapping_steps.count - 1 %> -<% if mapping_class == Import::AccountMapping %> +<% if mapping_class == Import::AccountMapping && import.account.nil? %> <% if import.requires_account? %>
<%= tag.p t(".no_accounts"), class: "text-sm" %> diff --git a/app/views/import/uploads/show.html.erb b/app/views/import/uploads/show.html.erb index 2f126a1c..ab7e4bb2 100644 --- a/app/views/import/uploads/show.html.erb +++ b/app/views/import/uploads/show.html.erb @@ -19,33 +19,33 @@
-
- <%= styled_form_with model: @import, scope: :import, url: import_upload_path(@import), multipart: true, class: "space-y-2" do |form| %> - <%= form.select :col_sep, [["Comma (,)", ","], ["Semicolon (;)", ";"]], label: true %> - <%= form.text_area :raw_file_str, + <% ["csv-paste-tab", "csv-upload-tab"].each do |tab| %> + <%= tag.div id: tab, data: { tabs_target: "tab" }, class: tab == "csv-upload-tab" ? "hidden" : "" do %> + <%= styled_form_with model: @import, scope: :import, url: import_upload_path(@import), multipart: true, class: "space-y-2" do |form| %> + <%= form.select :col_sep, Import::SEPARATORS, label: true %> + + <% unless @import.type == "MintImport" %> + <%= form.select :account_id, @import.family.accounts.pluck(:name, :id), { label: "Account (optional)", include_blank: "Multi-account import", selected: @import.account_id } %> + <% end %> + + <% if tab == "csv-paste-tab" %> + <%= form.text_area :raw_file_str, rows: 10, required: true, placeholder: "Paste your CSV file contents here", "data-auto-submit-form-target": "auto" %> + <% else %> + + <% end %> - <%= form.submit "Upload CSV", disabled: @import.complete? %> + <%= form.submit "Upload CSV", disabled: @import.complete? %> + <% end %> <% end %> - -
- - + <% end %> diff --git a/app/views/imports/_import.html.erb b/app/views/imports/_import.html.erb index 3ee7d68c..26c77ee9 100644 --- a/app/views/imports/_import.html.erb +++ b/app/views/imports/_import.html.erb @@ -2,6 +2,10 @@
<%= link_to import_path(import), class: "text-sm text-primary hover:underline" do %> + <% if import.account.present? %> + <%= import.account.name + " " %> + <% end %> + <%= t(".label", type: import.type.titleize, datetime: import.updated_at.strftime("%b %-d, %Y at %l:%M %p")) %> <% end %> diff --git a/app/views/imports/_nav.html.erb b/app/views/imports/_nav.html.erb index b68b6ce1..0b9bd817 100644 --- a/app/views/imports/_nav.html.erb +++ b/app/views/imports/_nav.html.erb @@ -6,7 +6,7 @@ { name: "Clean", path: import_clean_path(import), is_complete: import.cleaned?, step_number: 3 }, { name: "Map", path: import_confirm_path(import), is_complete: import.publishable?, step_number: 4 }, { name: "Confirm", path: import_path(import), is_complete: import.complete?, step_number: 5 } -] %> +].reject { |step| step[:name] == "Map" && import.mapping_steps.empty? } %>