Consolidate and simplify account pages

app/components/UI/account/chart.html.erb

@@ -1,32 +1,28 @@
-<%# locals: (account:, tooltip: nil, chart_view: nil, **args) %>
-
-<% period =  @period || Period.last_30_days %>
-<% default_value_title = account.asset? ? t(".balance") : t(".owed") %>
-
 <div id="<%= dom_id(account, :chart) %>" class="bg-container shadow-border-xs rounded-xl space-y-2">
   <div class="flex justify-between flex-col-reverse lg:flex-row gap-2 px-4 pt-4 mb-2">
     <div class="space-y-2 w-full">
       <div class="flex items-center gap-1">
-        <%= tag.p account.investment? ? "Total value" : default_value_title, class: "text-sm font-medium text-secondary" %>
+        <%= tag.p title, class: "text-sm font-medium text-secondary" %>
 
         <% if account.investment? %>
-          <%= render "investments/value_tooltip", balance: account.balance_money, holdings: account.balance_money - account.cash_balance_money, cash: account.cash_balance_money %>
+          <%= render "investments/value_tooltip", balance: account.balance_money, holdings: holdings_value_money, cash: account.cash_balance_money %>
         <% end %>
       </div>
       <div class="flex flex-row gap-2 items-baseline">
-        <%= tag.p format_money(account.balance_money), class: "text-primary text-3xl font-medium truncate" %>
-        <% if account.currency != Current.family.currency %>
-          <%= tag.p format_money(account.balance_money.exchange_to(Current.family.currency, fallback_rate: 1)), class: "text-sm font-medium text-secondary" %>
+        <%= tag.p view_balance_money.format, class: "text-primary text-3xl font-medium truncate" %>
+
+        <% if converted_balance_money %>
+          <%= tag.p converted_balance_money.format, class: "text-sm font-medium text-secondary" %>
         <% end %>
       </div>
     </div>
 
-    <%= form_with url: request.path, method: :get, data: { controller: "auto-submit-form" } do |form| %>
+    <%= form_with url: account_path(account), method: :get, data: { controller: "auto-submit-form" } do |form| %>
       <div class="flex items-center gap-2">
-        <% if chart_view.present? %>
+        <% if account.investment? %>
           <%= form.select :chart_view,
             [["Total value", "balance"], ["Holdings", "holdings_balance"], ["Cash", "cash_balance"]],
-            { selected: chart_view },
+            { selected: view },
             class: "bg-container border border-secondary rounded-lg text-sm pr-7 cursor-pointer text-primary focus:outline-hidden focus:ring-0",
             data: { "auto-submit-form-target": "auto" } %>
         <% end %>
@@ -40,7 +36,23 @@
     <% end %>
   </div>
 
-  <%= turbo_frame_tag dom_id(account, :chart_details), src: chart_account_path(account, period: period.key, chart_view: chart_view) do %>
-    <%= render "accounts/chart_loader" %>
+  <%= turbo_frame_tag dom_id(@account, :chart_details) do %>
+    <div class="px-4">
+      <%= render partial: "shared/trend_change", locals: { trend: trend, comparison_label: period.comparison_label } %>
+    </div>
+
+    <div class="h-64 pb-4">
+      <% if series.any? %>
+        <div
+        id="lineChart"
+        class="w-full h-full"
+        data-controller="time-series-chart"
+        data-time-series-chart-data-value="<%= series.to_json %>"></div>
+      <% else %>
+        <div class="w-full h-full flex items-center justify-center">
+          <p class="text-secondary text-sm">No data available</p>
+        </div>
+      <% end %>
+    </div>
   <% end %>
 </div>

app/components/UI/account/chart.rb

@@ -0,0 +1,72 @@
+class UI::Account::Chart < ApplicationComponent
+  attr_reader :account
+
+  def initialize(account:, period: nil, view: nil)
+    @account = account
+    @period = period
+    @view = view
+  end
+
+  def period
+    @period ||= Period.last_30_days
+  end
+
+  def holdings_value_money
+    account.balance_money - account.cash_balance_money
+  end
+
+  def view_balance_money
+    case view
+    when "balance"
+      account.balance_money
+    when "holdings_balance"
+      holdings_value_money
+    when "cash_balance"
+      account.cash_balance_money
+    end
+  end
+
+  def title
+    case account.accountable_type
+    when "Investment", "Crypto"
+      case view
+      when "balance"
+        "Total account value"
+      when "holdings_balance"
+        "Holdings value"
+      when "cash_balance"
+        "Cash value"
+      end
+    when "Property", "Vehicle"
+      "Estimated #{account.accountable_type.humanize.downcase} value"
+    when "CreditCard", "OtherLiability"
+      "Debt balance"
+    when "Loan"
+      "Remaining principal balance"
+    else
+      "Balance"
+    end
+  end
+
+  def foreign_currency?
+    account.currency != account.family.currency
+  end
+
+  def converted_balance_money
+    return nil unless foreign_currency?
+
+    account.balance_money.exchange_to(account.family.currency, fallback_rate: 1)
+  end
+
+  def view
+    @view ||= "balance"
+  end
+
+  def series
+    account.balance_series(period: period, view: view)
+  end
+
+  def trend
+    series.trend
+  end
+end

app/components/UI/account_page.html.erb

@@ -0,0 +1,29 @@
+<%= turbo_stream_from account %>
+
+<%= turbo_frame_tag dom_id(account, :container) do %>
+  <%= tag.div class: "space-y-4 pb-32" do %>
+    <%= render "accounts/show/header", account: account, title: title, subtitle: subtitle %>
+
+    <%= render UI::Account::Chart.new(account: account, period: chart_period, view: chart_view) %>
+
+    <div class="min-h-[800px]" data-testid="account-details">
+      <% if tabs.count > 1 %>
+        <%= render TabsComponent.new(active_tab: active_tab, url_param_key: "tab") do |tabs_container| %>
+          <% tabs_container.with_nav(classes: "max-w-fit") do |nav| %>
+            <% tabs.each do |tab| %>
+              <% nav.with_btn(id: tab, label: tab.to_s.humanize, classes: "px-6") %>
+            <% end %>
+          <% end %>
+
+          <% tabs.each do |tab| %>
+            <% tabs_container.with_panel(tab_id: tab) do %>
+              <%= render tab_partial_name(tab), account: account %>
+            <% end %>
+          <% end %>
+        <% end %>
+      <% else %>
+        <%= render tab_partial_name(tabs.first), account: account %>
+      <% end %>
+    </div>
+  <% end %>
+<% end %>

app/components/UI/account_page.rb

@@ -0,0 +1,45 @@
+class UI::AccountPage < ApplicationComponent
+  attr_reader :account, :chart_view, :chart_period
+
+  def initialize(account:, chart_view: nil, chart_period: nil, active_tab: nil)
+    @account = account
+    @chart_view = chart_view
+    @chart_period = chart_period
+    @active_tab = active_tab
+  end
+
+  def title
+    account.name
+  end
+
+  def subtitle
+    return nil unless account.property?
+
+    account.property.address
+  end
+
+  def active_tab
+    tabs.find { |tab| tab == @active_tab } || tabs.first
+  end
+
+  def tabs
+    case account.accountable_type
+    when "Investment"
+      [ :activity, :holdings ]
+    when "Property", "Vehicle", "Loan"
+      [ :activity, :overview ]
+    else
+      [ :activity ]
+    end
+  end
+
+  def tab_partial_name(tab)
+    case tab
+    when :activity
+      "accounts/show/activity"
+    when :holdings, :overview
+      # Accountable is responsible for implementing the partial in the correct folder
+      "#{account.accountable_type.downcase.pluralize}/tabs/#{tab}"
+    end
+  end
+end

app/components/application_component.rb

@@ -0,0 +1,4 @@
+class ApplicationComponent < ViewComponent::Base
+  # These don't work as expected with helpers.turbo_frame_tag, etc., so we include them here
+  include Turbo::FramesHelper, Turbo::StreamsHelper
+end

app/controllers/accounts_controller.rb

@@ -1,5 +1,5 @@
 class AccountsController < ApplicationController
-  before_action :set_account, only: %i[sync chart sparkline toggle_active]
+  before_action :set_account, only: %i[sync sparkline toggle_active show destroy]
   include Periodable
 
   def index
@@ -25,11 +25,6 @@ class AccountsController < ApplicationController
     redirect_to account_path(@account)
   end
 
-  def chart
-    @chart_view = params[:chart_view] || "balance"
-    render layout: "application"
-  end
-
   def sparkline
     etag_key = @account.family.build_cache_key("#{@account.id}_sparkline", invalidate_on_data_updates: true)
 
@@ -50,6 +45,15 @@ class AccountsController < ApplicationController
     redirect_to accounts_path
   end
 
+  def destroy
+    if @account.linked?
+      redirect_to account_path(@account), alert: "Cannot delete a linked account"
+    else
+      @account.destroy_later
+      redirect_to accounts_path, notice: "Account scheduled for deletion"
+    end
+  end
+
   private
     def family
       Current.family

app/controllers/concerns/accountable_resource.rb

@@ -4,7 +4,7 @@ module AccountableResource
   included do
     include Periodable
 
-    before_action :set_account, only: [ :show, :edit, :update, :destroy ]
+    before_action :set_account, only: [ :show, :edit, :update ]
     before_action :set_link_options, only: :new
   end
 
@@ -61,16 +61,7 @@ module AccountableResource
     end
 
     @account.lock_saved_attributes!
-    redirect_back_or_to @account, notice: t("accounts.update.success", type: accountable_type.name.underscore.humanize)
-  end
-
-  def destroy
-    if @account.linked?
-      redirect_to account_path(@account), alert: "Cannot delete a linked account"
-    else
-      @account.destroy_later
-      redirect_to accounts_path, notice: t("accounts.destroy.success", type: accountable_type.name.underscore.humanize)
-    end
+    redirect_back_or_to account_path(@account), notice: t("accounts.update.success", type: accountable_type.name.underscore.humanize)
   end
 
   private

app/views/accounts/chart.html.erb

@@ -1,22 +0,0 @@
-<% series = @account.balance_series(period: @period, view: @chart_view) %>
-<% trend = series.trend %>
-
-<%= turbo_frame_tag dom_id(@account, :chart_details) do %>
-  <div class="px-4">
-    <%= render partial: "shared/trend_change", locals: { trend: trend, comparison_label: @period.comparison_label } %>
-  </div>
-
-  <div class="h-64 pb-4">
-    <% if series.any? %>
-      <div
-        id="lineChart"
-        class="w-full h-full"
-        data-controller="time-series-chart"
-        data-time-series-chart-data-value="<%= series.to_json %>"></div>
-    <% else %>
-      <div class="w-full h-full flex items-center justify-center">
-        <p class="text-secondary text-sm"><%= t(".data_not_available") %></p>
-      </div>
-    <% end %>
-  </div>
-<% end %>

app/views/accounts/show.html.erb

@@ -1,25 +1,6 @@
-<%= turbo_stream_from @account %>
-
-<%= turbo_frame_tag dom_id(@account, :container) do %>
-  <%= tag.div class: "space-y-4 pb-32" do %>
-    <% if header.present? %>
-      <%= header %>
-    <% else %>
-      <%= render "accounts/show/header", account: account %>
-    <% end %>
-
-    <% if chart.present? %>
-      <%= chart %>
-    <% else %>
-      <%= render "accounts/show/chart", account: account, chart_view: chart_view %>
-    <% end %>
-
-    <div class="min-h-[800px]" data-testid="account-details">
-      <% if tabs.present? %>
-        <%= tabs %>
-      <% else %>
-        <%= render "accounts/show/activity", account: account %>
-      <% end %>
-    </div>
-  <% end %>
-<% end %>
+<%= render UI::AccountPage.new(
+      account: @account, 
+      chart_view: @chart_view, 
+      chart_period: @period, 
+      active_tab: @active_tab
+    ) %>

app/views/accounts/show/_header.html.erb

@@ -1,36 +1,30 @@
-<%# locals: (account:, title: nil, subtitle: nil) %>
+<%# locals: (account:, title:, subtitle: nil) %>
 
 <header class="space-y-4">
   <div class="flex items-center gap-4">
-    <% content = yield %>
+    <div class="flex items-center gap-3 overflow-hidden">
+      <%= render "accounts/logo", account: account %>
 
-    <% if content.present? %>
-      <%= content %>
-    <% else %>
-      <div class="flex items-center gap-3 overflow-hidden">
-        <%= render "accounts/logo", account: account %>
-
-        <div class="flex items-center gap-2">
-          <div class="truncate">
-            <div class="flex items-center gap-3">
-              <h2 class="font-medium text-xl truncate <%= "animate-pulse" if account.syncing? %>"><%= title || account.name %></h2>
-              <% if account.draft? %>
-                <%= render LinkComponent.new(
+      <div class="flex items-center gap-2">
+        <div class="truncate">
+          <div class="flex items-center gap-3">
+            <h2 class="font-medium text-xl truncate <%= "animate-pulse" if account.syncing? %>"><%= title %></h2>
+            <% if account.draft? %>
+              <%= render LinkComponent.new(
                   text: "Complete setup",
                   href: edit_account_path(account),
                   variant: :outline,
                   size: :sm,
                   frame: :modal
                 ) %>
-              <% end %>
-            </div>
-            <% if subtitle.present? %>
-              <p class="text-sm text-secondary"><%= subtitle %></p>
             <% end %>
           </div>
+          <% if subtitle.present? %>
+            <p class="text-sm text-secondary"><%= subtitle %></p>
+          <% end %>
         </div>
       </div>
-    <% end %>
+    </div>
 
     <div class="flex items-center gap-1 ml-auto">
       <% if Rails.env.development? || self_hosted? %>

app/views/credit_cards/show.html.erb

@@ -1,6 +0,0 @@
-<%= render "accounts/show/template",
-           account: @account,
-           tabs: render("accounts/show/tabs", account: @account, tabs: [
-            { key: "activity", contents: render("accounts/show/activity", account: @account) },
-            { key: "overview", contents: render("credit_cards/overview", account: @account) },
-           ]) %>

app/views/cryptos/show.html.erb

@@ -1 +0,0 @@
-<%= render "accounts/show/template", account: @account %>

app/views/depositories/show.html.erb

@@ -1 +0,0 @@
-<%= render "accounts/show/template", account: @account %>

app/views/investments/show.html.erb

@@ -1,7 +0,0 @@
-<%= render "accounts/show/template",
-           account: @account,
-           chart_view: @chart_view,
-           tabs: render("accounts/show/tabs", account: @account, tabs: [
-            { key: "activity", contents: render("accounts/show/activity", account: @account) },
-            { key: "holdings", contents: render("investments/holdings_tab", account: @account) },
-           ]) %>

app/views/loans/show.html.erb

@@ -1,6 +0,0 @@
-<%= render "accounts/show/template",
-           account: @account,
-           tabs: render("accounts/show/tabs", account: @account, tabs: [
-             { key: "activity", contents: render("accounts/show/activity", account: @account) },
-             { key: "overview", contents: render("loans/overview", account: @account) },
-           ]) %>

app/views/other_assets/show.html.erb

@@ -1 +0,0 @@
-<%= render "accounts/show/template", account: @account %>

app/views/other_liabilities/show.html.erb

@@ -1 +0,0 @@
-<%= render "accounts/show/template", account: @account %>

app/views/properties/show.html.erb

@@ -1,7 +0,0 @@
-<%= render "accounts/show/template",
-           account: @account,
-           header: render("accounts/show/header", account: @account, subtitle: @account.property.address),
-           tabs: render("accounts/show/tabs", account: @account, tabs: [
-            { key: "activity", contents: render("accounts/show/activity", account: @account) },
-            { key: "overview", contents: render("properties/overview", account: @account) },
-           ]) %>

app/views/vehicles/show.html.erb

@@ -1,6 +0,0 @@
-<%= render "accounts/show/template",
-           account: @account,
-           tabs: render("accounts/show/tabs", account: @account, tabs: [
-            { key: "activity", contents: render("accounts/show/activity", account: @account) },
-            { key: "overview", contents: render("vehicles/overview", account: @account) },
-           ]) %>

config/routes.rb

@@ -150,10 +150,9 @@ Rails.application.routes.draw do
     end
   end
 
-  resources :accounts, only: %i[index new], shallow: true do
+  resources :accounts, only: %i[index new show destroy], shallow: true do
     member do
       post :sync
-      get :chart
       get :sparkline
       patch :toggle_active
     end
@@ -161,17 +160,13 @@ Rails.application.routes.draw do
 
   # Convenience routes for polymorphic paths
   # Example: account_path(Account.new(accountable: Depository.new)) => /depositories/123
-  direct :account do |model, options|
-    route_for model.accountable_name, model, options
-  end
-
   direct :edit_account do |model, options|
     route_for "edit_#{model.accountable_name}", model, options
   end
 
-  resources :depositories, except: :index
-  resources :investments, except: :index
-  resources :properties, except: :index do
+  resources :depositories, except: [ :index, :show ]
+  resources :investments, except: [ :index, :show ]
+  resources :properties, except: [ :index, :show ] do
     member do
       get :balances
       patch :update_balances
@@ -180,12 +175,12 @@ Rails.application.routes.draw do
       patch :update_address
     end
   end
-  resources :vehicles, except: :index
-  resources :credit_cards, except: :index
-  resources :loans, except: :index
-  resources :cryptos, except: :index
-  resources :other_assets, except: :index
-  resources :other_liabilities, except: :index
+  resources :vehicles, except: [ :index, :show ]
+  resources :credit_cards, except: [ :index, :show ]
+  resources :loans, except: [ :index, :show ]
+  resources :cryptos, except: [ :index, :show ]
+  resources :other_assets, except: [ :index, :show ]
+  resources :other_liabilities, except: [ :index, :show ]
 
   resources :securities, only: :index
 

test/controllers/accounts_controller_test.rb

@@ -11,18 +11,25 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
+  test "should get show" do
+    get account_url(@account)
+    assert_response :success
+  end
+
   test "should sync account" do
     post sync_account_url(@account)
     assert_redirected_to account_url(@account)
   end
 
-  test "should get chart" do
-    get chart_account_url(@account)
-    assert_response :success
-  end
-
   test "should get sparkline" do
     get sparkline_account_url(@account)
     assert_response :success
   end
+
+  test "destroys account" do
+    delete account_url(@account)
+    assert_redirected_to accounts_path
+    assert_enqueued_with job: DestroyJob
+    assert_equal "Account scheduled for deletion", flash[:notice]
+  end
 end

test/controllers/credit_cards_controller_test.rb

@@ -48,7 +48,7 @@ class CreditCardsControllerTest < ActionDispatch::IntegrationTest
 
   test "updates with credit card details" do
     assert_no_difference [ "Account.count", "CreditCard.count" ] do
-      patch account_path(@account), params: {
+      patch credit_card_path(@account), params: {
         account: {
           name: "Updated Credit Card",
           balance: 2000,

test/controllers/loans_controller_test.rb

@@ -46,7 +46,7 @@ class LoansControllerTest < ActionDispatch::IntegrationTest
 
   test "updates with loan details" do
     assert_no_difference [ "Account.count", "Loan.count" ] do
-      patch account_path(@account), params: {
+      patch loan_path(@account), params: {
         account: {
           name: "Updated Loan",
           balance: 45000,

test/controllers/vehicles_controller_test.rb

@@ -45,7 +45,7 @@ class VehiclesControllerTest < ActionDispatch::IntegrationTest
 
   test "updates with vehicle details" do
     assert_no_difference [ "Account.count", "Vehicle.count" ] do
-      patch account_path(@account), params: {
+      patch vehicle_path(@account), params: {
         account: {
           name: "Updated Vehicle",
           balance: 28000,
@@ -64,7 +64,7 @@ class VehiclesControllerTest < ActionDispatch::IntegrationTest
       }
     end
 
-    assert_redirected_to @account
+    assert_redirected_to account_path(@account)
     assert_equal "Vehicle account updated", flash[:notice]
     assert_enqueued_with(job: SyncJob)
   end

test/interfaces/accountable_resource_interface_test.rb

@@ -15,15 +15,4 @@ module AccountableResourceInterfaceTest
     assert_response :success
   end
 
-  test "renders accountable page" do
-    get account_url(@account)
-    assert_response :success
-  end
-
-  test "destroys account" do
-    delete account_url(@account)
-    assert_redirected_to accounts_path
-    assert_enqueued_with job: DestroyJob
-    assert_equal "#{@account.accountable_name.underscore.humanize} account scheduled for deletion", flash[:notice]
-  end
 end