Multi-step account forms + clearer balance editing (#2427)

* Initial multi-step property form

* Improve form structure, add optional tooltip help icons to form fields

* Add basic inline alert component

* Clean up and improve property form lifecycle

* Implement Account status concept

* Lint fixes

* Remove whitespace

* Balance editing, scope updates for account

* Passing tests

* Fix brakeman warning

* Remove stale columns

* data constraint tweaks

* Redundant property

test/components/previews/alert_component_preview.rb

@@ -0,0 +1,7 @@
+class AlertComponentPreview < Lookbook::Preview
+  # @param message text
+  # @param variant select [info, success, warning, error]
+  def default(message: "This is an alert message.", variant: :info)
+    render AlertComponent.new(message: message, variant: variant.to_sym)
+  end
+end

test/controllers/api/v1/accounts_controller_test.rb

@@ -81,7 +81,7 @@ end
   test "should only return active accounts" do
     # Make one account inactive
     inactive_account = accounts(:depository)
-    inactive_account.update!(is_active: false)
+    inactive_account.disable!
 
     access_token = Doorkeeper::AccessToken.create!(
       application: @oauth_app,

test/controllers/properties_controller_test.rb

@@ -8,72 +8,169 @@ class PropertiesControllerTest < ActionDispatch::IntegrationTest
     @account = accounts(:property)
   end
 
-  test "creates with property details" do
-    assert_difference -> { Account.count } => 1,
-      -> { Property.count } => 1,
-      -> { Valuation.count } => 2,
-      -> { Entry.count } => 2 do
+  test "creates property in draft status and redirects to balances step" do
+    assert_difference -> { Account.count } => 1 do
       post properties_path, params: {
         account: {
-          name: "Property",
-          balance: 500000,
-          currency: "USD",
+          name: "New Property",
+          subtype: "house",
           accountable_type: "Property",
           accountable_attributes: {
-            year_built: 2002,
-            area_value: 1000,
-            area_unit: "sqft",
-            address_attributes: {
-              line1: "123 Main St",
-              line2: "Apt 1",
-              locality: "Los Angeles",
-              region: "CA", # ISO3166-2 code
-              country: "US", # ISO3166-1 Alpha-2 code
-              postal_code: "90001"
-            }
+            year_built: 1990,
+            area_value: 1200,
+            area_unit: "sqft"
           }
         }
       }
     end
 
     created_account = Account.order(:created_at).last
-
-    assert created_account.accountable.year_built.present?
-    assert created_account.accountable.address.line1.present?
-
-    assert_redirected_to created_account
-    assert_equal "Property account created", flash[:notice]
-    assert_enqueued_with(job: SyncJob)
+    assert created_account.accountable.is_a?(Property)
+    assert_equal "draft", created_account.status
+    assert_equal 0, created_account.balance
+    assert_equal 1990, created_account.accountable.year_built
+    assert_equal 1200, created_account.accountable.area_value
+    assert_equal "sqft", created_account.accountable.area_unit
+    assert_redirected_to balances_property_path(created_account)
   end
 
-  test "updates with property details" do
+  test "updates property overview" do
     assert_no_difference [ "Account.count", "Property.count" ] do
-      patch account_path(@account), params: {
+      patch property_path(@account), params: {
         account: {
           name: "Updated Property",
-          balance: 500000,
-          currency: "USD",
-          accountable_type: "Property",
-          accountable_attributes: {
-            id: @account.accountable_id,
-            year_built: 2002,
-            area_value: 1000,
-            area_unit: "sqft",
-            address_attributes: {
-              line1: "123 Main St",
-              line2: "Apt 1",
-              locality: "Los Angeles",
-              region: "CA", # ISO3166-2 code
-              country: "US", # ISO3166-1 Alpha-2 code
-              postal_code: "90001"
-            }
-          }
+          subtype: "condo"
         }
       }
     end
 
-    assert_redirected_to @account
-    assert_equal "Property account updated", flash[:notice]
-    assert_enqueued_with(job: SyncJob)
+    @account.reload
+    assert_equal "Updated Property", @account.name
+    assert_equal "condo", @account.subtype
+
+    # If account is active, it renders edit view; otherwise redirects to balances
+    if @account.active?
+      assert_response :success
+    else
+      assert_redirected_to balances_property_path(@account)
+    end
+  end
+
+  # Tab view tests
+  test "shows balances tab" do
+    get balances_property_path(@account)
+    assert_response :success
+  end
+
+  test "shows address tab" do
+    get address_property_path(@account)
+    assert_response :success
+  end
+
+  # Tab update tests
+  test "updates balances tab" do
+    original_balance = @account.balance
+
+    # Mock the update_balance method to return a successful result
+    Account::BalanceUpdater::Result.any_instance.stubs(:success?).returns(true)
+    Account::BalanceUpdater::Result.any_instance.stubs(:updated?).returns(true)
+
+    patch update_balances_property_path(@account), params: {
+      account: {
+        balance: 600000,
+        currency: "EUR"
+      }
+    }
+
+    # If account is active, it renders balances view; otherwise redirects to address
+    if @account.reload.active?
+      assert_response :success
+    else
+      assert_redirected_to address_property_path(@account)
+    end
+  end
+
+  test "updates address tab" do
+    patch update_address_property_path(@account), params: {
+      property: {
+        address_attributes: {
+          line1: "456 New Street",
+          locality: "San Francisco",
+          region: "CA",
+          country: "US",
+          postal_code: "94102"
+        }
+      }
+    }
+
+    @account.reload
+    assert_equal "456 New Street", @account.accountable.address.line1
+    assert_equal "San Francisco", @account.accountable.address.locality
+
+    # If account is draft, it activates and redirects; otherwise renders address
+    if @account.draft?
+      assert_redirected_to account_path(@account)
+    else
+      assert_response :success
+    end
+  end
+
+  test "balances update handles validation errors" do
+    # Mock update_balance to return a failure result
+    Account::BalanceUpdater::Result.any_instance.stubs(:success?).returns(false)
+    Account::BalanceUpdater::Result.any_instance.stubs(:error_message).returns("Invalid balance")
+
+    patch update_balances_property_path(@account), params: {
+      account: {
+        balance: 600000,
+        currency: "EUR"
+      }
+    }
+
+    assert_response :unprocessable_entity
+  end
+
+  test "address update handles validation errors" do
+    Property.any_instance.stubs(:update).returns(false)
+
+    patch update_address_property_path(@account), params: {
+      property: {
+        address_attributes: {
+          line1: "123 Test St"
+        }
+      }
+    }
+
+    assert_response :unprocessable_entity
+  end
+
+  test "address update activates draft account" do
+    # Create a draft property account
+    draft_account = Account.create!(
+      family: @user.family,
+      name: "Draft Property",
+      accountable: Property.new,
+      status: "draft",
+      balance: 500000,
+      currency: "USD"
+    )
+
+    assert draft_account.draft?
+
+    patch update_address_property_path(draft_account), params: {
+      property: {
+        address_attributes: {
+          line1: "789 Activate St",
+          locality: "New York",
+          region: "NY",
+          country: "US",
+          postal_code: "10001"
+        }
+      }
+    }
+
+    draft_account.reload
+    assert draft_account.active?
+    assert_redirected_to account_path(draft_account)
   end
 end

test/controllers/valuations_controller_test.rb

@@ -8,35 +8,24 @@ class ValuationsControllerTest < ActionDispatch::IntegrationTest
     @entry = entries(:valuation)
   end
 
-  test "error when valuation already exists for date" do
-    assert_no_difference [ "Entry.count", "Valuation.count" ] do
-      post valuations_url(@entry.account), params: {
-        entry: {
-          account_id: @entry.account_id,
-          amount: 19800,
-          date: @entry.date,
-          currency: "USD"
-        }
-      }
-    end
-
-    assert_response :unprocessable_entity
-  end
-
   test "creates entry with basic attributes" do
+    account = accounts(:investment)
+
     assert_difference [ "Entry.count", "Valuation.count" ], 1 do
       post valuations_url, params: {
         entry: {
-          name: "New entry",
-          amount: 10000,
+          amount: account.balance + 100,
           currency: "USD",
-          date: Date.current,
-          account_id: @entry.account_id
+          date: Date.current.to_s,
+          account_id: account.id
         }
       }
     end
 
     created_entry = Entry.order(created_at: :desc).first
+    assert_equal "Manual account value update", created_entry.name
+    assert_equal Date.current, created_entry.date
+    assert_equal account.balance + 100, created_entry.amount_money.to_f
 
     assert_enqueued_with job: SyncJob
 
@@ -47,7 +36,6 @@ class ValuationsControllerTest < ActionDispatch::IntegrationTest
     assert_no_difference [ "Entry.count", "Valuation.count" ] do
       patch valuation_url(@entry), params: {
         entry: {
-          name: "Updated entry",
           amount: 20000,
           currency: "USD",
           date: Date.current

test/fixtures/accounts.yml

@@ -5,6 +5,7 @@ other_asset:
   currency: USD
   accountable_type: OtherAsset
   accountable: one
+  status: active
 
 other_liability:
   family: dylan_family
@@ -13,6 +14,7 @@ other_liability:
   currency: USD
   accountable_type: OtherLiability
   accountable: one
+  status: active
 
 depository:
   family: dylan_family
@@ -21,6 +23,7 @@ depository:
   currency: USD
   accountable_type: Depository
   accountable: one
+  status: active
 
 connected:
   family: dylan_family
@@ -31,6 +34,7 @@ connected:
   accountable_type: Depository
   accountable: two
   plaid_account: one
+  status: active
 
 credit_card:
   family: dylan_family
@@ -39,6 +43,7 @@ credit_card:
   currency: USD
   accountable_type: CreditCard
   accountable: one
+  status: active
 
 investment:
   family: dylan_family
@@ -48,6 +53,7 @@ investment:
   currency: USD
   accountable_type: Investment
   accountable: one
+  status: active
 
 loan:
   family: dylan_family
@@ -56,6 +62,7 @@ loan:
   currency: USD
   accountable_type: Loan
   accountable: one
+  status: active
 
 property:
   family: dylan_family
@@ -64,6 +71,7 @@ property:
   currency: USD
   accountable_type: Property
   accountable: one
+  status: active
 
 vehicle:
   family: dylan_family
@@ -72,6 +80,7 @@ vehicle:
   currency: USD
   accountable_type: Vehicle
   accountable: one
+  status: active
 
 crypto:
   family: dylan_family
@@ -80,3 +89,4 @@ crypto:
   currency: USD
   accountable_type: Crypto
   accountable: one
+  status: active

test/interfaces/accountable_resource_interface_test.rb

@@ -26,66 +26,4 @@ module AccountableResourceInterfaceTest
     assert_enqueued_with job: DestroyJob
     assert_equal "#{@account.accountable_name.underscore.humanize} account scheduled for deletion", flash[:notice]
   end
-
-  test "updates basic account balances" do
-    assert_no_difference [ "Account.count", "@account.accountable_class.count" ] do
-      patch account_url(@account), params: {
-        account: {
-          name: "Updated name",
-          balance: 10000,
-          currency: "USD"
-        }
-      }
-    end
-
-    assert_redirected_to @account
-    assert_equal "#{@account.accountable_name.underscore.humanize} account updated", flash[:notice]
-  end
-
-  test "creates with basic attributes" do
-    assert_difference [ "Account.count", "@account.accountable_class.count" ], 1 do
-      post "/#{@account.accountable_name.pluralize}", params: {
-        account: {
-          accountable_type: @account.accountable_class,
-          name: "New accountable",
-          balance: 10000,
-          currency: "USD",
-          subtype: "checking"
-        }
-      }
-    end
-
-    assert_redirected_to Account.order(:created_at).last
-    assert_equal "#{@account.accountable_name.humanize} account created", flash[:notice]
-  end
-
-  test "updates account balance by creating new valuation if balance has changed" do
-    assert_difference [ "Entry.count", "Valuation.count" ], 1 do
-      patch account_url(@account), params: {
-        account: {
-          balance: 12000
-        }
-      }
-    end
-
-    assert_redirected_to @account
-    assert_enqueued_with job: SyncJob
-    assert_equal "#{@account.accountable_name.humanize} account updated", flash[:notice]
-  end
-
-  test "updates account balance by editing existing valuation for today" do
-    @account.entries.create! date: Date.current, amount: 6000, currency: "USD", name: "Balance update", entryable: Valuation.new
-
-    assert_no_difference [ "Entry.count", "Valuation.count" ] do
-      patch account_url(@account), params: {
-        account: {
-          balance: 12000
-        }
-      }
-    end
-
-    assert_redirected_to @account
-    assert_enqueued_with job: SyncJob
-    assert_equal "#{@account.accountable_name.humanize} account updated", flash[:notice]
-  end
 end

test/models/account/entry_test.rb

@@ -67,21 +67,21 @@ class EntryTest < ActiveSupport::TestCase
     assert_equal 0, family.entries.search(params).size
   end
 
-  test "active scope only returns entries from active accounts" do
+  test "visible scope only returns entries from visible accounts" do
     # Create transactions for all account types
-    active_transaction = create_transaction(account: accounts(:depository), name: "Active transaction")
-    inactive_transaction = create_transaction(account: accounts(:credit_card), name: "Inactive transaction")
+    visible_transaction = create_transaction(account: accounts(:depository), name: "Visible transaction")
+    invisible_transaction = create_transaction(account: accounts(:credit_card), name: "Invisible transaction")
 
     # Update account statuses
-    accounts(:credit_card).update!(is_active: false)
+    accounts(:credit_card).disable!
 
     # Test the scope
-    active_entries = Entry.active
+    visible_entries = Entry.visible
 
     # Should include entry from active account
-    assert_includes active_entries, active_transaction
+    assert_includes visible_entries, visible_transaction
 
-    # Should not include entry from inactive account
-    assert_not_includes active_entries, inactive_transaction
+    # Should not include entry from disabled account
+    assert_not_includes visible_entries, invisible_transaction
   end
 end

test/models/balance_sheet_test.rb

@@ -39,7 +39,7 @@ class BalanceSheetTest < ActiveSupport::TestCase
     create_account(balance: 10000, accountable: Depository.new)
 
     other_liability = create_account(balance: 5000, accountable: OtherLiability.new)
-    other_liability.update!(is_active: false)
+    other_liability.disable!
 
     assert_equal 10000 - 1000, BalanceSheet.new(@family).net_worth
     assert_equal 10000, BalanceSheet.new(@family).assets.total

test/models/family/auto_transfer_matchable_test.rb

@@ -89,7 +89,7 @@ class Family::AutoTransferMatchableTest < ActiveSupport::TestCase
   end
 
   test "does not consider inactive accounts when matching transfers" do
-    @depository.update!(is_active: false)
+    @depository.disable!
 
     outflow = create_transaction(date: Date.current, account: @depository, amount: 500)
     inflow = create_transaction(date: Date.current, account: @credit_card, amount: -500)

test/system/accounts_test.rb

@@ -23,15 +23,40 @@ class AccountsTest < ApplicationSystemTestCase
   end
 
   test "can create property account" do
-    assert_account_created "Property" do
-      fill_in "Year built", with: 2005
-      fill_in "Living area", with: 2250
-      fill_in "Street address", with: "123 Main St"
-      fill_in "City", with: "San Francisco"
-      fill_in "State/Province", with: "CA"
-      fill_in "ZIP/Postal code", with: "94101"
-      fill_in "Country", with: "US"
-    end
+    # Step 1: Select property type and enter basic details
+    click_link "Property"
+
+    account_name = "[system test] Property Account"
+    fill_in "Name*", with: account_name
+    select "Single Family Home", from: "Property type*"
+    fill_in "Year Built (optional)", with: 2005
+    fill_in "Area (optional)", with: 2250
+
+    click_button "Next"
+
+    # Step 2: Enter balance information
+    assert_text "Value"
+    fill_in "account[balance]", with: 500000
+    click_button "Next"
+
+    # Step 3: Enter address information
+    assert_text "Address"
+    fill_in "Address Line 1", with: "123 Main St"
+    fill_in "City", with: "San Francisco"
+    fill_in "State/Region", with: "CA"
+    fill_in "Postal Code", with: "94101"
+    fill_in "Country", with: "US"
+
+    click_button "Save"
+
+    # Verify account was created and is now active
+    assert_text account_name
+
+    created_account = Account.order(:created_at).last
+    assert_equal "active", created_account.status
+    assert_equal 500000, created_account.balance
+    assert_equal "123 Main St", created_account.property.address.line1
+    assert_equal "San Francisco", created_account.property.address.locality
   end
 
   test "can create vehicle account" do