From 560c9fbff3b80ea8f79192db3c2728356d2ae4a6 Mon Sep 17 00:00:00 2001 From: soky srm Date: Wed, 25 Mar 2026 10:50:23 +0100 Subject: [PATCH] Family sharing (#1272) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Initial account sharing changes * Update schema.rb * Update schema.rb * Change sharing UI to modal * UX fixes and sharing controls * Scope include in finances better * Update totals.rb * Update totals.rb * Scope reports to finance account scope * Update impersonation_sessions_controller_test.rb * Review fixes * Update schema.rb * Update show.html.erb * FIX db validation * Refine edit permissions * Review items * Review * Review * Add application level helper * Critical review * Address remaining review items * Fix modals * more scoping * linter * small UI fix * Fix: Sync broadcasts push unscoped balance sheet to all users * Update sync_complete_event.rb The fix removes the sidebar broadcasts (which rendered unscoped account groups using family.balance_sheet without user context) along with the now-unused sidebar_targets, account_group, and family_balance_sheet private methods. The sidebar will still update correctly — when the sync completes, Family::SyncCompleteEvent#broadcast fires family.broadcast_refresh, which triggers a morph-based page refresh for each user with their own authenticated session, rendering properly scoped sidebar content. --- app/components/DS/dialog.html.erb | 2 +- .../account_sharings_controller.rb | 62 ++++ app/controllers/accounts_controller.rb | 21 +- app/controllers/application_controller.rb | 10 + .../concerns/accountable_resource.rb | 32 +- .../concerns/entryable_resource.rb | 32 +- app/controllers/holdings_controller.rb | 20 +- app/controllers/import/uploads_controller.rb | 4 +- app/controllers/imports_controller.rb | 4 +- app/controllers/pages_controller.rb | 12 +- .../pending_duplicate_merges_controller.rb | 8 +- app/controllers/properties_controller.rb | 16 +- app/controllers/reports_controller.rb | 53 ++-- app/controllers/splits_controller.rb | 10 +- app/controllers/trades_controller.rb | 34 ++- .../transaction_attachments_controller.rb | 22 +- .../transaction_categories_controller.rb | 9 +- .../transactions/bulk_deletions_controller.rb | 18 +- app/controllers/transactions_controller.rb | 103 ++++++- .../transfer_matches_controller.rb | 14 +- app/controllers/transfers_controller.rb | 49 ++- app/controllers/users_controller.rb | 15 +- app/controllers/valuations_controller.rb | 40 ++- app/models/account.rb | 75 ++++- app/models/account/sync_complete_event.rb | 35 --- app/models/account_share.rb | 47 +++ app/models/balance_sheet.rb | 11 +- app/models/balance_sheet/account_totals.rb | 28 +- .../balance_sheet/classification_group.rb | 2 +- .../balance_sheet/net_worth_series_builder.rb | 14 +- app/models/current.rb | 15 + app/models/family.rb | 18 +- app/models/family/sync_complete_event.rb | 27 +- app/models/income_statement.rb | 22 +- app/models/income_statement/category_stats.rb | 9 +- app/models/income_statement/family_stats.rb | 9 +- app/models/income_statement/totals.rb | 17 +- app/models/investment_flow_statement.rb | 13 +- app/models/investment_statement.rb | 19 +- app/models/invitation.rb | 10 + app/models/transaction/search.rb | 11 +- app/models/user.rb | 28 ++ app/policies/account_policy.rb | 38 +++ app/views/account_sharings/show.html.erb | 92 ++++++ app/views/accounts/_account.html.erb | 34 ++- .../accounts/_accountable_group.html.erb | 3 + app/views/accounts/index/_account_groups.erb | 1 + app/views/accounts/show/_activity.html.erb | 42 +-- app/views/accounts/show/_header.html.erb | 3 + app/views/accounts/show/_menu.html.erb | 42 +-- app/views/imports/new.html.erb | 2 +- app/views/pages/dashboard.html.erb | 3 +- app/views/settings/preferences/show.html.erb | 19 ++ app/views/trades/_form.html.erb | 2 +- .../create.turbo_stream.erb | 2 +- .../destroy.turbo_stream.erb | 2 +- app/views/transactions/_attachments.html.erb | 24 +- app/views/transactions/_form.html.erb | 2 +- .../searches/filters/_account_filter.html.erb | 2 +- app/views/transactions/show.html.erb | 288 +++++++++--------- config/locales/views/account_sharings/en.yml | 29 ++ config/locales/views/accounts/en.yml | 2 + config/locales/views/settings/en.yml | 5 + config/routes.rb | 2 + ...60324100000_add_account_sharing_support.rb | 21 ++ ...0001_backfill_account_owners_and_shares.rb | 34 +++ ...dd_check_constraints_to_sharing_columns.rb | 6 + ...003_change_accounts_owner_fk_to_nullify.rb | 11 + db/schema.rb | 27 +- .../impersonation_sessions_controller_test.rb | 2 +- .../transactions_controller_test.rb | 4 +- test/fixtures/account_shares.yml | 13 + test/fixtures/accounts.yml | 10 + test/models/account_share_test.rb | 61 ++++ test/models/account_test.rb | 93 ++++++ 75 files changed, 1520 insertions(+), 401 deletions(-) create mode 100644 app/controllers/account_sharings_controller.rb create mode 100644 app/models/account_share.rb create mode 100644 app/policies/account_policy.rb create mode 100644 app/views/account_sharings/show.html.erb create mode 100644 config/locales/views/account_sharings/en.yml create mode 100644 db/migrate/20260324100000_add_account_sharing_support.rb create mode 100644 db/migrate/20260324100001_backfill_account_owners_and_shares.rb create mode 100644 db/migrate/20260324100002_add_check_constraints_to_sharing_columns.rb create mode 100644 db/migrate/20260324100003_change_accounts_owner_fk_to_nullify.rb create mode 100644 test/fixtures/account_shares.yml create mode 100644 test/models/account_share_test.rb diff --git a/app/components/DS/dialog.html.erb b/app/components/DS/dialog.html.erb index 3487ed89e..307303970 100644 --- a/app/components/DS/dialog.html.erb +++ b/app/components/DS/dialog.html.erb @@ -2,7 +2,7 @@ <%= tag.dialog class: "w-full h-full bg-transparent theme-dark:backdrop:bg-alpha-black-900 backdrop:bg-overlay pt-[env(safe-area-inset-top)] pb-[env(safe-area-inset-bottom)] #{(drawer? || responsive?) ? "lg:p-3" : "lg:p-1"}", **merged_opts do %> <%= tag.div class: dialog_outer_classes do %> <%= tag.div class: dialog_inner_classes, data: { DS__dialog_target: "content" } do %> -
+
"> <% if header? %> <%= header %> <% end %> diff --git a/app/controllers/account_sharings_controller.rb b/app/controllers/account_sharings_controller.rb new file mode 100644 index 000000000..f04bcee6e --- /dev/null +++ b/app/controllers/account_sharings_controller.rb @@ -0,0 +1,62 @@ +class AccountSharingsController < ApplicationController + before_action :set_account + + def show + @family_members = Current.family.users.where.not(id: @account.owner_id).where(active: true) + @account_shares = @account.account_shares.includes(:user).index_by(&:user_id) + end + + def update + # Non-owners can update their own include_in_finances preference + if !@account.owned_by?(Current.user) && params[:update_finance_inclusion].present? + share = @account.account_shares.find_by!(user: Current.user) + include_value = params.permit(:include_in_finances)[:include_in_finances] + share.update!(include_in_finances: ActiveModel::Type::Boolean.new.cast(include_value)) + redirect_back_or_to account_path(@account), notice: t("account_sharings.update.finance_toggle_success") + return + end + + unless @account.owned_by?(Current.user) + redirect_to account_path(@account), alert: t("account_sharings.update.not_owner") + return + end + + eligible_members = Current.family.users.where.not(id: @account.owner_id).where(active: true) + + AccountShare.transaction do + sharing_members_params.each do |member_params| + user = eligible_members.find_by(id: member_params[:user_id]) + next unless user + + share = @account.account_shares.find_by(user: user) + + if ActiveModel::Type::Boolean.new.cast(member_params[:shared]) + permission = AccountShare::PERMISSIONS.include?(member_params[:permission]) ? member_params[:permission] : (share&.permission || "read_only") + if share + share.update!(permission: permission) + else + @account.account_shares.create!(user: user, permission: permission, include_in_finances: true) + end + elsif share + share.destroy! + end + end + end + + redirect_back_or_to accounts_path, notice: t("account_sharings.update.success") + end + + private + + def set_account + @account = Current.user.accessible_accounts.find(params[:account_id]) + end + + def sharing_members_params + return [] unless params.dig(:sharing, :members) + + params.require(:sharing).permit( + members: [ :user_id, :shared, :permission ] + )[:members]&.values || [] + end +end diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 5fcb3df66..60dc96853 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -1,10 +1,15 @@ class AccountsController < ApplicationController - before_action :set_account, only: %i[sync sparkline toggle_active set_default remove_default show destroy unlink confirm_unlink select_provider] + include StreamExtensions + + before_action :set_account, only: %i[show sparkline sync set_default remove_default] + before_action :set_manageable_account, only: %i[toggle_active destroy unlink confirm_unlink select_provider] include Periodable def index + @accessible_account_ids = Current.user.accessible_accounts.pluck(:id) @manual_accounts = family.accounts .listable_manual + .where(id: @accessible_account_ids) .order(:name) @plaid_items = family.plaid_items.ordered.includes(:syncs, :plaid_accounts) @simplefin_items = family.simplefin_items.ordered.includes(:syncs) @@ -200,7 +205,19 @@ class AccountsController < ApplicationController end def set_account - @account = family.accounts.find(params[:id]) + @account = Current.user.accessible_accounts.find(params[:id]) + end + + def set_manageable_account + @account = Current.user.accessible_accounts.find(params[:id]) + permission = @account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_to account_path(@account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_to(account_path(@account), alert: t("accounts.not_authorized")) } + end + nil + end end # Builds sync stats maps for all provider types to avoid N+1 queries in views diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 310abfcd0..79c92d76c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -81,4 +81,14 @@ class ApplicationController < ActionController::Base def show_demo_warning? demo_host_match? end + + def accessible_accounts + Current.accessible_accounts + end + helper_method :accessible_accounts + + def finance_accounts + Current.finance_accounts + end + helper_method :finance_accounts end diff --git a/app/controllers/concerns/accountable_resource.rb b/app/controllers/concerns/accountable_resource.rb index 003d61edc..fcf51dbe9 100644 --- a/app/controllers/concerns/accountable_resource.rb +++ b/app/controllers/concerns/accountable_resource.rb @@ -2,9 +2,10 @@ module AccountableResource extend ActiveSupport::Concern included do - include Periodable + include Periodable, StreamExtensions - before_action :set_account, only: [ :show, :edit, :update ] + before_action :set_account, only: [ :show ] + before_action :set_manageable_account, only: [ :edit, :update ] before_action :set_link_options, only: :new end @@ -39,11 +40,14 @@ module AccountableResource rescue Date::Error nil end || (Time.zone.today - 2.years) - @account = Current.family.accounts.create_and_sync( - account_params.except(:return_to, :opening_balance_date), - opening_balance_date: opening_balance_date - ) - @account.lock_saved_attributes! + Account.transaction do + @account = Current.family.accounts.create_and_sync( + account_params.except(:return_to, :opening_balance_date).merge(owner: Current.user), + opening_balance_date: opening_balance_date + ) + @account.lock_saved_attributes! + @account.auto_share_with_family! if Current.family.share_all_by_default? + end redirect_to account_params[:return_to].presence || @account, notice: t("accounts.create.success", type: accountable_type.name.underscore.humanize) end @@ -87,7 +91,19 @@ module AccountableResource end def set_account - @account = Current.family.accounts.find(params[:id]) + @account = Current.user.accessible_accounts.find(params[:id]) + end + + def set_manageable_account + @account = Current.user.accessible_accounts.find(params[:id]) + permission = @account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_to account_path(@account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_to(account_path(@account), alert: t("accounts.not_authorized")) } + end + nil + end end def account_params diff --git a/app/controllers/concerns/entryable_resource.rb b/app/controllers/concerns/entryable_resource.rb index 443b04832..11b34d83b 100644 --- a/app/controllers/concerns/entryable_resource.rb +++ b/app/controllers/concerns/entryable_resource.rb @@ -5,13 +5,15 @@ module EntryableResource include StreamExtensions, ActionView::RecordIdentifier before_action :set_entry, only: %i[show update destroy] + + helper_method :can_edit_entry?, :can_annotate_entry? end def show end def new - account = Current.family.accounts.find_by(id: params[:account_id]) + account = accessible_accounts.find_by(id: params[:account_id]) @entry = Current.family.entries.new( account: account, @@ -29,11 +31,18 @@ module EntryableResource end def destroy - account = @entry.account + unless can_edit_entry? + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account), alert: t("accounts.not_authorized")) } + end + return + end + @entry.destroy! @entry.sync_account_later - redirect_back_or_to account_path(account), notice: t("account.entries.destroy.success") + redirect_back_or_to account_path(@entry.account), notice: t("account.entries.destroy.success") end private @@ -42,6 +51,21 @@ module EntryableResource end def set_entry - @entry = Current.family.entries.find(params[:id]) + @entry = Current.family.entries + .joins(:account) + .merge(Account.accessible_by(Current.user)) + .find(params[:id]) + end + + def entry_permission + @entry_permission ||= @entry&.account&.permission_for(Current.user) + end + + def can_edit_entry? + entry_permission.in?([ :owner, :full_control ]) + end + + def can_annotate_entry? + entry_permission.in?([ :owner, :full_control, :read_write ]) end end diff --git a/app/controllers/holdings_controller.rb b/app/controllers/holdings_controller.rb index 24f7c9726..98e30a03c 100644 --- a/app/controllers/holdings_controller.rb +++ b/app/controllers/holdings_controller.rb @@ -1,8 +1,11 @@ class HoldingsController < ApplicationController + include StreamExtensions + before_action :set_holding, only: %i[show update destroy unlock_cost_basis remap_security reset_security sync_prices] + before_action :require_holding_write_permission!, only: %i[update destroy unlock_cost_basis remap_security reset_security] def index - @account = Current.family.accounts.find(params[:account_id]) + @account = accessible_accounts.find(params[:account_id]) end def show @@ -137,7 +140,20 @@ class HoldingsController < ApplicationController private def set_holding - @holding = Current.family.holdings.find(params[:id]) + @holding = Current.family.holdings + .joins(:account) + .merge(Account.accessible_by(Current.user)) + .find(params[:id]) + end + + def require_holding_write_permission! + permission = @holding.account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@holding.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@holding.account), alert: t("accounts.not_authorized")) } + end + end end def holding_params diff --git a/app/controllers/import/uploads_controller.rb b/app/controllers/import/uploads_controller.rb index 3d8d44732..3cf58ffda 100644 --- a/app/controllers/import/uploads_controller.rb +++ b/app/controllers/import/uploads_controller.rb @@ -19,7 +19,7 @@ class Import::UploadsController < ApplicationController elsif @import.is_a?(SureImport) update_sure_import_upload elsif csv_valid?(csv_str) - @import.account = Current.family.accounts.find_by(id: import_account_id) + @import.account = accessible_accounts.find_by(id: import_account_id) @import.assign_attributes(raw_file_str: csv_str, col_sep: upload_params[:col_sep]) @import.save!(validate: false) @@ -78,7 +78,7 @@ class Import::UploadsController < ApplicationController end ActiveRecord::Base.transaction do - @import.account = Current.family.accounts.find(import_account_id) + @import.account = accessible_accounts.find(import_account_id) @import.raw_file_str = QifParser.normalize_encoding(csv_str) @import.save!(validate: false) @import.generate_rows_from_csv diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index d8f581c1b..4a3cc0492 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -8,7 +8,7 @@ class ImportsController < ApplicationController account_id = params.dig(:pdf_import, :account_id) || params.dig(:import, :account_id) if account_id.present? - account = Current.family.accounts.find_by(id: account_id) + account = accessible_accounts.find_by(id: account_id) unless account redirect_back_or_to import_path(@import), alert: t("imports.update.invalid_account", default: "Account not found.") return @@ -67,7 +67,7 @@ class ImportsController < ApplicationController type = params.dig(:import, :type).to_s type = "TransactionImport" unless Import::TYPES.include?(type) - account = Current.family.accounts.find_by(id: params.dig(:import, :account_id)) + account = accessible_accounts.find_by(id: params.dig(:import, :account_id)) import = Current.family.imports.create!( type: type, account: account, diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index 29a00cdf8..e0802c1c5 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -11,7 +11,7 @@ class PagesController < ApplicationController @balance_sheet = Current.family.balance_sheet @investment_statement = Current.family.investment_statement - @accounts = Current.family.accounts.visible.with_attached_logo + @accounts = Current.user.accessible_accounts.visible.with_attached_logo family_currency = Current.family.currency @@ -90,7 +90,7 @@ class PagesController < ApplicationController title: "pages.dashboard.cashflow_sankey.title", partial: "pages/dashboard/cashflow_sankey", locals: { sankey_data: @cashflow_sankey_data, period: @period }, - visible: Current.family.accounts.any?, + visible: @accounts.any?, collapsible: true }, { @@ -98,7 +98,7 @@ class PagesController < ApplicationController title: "pages.dashboard.outflows_donut.title", partial: "pages/dashboard/outflows_donut", locals: { outflows_data: @outflows_data, period: @period }, - visible: Current.family.accounts.any? && @outflows_data[:categories].present?, + visible: @accounts.any? && @outflows_data[:categories].present?, collapsible: true }, { @@ -106,7 +106,7 @@ class PagesController < ApplicationController title: "pages.dashboard.investment_summary.title", partial: "pages/dashboard/investment_summary", locals: { investment_statement: @investment_statement, period: @period }, - visible: Current.family.accounts.any? && @investment_statement.investment_accounts.any?, + visible: @accounts.any? && @investment_statement.investment_accounts.any?, collapsible: true }, { @@ -114,7 +114,7 @@ class PagesController < ApplicationController title: "pages.dashboard.net_worth_chart.title", partial: "pages/dashboard/net_worth_chart", locals: { balance_sheet: @balance_sheet, period: @period }, - visible: Current.family.accounts.any?, + visible: @accounts.any?, collapsible: true }, { @@ -122,7 +122,7 @@ class PagesController < ApplicationController title: "pages.dashboard.balance_sheet.title", partial: "pages/dashboard/balance_sheet", locals: { balance_sheet: @balance_sheet }, - visible: Current.family.accounts.any?, + visible: @accounts.any?, collapsible: true } ] diff --git a/app/controllers/pending_duplicate_merges_controller.rb b/app/controllers/pending_duplicate_merges_controller.rb index 460f9894e..3e1d68786 100644 --- a/app/controllers/pending_duplicate_merges_controller.rb +++ b/app/controllers/pending_duplicate_merges_controller.rb @@ -17,6 +17,12 @@ class PendingDuplicateMergesController < ApplicationController end def create + permission = @transaction.entry.account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + redirect_back_or_to transactions_path, alert: t("accounts.not_authorized") + return + end + # Manually merge the pending transaction with the selected posted transaction unless merge_params[:posted_entry_id].present? redirect_back_or_to transactions_path, alert: "Please select a posted transaction to merge with" @@ -54,7 +60,7 @@ class PendingDuplicateMergesController < ApplicationController private def set_transaction - entry = Current.family.entries.find(params[:transaction_id]) + entry = Current.accessible_entries.find(params[:transaction_id]) @transaction = entry.entryable unless @transaction.is_a?(Transaction) && @transaction.pending? diff --git a/app/controllers/properties_controller.rb b/app/controllers/properties_controller.rb index c222d5a74..563738efb 100644 --- a/app/controllers/properties_controller.rb +++ b/app/controllers/properties_controller.rb @@ -2,6 +2,7 @@ class PropertiesController < ApplicationController include AccountableResource, StreamExtensions before_action :set_property, only: [ :balances, :address, :update_balances, :update_address ] + before_action :require_property_write_permission!, only: [ :update_balances, :update_address ] def new @account = Current.family.accounts.build(accountable: Property.new) @@ -9,8 +10,9 @@ class PropertiesController < ApplicationController def create @account = Current.family.accounts.create!( - property_params.merge(currency: Current.family.currency, balance: 0, status: "draft") + property_params.merge(currency: Current.family.currency, balance: 0, status: "draft", owner: Current.user) ) + @account.auto_share_with_family! if Current.family.share_all_by_default? redirect_to balances_property_path(@account) end @@ -100,7 +102,17 @@ class PropertiesController < ApplicationController end def set_property - @account = Current.family.accounts.find(params[:id]) + @account = accessible_accounts.find(params[:id]) @property = @account.property end + + def require_property_write_permission! + permission = @account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@account), alert: t("accounts.not_authorized")) } + end + end + end end diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index d76a82c12..83c9b0f76 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -124,10 +124,10 @@ class ReportsController < ApplicationController @investment_metrics = build_investment_metrics # Investment flows (contributions/withdrawals) - @investment_flows = InvestmentFlowStatement.new(Current.family).period_totals(period: @period) + @investment_flows = InvestmentFlowStatement.new(Current.family, user: Current.user).period_totals(period: @period) # Flags for view rendering - @has_accounts = Current.family.accounts.any? + @has_accounts = accessible_accounts.any? end def preferences_params @@ -145,7 +145,7 @@ class ReportsController < ApplicationController title: "reports.net_worth.title", partial: "reports/net_worth", locals: { net_worth_metrics: @net_worth_metrics }, - visible: Current.family.accounts.any?, + visible: accessible_accounts.any?, collapsible: true }, { @@ -153,7 +153,7 @@ class ReportsController < ApplicationController title: "reports.trends.title", partial: "reports/trends_insights", locals: { trends_data: @trends_data }, - visible: Current.family.transactions.any?, + visible: @has_accounts, collapsible: true }, { @@ -182,7 +182,7 @@ class ReportsController < ApplicationController start_date: @start_date, end_date: @end_date }, - visible: Current.family.transactions.any?, + visible: @has_accounts, collapsible: true } ] @@ -353,7 +353,7 @@ class ReportsController < ApplicationController .where.not(kind: Transaction::BUDGET_EXCLUDED_KINDS) .includes(entry: :account, category: :parent) - # Apply filters + # Apply filters (includes finance account scoping) transactions = apply_transaction_filters(transactions) # Get trades in the period (matching income_statement logic) @@ -364,6 +364,8 @@ class ReportsController < ApplicationController .where(entries: { entryable_type: "Trade", excluded: false, date: @period.date_range }) .includes(entry: :account, category: :parent) + trades = apply_entry_filters(trades) + # Get sort parameters sort_by = params[:sort_by] || "amount" sort_direction = params[:sort_direction] || "desc" @@ -558,49 +560,60 @@ class ReportsController < ApplicationController } end - def apply_transaction_filters(transactions) + def apply_transaction_filters(scope) + scope = apply_entry_filters(scope) + + # Filter by tag (Transaction-specific — trades don't have taggings) + if params[:filter_tag_id].present? + scope = scope.joins(:taggings).where(taggings: { tag_id: params[:filter_tag_id] }) + end + + scope + end + + # Filters applicable to both transactions and trades (entry-level + category) + def apply_entry_filters(scope) + # Scope to user's finance accounts + finance_account_ids = Current.user&.finance_accounts&.pluck(:id) || [] + scope = scope.where(entries: { account_id: finance_account_ids }) + # Filter by category (including subcategories) if params[:filter_category_id].present? category_id = params[:filter_category_id] # Scope to family's categories to prevent cross-family data access subcategory_ids = Current.family.categories.where(parent_id: category_id).pluck(:id) all_category_ids = [ category_id ] + subcategory_ids - transactions = transactions.where(category_id: all_category_ids) + scope = scope.where(category_id: all_category_ids) end # Filter by account if params[:filter_account_id].present? - transactions = transactions.where(entries: { account_id: params[:filter_account_id] }) - end - - # Filter by tag - if params[:filter_tag_id].present? - transactions = transactions.joins(:taggings).where(taggings: { tag_id: params[:filter_tag_id] }) + scope = scope.where(entries: { account_id: params[:filter_account_id] }) end # Filter by amount range if params[:filter_amount_min].present? - transactions = transactions.where("ABS(entries.amount) >= ?", params[:filter_amount_min].to_f) + scope = scope.where("ABS(entries.amount) >= ?", params[:filter_amount_min].to_f) end if params[:filter_amount_max].present? - transactions = transactions.where("ABS(entries.amount) <= ?", params[:filter_amount_max].to_f) + scope = scope.where("ABS(entries.amount) <= ?", params[:filter_amount_max].to_f) end # Filter by date range (within the period) if params[:filter_date_start].present? filter_start = Date.parse(params[:filter_date_start]) - transactions = transactions.where("entries.date >= ?", filter_start) if filter_start >= @start_date + scope = scope.where("entries.date >= ?", filter_start) if filter_start >= @start_date end if params[:filter_date_end].present? filter_end = Date.parse(params[:filter_date_end]) - transactions = transactions.where("entries.date <= ?", filter_end) if filter_end <= @end_date + scope = scope.where("entries.date <= ?", filter_end) if filter_end <= @end_date end - transactions + scope rescue Date::Error - transactions + scope end def build_transactions_breakdown_for_export diff --git a/app/controllers/splits_controller.rb b/app/controllers/splits_controller.rb index 3fab45a1b..c7b264af5 100644 --- a/app/controllers/splits_controller.rb +++ b/app/controllers/splits_controller.rb @@ -1,5 +1,6 @@ class SplitsController < ApplicationController before_action :set_entry + before_action :require_split_write_permission!, only: %i[create update destroy] def new @categories = Current.family.categories.alphabetically @@ -82,7 +83,14 @@ class SplitsController < ApplicationController private def set_entry - @entry = Current.family.entries.find(params[:transaction_id]) + @entry = Current.accessible_entries.find(params[:transaction_id]) + end + + def require_split_write_permission! + permission = @entry.account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + redirect_back_or_to transactions_path, alert: t("accounts.not_authorized") + end end def resolve_to_parent! diff --git a/app/controllers/trades_controller.rb b/app/controllers/trades_controller.rb index e3061a719..8dc280e70 100644 --- a/app/controllers/trades_controller.rb +++ b/app/controllers/trades_controller.rb @@ -5,7 +5,7 @@ class TradesController < ApplicationController # Defaults to a buy trade def new - @account = Current.family.accounts.find_by(id: params[:account_id]) + @account = accessible_accounts.find_by(id: params[:account_id]) @model = Current.family.entries.new( account: @account, currency: @account ? @account.currency : Current.family.currency, @@ -15,7 +15,16 @@ class TradesController < ApplicationController # Can create a trade, transaction (e.g. "fees"), or transfer (e.g. "withdrawal") def create - @account = Current.family.accounts.find(params[:account_id]) + @account = accessible_accounts.find(params[:account_id]) + + unless @account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@account), alert: t("accounts.not_authorized")) } + end + return + end + @model = Trade::CreateForm.new(create_params.merge(account: @account)).create if @model.persisted? @@ -37,6 +46,14 @@ class TradesController < ApplicationController end def update + unless can_edit_entry? + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account), alert: t("accounts.not_authorized")) } + end + return + end + if @entry.update(update_entry_params) @entry.lock_saved_attributes! @entry.mark_user_modified! @@ -69,6 +86,14 @@ class TradesController < ApplicationController end def unlock + unless @entry.account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account), alert: t("accounts.not_authorized")) } + end + return + end + @entry.unlock_for_sync! flash[:notice] = t("entries.unlock.success") @@ -77,7 +102,10 @@ class TradesController < ApplicationController private def set_entry_for_unlock - trade = Current.family.trades.find(params[:id]) + trade = Current.family.trades + .joins(entry: :account) + .merge(Account.accessible_by(Current.user)) + .find(params[:id]) @entry = trade.entry end diff --git a/app/controllers/transaction_attachments_controller.rb b/app/controllers/transaction_attachments_controller.rb index 6aa39bffd..2a9ff5567 100644 --- a/app/controllers/transaction_attachments_controller.rb +++ b/app/controllers/transaction_attachments_controller.rb @@ -1,6 +1,7 @@ class TransactionAttachmentsController < ApplicationController before_action :set_transaction before_action :set_attachment, only: [ :show, :destroy ] + before_action :set_permissions, only: [ :create, :destroy ] def show disposition = params[:disposition] == "attachment" ? "attachment" : "inline" @@ -8,6 +9,11 @@ class TransactionAttachmentsController < ApplicationController end def create + unless @can_upload + redirect_back_or_to transaction_path(@transaction), alert: t("accounts.not_authorized") + return + end + attachments = attachment_params if attachments.present? @@ -60,6 +66,11 @@ class TransactionAttachmentsController < ApplicationController end def destroy + unless @can_delete + redirect_back_or_to transaction_path(@transaction), alert: t("accounts.not_authorized") + return + end + @attachment.purge message = t("transactions.attachments.attachment_deleted") respond_to do |format| @@ -77,13 +88,22 @@ class TransactionAttachmentsController < ApplicationController private def set_transaction - @transaction = Current.family.transactions.find(params[:transaction_id]) + @transaction = Current.family.transactions + .joins(entry: :account) + .merge(Account.accessible_by(Current.user)) + .find(params[:transaction_id]) end def set_attachment @attachment = @transaction.attachments.find(params[:id]) end + def set_permissions + permission = @transaction.entry.account.permission_for(Current.user) + @can_upload = permission.in?([ :owner, :full_control, :read_write ]) + @can_delete = permission.in?([ :owner, :full_control ]) + end + def attachment_params if params.has_key?(:attachments) Array(params.fetch(:attachments, [])).reject(&:blank?).map do |param| diff --git a/app/controllers/transaction_categories_controller.rb b/app/controllers/transaction_categories_controller.rb index d8fc64e96..1acffed57 100644 --- a/app/controllers/transaction_categories_controller.rb +++ b/app/controllers/transaction_categories_controller.rb @@ -2,7 +2,14 @@ class TransactionCategoriesController < ApplicationController include ActionView::RecordIdentifier def update - @entry = Current.family.entries.transactions.find(params[:transaction_id]) + @entry = Current.accessible_entries.transactions.find(params[:transaction_id]) + + permission = @entry.account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control, :read_write ]) + redirect_back_or_to transaction_path(@entry), alert: t("accounts.not_authorized") + return + end + @entry.update!(entry_params) transaction = @entry.transaction diff --git a/app/controllers/transactions/bulk_deletions_controller.rb b/app/controllers/transactions/bulk_deletions_controller.rb index 99951d26d..37a85ed88 100644 --- a/app/controllers/transactions/bulk_deletions_controller.rb +++ b/app/controllers/transactions/bulk_deletions_controller.rb @@ -1,7 +1,11 @@ class Transactions::BulkDeletionsController < ApplicationController def create # Exclude split children from bulk delete - they must be deleted via unsplit on parent - entries_scope = Current.family.entries.where(parent_entry_id: nil) + # Only allow deletion from accounts where user has owner or full_control permission + writable_account_ids = writable_accounts.pluck(:id) + entries_scope = Current.family.entries + .where(account_id: writable_account_ids) + .where(parent_entry_id: nil) destroyed = entries_scope.destroy_by(id: bulk_delete_params[:entry_ids]) destroyed.map(&:account).uniq.each(&:sync_later) redirect_back_or_to transactions_url, notice: "#{destroyed.count} transaction#{destroyed.count == 1 ? "" : "s"} deleted" @@ -11,4 +15,16 @@ class Transactions::BulkDeletionsController < ApplicationController def bulk_delete_params params.require(:bulk_delete).permit(entry_ids: []) end + + # Accounts where the user can delete entries (owner or full_control) + def writable_accounts + Current.family.accounts + .left_joins(:account_shares) + .where( + "accounts.owner_id = :uid OR (account_shares.user_id = :uid AND account_shares.permission = :perm)", + uid: Current.user.id, + perm: "full_control" + ) + .distinct + end end diff --git a/app/controllers/transactions_controller.rb b/app/controllers/transactions_controller.rb index 096c313e5..43c4c9ff0 100644 --- a/app/controllers/transactions_controller.rb +++ b/app/controllers/transactions_controller.rb @@ -15,7 +15,8 @@ class TransactionsController < ApplicationController def index @q = search_params - @search = Transaction::Search.new(Current.family, filters: @q) + accessible_account_ids = Current.user.accessible_accounts.pluck(:id) + @search = Transaction::Search.new(Current.family, filters: @q, accessible_account_ids: accessible_account_ids) base_scope = @search.transactions_scope .reverse_chronological @@ -90,7 +91,16 @@ class TransactionsController < ApplicationController end def create - account = Current.family.accounts.find(params.dig(:entry, :account_id)) + account = Current.user.accessible_accounts.find(params.dig(:entry, :account_id)) + + unless account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(account), alert: t("accounts.not_authorized")) } + end + return + end + @entry = account.entries.new(entry_params) if @entry.save @@ -111,7 +121,7 @@ class TransactionsController < ApplicationController end def update - if @entry.update(entry_params) + if @entry.update(permitted_entry_params) transaction = @entry.transaction if needs_rule_notification?(transaction) @@ -155,7 +165,15 @@ class TransactionsController < ApplicationController end def merge_duplicate - transaction = Current.family.transactions.includes(entry: :account).find(params[:id]) + transaction = accessible_transactions.includes(entry: :account).find(params[:id]) + + unless transaction.entry.account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(transaction.entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(transaction.entry.account), alert: t("accounts.not_authorized")) } + end + return + end if transaction.merge_with_duplicate! flash[:notice] = t("transactions.merge_duplicate.success") @@ -171,7 +189,15 @@ class TransactionsController < ApplicationController end def dismiss_duplicate - transaction = Current.family.transactions.includes(entry: :account).find(params[:id]) + transaction = accessible_transactions.includes(entry: :account).find(params[:id]) + + unless transaction.entry.account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(transaction.entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(transaction.entry.account), alert: t("accounts.not_authorized")) } + end + return + end if transaction.dismiss_duplicate_suggestion! flash[:notice] = t("transactions.dismiss_duplicate.success") @@ -187,9 +213,17 @@ class TransactionsController < ApplicationController end def convert_to_trade - @transaction = Current.family.transactions.includes(entry: :account).find(params[:id]) + @transaction = accessible_transactions.includes(entry: :account).find(params[:id]) @entry = @transaction.entry + unless @entry.account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account), alert: t("accounts.not_authorized")) } + end + return + end + unless @entry.account.investment? flash[:alert] = t("transactions.convert_to_trade.errors.not_investment_account") redirect_back_or_to transactions_path @@ -200,9 +234,17 @@ class TransactionsController < ApplicationController end def create_trade_from_transaction - @transaction = Current.family.transactions.includes(entry: :account).find(params[:id]) + @transaction = accessible_transactions.includes(entry: :account).find(params[:id]) @entry = @transaction.entry + unless @entry.account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account), alert: t("accounts.not_authorized")) } + end + return + end + # Pre-transaction validations unless @entry.account.investment? flash[:alert] = t("transactions.convert_to_trade.errors.not_investment_account") @@ -277,6 +319,14 @@ class TransactionsController < ApplicationController end def unlock + unless @entry.account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account), alert: t("accounts.not_authorized")) } + end + return + end + @entry.unlock_for_sync! flash[:notice] = t("entries.unlock.success") @@ -284,7 +334,15 @@ class TransactionsController < ApplicationController end def mark_as_recurring - transaction = Current.family.transactions.includes(entry: :account).find(params[:id]) + transaction = accessible_transactions.includes(entry: :account).find(params[:id]) + + unless transaction.entry.account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(transaction.entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(transaction.entry.account), alert: t("accounts.not_authorized")) } + end + return + end # Check if a recurring transaction already exists for this pattern existing = Current.family.recurring_transactions.find_by( @@ -334,10 +392,16 @@ class TransactionsController < ApplicationController end private + def accessible_transactions + Current.family.transactions + .joins(entry: :account) + .merge(Account.accessible_by(Current.user)) + end + def duplicate_source return @duplicate_source if defined?(@duplicate_source) @duplicate_source = if params[:duplicate_entry_id].present? - source = Current.family.entries.find_by(id: params[:duplicate_entry_id]) + source = Current.family.entries.joins(:account).merge(Account.accessible_by(Current.user)).find_by(id: params[:duplicate_entry_id]) source if source&.transaction? end end @@ -364,7 +428,7 @@ class TransactionsController < ApplicationController end def set_entry_for_unlock - transaction = Current.family.transactions.find(params[:id]) + transaction = accessible_transactions.find(params[:id]) @entry = transaction.entry end @@ -398,6 +462,25 @@ class TransactionsController < ApplicationController entry_params end + # Filters entry_params based on the user's permission on the account. + # read_write users can only annotate (category, tags, notes, merchant). + # read_only users cannot update anything. + def permitted_entry_params + case entry_permission + when :owner, :full_control + entry_params + when :read_write + # Annotate only: category, tags, merchant, notes + ep = entry_params.slice(:notes) + if entry_params[:entryable_attributes].present? + ep[:entryable_attributes] = entry_params[:entryable_attributes].slice(:id, :category_id, :merchant_id, :tag_ids) + end + ep + else + {} # read_only — no edits allowed + end + end + def search_params cleaned_params = params.fetch(:q, {}) .permit( diff --git a/app/controllers/transfer_matches_controller.rb b/app/controllers/transfer_matches_controller.rb index ea9425039..5792db0be 100644 --- a/app/controllers/transfer_matches_controller.rb +++ b/app/controllers/transfer_matches_controller.rb @@ -2,11 +2,17 @@ class TransferMatchesController < ApplicationController before_action :set_entry def new - @accounts = Current.family.accounts.visible.alphabetically.where.not(id: @entry.account_id) + @accounts = accessible_accounts.visible.alphabetically.where.not(id: @entry.account_id) @transfer_match_candidates = @entry.transaction.transfer_match_candidates end def create + permission = @entry.account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + redirect_back_or_to transactions_path, alert: t("accounts.not_authorized") + return + end + @transfer = build_transfer Transfer.transaction do @transfer.save! @@ -32,7 +38,7 @@ class TransferMatchesController < ApplicationController private def set_entry - @entry = Current.family.entries.find(params[:transaction_id]) + @entry = Current.accessible_entries.find(params[:transaction_id]) end def transfer_match_params @@ -41,7 +47,7 @@ class TransferMatchesController < ApplicationController def build_transfer if transfer_match_params[:method] == "new" - target_account = Current.family.accounts.find(transfer_match_params[:target_account_id]) + target_account = accessible_accounts.find(transfer_match_params[:target_account_id]) missing_transaction = Transaction.new( entry: target_account.entries.build( @@ -60,7 +66,7 @@ class TransferMatchesController < ApplicationController transfer.status = "confirmed" transfer else - target_transaction = Current.family.entries.find(transfer_match_params[:matched_entry_id]) + target_transaction = Current.accessible_entries.find(transfer_match_params[:matched_entry_id]) transfer = Transfer.find_or_initialize_by( inflow_transaction: @entry.amount.negative? ? @entry.transaction : target_transaction.transaction, diff --git a/app/controllers/transfers_controller.rb b/app/controllers/transfers_controller.rb index 21d68bb78..b7c571d85 100644 --- a/app/controllers/transfers_controller.rb +++ b/app/controllers/transfers_controller.rb @@ -7,7 +7,7 @@ class TransfersController < ApplicationController @transfer = Transfer.new @from_account_id = params[:from_account_id] - @accounts = Current.family.accounts + @accounts = accessible_accounts .alphabetically .includes( :account_providers, @@ -20,10 +20,23 @@ class TransfersController < ApplicationController end def create + # Validate user has write access to both accounts + source_account = accessible_accounts.find(transfer_params[:from_account_id]) + destination_account = accessible_accounts.find(transfer_params[:to_account_id]) + + unless source_account.permission_for(Current.user).in?([ :owner, :full_control ]) && + destination_account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to transactions_path, alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(transactions_path, alert: t("accounts.not_authorized")) } + end + return + end + @transfer = Transfer::Creator.new( family: Current.family, - source_account_id: transfer_params[:from_account_id], - destination_account_id: transfer_params[:to_account_id], + source_account_id: source_account.id, + destination_account_id: destination_account.id, date: Date.parse(transfer_params[:date]), amount: transfer_params[:amount].to_d ).create @@ -41,6 +54,16 @@ class TransfersController < ApplicationController end def update + outflow_account = @transfer.outflow_transaction.entry.account + permission = outflow_account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to transactions_url, alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(transactions_url, alert: t("accounts.not_authorized")) } + end + return + end + Transfer.transaction do update_transfer_status update_transfer_details unless transfer_update_params[:status] == "rejected" @@ -53,16 +76,32 @@ class TransfersController < ApplicationController end def destroy + # Require write permission on at least the outflow account + outflow_account = @transfer.outflow_transaction.entry.account + permission = outflow_account.permission_for(Current.user) + unless permission.in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to transactions_url, alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(transactions_url, alert: t("accounts.not_authorized")) } + end + return + end + @transfer.destroy! redirect_back_or_to transactions_url, notice: t(".success") end private def set_transfer - # Finds the transfer and ensures the family owns it + # Finds the transfer and ensures the user has access to it + accessible_transaction_ids = Current.family.transactions + .joins(entry: :account) + .merge(Account.accessible_by(Current.user)) + .select(:id) + @transfer = Transfer .where(id: params[:id]) - .where(inflow_transaction_id: Current.family.transactions.select(:id)) + .where(inflow_transaction_id: accessible_transaction_ids) .first! end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index fd0c33f6e..937452e95 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -12,7 +12,7 @@ class UsersController < ApplicationController def update @user = Current.user - return if moniker_change_requested? && !ensure_admin + return if admin_family_change_requested? && !ensure_admin if email_changed? if @user.initiate_email_change(user_params[:email]) @@ -109,7 +109,7 @@ class UsersController < ApplicationController params.require(:user).permit( :first_name, :last_name, :email, :profile_image, :redirect_to, :delete_profile_image, :onboarded_at, :show_sidebar, :default_period, :default_account_order, :show_ai_sidebar, :ai_enabled, :theme, :set_onboarding_preferences_at, :set_onboarding_goals_at, :locale, - family_attributes: [ :name, :currency, :country, :date_format, :timezone, :locale, :month_start_day, :moniker, :id ], + family_attributes: [ :name, :currency, :country, :date_format, :timezone, :locale, :month_start_day, :moniker, :default_account_sharing, :id ], goals: [] ) end @@ -118,11 +118,14 @@ class UsersController < ApplicationController @user = Current.user end - def moniker_change_requested? - requested_moniker = params.dig(:user, :family_attributes, :moniker) - return false if requested_moniker.blank? + def admin_family_change_requested? + family_attrs = params.dig(:user, :family_attributes) + return false if family_attrs.blank? - requested_moniker != Current.family.moniker + moniker_changed = family_attrs[:moniker].present? && family_attrs[:moniker] != Current.family.moniker + sharing_changed = family_attrs[:default_account_sharing].present? && family_attrs[:default_account_sharing] != Current.family.default_account_sharing + + moniker_changed || sharing_changed end def ensure_admin diff --git a/app/controllers/valuations_controller.rb b/app/controllers/valuations_controller.rb index 5234c0bea..e8c08736c 100644 --- a/app/controllers/valuations_controller.rb +++ b/app/controllers/valuations_controller.rb @@ -2,7 +2,16 @@ class ValuationsController < ApplicationController include EntryableResource, StreamExtensions def confirm_create - @account = Current.family.accounts.find(params.dig(:entry, :account_id)) + @account = accessible_accounts.find(params.dig(:entry, :account_id)) + + unless @account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@account), alert: t("accounts.not_authorized")) } + end + return + end + @entry = @account.entries.build(entry_params.merge(currency: @account.currency)) @reconciliation_dry_run = @entry.account.create_reconciliation( @@ -15,7 +24,16 @@ class ValuationsController < ApplicationController end def confirm_update - @entry = Current.family.entries.find(params[:id]) + @entry = Current.accessible_entries.find(params[:id]) + + unless @entry.account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account), alert: t("accounts.not_authorized")) } + end + return + end + @account = @entry.account @entry.assign_attributes(entry_params.merge(currency: @account.currency)) @@ -30,7 +48,15 @@ class ValuationsController < ApplicationController end def create - account = Current.family.accounts.find(params.dig(:entry, :account_id)) + account = accessible_accounts.find(params.dig(:entry, :account_id)) + + unless account.permission_for(Current.user).in?([ :owner, :full_control ]) + respond_to do |format| + format.html { redirect_back_or_to account_path(account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(account), alert: t("accounts.not_authorized")) } + end + return + end result = account.create_reconciliation( balance: entry_params[:amount], @@ -49,6 +75,14 @@ class ValuationsController < ApplicationController end def update + unless can_edit_entry? + respond_to do |format| + format.html { redirect_back_or_to account_path(@entry.account), alert: t("accounts.not_authorized") } + format.turbo_stream { stream_redirect_back_or_to(account_path(@entry.account), alert: t("accounts.not_authorized")) } + end + return + end + # Notes updating is independent of reconciliation, just a simple CRUD operation @entry.update!(notes: entry_params[:notes]) if entry_params[:notes].present? diff --git a/app/models/account.rb b/app/models/account.rb index 45a4635e4..54e44b27b 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1,11 +1,16 @@ class Account < ApplicationRecord include AASM, Syncable, Monetizable, Chartable, Linkable, Enrichable, Anchorable, Reconcileable, TaxTreatable + before_validation :assign_default_owner, on: :create + validates :name, :balance, :currency, presence: true belongs_to :family + belongs_to :owner, class_name: "User", optional: true belongs_to :import, optional: true + has_many :account_shares, dependent: :destroy + has_many :shared_users, through: :account_shares, source: :user has_many :import_mappings, as: :mappable, dependent: :destroy, class_name: "Import::Mapping" has_many :entries, dependent: :destroy has_many :transactions, through: :entries, source: :entryable, source_type: "Transaction" @@ -36,6 +41,24 @@ class Account < ApplicationRecord manual.where.not(status: :pending_deletion) } + # All accounts a user can access (owned + shared with them) + scope :accessible_by, ->(user) { + left_joins(:account_shares) + .where("accounts.owner_id = :uid OR account_shares.user_id = :uid", uid: user.id) + .distinct + } + + # Accounts that count in a user's financial calculations + scope :included_in_finances_for, ->(user) { + left_joins(:account_shares) + .where( + "accounts.owner_id = :uid OR " \ + "(account_shares.user_id = :uid AND account_shares.include_in_finances = true)", + uid: user.id + ) + .distinct + } + has_one_attached :logo, dependent: :purge_later delegated_type :accountable, types: Accountable::TYPES, dependent: :destroy @@ -138,8 +161,9 @@ class Account < ApplicationRecord end end + family = simplefin_account.simplefin_item.family attributes = { - family: simplefin_account.simplefin_item.family, + family: family, name: simplefin_account.name, balance: balance, cash_balance: cash_balance, @@ -165,8 +189,9 @@ class Account < ApplicationRecord cash_balance = balance + family = enable_banking_account.enable_banking_item.family attributes = { - family: enable_banking_account.enable_banking_item.family, + family: family, name: enable_banking_account.name, balance: balance, cash_balance: cash_balance, @@ -344,4 +369,50 @@ class Account < ApplicationRecord raise "Unknown account type: #{accountable_type}" end end + + def owned_by?(user) + user.present? && owner_id == user.id + end + + def shared_with?(user) + owned_by?(user) || + if account_shares.loaded? + account_shares.any? { |s| s.user_id == user.id } + else + account_shares.exists?(user: user) + end + end + + def shared? + account_shares.any? + end + + def permission_for(user) + return :owner if owned_by?(user) + account_shares.find_by(user: user)&.permission&.to_sym + end + + def share_with!(user, permission: "read_only", include_in_finances: true) + account_shares.create!(user: user, permission: permission, include_in_finances: include_in_finances) + end + + def unshare_with!(user) + account_shares.where(user: user).destroy_all + end + + def auto_share_with_family! + records = family.users.where.not(id: owner_id).pluck(:id).map do |user_id| + { account_id: id, user_id: user_id, permission: "read_write", + include_in_finances: true, created_at: Time.current, updated_at: Time.current } + end + + AccountShare.insert_all(records, unique_by: %i[account_id user_id]) if records.any? + end + + private + + def assign_default_owner + return if owner.present? + self.owner = Current.user || family&.users&.find_by(role: %w[admin super_admin]) || family&.users&.order(:created_at)&.first + end end diff --git a/app/models/account/sync_complete_event.rb b/app/models/account/sync_complete_event.rb index d26b62f83..79e8ec16c 100644 --- a/app/models/account/sync_complete_event.rb +++ b/app/models/account/sync_complete_event.rb @@ -16,16 +16,6 @@ class Account::SyncCompleteEvent locals: { account: account } ) - # Replace the groups this account belongs to in both desktop and mobile sidebars - sidebar_targets.each do |(tab, mobile_flag)| - account.broadcast_replace_to( - account.family, - target: account_group.dom_id(tab: tab, mobile: mobile_flag), - partial: "accounts/accountable_group", - locals: { account_group: account_group, open: true, all_tab: tab == :all, mobile: mobile_flag } - ) - end - # If this is a manual, unlinked account (i.e. not part of a Plaid Item), # trigger the family sync complete broadcast so net worth graph is updated unless account.linked? @@ -35,29 +25,4 @@ class Account::SyncCompleteEvent # Refresh entire account page (only applies if currently viewing this account) account.broadcast_refresh end - - private - # Returns an array of [tab, mobile?] tuples that should receive an update. - # We broadcast to both the classification-specific tab and the "all" tab, - # for desktop (mobile: false) and mobile (mobile: true) variants. - def sidebar_targets - return [] unless account_group.present? - - [ - [ account_group.classification.to_sym, false ], - [ :all, false ], - [ account_group.classification.to_sym, true ], - [ :all, true ] - ] - end - - def account_group - family_balance_sheet.account_groups.find do |group| - group.accounts.any? { |a| a.id == account.id } - end - end - - def family_balance_sheet - account.family.balance_sheet - end end diff --git a/app/models/account_share.rb b/app/models/account_share.rb new file mode 100644 index 000000000..8737df625 --- /dev/null +++ b/app/models/account_share.rb @@ -0,0 +1,47 @@ +class AccountShare < ApplicationRecord + belongs_to :account + belongs_to :user + + PERMISSIONS = %w[full_control read_write read_only].freeze + + validates :permission, inclusion: { in: PERMISSIONS } + validates :user_id, uniqueness: { scope: :account_id } + validate :cannot_share_with_owner + validate :user_in_same_family + + scope :with_permission, ->(permission) { where(permission: permission) } + + def full_control? + permission == "full_control" + end + + def read_write? + permission == "read_write" + end + + def read_only? + permission == "read_only" + end + + def can_annotate? + full_control? || read_write? + end + + def can_edit? + full_control? + end + + private + + def cannot_share_with_owner + if account && user && account.owner_id == user_id + errors.add(:user, "is already the owner of this account") + end + end + + def user_in_same_family + if account && user && user.family_id != account.family_id + errors.add(:user, "must be in the same family") + end + end +end diff --git a/app/models/balance_sheet.rb b/app/models/balance_sheet.rb index e8f5f8442..6dd2db3ac 100644 --- a/app/models/balance_sheet.rb +++ b/app/models/balance_sheet.rb @@ -3,10 +3,11 @@ class BalanceSheet monetize :net_worth - attr_reader :family + attr_reader :family, :user - def initialize(family) + def initialize(family, user: nil) @family = family + @user = user || Current.user end def assets @@ -55,15 +56,15 @@ class BalanceSheet end def account_totals - @account_totals ||= AccountTotals.new(family, sync_status_monitor: sync_status_monitor) + @account_totals ||= AccountTotals.new(family, user: user, sync_status_monitor: sync_status_monitor) end def net_worth_series_builder - @net_worth_series_builder ||= NetWorthSeriesBuilder.new(family) + @net_worth_series_builder ||= NetWorthSeriesBuilder.new(family, user: user) end def sorted(accounts) - account_order = Current.user&.account_order + account_order = user&.account_order order_key = account_order&.key || "name_asc" case order_key diff --git a/app/models/balance_sheet/account_totals.rb b/app/models/balance_sheet/account_totals.rb index 2a1f63761..10f6f94ee 100644 --- a/app/models/balance_sheet/account_totals.rb +++ b/app/models/balance_sheet/account_totals.rb @@ -1,6 +1,7 @@ class BalanceSheet::AccountTotals - def initialize(family, sync_status_monitor:) + def initialize(family, user: nil, sync_status_monitor:) @family = family + @user = user @sync_status_monitor = sync_status_monitor end @@ -13,10 +14,11 @@ class BalanceSheet::AccountTotals end private - attr_reader :family, :sync_status_monitor + attr_reader :family, :user, :sync_status_monitor - AccountRow = Data.define(:account, :converted_balance, :is_syncing) do + AccountRow = Data.define(:account, :converted_balance, :is_syncing, :included_in_finances) do def syncing? = is_syncing + def included_in_finances? = included_in_finances # Allows Rails path helpers to generate URLs from the wrapper def to_param = account.to_param @@ -24,7 +26,19 @@ class BalanceSheet::AccountTotals end def visible_accounts - @visible_accounts ||= family.accounts.visible.with_attached_logo + @visible_accounts ||= begin + scope = family.accounts.visible.with_attached_logo + scope = scope.accessible_by(user) if user + scope + end + end + + def finance_account_ids + @finance_account_ids ||= if user + family.accounts.included_in_finances_for(user).pluck(:id).to_set + else + nil + end end # Wraps each account in an AccountRow with its converted balance and sync status. @@ -33,15 +47,17 @@ class BalanceSheet::AccountTotals AccountRow.new( account: account, converted_balance: converted_balance_for(account), - is_syncing: sync_status_monitor.account_syncing?(account) + is_syncing: sync_status_monitor.account_syncing?(account), + included_in_finances: finance_account_ids.nil? || finance_account_ids.include?(account.id) ) end end # Returns the cache key for storing visible account IDs, invalidated on data updates. def cache_key + shares_version = user ? AccountShare.where(user: user).maximum(:updated_at)&.to_i : nil family.build_cache_key( - "balance_sheet_account_ids", + [ "balance_sheet_account_ids", user&.id, shares_version ].compact.join("_"), invalidate_on_data_updates: true ) end diff --git a/app/models/balance_sheet/classification_group.rb b/app/models/balance_sheet/classification_group.rb index 32e64214c..968d4b6c0 100644 --- a/app/models/balance_sheet/classification_group.rb +++ b/app/models/balance_sheet/classification_group.rb @@ -21,7 +21,7 @@ class BalanceSheet::ClassificationGroup end def total - accounts.sum(&:converted_balance) + accounts.select { |a| a.respond_to?(:included_in_finances?) ? a.included_in_finances? : true }.sum(&:converted_balance) end def syncing? diff --git a/app/models/balance_sheet/net_worth_series_builder.rb b/app/models/balance_sheet/net_worth_series_builder.rb index f3e61a995..7c29a6ece 100644 --- a/app/models/balance_sheet/net_worth_series_builder.rb +++ b/app/models/balance_sheet/net_worth_series_builder.rb @@ -1,6 +1,7 @@ class BalanceSheet::NetWorthSeriesBuilder - def initialize(family) + def initialize(family, user: nil) @family = family + @user = user end def net_worth_series(period: Period.last_30_days) @@ -17,15 +18,22 @@ class BalanceSheet::NetWorthSeriesBuilder end private - attr_reader :family + attr_reader :family, :user def visible_account_ids - @visible_account_ids ||= family.accounts.visible.with_attached_logo.pluck(:id) + @visible_account_ids ||= begin + scope = family.accounts.visible + scope = scope.included_in_finances_for(user) if user + scope.pluck(:id) + end end def cache_key(period) + shares_version = user ? AccountShare.where(user: user).maximum(:updated_at)&.to_i : nil key = [ "balance_sheet_net_worth_series", + user&.id, + shares_version, period.start_date, period.end_date ].compact.join("_") diff --git a/app/models/current.rb b/app/models/current.rb index 86b9e97ac..612c2af76 100644 --- a/app/models/current.rb +++ b/app/models/current.rb @@ -16,4 +16,19 @@ class Current < ActiveSupport::CurrentAttributes def true_user session&.user end + + def accessible_accounts + return family&.accounts unless user + user.accessible_accounts + end + + def finance_accounts + return family&.accounts unless user + user.finance_accounts + end + + def accessible_entries + return family&.entries unless user + family.entries.joins(:account).merge(Account.accessible_by(user)) + end end diff --git a/app/models/family.rb b/app/models/family.rb index a43b118a2..0ba8dc8b2 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -20,6 +20,7 @@ class Family < ApplicationRecord MONIKERS = [ "Family", "Group" ].freeze ASSISTANT_TYPES = %w[builtin external].freeze + SHARING_DEFAULTS = %w[shared private].freeze has_many :users, dependent: :destroy has_many :accounts, dependent: :destroy @@ -49,6 +50,7 @@ class Family < ApplicationRecord validates :month_start_day, inclusion: { in: 1..28 } validates :moniker, inclusion: { in: MONIKERS } validates :assistant_type, inclusion: { in: ASSISTANT_TYPES } + validates :default_account_sharing, inclusion: { in: SHARING_DEFAULTS } def moniker_label @@ -59,6 +61,10 @@ class Family < ApplicationRecord moniker_label == "Group" ? "Groups" : "Families" end + def share_all_by_default? + default_account_sharing == "shared" + end + def uses_custom_month_start? month_start_day != 1 end @@ -115,12 +121,12 @@ class Family < ApplicationRecord AutoMerchantDetector.new(self, transaction_ids: transaction_ids).auto_detect end - def balance_sheet - @balance_sheet ||= BalanceSheet.new(self) + def balance_sheet(user: Current.user) + BalanceSheet.new(self, user: user) end - def income_statement - @income_statement ||= IncomeStatement.new(self) + def income_statement(user: Current.user) + IncomeStatement.new(self, user: user) end # Returns the Investment Contributions category for this family, creating it if it doesn't exist. @@ -190,8 +196,8 @@ class Family < ApplicationRecord end end - def investment_statement - @investment_statement ||= InvestmentStatement.new(self) + def investment_statement(user: Current.user) + InvestmentStatement.new(self, user: user) end def eu? diff --git a/app/models/family/sync_complete_event.rb b/app/models/family/sync_complete_event.rb index 925384605..ef10e14dc 100644 --- a/app/models/family/sync_complete_event.rb +++ b/app/models/family/sync_complete_event.rb @@ -6,28 +6,11 @@ class Family::SyncCompleteEvent end def broadcast - # Dashboard partials can occasionally raise when rendered from background jobs - # (e.g., if intermediate series values are nil during a sync). Make broadcasts - # resilient so a post-sync UI refresh never causes the overall sync to report an error. - begin - family.broadcast_replace( - target: "balance-sheet", - partial: "pages/dashboard/balance_sheet", - locals: { balance_sheet: family.balance_sheet } - ) - rescue => e - Rails.logger.error("Family::SyncCompleteEvent balance_sheet broadcast failed: #{e.message}\n#{e.backtrace&.join("\n")}") - end - - begin - family.broadcast_replace( - target: "net-worth-chart", - partial: "pages/dashboard/net_worth_chart", - locals: { balance_sheet: family.balance_sheet, period: Period.last_30_days } - ) - rescue => e - Rails.logger.error("Family::SyncCompleteEvent net_worth_chart broadcast failed: #{e.message}\n#{e.backtrace&.join("\n")}") - end + # Broadcast a refresh signal instead of rendered HTML. Each user's browser + # re-fetches via their own authenticated request, so the balance sheet and + # net worth chart are correctly scoped to the current user (Current.user is + # nil in background jobs, which would produce an unscoped family-wide view). + family.broadcast_refresh # Schedule recurring transaction pattern identification (debounced to run after all syncs complete) begin diff --git a/app/models/income_statement.rb b/app/models/income_statement.rb index 8f1b8f0a9..080d350ad 100644 --- a/app/models/income_statement.rb +++ b/app/models/income_statement.rb @@ -5,10 +5,11 @@ class IncomeStatement monetize :median_expense, :median_income - attr_reader :family + attr_reader :family, :user - def initialize(family) + def initialize(family, user: nil) @family = family + @user = user || Current.user end def totals(transactions_scope: nil, date_range:) @@ -175,23 +176,28 @@ class IncomeStatement def family_stats(interval: "month") @family_stats ||= {} @family_stats[interval] ||= Rails.cache.fetch([ - "income_statement", "family_stats", family.id, interval, family.entries_cache_version - ]) { FamilyStats.new(family, interval:).call } + "income_statement", "family_stats", family.id, user&.id, interval, family.entries_cache_version + ]) { FamilyStats.new(family, interval:, account_ids: included_account_ids).call } end def category_stats(interval: "month") @category_stats ||= {} @category_stats[interval] ||= Rails.cache.fetch([ - "income_statement", "category_stats", family.id, interval, family.entries_cache_version - ]) { CategoryStats.new(family, interval:).call } + "income_statement", "category_stats", family.id, user&.id, interval, family.entries_cache_version + ]) { CategoryStats.new(family, interval:, account_ids: included_account_ids).call } + end + + def included_account_ids + @included_account_ids ||= user ? user.finance_accounts.pluck(:id) : nil end def totals_query(transactions_scope:, date_range:) sql_hash = Digest::MD5.hexdigest(transactions_scope.to_sql) + account_ids_hash = included_account_ids ? Digest::MD5.hexdigest(included_account_ids.sort.join(",")) : nil Rails.cache.fetch([ - "income_statement", "totals_query", "v2", family.id, sql_hash, family.entries_cache_version - ]) { Totals.new(family, transactions_scope: transactions_scope, date_range: date_range).call } + "income_statement", "totals_query", "v2", family.id, user&.id, account_ids_hash, sql_hash, family.entries_cache_version + ]) { Totals.new(family, transactions_scope: transactions_scope, date_range: date_range, included_account_ids: included_account_ids).call } end def monetizable_currency diff --git a/app/models/income_statement/category_stats.rb b/app/models/income_statement/category_stats.rb index 0476549e4..7fd2552c6 100644 --- a/app/models/income_statement/category_stats.rb +++ b/app/models/income_statement/category_stats.rb @@ -1,7 +1,8 @@ class IncomeStatement::CategoryStats - def initialize(family, interval: "month") + def initialize(family, interval: "month", account_ids: nil) @family = family @interval = interval + @account_ids = account_ids end def call @@ -48,6 +49,11 @@ class IncomeStatement::CategoryStats "AND a.id NOT IN (:tax_advantaged_account_ids)" end + def scope_to_account_ids_sql + return "" if @account_ids.blank? + ActiveRecord::Base.sanitize_sql([ "AND a.id IN (?)", @account_ids ]) + end + def query_sql <<~SQL WITH period_totals AS ( @@ -71,6 +77,7 @@ class IncomeStatement::CategoryStats AND (t.extra -> 'simplefin' ->> 'pending')::boolean IS DISTINCT FROM true AND (t.extra -> 'plaid' ->> 'pending')::boolean IS DISTINCT FROM true #{exclude_tax_advantaged_sql} + #{scope_to_account_ids_sql} GROUP BY c.id, period, CASE WHEN t.kind = 'investment_contribution' THEN 'expense' WHEN ae.amount < 0 THEN 'income' ELSE 'expense' END ) SELECT diff --git a/app/models/income_statement/family_stats.rb b/app/models/income_statement/family_stats.rb index 8ce0e2e2d..a8616ef96 100644 --- a/app/models/income_statement/family_stats.rb +++ b/app/models/income_statement/family_stats.rb @@ -1,7 +1,8 @@ class IncomeStatement::FamilyStats - def initialize(family, interval: "month") + def initialize(family, interval: "month", account_ids: nil) @family = family @interval = interval + @account_ids = account_ids end def call @@ -47,6 +48,11 @@ class IncomeStatement::FamilyStats "AND a.id NOT IN (:tax_advantaged_account_ids)" end + def scope_to_account_ids_sql + return "" if @account_ids.blank? + ActiveRecord::Base.sanitize_sql([ "AND a.id IN (?)", @account_ids ]) + end + def query_sql <<~SQL WITH period_totals AS ( @@ -68,6 +74,7 @@ class IncomeStatement::FamilyStats AND (t.extra -> 'simplefin' ->> 'pending')::boolean IS DISTINCT FROM true AND (t.extra -> 'plaid' ->> 'pending')::boolean IS DISTINCT FROM true #{exclude_tax_advantaged_sql} + #{scope_to_account_ids_sql} GROUP BY period, CASE WHEN t.kind = 'investment_contribution' THEN 'expense' WHEN ae.amount < 0 THEN 'income' ELSE 'expense' END ) SELECT diff --git a/app/models/income_statement/totals.rb b/app/models/income_statement/totals.rb index 398bd1377..54fb56732 100644 --- a/app/models/income_statement/totals.rb +++ b/app/models/income_statement/totals.rb @@ -1,14 +1,18 @@ class IncomeStatement::Totals - def initialize(family, transactions_scope:, date_range:, include_trades: true) + def initialize(family, transactions_scope:, date_range:, include_trades: true, included_account_ids: nil) @family = family @transactions_scope = transactions_scope @date_range = date_range @include_trades = include_trades + @included_account_ids = included_account_ids validate_date_range! end def call + # No finance accounts means no transactions to report + return [] if @included_account_ids&.empty? + ActiveRecord::Base.connection.select_all(query_sql).map do |row| TotalsRow.new( parent_category_id: row["parent_category_id"], @@ -74,6 +78,7 @@ class IncomeStatement::Totals AND a.family_id = :family_id AND a.status IN ('draft', 'active') #{exclude_tax_advantaged_sql} + #{include_finance_accounts_sql} GROUP BY c.id, c.parent_id, CASE WHEN at.kind = 'investment_contribution' THEN 'expense' WHEN ae.amount < 0 THEN 'income' ELSE 'expense' END; SQL end @@ -105,6 +110,7 @@ class IncomeStatement::Totals AND a.family_id = :family_id AND a.status IN ('draft', 'active') #{exclude_tax_advantaged_sql} + #{include_finance_accounts_sql} GROUP BY c.id, c.parent_id, CASE WHEN at.kind = 'investment_contribution' THEN 'expense' WHEN ae.amount < 0 THEN 'income' ELSE 'expense' END SQL end @@ -133,6 +139,9 @@ class IncomeStatement::Totals ids = @family.tax_advantaged_account_ids params[:tax_advantaged_account_ids] = ids if ids.present? + # Add included account IDs for per-user finance scoping + params[:included_account_ids] = @included_account_ids if @included_account_ids + params end @@ -144,6 +153,12 @@ class IncomeStatement::Totals "AND a.id NOT IN (:tax_advantaged_account_ids)" end + # Returns SQL clause to filter to only accounts included in the user's finances. + def include_finance_accounts_sql + return "" if @included_account_ids.nil? + "AND a.id IN (:included_account_ids)" + end + def budget_excluded_kinds_sql @budget_excluded_kinds_sql ||= Transaction::BUDGET_EXCLUDED_KINDS.map { |k| "'#{k}'" }.join(", ") end diff --git a/app/models/investment_flow_statement.rb b/app/models/investment_flow_statement.rb index d17a71266..66d68b7a9 100644 --- a/app/models/investment_flow_statement.rb +++ b/app/models/investment_flow_statement.rb @@ -1,21 +1,28 @@ class InvestmentFlowStatement include Monetizable - attr_reader :family + attr_reader :family, :user - def initialize(family) + def initialize(family, user: nil) @family = family + @user = user end # Get contribution/withdrawal totals for a period def period_totals(period: Period.current_month) - transactions = family.transactions + scope = family.transactions .visible .excluding_pending .where(entries: { date: period.date_range }) .where(kind: %w[standard investment_contribution]) .where(investment_activity_label: %w[Contribution Withdrawal]) + if user + scope = scope.joins(entry: :account).merge(Account.included_in_finances_for(user)) + end + + transactions = scope + contributions = transactions.where(investment_activity_label: "Contribution").sum("entries.amount").abs withdrawals = transactions.where(investment_activity_label: "Withdrawal").sum("entries.amount").abs diff --git a/app/models/investment_statement.rb b/app/models/investment_statement.rb index 11cfdffa4..4475f1fce 100644 --- a/app/models/investment_statement.rb +++ b/app/models/investment_statement.rb @@ -5,17 +5,18 @@ class InvestmentStatement monetize :total_contributions, :total_dividends, :total_interest, :unrealized_gains - attr_reader :family + attr_reader :family, :user - def initialize(family) + def initialize(family, user: nil) @family = family + @user = user || Current.user end # Get totals for a specific period def totals(period: Period.current_month) trades_in_period = family.trades .joins(:entry) - .where(entries: { date: period.date_range }) + .where(entries: { date: period.date_range, account_id: investment_account_ids }) result = totals_query(trades_scope: trades_in_period) @@ -161,7 +162,11 @@ class InvestmentStatement # Investment accounts def investment_accounts - @investment_accounts ||= family.accounts.visible.where(accountable_type: %w[Investment Crypto]) + @investment_accounts ||= begin + scope = family.accounts.visible.where(accountable_type: %w[Investment Crypto]) + scope = scope.included_in_finances_for(user) if user + scope + end end private @@ -181,11 +186,15 @@ class InvestmentStatement HoldingAllocation = Data.define(:security, :amount, :weight, :trend) + def investment_account_ids + @investment_account_ids ||= investment_accounts.pluck(:id) + end + def totals_query(trades_scope:) sql_hash = Digest::MD5.hexdigest(trades_scope.to_sql) Rails.cache.fetch([ - "investment_statement", "totals_query", family.id, sql_hash, family.entries_cache_version + "investment_statement", "totals_query", family.id, user&.id, sql_hash, family.entries_cache_version ]) { Totals.new(family, trades_scope: trades_scope).call } end diff --git a/app/models/invitation.rb b/app/models/invitation.rb index 42bee7e9d..99ecb0da7 100644 --- a/app/models/invitation.rb +++ b/app/models/invitation.rb @@ -36,6 +36,7 @@ class Invitation < ApplicationRecord transaction do user.update!(family_id: family_id, role: role.to_s) update!(accepted_at: Time.current) + auto_share_existing_accounts(user) if family.share_all_by_default? end true end @@ -95,4 +96,13 @@ class Invitation < ApplicationRecord def inviter_is_admin inviter.admin? end + + def auto_share_existing_accounts(user) + records = family.accounts.where.not(owner_id: user.id).pluck(:id).map do |account_id| + { account_id: account_id, user_id: user.id, permission: "read_write", + include_in_finances: true, created_at: Time.current, updated_at: Time.current } + end + + AccountShare.insert_all(records, unique_by: %i[account_id user_id]) if records.any? + end end diff --git a/app/models/transaction/search.rb b/app/models/transaction/search.rb index 5a9cf441e..442324461 100644 --- a/app/models/transaction/search.rb +++ b/app/models/transaction/search.rb @@ -16,10 +16,11 @@ class Transaction::Search attribute :tags, array: true attribute :active_accounts_only, :boolean, default: true - attr_reader :family + attr_reader :family, :accessible_account_ids - def initialize(family, filters: {}) + def initialize(family, filters: {}, accessible_account_ids: nil) @family = family + @accessible_account_ids = accessible_account_ids super(filters) end @@ -28,6 +29,9 @@ class Transaction::Search # This already joins entries + accounts. To avoid expensive double-joins, don't join them again (causes full table scan) query = family.transactions.merge(Entry.excluding_split_parents) + # Scope to accessible accounts when provided + query = query.where(entries: { account_id: accessible_account_ids }) if accessible_account_ids + query = apply_active_accounts_filter(query, active_accounts_only) query = apply_category_filter(query, categories) query = apply_type_filter(query, types) @@ -89,7 +93,8 @@ class Transaction::Search family.id, Digest::SHA256.hexdigest(attributes.sort.to_h.to_json), # cached by filters family.entries_cache_version, - Digest::SHA256.hexdigest(family.tax_advantaged_account_ids.sort.to_json) # stable across processes + Digest::SHA256.hexdigest(family.tax_advantaged_account_ids.sort.to_json), # stable across processes + accessible_account_ids ? Digest::SHA256.hexdigest(accessible_account_ids.sort.to_json) : "all" ].join("/") end diff --git a/app/models/user.rb b/app/models/user.rb index 752338bc2..8d7ed8414 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -34,6 +34,9 @@ class User < ApplicationRecord has_many :impersonated_support_sessions, class_name: "ImpersonationSession", foreign_key: :impersonated_id, dependent: :destroy has_many :oidc_identities, dependent: :destroy has_many :sso_audit_logs, dependent: :nullify + has_many :owned_accounts, class_name: "Account", foreign_key: :owner_id + has_many :account_shares, dependent: :destroy + has_many :shared_accounts, through: :account_shares, source: :account accepts_nested_attributes_for :family, update_only: true validates :email, presence: true, uniqueness: true, format: { with: URI::MailTo::EMAIL_REGEXP } @@ -116,6 +119,14 @@ class User < ApplicationRecord super_admin? || role == "admin" end + def accessible_accounts + family.accounts.accessible_by(self) + end + + def finance_accounts + family.accounts.included_in_finances_for(self) + end + def display_name [ first_name, last_name ].compact.join(" ").presence || email end @@ -194,6 +205,7 @@ class User < ApplicationRecord if last_user_in_family? family.destroy else + reassign_owned_accounts! destroy end end @@ -400,6 +412,22 @@ class User < ApplicationRecord family.users.count == 1 end + def reassign_owned_accounts! + account_ids = owned_accounts.pluck(:id) + return if account_ids.empty? + + new_owner = family.users.where.not(id: id) + .find_by(role: %w[admin super_admin]) || + family.users.where.not(id: id) + .order(:created_at).first + + return unless new_owner + + Account.where(id: account_ids).update_all(owner_id: new_owner.id) + # Remove shares the new owner had for these accounts (they now own them) + AccountShare.where(account_id: account_ids, user_id: new_owner.id).delete_all + end + def deactivated_email email.gsub(/@/, "-deactivated-#{SecureRandom.uuid}@") end diff --git a/app/policies/account_policy.rb b/app/policies/account_policy.rb new file mode 100644 index 000000000..2d1e5a036 --- /dev/null +++ b/app/policies/account_policy.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +class AccountPolicy < ApplicationPolicy + def show? + record.shared_with?(user) + end + + def create? + user.member? || user.admin? + end + + def update? + permission = record.permission_for(user) + permission.in?([ :owner, :full_control ]) + end + + # For read_write users: categorize, tag, add notes/receipts on transactions + def annotate? + permission = record.permission_for(user) + permission.in?([ :owner, :full_control, :read_write ]) + end + + # Only the owner can delete the account itself. + # full_control users can delete transactions but not the account. + def destroy? + record.owned_by?(user) + end + + def manage_sharing? + record.owned_by?(user) + end + + class Scope < ApplicationPolicy::Scope + def resolve + scope.accessible_by(user) + end + end +end diff --git a/app/views/account_sharings/show.html.erb b/app/views/account_sharings/show.html.erb new file mode 100644 index 000000000..0eb8524f7 --- /dev/null +++ b/app/views/account_sharings/show.html.erb @@ -0,0 +1,92 @@ +<%= render DS::Dialog.new do |dialog| %> + <% dialog.with_header(title: t(".title")) %> + <% dialog.with_body do %> + <% if @account.owned_by?(Current.user) %> + <% if @family_members.any? %> + <%= styled_form_with url: account_sharing_path(@account), method: :patch, class: "space-y-4" do |form| %> +
+
+
+

<%= t(".member") %>

+

<%= t(".permission") %>

+

<%= t(".shared") %>

+
+
+ <% @family_members.each_with_index do |member, index| %> + <% share = @account_shares[member.id] %> +
+ +
+
+
+ <%= member.initials %> +
+ <%= member.display_name %> +
+
+ +
+
+ <%= render DS::Toggle.new( + id: "sharing_members_#{index}_shared", + name: "sharing[members][#{index}][shared]", + checked: share.present?, + "aria-labelledby": "member-name-#{index}" + ) %> +
+
+
+ <% end %> +
+
+ <%= render DS::Button.new(text: t(".save"), class: "md:w-auto w-full justify-center") %> +
+ <% end %> + <% else %> +

<%= t(".no_members", moniker: family_moniker_downcase) %>

+ <% end %> + <% else %> + <%# Non-owner can only toggle finance inclusion %> + <% share = @account.account_shares.find_by(user: Current.user) %> + <% if share %> +
+
+
+ <%= @account.owner.initials %> +
+
+

<%= t(".owner_label", name: @account.owner.display_name) %>

+

<%= t(".permissions.#{share.permission}") %>

+
+
+ + <%= styled_form_with url: account_sharing_path(@account), method: :patch, class: "space-y-4" do |form| %> + +
+
+
+

<%= t(".include_in_finances") %>

+

<%= t(".finance_toggle_description") %>

+
+ <%= render DS::Toggle.new( + id: "include_in_finances", + name: "include_in_finances", + checked: share.include_in_finances? + ) %> +
+
+
+ <%= render DS::Button.new(text: t(".save"), class: "md:w-auto w-full justify-center") %> +
+ <% end %> +
+ <% end %> + <% end %> + <% end %> +<% end %> diff --git a/app/views/accounts/_account.html.erb b/app/views/accounts/_account.html.erb index 82ce6b361..0dc54e788 100644 --- a/app/views/accounts/_account.html.erb +++ b/app/views/accounts/_account.html.erb @@ -21,6 +21,10 @@
<%= link_to account.name, account, class: [(account.active? ? "text-primary" : "text-subdued"), "text-sm font-medium hover:underline"], data: { turbo_frame: "_top" } %> + <% if account.shared? %> + <%= icon("users", class: "w-3.5 h-3.5 text-secondary", title: account.owned_by?(Current.user) ? nil : account.owner&.display_name) %> + <% end %> + <% if account.institution_name.present? %> <% end %> @@ -56,21 +60,27 @@ frame: :modal ) %> <% elsif !account.pending_deletion? %> + <% permission = account.permission_for(Current.user) %> <%= render DS::Menu.new(icon_vertical: true, mobile_fullwidth: false, max_width: "280px") do |menu| %> - <% menu.with_item(variant: "link", text: t("accounts.account.edit"), href: edit_account_path(account, return_to: return_to), icon: "pencil-line", data: { turbo_frame: :modal }) %> - - <% if !account.linked? && %w[Depository CreditCard Investment Crypto].include?(account.accountable_type) %> - <% menu.with_item(variant: "link", text: t("accounts.account.link_provider"), href: select_provider_account_path(account), icon: "link", data: { turbo_frame: :modal }) %> - <% elsif account.linked? %> - <% menu.with_item(variant: "link", text: t("accounts.account.unlink_provider"), href: confirm_unlink_account_path(account), icon: "unlink", data: { turbo_frame: :modal }) %> + <% if permission.in?([ :owner, :full_control ]) %> + <% menu.with_item(variant: "link", text: t("accounts.account.edit"), href: edit_account_path(account, return_to: return_to), icon: "pencil-line", data: { turbo_frame: :modal }) %> <% end %> + <% menu.with_item(variant: "link", text: t("accounts.account.sharing"), href: account_sharing_path(account), icon: "users", data: { turbo_frame: :modal }) %> - <% menu.with_item(variant: "divider") %> + <% if permission.in?([ :owner, :full_control ]) %> + <% if !account.linked? && %w[Depository CreditCard Investment Crypto].include?(account.accountable_type) %> + <% menu.with_item(variant: "link", text: t("accounts.account.link_provider"), href: select_provider_account_path(account), icon: "link", data: { turbo_frame: :modal }) %> + <% elsif account.linked? %> + <% menu.with_item(variant: "link", text: t("accounts.account.unlink_provider"), href: confirm_unlink_account_path(account), icon: "unlink", data: { turbo_frame: :modal }) %> + <% end %> - <% if account.active? %> - <% menu.with_item(variant: "button", text: t("accounts.account.disable"), href: toggle_active_account_path(account), method: :patch, icon: "toggle-right", data: { turbo_frame: :_top }) %> - <% elsif account.disabled? %> - <% menu.with_item(variant: "button", text: t("accounts.account.enable"), href: toggle_active_account_path(account), method: :patch, icon: "toggle-left", data: { turbo_frame: :_top }) %> + <% menu.with_item(variant: "divider") %> + + <% if account.active? %> + <% menu.with_item(variant: "button", text: t("accounts.account.disable"), href: toggle_active_account_path(account), method: :patch, icon: "toggle-right", data: { turbo_frame: :_top }) %> + <% elsif account.disabled? %> + <% menu.with_item(variant: "button", text: t("accounts.account.enable"), href: toggle_active_account_path(account), method: :patch, icon: "toggle-left", data: { turbo_frame: :_top }) %> + <% end %> <% end %> <% if is_default %> @@ -79,7 +89,7 @@ <% menu.with_item(variant: "button", text: t("accounts.account.set_default"), href: set_default_account_path(account), method: :patch, icon: "star", data: { turbo_frame: :_top }) %> <% end %> - <% unless account.linked? %> + <% if account.owned_by?(Current.user) && !account.linked? %> <% menu.with_item(variant: "divider") %> <% menu.with_item(variant: "button", text: t("accounts.account.delete"), href: account_path(account), method: :delete, icon: "trash-2", confirm: CustomConfirm.for_resource_deletion("account", high_severity: true), data: { turbo_frame: :_top }) %> <% end %> diff --git a/app/views/accounts/_accountable_group.html.erb b/app/views/accounts/_accountable_group.html.erb index 2ccea54a1..64f655797 100644 --- a/app/views/accounts/_accountable_group.html.erb +++ b/app/views/accounts/_accountable_group.html.erb @@ -34,6 +34,9 @@
<%= tag.p account.name, class: class_names("text-sm text-primary font-medium truncate", "animate-pulse" => account.syncing?) %> + <% if account.shared? %> + <%= icon("users", class: "w-3 h-3 text-secondary shrink-0") %> + <% end %>
<%= tag.p account.short_subtype_label, class: "text-sm text-secondary truncate" %>
diff --git a/app/views/accounts/index/_account_groups.erb b/app/views/accounts/index/_account_groups.erb index b5ebcf7f4..a66ddf5e3 100644 --- a/app/views/accounts/index/_account_groups.erb +++ b/app/views/accounts/index/_account_groups.erb @@ -1,5 +1,6 @@ <%# locals: (accounts:) %> +<% ActiveRecord::Associations::Preloader.new(records: accounts, associations: :account_shares).call if accounts.any? %> <% accounts.group_by(&:accountable_type).sort_by { |group, _| group }.each do |group, accounts| %>
diff --git a/app/views/accounts/show/_activity.html.erb b/app/views/accounts/show/_activity.html.erb index ae8aeb8b2..76de75102 100644 --- a/app/views/accounts/show/_activity.html.erb +++ b/app/views/accounts/show/_activity.html.erb @@ -5,30 +5,32 @@
<%= tag.h2 t(".title"), class: "font-medium text-lg" %> <% unless @account.linked? %> - <%= render DS::Menu.new(variant: "button") do |menu| %> - <% menu.with_button(text: "New", variant: "secondary", icon: "plus") %> + <% if @account.permission_for(Current.user).in?([ :owner, :full_control ]) %> + <%= render DS::Menu.new(variant: "button") do |menu| %> + <% menu.with_button(text: "New", variant: "secondary", icon: "plus") %> - <% menu.with_item( - variant: "link", - text: "New balance", - icon: "circle-dollar-sign", - href: new_valuation_path(account_id: @account.id), - data: { turbo_frame: :modal }) %> - - <% if @account.supports_trades? %> <% menu.with_item( variant: "link", - text: t(".new_trade"), - icon: "credit-card", - href: new_trade_path(account_id: @account.id), - data: { turbo_frame: :modal }) %> - <% elsif !@account.crypto? %> - <% menu.with_item( - variant: "link", - text: t(".new_transaction"), - icon: "credit-card", - href: new_transaction_path(account_id: @account.id), + text: "New balance", + icon: "circle-dollar-sign", + href: new_valuation_path(account_id: @account.id), data: { turbo_frame: :modal }) %> + + <% if @account.supports_trades? %> + <% menu.with_item( + variant: "link", + text: t(".new_trade"), + icon: "credit-card", + href: new_trade_path(account_id: @account.id), + data: { turbo_frame: :modal }) %> + <% elsif !@account.crypto? %> + <% menu.with_item( + variant: "link", + text: t(".new_transaction"), + icon: "credit-card", + href: new_transaction_path(account_id: @account.id), + data: { turbo_frame: :modal }) %> + <% end %> <% end %> <% end %> <% end %> diff --git a/app/views/accounts/show/_header.html.erb b/app/views/accounts/show/_header.html.erb index 41d731236..cafd97811 100644 --- a/app/views/accounts/show/_header.html.erb +++ b/app/views/accounts/show/_header.html.erb @@ -9,6 +9,9 @@

"><%= title %>

+ <% if account.shared? %> + <%= icon("users", class: "w-4 h-4 text-secondary", title: account.owned_by?(Current.user) ? nil : account.owner&.display_name) %> + <% end %> <% if account.tax_treatment.present? %> <%= render partial: "accounts/tax_treatment_badge", locals: { account: account } %> <% end %> diff --git a/app/views/accounts/show/_menu.html.erb b/app/views/accounts/show/_menu.html.erb index ee3db0c0d..45a39fe18 100644 --- a/app/views/accounts/show/_menu.html.erb +++ b/app/views/accounts/show/_menu.html.erb @@ -1,27 +1,33 @@ <%# locals: (account:) %> +<% permission = account.permission_for(Current.user) %> <%= render DS::Menu.new(testid: "account-menu") do |menu| %> - <% menu.with_item(variant: "link", text: "Edit", href: edit_account_path(account), icon: "pencil-line", data: { turbo_frame: :modal }) %> + <% if permission.in?([ :owner, :full_control ]) %> + <% menu.with_item(variant: "link", text: "Edit", href: edit_account_path(account), icon: "pencil-line", data: { turbo_frame: :modal }) %> + <% end %> + <% menu.with_item(variant: "link", text: "Sharing", href: account_sharing_path(account), icon: "users", data: { turbo_frame: :modal }) %> - <% if account.supports_trades? %> - <% menu.with_item( - variant: "link", - text: t(".import_trades"), - href: imports_path({ import: { type: "TradeImport", account_id: account.id } }), - icon: "download", - data: { turbo_frame: :_top } - ) %> - <% elsif !account.crypto? %> - <% menu.with_item( - variant: "link", - text: t(".import_transactions"), - href: imports_path({ import: { type: "TransactionImport", account_id: account.id } }), - icon: "download", - data: { turbo_frame: :_top } - ) %> + <% if permission.in?([ :owner, :full_control ]) %> + <% if account.supports_trades? %> + <% menu.with_item( + variant: "link", + text: t(".import_trades"), + href: imports_path({ import: { type: "TradeImport", account_id: account.id } }), + icon: "download", + data: { turbo_frame: :_top } + ) %> + <% elsif !account.crypto? %> + <% menu.with_item( + variant: "link", + text: t(".import_transactions"), + href: imports_path({ import: { type: "TransactionImport", account_id: account.id } }), + icon: "download", + data: { turbo_frame: :_top } + ) %> + <% end %> <% end %> - <% unless account.linked? %> + <% if account.owned_by?(Current.user) && !account.linked? %> <% menu.with_item( variant: "button", text: "Delete account", diff --git a/app/views/imports/new.html.erb b/app/views/imports/new.html.erb index f0a41ecc6..33d07ddfd 100644 --- a/app/views/imports/new.html.erb +++ b/app/views/imports/new.html.erb @@ -2,7 +2,7 @@ <% dialog.with_header(title: t(".title"), subtitle: t(".description")) %> <% dialog.with_body do %> - <% has_accounts = Current.family.accounts.any? %> + <% has_accounts = accessible_accounts.any? %> <% requires_account_message = t(".requires_account") %> <% if @pending_import.present? && params[:type].nil? %> diff --git a/app/views/pages/dashboard.html.erb b/app/views/pages/dashboard.html.erb index 0a429d0b0..5df1313ef 100644 --- a/app/views/pages/dashboard.html.erb +++ b/app/views/pages/dashboard.html.erb @@ -29,8 +29,9 @@
<% end %> +
gap-6 pb-6 lg:pb-12" data-controller="dashboard-sortable" data-action="dragover->dashboard-sortable#dragOver drop->dashboard-sortable#drop" role="list" aria-label="Dashboard sections"> - <% if Current.family.accounts.any? %> + <% if accessible_accounts.any? %> <% @dashboard_sections.each do |section| %> <% next unless section[:visible] %>
<% end %> + +<% if Current.user.admin? %> + <%= settings_section title: t(".sharing_title", moniker: family_moniker), subtitle: t(".sharing_subtitle", moniker: family_moniker_downcase) do %> +
+ <%= styled_form_with model: @user, class: "space-y-4", data: { controller: "auto-submit-form" } do |form| %> + <%= form.hidden_field :redirect_to, value: "preferences" %> + <%= form.fields_for :family do |family_form| %> + <%= family_form.select :default_account_sharing, + [ + [t(".sharing_shared"), "shared"], + [t(".sharing_private"), "private"] + ], + { label: t(".sharing_default_label") }, + { data: { auto_submit_form_target: "auto" } } %> + <% end %> + <% end %> +
+ <% end %> +<% end %> diff --git a/app/views/trades/_form.html.erb b/app/views/trades/_form.html.erb index 034e890bf..78c1b1b0b 100644 --- a/app/views/trades/_form.html.erb +++ b/app/views/trades/_form.html.erb @@ -45,7 +45,7 @@ <% end %> <% if %w[deposit withdrawal].include?(type) %> - <%= form.collection_select :transfer_account_id, Current.family.accounts.visible.alphabetically, :id, :name, { prompt: t(".account_prompt"), label: t(".account") } %> + <%= form.collection_select :transfer_account_id, accessible_accounts.visible.alphabetically, :id, :name, { prompt: t(".account_prompt"), label: t(".account") } %> <% end %> <% if %w[buy sell].include?(type) %> diff --git a/app/views/transaction_attachments/create.turbo_stream.erb b/app/views/transaction_attachments/create.turbo_stream.erb index 571379426..f8a689461 100644 --- a/app/views/transaction_attachments/create.turbo_stream.erb +++ b/app/views/transaction_attachments/create.turbo_stream.erb @@ -1,4 +1,4 @@ -<%= turbo_stream.replace "transaction_attachments_#{@transaction.id}", partial: "transactions/attachments", locals: { transaction: @transaction } %> +<%= turbo_stream.replace "transaction_attachments_#{@transaction.id}", partial: "transactions/attachments", locals: { transaction: @transaction, can_upload: @can_upload, can_delete: @can_delete } %> <% flash_notification_stream_items.each do |item| %> <%= item %> <% end %> diff --git a/app/views/transaction_attachments/destroy.turbo_stream.erb b/app/views/transaction_attachments/destroy.turbo_stream.erb index 571379426..f8a689461 100644 --- a/app/views/transaction_attachments/destroy.turbo_stream.erb +++ b/app/views/transaction_attachments/destroy.turbo_stream.erb @@ -1,4 +1,4 @@ -<%= turbo_stream.replace "transaction_attachments_#{@transaction.id}", partial: "transactions/attachments", locals: { transaction: @transaction } %> +<%= turbo_stream.replace "transaction_attachments_#{@transaction.id}", partial: "transactions/attachments", locals: { transaction: @transaction, can_upload: @can_upload, can_delete: @can_delete } %> <% flash_notification_stream_items.each do |item| %> <%= item %> <% end %> diff --git a/app/views/transactions/_attachments.html.erb b/app/views/transactions/_attachments.html.erb index 8688e6285..dba4b464f 100644 --- a/app/views/transactions/_attachments.html.erb +++ b/app/views/transactions/_attachments.html.erb @@ -1,6 +1,8 @@ +<%# locals: (transaction:, can_upload: false, can_delete: false) %> +
- <% if transaction.attachments.count < Transaction::MAX_ATTACHMENTS_PER_TRANSACTION %> + <% if can_upload && transaction.attachments.count < Transaction::MAX_ATTACHMENTS_PER_TRANSACTION %> <%= styled_form_with url: transaction_attachments_path(transaction), method: :post, multipart: true, @@ -54,7 +56,7 @@
<% end %> - <% else %> + <% elsif can_upload %>
<%= icon "alert-circle", size: "sm", color: "warning", class: "mt-0.5" %>
@@ -103,14 +105,16 @@ data: { turbo: false } ) %> - <%= render DS::Button.new( - href: transaction_attachment_path(transaction, attachment), - method: :delete, - variant: :outline_destructive, - size: :sm, - icon: "trash-2", - confirm: CustomConfirm.for_resource_deletion("attachment") - ) %> + <% if can_delete %> + <%= render DS::Button.new( + href: transaction_attachment_path(transaction, attachment), + method: :delete, + variant: :outline_destructive, + size: :sm, + icon: "trash-2", + confirm: CustomConfirm.for_resource_deletion("attachment") + ) %> + <% end %>
<% end %> diff --git a/app/views/transactions/_form.html.erb b/app/views/transactions/_form.html.erb index 7bfde6e9a..cc7466898 100644 --- a/app/views/transactions/_form.html.erb +++ b/app/views/transactions/_form.html.erb @@ -18,7 +18,7 @@ <% if @entry.account_id %> <%= f.hidden_field :account_id %> <% else %> - <%= f.collection_select :account_id, Current.family.accounts.manual.active.alphabetically, :id, :name, { prompt: t(".account_prompt"), label: t(".account"), selected: Current.user.default_account_for_transactions&.id, variant: :logo }, required: true, class: "form-field__input text-ellipsis" %> + <%= f.collection_select :account_id, accessible_accounts.manual.active.alphabetically, :id, :name, { prompt: t(".account_prompt"), label: t(".account"), selected: Current.user.default_account_for_transactions&.id, variant: :logo }, required: true, class: "form-field__input text-ellipsis" %> <% end %> <%= f.money_field :amount, label: t(".amount"), required: true %> diff --git a/app/views/transactions/searches/filters/_account_filter.html.erb b/app/views/transactions/searches/filters/_account_filter.html.erb index 8182dedcd..4641a3ddb 100644 --- a/app/views/transactions/searches/filters/_account_filter.html.erb +++ b/app/views/transactions/searches/filters/_account_filter.html.erb @@ -5,7 +5,7 @@ <%= icon("search", class: "absolute inset-y-0 left-2 top-1/2 transform -translate-y-1/2") %>
- <% Current.family.accounts.alphabetically.each do |account| %> + <% Current.user.accessible_accounts.alphabetically.each do |account| %>
<%= form.check_box :accounts, { diff --git a/app/views/transactions/show.html.erb b/app/views/transactions/show.html.erb index eab1586d2..4f8854fdf 100644 --- a/app/views/transactions/show.html.erb +++ b/app/views/transactions/show.html.erb @@ -48,32 +48,34 @@ <% dialog.with_section(title: t(".overview"), open: true) do %>
<% split_locked = @entry.split_child? || @entry.split_parent? %> + <% edit_locked = !can_edit_entry? %> + <% annotate_locked = !can_annotate_entry? %> <%= styled_form_with model: @entry, url: transaction_path(@entry), class: "space-y-2", data: { controller: "auto-submit-form" } do |f| %> <%= f.text_field :name, label: t(".name_label"), - disabled: @entry.split_child?, + disabled: @entry.split_child? || edit_locked, "data-auto-submit-form-target": "auto" %> <%= f.date_field :date, label: t(".date_label"), max: Date.current, - disabled: @entry.linked? || split_locked, + disabled: @entry.linked? || split_locked || edit_locked, "data-auto-submit-form-target": "auto" %> <% unless @entry.transaction.transfer? %>
<%= f.select :nature, [["Expense", "outflow"], ["Income", "inflow"]], { container_class: "w-1/3", label: t(".nature"), selected: @entry.amount.negative? ? "inflow" : "outflow" }, - { data: { "auto-submit-form-target": "auto" }, disabled: @entry.linked? || split_locked } %> + { data: { "auto-submit-form-target": "auto" }, disabled: @entry.linked? || split_locked || edit_locked } %> <%= f.money_field :amount, label: t(".amount"), container_class: "w-2/3", auto_submit: true, min: 0, value: @entry.amount.abs, - disabled: @entry.linked? || split_locked, - disable_currency: @entry.linked? || split_locked %> + disabled: @entry.linked? || split_locked || edit_locked, + disable_currency: @entry.linked? || split_locked || edit_locked %>
<%= f.fields_for :entryable do |ef| %> <%= ef.collection_select :category_id, @@ -81,7 +83,7 @@ :id, :name, { label: t(".category_label"), class: "text-subdued", include_blank: t(".uncategorized"), - variant: :badge, searchable: true, disabled: @entry.split_child? }, + variant: :badge, searchable: true, disabled: @entry.split_child? || annotate_locked }, "data-auto-submit-form-target": "auto" %> <% end %> <% end %> @@ -96,7 +98,7 @@ <% unless @entry.transaction.transfer? %> <%= f.select :account, options_for_select( - Current.family.accounts.alphabetically.pluck(:name, :id), + accessible_accounts.alphabetically.pluck(:name, :id), @entry.account_id ), { label: t(".account_label") }, @@ -107,7 +109,7 @@ :id, :name, { include_blank: t(".none"), label: t(".merchant_label"), - class: "text-subdued", variant: :logo, searchable: true, disabled: @entry.split_child? }, + class: "text-subdued", variant: :logo, searchable: true, disabled: @entry.split_child? || !can_annotate_entry? }, "data-auto-submit-form-target": "auto" %> <%= ef.select :tag_ids, Current.family.tags.alphabetically.pluck(:name, :id), @@ -115,7 +117,7 @@ include_blank: t(".none"), multiple: true, label: t(".tags_label"), - disabled: @entry.split_child? + disabled: @entry.split_child? || !can_annotate_entry? }, { "data-controller": "multi-select", "data-auto-submit-form-target": "auto" } %> <% end %> @@ -124,13 +126,13 @@ label: t(".note_label"), placeholder: t(".note_placeholder"), rows: 5, - disabled: @entry.split_child?, + disabled: @entry.split_child? || !can_annotate_entry?, "data-auto-submit-form-target": "auto" %> <% end %> <% end %> <% dialog.with_section(title: t(".attachments")) do %> - <%= render "transactions/attachments", transaction: @entry.transaction %> + <%= render "transactions/attachments", transaction: @entry.transaction, can_upload: can_annotate_entry?, can_delete: can_edit_entry? %> <% end %> <% if (details = build_transaction_extra_details(@entry)) %> @@ -263,166 +265,168 @@ <% end %> <% end %> - <% dialog.with_section(title: t(".settings")) do %> - <% unless @entry.split_parent? || @entry.split_child? %> -
- <%= styled_form_with model: @entry, - url: transaction_path(@entry), - class: "p-3", - data: { controller: "auto-submit-form" } do |f| %> -
-
-

<%= t(".exclude") %>

-

<%= t(".exclude_description") %>

-
- <%= f.toggle :excluded, { data: { auto_submit_form_target: "auto" } } %> -
- <% end %> -
- <% end %> - <% if @entry.account.investment? || @entry.account.crypto? %> -
- <%= styled_form_with model: @entry, - url: transaction_path(@entry), - class: "p-3", - data: { controller: "auto-submit-form" } do |f| %> - <%= f.fields_for :entryable do |ef| %> + <% if can_edit_entry? %> + <% dialog.with_section(title: t(".settings")) do %> + <% unless @entry.split_parent? || @entry.split_child? %> +
+ <%= styled_form_with model: @entry, + url: transaction_path(@entry), + class: "p-3", + data: { controller: "auto-submit-form" } do |f| %>
-

<%= t(".activity_type") %>

-

<%= t(".activity_type_description") %>

+

<%= t(".exclude") %>

+

<%= t(".exclude_description") %>

- <%= ef.select :investment_activity_label, - options_for_select( - [["—", nil]] + Transaction::ACTIVITY_LABELS.map { |l| [t("transactions.activity_labels.#{l.parameterize(separator: '_')}"), l] }, - @entry.entryable.investment_activity_label - ), - { label: false }, - { class: "form-field__input border border-secondary rounded-lg px-3 py-1.5 max-w-40 text-sm", - data: { auto_submit_form_target: "auto" } } %> + <%= f.toggle :excluded, { data: { auto_submit_form_target: "auto" } } %>
<% end %> - <% end %> -
- <% end %> - <% unless @entry.split_child? %> -
- <%= styled_form_with model: @entry, - url: transaction_path(@entry), - class: "p-3", - data: { controller: "auto-submit-form" } do |f| %> - <%= f.fields_for :entryable do |ef| %> -
-
-

<%= t(".one_time_title", type: @entry.amount.negative? ? t("transactions.form.income") : t("transactions.form.expense")) %>

-

<%= t(".one_time_description") %>

+
+ <% end %> + <% if @entry.account.investment? || @entry.account.crypto? %> +
+ <%= styled_form_with model: @entry, + url: transaction_path(@entry), + class: "p-3", + data: { controller: "auto-submit-form" } do |f| %> + <%= f.fields_for :entryable do |ef| %> +
+
+

<%= t(".activity_type") %>

+

<%= t(".activity_type_description") %>

+
+ <%= ef.select :investment_activity_label, + options_for_select( + [["—", nil]] + Transaction::ACTIVITY_LABELS.map { |l| [t("transactions.activity_labels.#{l.parameterize(separator: '_')}"), l] }, + @entry.entryable.investment_activity_label + ), + { label: false }, + { class: "form-field__input border border-secondary rounded-lg px-3 py-1.5 max-w-40 text-sm", + data: { auto_submit_form_target: "auto" } } %>
- <%= ef.toggle :kind, { - checked: @entry.transaction.one_time?, - data: { auto_submit_form_target: "auto" } - }, "one_time", "standard" %> -
+ <% end %> <% end %> - <% end %> -
- <% end %> - <%# Split Transaction %> - <% if @entry.transaction.splittable? %> -
-
-

<%= t("splits.show.button_title") %>

-

<%= t("splits.show.button_description") %>

- <%= render DS::Link.new( - text: t("splits.show.button"), - icon: "split", - variant: "outline", - href: new_transaction_split_path(@entry), - frame: :modal - ) %> -
- <% end %> - <% unless @entry.split_child? %> -
-
-

Transfer or Debt Payment?

-

Transfers and payments are special types of transactions that indicate money movement between 2 accounts.

+ <% end %> + <% unless @entry.split_child? %> +
+ <%= styled_form_with model: @entry, + url: transaction_path(@entry), + class: "p-3", + data: { controller: "auto-submit-form" } do |f| %> + <%= f.fields_for :entryable do |ef| %> +
+
+

<%= t(".one_time_title", type: @entry.amount.negative? ? t("transactions.form.income") : t("transactions.form.expense")) %>

+

<%= t(".one_time_description") %>

+
+ <%= ef.toggle :kind, { + checked: @entry.transaction.one_time?, + data: { auto_submit_form_target: "auto" } + }, "one_time", "standard" %> +
+ <% end %> + <% end %>
- <%= render DS::Link.new( - text: "Open matcher", - icon: "arrow-left-right", - variant: "outline", - href: new_transaction_transfer_match_path(@entry), - frame: :modal - ) %> -
- - <% if @entry.entryable.is_a?(Transaction) && @entry.entryable.pending? %> + <% end %> + <%# Split Transaction %> + <% if @entry.transaction.splittable? %>
-

<%= t("transactions.show.pending_duplicate_merger_title") %>

-

<%= t("transactions.show.pending_duplicate_merger_description") %>

+

<%= t("splits.show.button_title") %>

+

<%= t("splits.show.button_description") %>

<%= render DS::Link.new( - text: t("transactions.show.pending_duplicate_merger_button"), - icon: "merge", + text: t("splits.show.button"), + icon: "split", variant: "outline", - href: new_transaction_pending_duplicate_merges_path(@entry), + href: new_transaction_split_path(@entry), frame: :modal ) %>
<% end %> - - <% if @entry.account.investment? && @entry.entryable.is_a?(Transaction) && !@entry.excluded? %> + <% unless @entry.split_child? %> +
+
+

Transfer or Debt Payment?

+

Transfers and payments are special types of transactions that indicate money movement between 2 accounts.

+
+ <%= render DS::Link.new( + text: "Open matcher", + icon: "arrow-left-right", + variant: "outline", + href: new_transaction_transfer_match_path(@entry), + frame: :modal + ) %> +
+ + <% if @entry.entryable.is_a?(Transaction) && @entry.entryable.pending? %> +
+
+

<%= t("transactions.show.pending_duplicate_merger_title") %>

+

<%= t("transactions.show.pending_duplicate_merger_description") %>

+
+ <%= render DS::Link.new( + text: t("transactions.show.pending_duplicate_merger_button"), + icon: "merge", + variant: "outline", + href: new_transaction_pending_duplicate_merges_path(@entry), + frame: :modal + ) %> +
+ <% end %> + + <% if @entry.account.investment? && @entry.entryable.is_a?(Transaction) && !@entry.excluded? %> +
+
+

Convert to Security Trade

+

Convert this transaction into a security trade (buy/sell) by providing ticker, shares, and price.

+
+ <%= render DS::Button.new( + text: "Convert", + variant: "outline", + icon: "arrow-right-left", + href: convert_to_trade_transaction_path(@entry.transaction), + method: :get, + frame: :modal + ) %> +
+ <% end %> +
-

Convert to Security Trade

-

Convert this transaction into a security trade (buy/sell) by providing ticker, shares, and price.

+

<%= t(".mark_recurring_title") %>

+

<%= t(".mark_recurring_subtitle") %>

<%= render DS::Button.new( - text: "Convert", + text: t(".mark_recurring"), variant: "outline", - icon: "arrow-right-left", - href: convert_to_trade_transaction_path(@entry.transaction), - method: :get, - frame: :modal + icon: "repeat", + href: mark_as_recurring_transaction_path(@entry.transaction), + method: :post, + frame: "_top" ) %>
<% end %> - -
-
-

<%= t(".mark_recurring_title") %>

-

<%= t(".mark_recurring_subtitle") %>

+ <%# Delete Transaction Form - hidden for split children %> + <% unless @entry.split_child? %> +
+
+

<%= t(".delete_title") %>

+

<%= t(".delete_subtitle") %>

+
+ <%= render DS::Button.new( + text: t(".delete"), + variant: "outline-destructive", + href: entry_path(@entry), + method: :delete, + confirm: CustomConfirm.for_resource_deletion("transaction"), + frame: "_top" + ) %>
- <%= render DS::Button.new( - text: t(".mark_recurring"), - variant: "outline", - icon: "repeat", - href: mark_as_recurring_transaction_path(@entry.transaction), - method: :post, - frame: "_top" - ) %> + <% end %>
<% end %> - <%# Delete Transaction Form - hidden for split children %> - <% unless @entry.split_child? %> -
-
-

<%= t(".delete_title") %>

-

<%= t(".delete_subtitle") %>

-
- <%= render DS::Button.new( - text: t(".delete"), - variant: "outline-destructive", - href: entry_path(@entry), - method: :delete, - confirm: CustomConfirm.for_resource_deletion("transaction"), - frame: "_top" - ) %> -
- <% end %> -
<% end %> <% end %> <% end %> diff --git a/config/locales/views/account_sharings/en.yml b/config/locales/views/account_sharings/en.yml new file mode 100644 index 000000000..2a219a054 --- /dev/null +++ b/config/locales/views/account_sharings/en.yml @@ -0,0 +1,29 @@ +--- +en: + account_sharings: + show: + title: Account Sharing + subtitle: Control who can see and interact with this account + member: Member + permission: Permission + shared: Shared + no_members: No other members in your %{moniker} to share with + permissions: + full_control: Full control + full_control_description: Can view, edit, and manage transactions + read_write: Can annotate + read_write_description: Can categorize, tag, and add notes + read_only: View only + read_only_description: Can only view account data + save: Save sharing settings + owner_label: "Owner: %{name}" + shared_with_count: + one: Shared with 1 member + other: "Shared with %{count} members" + include_in_finances: Include in my budgets & reports + exclude_from_finances: Exclude from my budgets & reports + finance_toggle_description: Count this account in your net worth, budgets, and reports + update: + success: Sharing settings updated + not_owner: Only the account owner can manage sharing + finance_toggle_success: Finance inclusion preference updated diff --git a/config/locales/views/accounts/en.yml b/config/locales/views/accounts/en.yml index 1a721e19a..e64f84972 100644 --- a/config/locales/views/accounts/en.yml +++ b/config/locales/views/accounts/en.yml @@ -1,6 +1,7 @@ --- en: accounts: + not_authorized: "You don't have permission to manage this account" account: edit: Edit link_lunchflow: Link with Lunch Flow @@ -13,6 +14,7 @@ en: remove_default: Unset default default_label: Default delete: Delete account + sharing: Sharing chart: data_not_available: Data not available for the selected period create: diff --git a/config/locales/views/settings/en.yml b/config/locales/views/settings/en.yml index 9dada3442..d590798ca 100644 --- a/config/locales/views/settings/en.yml +++ b/config/locales/views/settings/en.yml @@ -58,6 +58,11 @@ en: month_start_day: Budget month starts on month_start_day_hint: Set when your budget month starts (e.g., payday) month_start_day_warning: Your budgets and MTD calculations will use this custom start day instead of the 1st of each month. + sharing_title: "%{moniker} Sharing" + sharing_subtitle: "Control how accounts are shared in your %{moniker}" + sharing_default_label: Default sharing for new accounts + sharing_shared: Share with all members + sharing_private: Keep private by default profiles: destroy: cannot_remove_self: You cannot remove yourself from the account. diff --git a/config/routes.rb b/config/routes.rb index 9ef960239..8514a9407 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -348,6 +348,8 @@ Rails.application.routes.draw do collection do post :sync_all end + + resource :sharing, only: [ :show, :update ], controller: "account_sharings" end # Convenience routes for polymorphic paths diff --git a/db/migrate/20260324100000_add_account_sharing_support.rb b/db/migrate/20260324100000_add_account_sharing_support.rb new file mode 100644 index 000000000..253606850 --- /dev/null +++ b/db/migrate/20260324100000_add_account_sharing_support.rb @@ -0,0 +1,21 @@ +class AddAccountSharingSupport < ActiveRecord::Migration[7.2] + def change + # Family-level default: whether new accounts are shared with all members by default + add_column :families, :default_account_sharing, :string, default: "shared", null: false + + # Account ownership: who created/owns the account + add_reference :accounts, :owner, type: :uuid, foreign_key: { to_table: :users }, null: true, index: true + + # Sharing join table: per-user access to accounts they don't own + create_table :account_shares, id: :uuid, default: -> { "gen_random_uuid()" } do |t| + t.references :account, type: :uuid, null: false, foreign_key: true + t.references :user, type: :uuid, null: false, foreign_key: true + t.string :permission, null: false, default: "read_only" + t.boolean :include_in_finances, null: false, default: true + t.timestamps + end + + add_index :account_shares, [ :account_id, :user_id ], unique: true + add_index :account_shares, [ :user_id, :include_in_finances ] + end +end diff --git a/db/migrate/20260324100001_backfill_account_owners_and_shares.rb b/db/migrate/20260324100001_backfill_account_owners_and_shares.rb new file mode 100644 index 000000000..8ed6b0416 --- /dev/null +++ b/db/migrate/20260324100001_backfill_account_owners_and_shares.rb @@ -0,0 +1,34 @@ +class BackfillAccountOwnersAndShares < ActiveRecord::Migration[7.2] + def up + # Existing families keep current behavior: all accounts shared + Family.update_all(default_account_sharing: "shared") + + # For each family, assign all accounts to the admin (or first user) + Family.find_each do |family| + admin = family.users.find_by(role: %w[admin super_admin]) || family.users.order(:created_at).first + next unless admin + + family.accounts.where(owner_id: nil).update_all(owner_id: admin.id) + + # Create shares for non-owner members (preserves current full-access behavior) + member_ids = family.users.where.not(id: admin.id).pluck(:id) + account_ids = family.accounts.pluck(:id) + + if member_ids.any? && account_ids.any? + records = member_ids.product(account_ids).map do |user_id, account_id| + { user_id: user_id, account_id: account_id, permission: "full_control", + include_in_finances: true, created_at: Time.current, updated_at: Time.current } + end + + AccountShare.upsert_all(records, unique_by: %i[account_id user_id]) + end + end + + # Owner is enforced at the model level via before_validation callback + # Keeping nullable at DB level for backward compatibility with tests/seeds + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20260324100002_add_check_constraints_to_sharing_columns.rb b/db/migrate/20260324100002_add_check_constraints_to_sharing_columns.rb new file mode 100644 index 000000000..0a21a4113 --- /dev/null +++ b/db/migrate/20260324100002_add_check_constraints_to_sharing_columns.rb @@ -0,0 +1,6 @@ +class AddCheckConstraintsToSharingColumns < ActiveRecord::Migration[7.2] + def change + add_check_constraint :families, "default_account_sharing IN ('shared', 'private')", name: "chk_families_default_account_sharing" + add_check_constraint :account_shares, "permission IN ('full_control', 'read_write', 'read_only')", name: "chk_account_shares_permission" + end +end diff --git a/db/migrate/20260324100003_change_accounts_owner_fk_to_nullify.rb b/db/migrate/20260324100003_change_accounts_owner_fk_to_nullify.rb new file mode 100644 index 000000000..bd2ce4298 --- /dev/null +++ b/db/migrate/20260324100003_change_accounts_owner_fk_to_nullify.rb @@ -0,0 +1,11 @@ +class ChangeAccountsOwnerFkToNullify < ActiveRecord::Migration[7.2] + def up + remove_foreign_key :accounts, :users, column: :owner_id + add_foreign_key :accounts, :users, column: :owner_id, on_delete: :nullify + end + + def down + remove_foreign_key :accounts, :users, column: :owner_id + add_foreign_key :accounts, :users, column: :owner_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 8355c6564..fce042e9c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2026_03_20_080659) do +ActiveRecord::Schema[7.2].define(version: 2026_03_24_100003) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -29,6 +29,20 @@ ActiveRecord::Schema[7.2].define(version: 2026_03_20_080659) do t.index ["provider_type", "provider_id"], name: "index_account_providers_on_provider_type_and_provider_id", unique: true end + create_table "account_shares", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.uuid "account_id", null: false + t.uuid "user_id", null: false + t.string "permission", default: "read_only", null: false + t.boolean "include_in_finances", default: true, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["account_id", "user_id"], name: "index_account_shares_on_account_id_and_user_id", unique: true + t.index ["account_id"], name: "index_account_shares_on_account_id" + t.index ["user_id", "include_in_finances"], name: "index_account_shares_on_user_id_and_include_in_finances" + t.index ["user_id"], name: "index_account_shares_on_user_id" + t.check_constraint "permission::text = ANY (ARRAY['full_control'::character varying, 'read_write'::character varying, 'read_only'::character varying]::text[])", name: "chk_account_shares_permission" + end + create_table "accounts", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.string "subtype" t.uuid "family_id", null: false @@ -49,6 +63,9 @@ ActiveRecord::Schema[7.2].define(version: 2026_03_20_080659) do t.string "institution_name" t.string "institution_domain" t.text "notes" + t.jsonb "holdings_snapshot_data" + t.datetime "holdings_snapshot_at" + t.uuid "owner_id" t.index ["accountable_id", "accountable_type"], name: "index_accounts_on_accountable_id_and_accountable_type" t.index ["accountable_type"], name: "index_accounts_on_accountable_type" t.index ["currency"], name: "index_accounts_on_currency" @@ -58,6 +75,7 @@ ActiveRecord::Schema[7.2].define(version: 2026_03_20_080659) do t.index ["family_id", "status"], name: "index_accounts_on_family_id_and_status" t.index ["family_id"], name: "index_accounts_on_family_id" t.index ["import_id"], name: "index_accounts_on_import_id" + t.index ["owner_id"], name: "index_accounts_on_owner_id" t.index ["plaid_account_id"], name: "index_accounts_on_plaid_account_id" t.index ["simplefin_account_id"], name: "index_accounts_on_simplefin_account_id" t.index ["status"], name: "index_accounts_on_status" @@ -516,6 +534,8 @@ ActiveRecord::Schema[7.2].define(version: 2026_03_20_080659) do t.string "vector_store_id" t.string "moniker", default: "Family", null: false t.string "assistant_type", default: "builtin", null: false + t.string "default_account_sharing", default: "shared", null: false + t.check_constraint "default_account_sharing::text = ANY (ARRAY['shared'::character varying, 'private'::character varying]::text[])", name: "chk_families_default_account_sharing" t.check_constraint "month_start_day >= 1 AND month_start_day <= 28", name: "month_start_day_range" end @@ -705,8 +725,8 @@ ActiveRecord::Schema[7.2].define(version: 2026_03_20_080659) do t.date "sync_start_date" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["indexa_capital_item_id", "indexa_capital_account_id"], name: "index_indexa_capital_accounts_on_item_and_account_id", unique: true, where: "(indexa_capital_account_id IS NOT NULL)" t.index ["indexa_capital_authorization_id"], name: "idx_on_indexa_capital_authorization_id_58db208d52" + t.index ["indexa_capital_item_id", "indexa_capital_account_id"], name: "index_indexa_capital_accounts_on_item_and_account_id", unique: true, where: "(indexa_capital_account_id IS NOT NULL)" t.index ["indexa_capital_item_id"], name: "index_indexa_capital_accounts_on_indexa_capital_item_id" end @@ -1497,10 +1517,13 @@ ActiveRecord::Schema[7.2].define(version: 2026_03_20_080659) do end add_foreign_key "account_providers", "accounts", on_delete: :cascade + add_foreign_key "account_shares", "accounts" + add_foreign_key "account_shares", "users" add_foreign_key "accounts", "families" add_foreign_key "accounts", "imports" add_foreign_key "accounts", "plaid_accounts" add_foreign_key "accounts", "simplefin_accounts" + add_foreign_key "accounts", "users", column: "owner_id", on_delete: :nullify add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" add_foreign_key "api_keys", "users" diff --git a/test/controllers/impersonation_sessions_controller_test.rb b/test/controllers/impersonation_sessions_controller_test.rb index 7adda7b74..7dce7f5ac 100644 --- a/test/controllers/impersonation_sessions_controller_test.rb +++ b/test/controllers/impersonation_sessions_controller_test.rb @@ -11,7 +11,7 @@ class ImpersonationSessionsControllerTest < ActionDispatch::IntegrationTest assert_difference "impersonator_session.logs.count", 2 do get root_path - get account_path(impersonated.family.accounts.first) + get account_path(impersonated.accessible_accounts.first) end end diff --git a/test/controllers/transactions_controller_test.rb b/test/controllers/transactions_controller_test.rb index f3faea941..fdc9ce851 100644 --- a/test/controllers/transactions_controller_test.rb +++ b/test/controllers/transactions_controller_test.rb @@ -162,7 +162,7 @@ end income_money: Money.new(0, "USD") ) - Transaction::Search.expects(:new).with(family, filters: {}).returns(search) + Transaction::Search.expects(:new).with(family, filters: {}, accessible_account_ids: [ account.id ]).returns(search) search.expects(:totals).once.returns(totals) get transactions_url @@ -184,7 +184,7 @@ end income_money: Money.new(0, "USD") ) - Transaction::Search.expects(:new).with(family, filters: { "categories" => [ "Food" ], "types" => [ "expense" ] }).returns(search) + Transaction::Search.expects(:new).with(family, filters: { "categories" => [ "Food" ], "types" => [ "expense" ] }, accessible_account_ids: [ account.id ]).returns(search) search.expects(:totals).once.returns(totals) get transactions_url(q: { categories: [ "Food" ], types: [ "expense" ] }) diff --git a/test/fixtures/account_shares.yml b/test/fixtures/account_shares.yml new file mode 100644 index 000000000..200af469d --- /dev/null +++ b/test/fixtures/account_shares.yml @@ -0,0 +1,13 @@ +# Share the checking account with family_member (full control) +depository_shared_with_member: + account: depository + user: family_member + permission: full_control + include_in_finances: true + +# Share the credit card with family_member (read only) +credit_card_shared_with_member: + account: credit_card + user: family_member + permission: read_only + include_in_finances: true diff --git a/test/fixtures/accounts.yml b/test/fixtures/accounts.yml index 3b0354d39..e2554e5dc 100644 --- a/test/fixtures/accounts.yml +++ b/test/fixtures/accounts.yml @@ -1,5 +1,6 @@ other_asset: family: dylan_family + owner: family_admin name: Collectable Account balance: 550 currency: USD @@ -9,6 +10,7 @@ other_asset: other_liability: family: dylan_family + owner: family_admin name: IOU (personal debt to friend) balance: 200 currency: USD @@ -18,6 +20,7 @@ other_liability: depository: family: dylan_family + owner: family_admin name: Checking Account balance: 5000 currency: USD @@ -27,6 +30,7 @@ depository: connected: family: dylan_family + owner: family_admin name: Plaid Depository Account balance: 5000 currency: USD @@ -37,6 +41,7 @@ connected: credit_card: family: dylan_family + owner: family_admin name: Credit Card balance: 1000 currency: USD @@ -46,6 +51,7 @@ credit_card: investment: family: dylan_family + owner: family_admin name: Robinhood Brokerage Account balance: 10000 cash_balance: 5000 @@ -56,6 +62,7 @@ investment: loan: family: dylan_family + owner: family_admin name: Mortgage Loan balance: 500000 currency: USD @@ -65,6 +72,7 @@ loan: property: family: dylan_family + owner: family_admin name: 123 Maybe Court balance: 550000 currency: USD @@ -74,6 +82,7 @@ property: vehicle: family: dylan_family + owner: family_admin name: Honda Accord balance: 18000 currency: USD @@ -83,6 +92,7 @@ vehicle: crypto: family: dylan_family + owner: family_admin name: Bitcoin balance: 10000 currency: USD diff --git a/test/models/account_share_test.rb b/test/models/account_share_test.rb new file mode 100644 index 000000000..99763ae53 --- /dev/null +++ b/test/models/account_share_test.rb @@ -0,0 +1,61 @@ +require "test_helper" + +class AccountShareTest < ActiveSupport::TestCase + setup do + @admin = users(:family_admin) + @member = users(:family_member) + @account = accounts(:depository) + end + + test "valid share" do + # Use an account that doesn't already have a share with member + account = accounts(:investment) + account.account_shares.where(user: @member).destroy_all + share = AccountShare.new(account: account, user: @member, permission: "read_only") + assert share.valid? + end + + test "invalid permission" do + share = AccountShare.new(account: @account, user: @member, permission: "invalid") + assert_not share.valid? + assert_includes share.errors[:permission], "is not included in the list" + end + + test "cannot share with account owner" do + share = AccountShare.new(account: @account, user: @admin, permission: "read_only") + assert_not share.valid? + assert_includes share.errors[:user], "is already the owner of this account" + end + + test "cannot duplicate share for same user and account" do + # depository already shared with member via fixture + duplicate = AccountShare.new(account: @account, user: @member, permission: "read_only") + assert_not duplicate.valid? + end + + test "permission helper methods" do + share = AccountShare.new(permission: "full_control") + assert share.full_control? + assert_not share.read_write? + assert_not share.read_only? + assert share.can_annotate? + assert share.can_edit? + + share.permission = "read_write" + assert share.read_write? + assert share.can_annotate? + assert_not share.can_edit? + + share.permission = "read_only" + assert share.read_only? + assert_not share.can_annotate? + assert_not share.can_edit? + end + + test "cannot share with user from different family" do + other_user = users(:empty) + share = AccountShare.new(account: @account, user: other_user, permission: "read_only") + assert_not share.valid? + assert_includes share.errors[:user], "must be in the same family" + end +end diff --git a/test/models/account_test.rb b/test/models/account_test.rb index a8284d3db..c1daf738a 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -6,6 +6,8 @@ class AccountTest < ActiveSupport::TestCase setup do @account = @syncable = accounts(:depository) @family = families(:dylan_family) + @admin = users(:family_admin) + @member = users(:family_member) end test "can destroy" do @@ -19,6 +21,7 @@ class AccountTest < ActiveSupport::TestCase account = Account.create_and_sync({ family: @family, + owner: @admin, name: "Test Account", balance: 100, currency: "USD", @@ -37,6 +40,7 @@ class AccountTest < ActiveSupport::TestCase account = Account.create_and_sync( { family: @family, + owner: @admin, name: "Linked Account", balance: 500, currency: "EUR", @@ -57,6 +61,7 @@ class AccountTest < ActiveSupport::TestCase account = Account.create_and_sync( { family: @family, + owner: @admin, name: "Test Account", balance: 1000, currency: "GBP", @@ -79,6 +84,7 @@ class AccountTest < ActiveSupport::TestCase account = Account.create_and_sync( { family: @family, + owner: @admin, name: "Test Account", balance: 1000, currency: "USD", @@ -96,6 +102,7 @@ class AccountTest < ActiveSupport::TestCase test "gets short/long subtype label" do investment = Investment.new(subtype: "hsa") account = @family.accounts.create!( + owner: @admin, name: "Test Investment", balance: 1000, currency: "USD", @@ -116,6 +123,7 @@ class AccountTest < ActiveSupport::TestCase test "tax_treatment delegates to accountable for Investment" do investment = Investment.new(subtype: "401k") account = @family.accounts.create!( + owner: @admin, name: "Test 401k", balance: 1000, currency: "USD", @@ -129,6 +137,7 @@ class AccountTest < ActiveSupport::TestCase test "tax_treatment delegates to accountable for Crypto" do crypto = Crypto.new(tax_treatment: :taxable) account = @family.accounts.create!( + owner: @admin, name: "Test Crypto", balance: 500, currency: "USD", @@ -148,6 +157,7 @@ class AccountTest < ActiveSupport::TestCase test "tax_advantaged? returns true for tax-advantaged accounts" do investment = Investment.new(subtype: "401k") account = @family.accounts.create!( + owner: @admin, name: "Test 401k", balance: 1000, currency: "USD", @@ -161,6 +171,7 @@ class AccountTest < ActiveSupport::TestCase test "tax_advantaged? returns false for taxable accounts" do investment = Investment.new(subtype: "brokerage") account = @family.accounts.create!( + owner: @admin, name: "Test Brokerage", balance: 1000, currency: "USD", @@ -193,4 +204,86 @@ class AccountTest < ActiveSupport::TestCase assert_not ActiveStorage::Attachment.exists?(attachment_id) end + + # Account sharing tests + + test "owned_by? returns true for account owner" do + assert @account.owned_by?(@admin) + assert_not @account.owned_by?(@member) + end + + test "shared_with? returns true for owner and shared users" do + assert @account.shared_with?(@admin) # owner + # depository already shared with member via fixture + assert @account.shared_with?(@member) + end + + test "shared? returns true when account has shares" do + account = accounts(:investment) + account.account_shares.destroy_all + assert_not account.shared? + + account.share_with!(@member, permission: "read_only") + assert account.shared? + end + + test "permission_for returns correct permission level" do + assert_equal :owner, @account.permission_for(@admin) + + # depository already shared with member via fixture + share = @account.account_shares.find_by(user: @member) + share.update!(permission: "read_write") + assert_equal :read_write, @account.permission_for(@member) + end + + test "accessible_by scope returns owned and shared accounts" do + # Clear existing shares for clean test + AccountShare.delete_all + + admin_accessible = @family.accounts.accessible_by(@admin) + member_accessible = @family.accounts.accessible_by(@member) + + # Admin owns all fixture accounts + assert_equal @family.accounts.count, admin_accessible.count + # Member has no access (no shares, no owned accounts) + assert_equal 0, member_accessible.count + + # Share one account + @account.share_with!(@member, permission: "read_only") + member_accessible = @family.accounts.accessible_by(@member) + assert_equal 1, member_accessible.count + assert_includes member_accessible, @account + end + + test "included_in_finances_for scope respects include_in_finances flag" do + AccountShare.delete_all + + @account.share_with!(@member, permission: "read_only", include_in_finances: true) + assert_includes @family.accounts.included_in_finances_for(@member), @account + + share = @account.account_shares.find_by(user: @member) + share.update!(include_in_finances: false) + assert_not_includes @family.accounts.included_in_finances_for(@member), @account + end + + test "auto_share_with_family creates shares for all non-owner members" do + account = Account.create_and_sync({ + family: @family, + owner: @admin, + name: "New Shared Account", + balance: 100, + currency: "USD", + accountable_type: "Depository", + accountable_attributes: {} + }) + + assert_difference -> { AccountShare.count }, @family.users.where.not(id: @admin.id).count do + account.auto_share_with_family! + end + + share = account.account_shares.find_by(user: @member) + assert_not_nil share + assert_equal "read_write", share.permission + assert share.include_in_finances? + end end