From 72c6840f5a6ee59b94e954f38f6e5529979a7c95 Mon Sep 17 00:00:00 2001 From: Cameron Roudebush Date: Sat, 17 May 2025 11:08:17 -0400 Subject: [PATCH] fix(simplefin): Fix some querying issues and various rendering bugs - Also adding some nice looking progress indicators for loading SimpleFIN data --- app/controllers/simple_fin_controller.rb | 30 +++++-- app/models/assistant/function/get_accounts.rb | 2 +- app/models/provider/simple_fin.rb | 14 +-- app/models/simple_fin_account.rb | 88 +++++++++++-------- app/views/accounts/_account_error.html.erb | 2 +- app/views/accounts/new.html.erb | 7 ++ app/views/accounts/show/_activity.html.erb | 2 +- app/views/accounts/show/_header.html.erb | 3 +- app/views/holdings/index.html.erb | 26 +++--- app/views/holdings/show.html.erb | 2 +- .../_accounts_list_content.html.erb | 65 ++++++++++++++ app/views/simple_fin/_error_display.html.erb | 4 + app/views/simple_fin/accounts_list.html.erb | 7 ++ app/views/simple_fin/new.html.erb | 72 ++------------- .../_simple_fin_item.html.erb | 38 ++++---- config/locales/simple_fin/en.yml | 2 + config/routes.rb | 1 + 17 files changed, 209 insertions(+), 156 deletions(-) create mode 100644 app/views/simple_fin/_accounts_list_content.html.erb create mode 100644 app/views/simple_fin/_error_display.html.erb create mode 100644 app/views/simple_fin/accounts_list.html.erb diff --git a/app/controllers/simple_fin_controller.rb b/app/controllers/simple_fin_controller.rb index d7bc5f45..cf990ca8 100644 --- a/app/controllers/simple_fin_controller.rb +++ b/app/controllers/simple_fin_controller.rb @@ -3,16 +3,11 @@ class SimpleFinController < ApplicationController before_action :set_accountable_type before_action :authenticate_user! - before_action :set_simple_fin_provider, only: %i[create new] - before_action :require_simple_fin_provider, only: %i[create new] + before_action :set_simple_fin_provider, only: %i[create new accounts_list] + before_action :require_simple_fin_provider, only: %i[create new accounts_list] def new - @simple_fin_accounts = @simple_fin_provider.get_available_accounts(@accountable_type) - # Filter accounts we already have - @simple_fin_accounts = @simple_fin_accounts.filter { |acc| !account_exists(acc) } - rescue StandardError => e - Rails.logger.error "SimpleFIN: Failed to fetch accounts - #{e.message}" - redirect_to root_path, alert: t(".fetch_failed") + # This action now simply renders the view, which will trigger the Turbo Frame load. end ## @@ -21,6 +16,23 @@ class SimpleFinController < ApplicationController Current.family.accounts.find_by(name: acc["name"]) end + def accounts_list + simple_fin_accounts_data = @simple_fin_provider.get_available_accounts(@accountable_type) + # Filter accounts we already have + @simple_fin_accounts = simple_fin_accounts_data.filter { |acc| !account_exists(acc) } + # Implicitly renders app/views/simple_fin/accounts_list.html.erb + rescue Provider::SimpleFin::RateLimitExceededError => e + @error_message = t("simple_fin.new.rate_limit_hit") + # Implicitly renders app/views/simple_fin/accounts_list.html.erb + render :accounts_list # Or just let it render implicitly + rescue StandardError => e + Rails.logger.error "SimpleFIN: Failed to fetch accounts from accounts_list - #{e.message}" + @error_message = t("simple_fin.new.fetch_failed") + # Implicitly renders app/views/simple_fin/accounts_list.html.erb + render :accounts_list # Or just let it render implicitly + end + + ## # Requests all accounts to be re-synced def sync @@ -80,6 +92,8 @@ class SimpleFinController < ApplicationController end redirect_to root_path, notice: t(".accounts_created_success") + rescue Provider::SimpleFin::RateLimitExceededError => e + redirect_to new_account_path, alert: t(".rate_limit_hit") rescue StandardError => e Rails.logger.error "SimpleFIN: Failed to create accounts - #{e.message}" redirect_to new_simple_fin_path diff --git a/app/models/assistant/function/get_accounts.rb b/app/models/assistant/function/get_accounts.rb index 45125115..e703b846 100644 --- a/app/models/assistant/function/get_accounts.rb +++ b/app/models/assistant/function/get_accounts.rb @@ -22,7 +22,7 @@ class Assistant::Function::GetAccounts < Assistant::Function type: account.accountable_type, start_date: account.start_date, is_plaid_linked: account.plaid_account_id.present?, - is_simple_fin_linked: account.simple_fin_id.present?, + is_simple_fin_linked: account.simple_fin_account_id.present?, is_active: account.is_active, historical_balances: historical_balances(account) } diff --git a/app/models/provider/simple_fin.rb b/app/models/provider/simple_fin.rb index 0617d145..749ec4f4 100644 --- a/app/models/provider/simple_fin.rb +++ b/app/models/provider/simple_fin.rb @@ -107,20 +107,20 @@ class Provider::SimpleFin endpoint += "&end-date=#{trans_end_date}" end - # account_info = send_request_to_sf(endpoint) - # accounts = account_info["accounts"] - # TODO: Remove JSON Reading for real requests. Disabled currently due to preventing rate limits. + # request_content = send_request_to_sf(endpoint) + # # TODO: Remove JSON Reading for real requests. Disabled currently due to preventing rate limits. json_file_path = Rails.root.join("sample.simple.fin.json") accounts = [] error_messages = [] if File.exist?(json_file_path) - file_content = File.read(json_file_path) - parsed_json = JSON.parse(file_content) - accounts = parsed_json["accounts"] || [] - error_messages = parsed_json["errors"] || [] + request_content = File.read(json_file_path) else Rails.logger.warn "SimpleFIN: Sample JSON file not found at #{json_file_path}. Returning empty accounts." end + # Parse our content + parsed_json = JSON.parse(request_content) + accounts = parsed_json["accounts"] || [] + error_messages = parsed_json["errors"] || [] # The only way we can really determine types right now is by some properties. Try and set their types diff --git a/app/models/simple_fin_account.rb b/app/models/simple_fin_account.rb index 23ffe543..a68bd1dd 100644 --- a/app/models/simple_fin_account.rb +++ b/app/models/simple_fin_account.rb @@ -35,40 +35,53 @@ class SimpleFinAccount < ApplicationRecord sfc.simple_fin_accounts.find_or_create_by!(external_id: sf_account_data["id"]) do |sfa| balance = get_adjusted_balance(sf_account_data) sfa.current_balance = balance - sfa.available_balance = balance + sfa.available_balance = sf_account_data["available-balance"]&.to_d sfa.currency = sf_account_data["currency"] - new_account = sfc.family.accounts.new( - name: sf_account_data["name"], - balance: 0, - currency: sf_account_data["currency"], - accountable: TYPE_MAPPING[sf_account_data["type"]].new, - subtype: sf_account_data["subtype"], - simple_fin_account: sfa, # Explicitly associate back - last_synced_at: Time.current, # Mark as synced upon creation - # Set cash_balance similar to how Account.create_and_sync might - cash_balance: 0 - ) - new_account.entries.build( - name: "Current Balance", - date: Date.current, - amount: balance, - currency: new_account.currency, - entryable: Valuation.new - ) - new_account.entries.build( - name: "Initial Balance", # This will be the balance as of "yesterday" - date: 1.day.ago.to_date, - amount: 0, - currency: new_account.currency, - entryable: Valuation.new - ) + if sfa.account + account = sfa.account + else + sfa.account = sfc.family.accounts.new( + name: sf_account_data["name"], + balance: sfa.current_balance, + currency: sf_account_data["currency"], + accountable: TYPE_MAPPING[sf_account_data["type"]].new, + subtype: sf_account_data["subtype"], + simple_fin_account: sfa, # Explicitly associate back + last_synced_at: Time.current, # Mark as synced upon creation + # Set cash_balance similar to how Account.create_and_sync might + cash_balance: sfa.available_balance + ) + account = sfa.account + account.save! + transaction do + # Create 2 valuations for new accounts to establish a value history for users to see + account.entries.build( + name: "Current Balance", + date: Date.current, + amount: sfa.current_balance, + currency: account.currency, + entryable: Valuation.new + ) + account.entries.build( + name: "Initial Balance", + date: 1.day.ago.to_date, + amount: 0, + currency: account.currency, + entryable: Valuation.new + ) + + account.save! + end + end + + # Make sure SFA is up to date sfa.save! - new_account.save! - new_account.sync_later - sfa.account = new_account + sfa.sync_account_data!(sf_account_data) + # Sync this account to trick it into showing a correct current balance + account.sync_later end end @@ -81,18 +94,15 @@ class SimpleFinAccount < ApplicationRecord ## # Syncs all account data for the given sf_account_data parameter def sync_account_data!(sf_account_data) - accountable_attributes = { id: self.account.accountable_id } balance = SimpleFinAccount.get_adjusted_balance(sf_account_data) + puts "SFA #{sf_account_data} #{self.account.inspect}" self.update!( current_balance: balance, - available_balance: sf_account_data["available-balance"]&.to_d, - currency: sf_account_data["currency"], - account_attributes: { - id: self.account.id, - balance: balance, - last_synced_at: Time.current, - accountable_attributes: accountable_attributes - } + available_balance: sf_account_data["available-balance"]&.to_d + ) + + self.account.update!( + balance: balance ) institution_errors = sf_account_data["org"]["institution_errors"] @@ -107,7 +117,7 @@ class SimpleFinAccount < ApplicationRecord sync_transactions!(sf_account_data["transactions"]) end - # Sync holdings if present in the data and it's an investment account + # Sync holdings if present in the data and it's an investment account. SimpleFIN doesn't support transactions for holdings accounts if self.account&.investment? && sf_account_data["holdings"].is_a?(Array) sync_holdings!(sf_account_data["holdings"]) end diff --git a/app/views/accounts/_account_error.html.erb b/app/views/accounts/_account_error.html.erb index 157f2065..03b969bc 100644 --- a/app/views/accounts/_account_error.html.erb +++ b/app/views/accounts/_account_error.html.erb @@ -1,7 +1,7 @@ <%# locals: (account:, link_to_path: nil, given_title: nil, target: nil) %> <%# Flag indicators of account issues %> -<% if account.simple_fin_account.simple_fin_item.status == "requires_update" %> +<% if account.simple_fin_account&.simple_fin_item&.status == "requires_update" %> <%= link_to link_to_path, target: target do %> <%= icon( "alert-triangle", diff --git a/app/views/accounts/new.html.erb b/app/views/accounts/new.html.erb index 1ca5229e..4a30ecad 100644 --- a/app/views/accounts/new.html.erb +++ b/app/views/accounts/new.html.erb @@ -1,4 +1,11 @@ <%= render layout: "accounts/new/container", locals: { title: t(".title") } do %> + +<%# Display flash messages, especially alerts for errors %> + <% if flash[:alert] %> + + <% end %>
<% unless params[:classification] == "liability" %> <%= render "account_type", accountable: Depository.new %> diff --git a/app/views/accounts/show/_activity.html.erb b/app/views/accounts/show/_activity.html.erb index 06989684..85b23469 100644 --- a/app/views/accounts/show/_activity.html.erb +++ b/app/views/accounts/show/_activity.html.erb @@ -4,7 +4,7 @@
<%= tag.h2 t(".title"), class: "font-medium text-lg" %> - <% unless (@account.plaid_account_id.present? || @account.simple_fin_account.present?) %> + <% unless (@account.plaid_account_id.present? || @account.simple_fin_account_id.present?) %> <%= render MenuComponent.new(variant: "button") do |menu| %> <% menu.with_button(text: "New", variant: "secondary", icon: "plus") %> diff --git a/app/views/accounts/show/_header.html.erb b/app/views/accounts/show/_header.html.erb index 3780f904..de7dd316 100644 --- a/app/views/accounts/show/_header.html.erb +++ b/app/views/accounts/show/_header.html.erb @@ -36,7 +36,8 @@ size: "sm", href: sync_plaid_item_path(account.plaid_account.plaid_item), disabled: account.syncing?, - frame: :_top + frame: :_top, + title: "Refresh all SimpleFIN accounts" ) %> <% end %> <% elsif account.simple_fin_account_id.present? %> diff --git a/app/views/holdings/index.html.erb b/app/views/holdings/index.html.erb index 2fcd0714..6c0e1ad1 100644 --- a/app/views/holdings/index.html.erb +++ b/app/views/holdings/index.html.erb @@ -1,17 +1,19 @@ <%= turbo_frame_tag dom_id(@account, "holdings") do %>
-
- <%= tag.h2 t(".holdings"), class: "font-medium text-lg" %> - <%= link_to new_trade_path(account_id: @account.id), - id: dom_id(@account, "new_trade"), - data: { turbo_frame: :modal }, - class: "flex gap-1 font-medium items-center bg-gray-50 text-primary p-2 rounded-lg" do %> - - <%= icon("plus", color: "current") %> - - <%= tag.span t(".new_holding"), class: "text-sm" %> - <% end %> -
+ <% unless (@account.plaid_account_id.present? || @account.simple_fin_account_id.present?) %> +
+ <%= tag.h2 t(".holdings"), class: "font-medium text-lg" %> + <%= link_to new_trade_path(account_id: @account.id), + id: dom_id(@account, "new_trade"), + data: { turbo_frame: :modal }, + class: "flex gap-1 font-medium items-center bg-gray-50 text-primary p-2 rounded-lg" do %> + + <%= icon("plus", color: "current") %> + + <%= tag.span t(".new_holding"), class: "text-sm" %> + <% end %> +
+ <% end %>
diff --git a/app/views/holdings/show.html.erb b/app/views/holdings/show.html.erb index 19e4b504..d194438e 100644 --- a/app/views/holdings/show.html.erb +++ b/app/views/holdings/show.html.erb @@ -79,7 +79,7 @@
<% end %> - <% unless @holding.account.plaid_account_id.present? %> + <% unless (@holding.account.plaid_account_id.present? || @holding.account.simple_fin_account_id.present? ) %> <% dialog.with_section(title: t(".settings"), open: true) do %>
diff --git a/app/views/simple_fin/_accounts_list_content.html.erb b/app/views/simple_fin/_accounts_list_content.html.erb new file mode 100644 index 00000000..f55f6247 --- /dev/null +++ b/app/views/simple_fin/_accounts_list_content.html.erb @@ -0,0 +1,65 @@ +<%# locals: simple_fin_accounts, accountable_type %> + +<% if simple_fin_accounts.blank? %> +
+

No accounts found matching this type from SimpleFIN.

+

Please ensure your SimpleFIN subscription is active.

+
+<% else %> + <%= styled_form_with url: simple_fin_index_path(accountable_type: accountable_type), method: :post, data: { turbo: false } do |form| %> + <%# Render each account option parsed from SimpleFIN %> + <% simple_fin_accounts.each_with_index do |account, index| %> +
+ +
+ <% end %> + +
+ <%= form.submit t("simple_fin.form.add_accounts"), class: "w-full btn btn-primary", + data: { + disable_with: "Adding Accounts..." + } %> +
+ <% end %> +<% end %> \ No newline at end of file diff --git a/app/views/simple_fin/_error_display.html.erb b/app/views/simple_fin/_error_display.html.erb new file mode 100644 index 00000000..fc397c8c --- /dev/null +++ b/app/views/simple_fin/_error_display.html.erb @@ -0,0 +1,4 @@ +<%# locals: message, accountable_type %> +
+

<%= message %>

+
\ No newline at end of file diff --git a/app/views/simple_fin/accounts_list.html.erb b/app/views/simple_fin/accounts_list.html.erb new file mode 100644 index 00000000..5773202e --- /dev/null +++ b/app/views/simple_fin/accounts_list.html.erb @@ -0,0 +1,7 @@ + + <% if @error_message %> + <%= render partial: "simple_fin/error_display", locals: { message: @error_message, accountable_type: @accountable_type } %> + <% else %> + <%= render partial: "simple_fin/accounts_list_content", locals: { simple_fin_accounts: @simple_fin_accounts, accountable_type: @accountable_type } %> + <% end %> + \ No newline at end of file diff --git a/app/views/simple_fin/new.html.erb b/app/views/simple_fin/new.html.erb index fafb3de0..2308369c 100644 --- a/app/views/simple_fin/new.html.erb +++ b/app/views/simple_fin/new.html.erb @@ -1,70 +1,14 @@ <%# locals: @simple_fin_accounts %> <%= render layout: "accounts/new/container", locals: { title: "Select SimpleFIN Accounts", back_path: new_account_path } do %> - <% if @simple_fin_accounts.blank? %> +
-

No accounts found matching this type from SimpleFIN.

-

Please ensure your SimpleFIN subscription is active.

- <%= link_to "Try Again", new_simple_fin_path(accountable_type: @accountable_type), class: "mt-4 inline-block text-primary hover:underline" %> +

Loading accounts...

+ <%# Basic SVG Spinner (Tailwind CSS for animation) %> + + + +
- <% else %> - <%= styled_form_with url: simple_fin_index_path(accountable_type: @accountable_type), method: :post, data: { turbo: false } do |form| %> - <%# Render each account option parsed from SimpleFIN %> - <% @simple_fin_accounts.each_with_index do |account, index| %> -
- -
- <% end %> - - -
- <%= form.submit t("simple_fin.form.add_accounts"), class: "w-full btn btn-primary", - data: { - disable_with: "Adding Accounts..." - } %> -
- <% end %> - <% end %> + <% end %> \ No newline at end of file diff --git a/app/views/simple_fin_items/_simple_fin_item.html.erb b/app/views/simple_fin_items/_simple_fin_item.html.erb index 7553498d..16e0e2d0 100644 --- a/app/views/simple_fin_items/_simple_fin_item.html.erb +++ b/app/views/simple_fin_items/_simple_fin_item.html.erb @@ -1,26 +1,22 @@ <%# locals: (simple_fin_item:) %> -
-
-
-
- <% if simple_fin_item.logo.attached? %> - <%= image_tag simple_fin_item.logo, class: "h-8 w-8 rounded-full" %> - <% else %> -
- <%= simple_fin_item.institution_name&.first || "?" %> -
- <% end %> -

- <%= simple_fin_item.institution_name || "SimpleFIN Connection" %> -

+<% if simple_fin_item.accounts.any? %> +
+
+
+
+

+ <%= simple_fin_item.institution_name || "SimpleFIN Connection" %> +

+
+ <%= image_tag "simple-fin-logo.svg", class: "h-6 w-auto", title: "Connected via SimpleFIN" %>
-
-
    - <% simple_fin_item.accounts.includes(:accountable, :logo_attachment).active.alphabetically.each do |account| %> - <%= render "accounts/account", account: account %> - <% end %> -
-
\ No newline at end of file +
    + <% simple_fin_item.accounts.includes(:accountable, :logo_attachment).active.alphabetically.each do |account| %> + <%= render "accounts/account", account: account %> + <% end %> +
+
+<% end %> \ No newline at end of file diff --git a/config/locales/simple_fin/en.yml b/config/locales/simple_fin/en.yml index e4a2a76b..e2d30edd 100644 --- a/config/locales/simple_fin/en.yml +++ b/config/locales/simple_fin/en.yml @@ -5,7 +5,9 @@ en: add_accounts: "Add Selected Accounts" new: fetch_failed: "Failed to fetch accounts" + rate_limit_hit: "You've exceeded the SimpleFIN Rate limit for today" create: accounts_created_success: "Accounts Successfully Created" fetch_failed: "Failed to fetch accounts" no_acc_selected: "No accounts were selected to add" + rate_limit_hit: "You've exceeded the SimpleFIN Rate limit for today" diff --git a/config/routes.rb b/config/routes.rb index 1bea328c..4c84973a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -201,6 +201,7 @@ Rails.application.routes.draw do end # SimpleFIN routes + get "/simple_fin/accounts_list", to: "simple_fin#accounts_list", as: :simple_fin_accounts_list resources :simple_fin, only: %i[create new] do member do post :sync