From 07e37065d9830bbcf8b1ed70208f5e1b90f2ad4b Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 24 Jul 2025 10:43:19 -0400 Subject: [PATCH] Lint fixes, brakeman update --- app/controllers/family_exports_controller.rb | 28 +- app/jobs/family_data_export_job.rb | 8 +- app/models/family/data_exporter.rb | 408 +++++++++--------- app/models/family_export.rb | 10 +- app/views/family_exports/_list.html.erb | 14 +- app/views/family_exports/index.html.erb | 2 +- app/views/family_exports/new.html.erb | 2 +- app/views/settings/profiles/show.html.erb | 2 +- config/brakeman.ignore | 25 +- .../family_exports_controller_test.rb | 34 +- test/jobs/family_data_export_job_test.rb | 14 +- test/models/family/data_exporter_test.rb | 48 +-- 12 files changed, 309 insertions(+), 286 deletions(-) diff --git a/app/controllers/family_exports_controller.rb b/app/controllers/family_exports_controller.rb index ab3b4c76..992d68b3 100644 --- a/app/controllers/family_exports_controller.rb +++ b/app/controllers/family_exports_controller.rb @@ -1,8 +1,8 @@ class FamilyExportsController < ApplicationController include StreamExtensions - + before_action :require_admin - before_action :set_export, only: [:download] + before_action :set_export, only: [ :download ] def new # Modal view for initiating export @@ -11,10 +11,10 @@ class FamilyExportsController < ApplicationController def create @export = Current.family.family_exports.create! FamilyDataExportJob.perform_later(@export) - + respond_to do |format| format.html { redirect_to settings_profile_path, notice: "Export started. You'll be able to download it shortly." } - format.turbo_stream { + format.turbo_stream { stream_redirect_to settings_profile_path, notice: "Export started. You'll be able to download it shortly." } end @@ -24,7 +24,7 @@ class FamilyExportsController < ApplicationController @exports = Current.family.family_exports.ordered.limit(10) render layout: false # For turbo frame end - + def download if @export.downloadable? redirect_to @export.export_file, allow_other_host: true @@ -34,14 +34,14 @@ class FamilyExportsController < ApplicationController end private - - def set_export - @export = Current.family.family_exports.find(params[:id]) - end - def require_admin - unless Current.user.admin? - redirect_to root_path, alert: "Access denied" + def set_export + @export = Current.family.family_exports.find(params[:id]) end - end -end \ No newline at end of file + + def require_admin + unless Current.user.admin? + redirect_to root_path, alert: "Access denied" + end + end +end diff --git a/app/jobs/family_data_export_job.rb b/app/jobs/family_data_export_job.rb index 3496aba8..1b62bd66 100644 --- a/app/jobs/family_data_export_job.rb +++ b/app/jobs/family_data_export_job.rb @@ -3,20 +3,20 @@ class FamilyDataExportJob < ApplicationJob def perform(family_export) family_export.update!(status: :processing) - + exporter = Family::DataExporter.new(family_export.family) zip_file = exporter.generate_export - + family_export.export_file.attach( io: zip_file, filename: family_export.filename, content_type: "application/zip" ) - + family_export.update!(status: :completed) rescue => e Rails.logger.error "Family export failed: #{e.message}" Rails.logger.error e.backtrace.join("\n") family_export.update!(status: :failed) end -end \ No newline at end of file +end diff --git a/app/models/family/data_exporter.rb b/app/models/family/data_exporter.rb index aba036d9..93546cd6 100644 --- a/app/models/family/data_exporter.rb +++ b/app/models/family/data_exporter.rb @@ -1,238 +1,238 @@ -require 'zip' -require 'csv' +require "zip" +require "csv" class Family::DataExporter def initialize(family) @family = family end - + def generate_export # Create a StringIO to hold the zip data in memory zip_data = Zip::OutputStream.write_buffer do |zipfile| # Add accounts.csv zipfile.put_next_entry("accounts.csv") zipfile.write generate_accounts_csv - + # Add transactions.csv zipfile.put_next_entry("transactions.csv") zipfile.write generate_transactions_csv - + # Add trades.csv zipfile.put_next_entry("trades.csv") zipfile.write generate_trades_csv - + # Add categories.csv zipfile.put_next_entry("categories.csv") zipfile.write generate_categories_csv - + # Add all.ndjson zipfile.put_next_entry("all.ndjson") zipfile.write generate_ndjson end - + # Rewind and return the StringIO zip_data.rewind zip_data end - + private - - def generate_accounts_csv - CSV.generate do |csv| - csv << ["id", "name", "type", "subtype", "balance", "currency", "created_at"] - - # Only export accounts belonging to this family + + def generate_accounts_csv + CSV.generate do |csv| + csv << [ "id", "name", "type", "subtype", "balance", "currency", "created_at" ] + + # Only export accounts belonging to this family + @family.accounts.includes(:accountable).find_each do |account| + csv << [ + account.id, + account.name, + account.accountable_type, + account.subtype, + account.balance.to_s, + account.currency, + account.created_at.iso8601 + ] + end + end + end + + def generate_transactions_csv + CSV.generate do |csv| + csv << [ "date", "account_name", "amount", "name", "category", "tags", "notes", "currency" ] + + # Only export transactions from accounts belonging to this family + @family.transactions + .includes(:category, :tags, entry: :account) + .find_each do |transaction| + csv << [ + transaction.entry.date.iso8601, + transaction.entry.account.name, + transaction.entry.amount.to_s, + transaction.entry.name, + transaction.category&.name, + transaction.tags.pluck(:name).join(","), + transaction.entry.notes, + transaction.entry.currency + ] + end + end + end + + def generate_trades_csv + CSV.generate do |csv| + csv << [ "date", "account_name", "ticker", "quantity", "price", "amount", "currency" ] + + # Only export trades from accounts belonging to this family + @family.trades + .includes(:security, entry: :account) + .find_each do |trade| + csv << [ + trade.entry.date.iso8601, + trade.entry.account.name, + trade.security.ticker, + trade.qty.to_s, + trade.price.to_s, + trade.entry.amount.to_s, + trade.currency + ] + end + end + end + + def generate_categories_csv + CSV.generate do |csv| + csv << [ "name", "color", "parent_category", "classification" ] + + # Only export categories belonging to this family + @family.categories.includes(:parent).find_each do |category| + csv << [ + category.name, + category.color, + category.parent&.name, + category.classification + ] + end + end + end + + def generate_ndjson + lines = [] + + # Export accounts with full accountable data @family.accounts.includes(:accountable).find_each do |account| - csv << [ - account.id, - account.name, - account.accountable_type, - account.subtype, - account.balance.to_s, - account.currency, - account.created_at.iso8601 - ] + lines << { + type: "Account", + data: account.as_json( + include: { + accountable: {} + } + ) + }.to_json end - end - end - - def generate_transactions_csv - CSV.generate do |csv| - csv << ["date", "account_name", "amount", "name", "category", "tags", "notes", "currency"] - - # Only export transactions from accounts belonging to this family - @family.transactions - .includes(:category, :tags, entry: :account) - .find_each do |transaction| - csv << [ - transaction.entry.date.iso8601, - transaction.entry.account.name, - transaction.entry.amount.to_s, - transaction.entry.name, - transaction.category&.name, - transaction.tags.pluck(:name).join(","), - transaction.entry.notes, - transaction.entry.currency - ] - end - end - end - - def generate_trades_csv - CSV.generate do |csv| - csv << ["date", "account_name", "ticker", "quantity", "price", "amount", "currency"] - - # Only export trades from accounts belonging to this family - @family.trades - .includes(:security, entry: :account) - .find_each do |trade| - csv << [ - trade.entry.date.iso8601, - trade.entry.account.name, - trade.security.ticker, - trade.qty.to_s, - trade.price.to_s, - trade.entry.amount.to_s, - trade.currency - ] - end - end - end - - def generate_categories_csv - CSV.generate do |csv| - csv << ["name", "color", "parent_category", "classification"] - - # Only export categories belonging to this family - @family.categories.includes(:parent).find_each do |category| - csv << [ - category.name, - category.color, - category.parent&.name, - category.classification - ] + + # Export categories + @family.categories.find_each do |category| + lines << { + type: "Category", + data: category.as_json + }.to_json end - end - end - - def generate_ndjson - lines = [] - - # Export accounts with full accountable data - @family.accounts.includes(:accountable).find_each do |account| - lines << { - type: "Account", - data: account.as_json( - include: { - accountable: {} + + # Export tags + @family.tags.find_each do |tag| + lines << { + type: "Tag", + data: tag.as_json + }.to_json + end + + # Export merchants (only family merchants) + @family.merchants.find_each do |merchant| + lines << { + type: "Merchant", + data: merchant.as_json + }.to_json + end + + # Export transactions with full data + @family.transactions.includes(:category, :merchant, :tags, entry: :account).find_each do |transaction| + lines << { + type: "Transaction", + data: { + id: transaction.id, + entry_id: transaction.entry.id, + account_id: transaction.entry.account_id, + date: transaction.entry.date, + amount: transaction.entry.amount, + currency: transaction.entry.currency, + name: transaction.entry.name, + notes: transaction.entry.notes, + excluded: transaction.entry.excluded, + category_id: transaction.category_id, + merchant_id: transaction.merchant_id, + tag_ids: transaction.tag_ids, + kind: transaction.kind, + created_at: transaction.created_at, + updated_at: transaction.updated_at } - ) - }.to_json + }.to_json + end + + # Export trades with full data + @family.trades.includes(:security, entry: :account).find_each do |trade| + lines << { + type: "Trade", + data: { + id: trade.id, + entry_id: trade.entry.id, + account_id: trade.entry.account_id, + security_id: trade.security_id, + ticker: trade.security.ticker, + date: trade.entry.date, + qty: trade.qty, + price: trade.price, + amount: trade.entry.amount, + currency: trade.currency, + created_at: trade.created_at, + updated_at: trade.updated_at + } + }.to_json + end + + # Export valuations + @family.entries.valuations.includes(:account, :entryable).find_each do |entry| + lines << { + type: "Valuation", + data: { + id: entry.entryable.id, + entry_id: entry.id, + account_id: entry.account_id, + date: entry.date, + amount: entry.amount, + currency: entry.currency, + name: entry.name, + created_at: entry.created_at, + updated_at: entry.updated_at + } + }.to_json + end + + # Export budgets + @family.budgets.find_each do |budget| + lines << { + type: "Budget", + data: budget.as_json + }.to_json + end + + # Export budget categories + @family.budget_categories.includes(:budget, :category).find_each do |budget_category| + lines << { + type: "BudgetCategory", + data: budget_category.as_json + }.to_json + end + + lines.join("\n") end - - # Export categories - @family.categories.find_each do |category| - lines << { - type: "Category", - data: category.as_json - }.to_json - end - - # Export tags - @family.tags.find_each do |tag| - lines << { - type: "Tag", - data: tag.as_json - }.to_json - end - - # Export merchants (only family merchants) - @family.merchants.find_each do |merchant| - lines << { - type: "Merchant", - data: merchant.as_json - }.to_json - end - - # Export transactions with full data - @family.transactions.includes(:category, :merchant, :tags, entry: :account).find_each do |transaction| - lines << { - type: "Transaction", - data: { - id: transaction.id, - entry_id: transaction.entry.id, - account_id: transaction.entry.account_id, - date: transaction.entry.date, - amount: transaction.entry.amount, - currency: transaction.entry.currency, - name: transaction.entry.name, - notes: transaction.entry.notes, - excluded: transaction.entry.excluded, - category_id: transaction.category_id, - merchant_id: transaction.merchant_id, - tag_ids: transaction.tag_ids, - kind: transaction.kind, - created_at: transaction.created_at, - updated_at: transaction.updated_at - } - }.to_json - end - - # Export trades with full data - @family.trades.includes(:security, entry: :account).find_each do |trade| - lines << { - type: "Trade", - data: { - id: trade.id, - entry_id: trade.entry.id, - account_id: trade.entry.account_id, - security_id: trade.security_id, - ticker: trade.security.ticker, - date: trade.entry.date, - qty: trade.qty, - price: trade.price, - amount: trade.entry.amount, - currency: trade.currency, - created_at: trade.created_at, - updated_at: trade.updated_at - } - }.to_json - end - - # Export valuations - @family.entries.valuations.includes(:account, :entryable).find_each do |entry| - lines << { - type: "Valuation", - data: { - id: entry.entryable.id, - entry_id: entry.id, - account_id: entry.account_id, - date: entry.date, - amount: entry.amount, - currency: entry.currency, - name: entry.name, - created_at: entry.created_at, - updated_at: entry.updated_at - } - }.to_json - end - - # Export budgets - @family.budgets.find_each do |budget| - lines << { - type: "Budget", - data: budget.as_json - }.to_json - end - - # Export budget categories - @family.budget_categories.includes(:budget, :category).find_each do |budget_category| - lines << { - type: "BudgetCategory", - data: budget_category.as_json - }.to_json - end - - lines.join("\n") - end -end \ No newline at end of file +end diff --git a/app/models/family_export.rb b/app/models/family_export.rb index 86abb77c..292ab9e0 100644 --- a/app/models/family_export.rb +++ b/app/models/family_export.rb @@ -1,21 +1,21 @@ class FamilyExport < ApplicationRecord belongs_to :family - + has_one_attached :export_file - + enum :status, { pending: "pending", processing: "processing", completed: "completed", failed: "failed" }, default: :pending, validate: true - + scope :ordered, -> { order(created_at: :desc) } - + def filename "maybe_export_#{created_at.strftime('%Y%m%d_%H%M%S')}.zip" end - + def downloadable? completed? && export_file.attached? end diff --git a/app/views/family_exports/_list.html.erb b/app/views/family_exports/_list.html.erb index 68a9b006..f4e979d8 100644 --- a/app/views/family_exports/_list.html.erb +++ b/app/views/family_exports/_list.html.erb @@ -1,7 +1,7 @@ -<%= turbo_frame_tag "family_exports", - data: exports.any? { |e| e.pending? || e.processing? } ? { - turbo_refresh_url: family_exports_path, - turbo_refresh_interval: 3000 +<%= turbo_frame_tag "family_exports", + data: exports.any? { |e| e.pending? || e.processing? } ? { + turbo_refresh_url: family_exports_path, + turbo_refresh_interval: 3000 } : {} do %>
<% if exports.any? %> @@ -11,14 +11,14 @@

Export from <%= export.created_at.strftime("%B %d, %Y at %I:%M %p") %>

<%= export.filename %>

- + <% if export.processing? || export.pending? %>
Exporting...
<% elsif export.completed? %> - <%= link_to download_family_export_path(export), + <%= link_to download_family_export_path(export), class: "flex items-center gap-2 text-primary hover:text-primary-hover", data: { turbo_frame: "_top" } do %> <%= icon "download", class: "w-5 h-5" %> @@ -36,4 +36,4 @@

No exports yet

<% end %> -<% end %> \ No newline at end of file +<% end %> diff --git a/app/views/family_exports/index.html.erb b/app/views/family_exports/index.html.erb index a5a7d4c0..530b8151 100644 --- a/app/views/family_exports/index.html.erb +++ b/app/views/family_exports/index.html.erb @@ -1 +1 @@ -<%= render "list", exports: @exports %> \ No newline at end of file +<%= render "list", exports: @exports %> diff --git a/app/views/family_exports/new.html.erb b/app/views/family_exports/new.html.erb index de936562..5bf02352 100644 --- a/app/views/family_exports/new.html.erb +++ b/app/views/family_exports/new.html.erb @@ -39,4 +39,4 @@ <% end %> <% end %> -<% end %> \ No newline at end of file +<% end %> diff --git a/app/views/settings/profiles/show.html.erb b/app/views/settings/profiles/show.html.erb index 9a51357b..1dfe2c15 100644 --- a/app/views/settings/profiles/show.html.erb +++ b/app/views/settings/profiles/show.html.erb @@ -135,7 +135,7 @@ data: { turbo_frame: :modal } ) %> - + <%= turbo_frame_tag "family_exports", src: family_exports_path, loading: :lazy do %>
diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 5ca90443..05172fd2 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -1,5 +1,28 @@ { "ignored_warnings": [ + { + "warning_type": "Redirect", + "warning_code": 18, + "fingerprint": "723b1970ca6bf16ea0c2c1afa0c00d3c54854a16568d6cb933e497947565d9ab", + "check_name": "Redirect", + "message": "Possible unprotected redirect", + "file": "app/controllers/family_exports_controller.rb", + "line": 30, + "link": "https://brakemanscanner.org/docs/warning_types/redirect/", + "code": "redirect_to(Current.family.family_exports.find(params[:id]).export_file, :allow_other_host => true)", + "render_path": null, + "location": { + "type": "method", + "class": "FamilyExportsController", + "method": "download" + }, + "user_input": "Current.family.family_exports.find(params[:id]).export_file", + "confidence": "Weak", + "cwe_id": [ + 601 + ], + "note": "" + }, { "warning_type": "Mass Assignment", "warning_code": 105, @@ -105,5 +128,5 @@ "note": "" } ], - "brakeman_version": "7.0.2" + "brakeman_version": "7.1.0" } diff --git a/test/controllers/family_exports_controller_test.rb b/test/controllers/family_exports_controller_test.rb index 13de0c10..63adf788 100644 --- a/test/controllers/family_exports_controller_test.rb +++ b/test/controllers/family_exports_controller_test.rb @@ -5,52 +5,52 @@ class FamilyExportsControllerTest < ActionDispatch::IntegrationTest @admin = users(:family_admin) @non_admin = users(:family_member) @family = @admin.family - + sign_in @admin end - + test "non-admin cannot access exports" do sign_in @non_admin - + get new_family_export_path assert_redirected_to root_path - + post family_exports_path assert_redirected_to root_path - + get family_exports_path assert_redirected_to root_path end - + test "admin can view export modal" do get new_family_export_path assert_response :success assert_select "h2", text: "Export your data" end - + test "admin can create export" do assert_enqueued_with(job: FamilyDataExportJob) do post family_exports_path end - + assert_redirected_to settings_profile_path assert_equal "Export started. You'll be able to download it shortly.", flash[:notice] - + export = @family.family_exports.last assert_equal "pending", export.status end - + test "admin can view export list" do export1 = @family.family_exports.create!(status: "completed") export2 = @family.family_exports.create!(status: "processing") - + get family_exports_path assert_response :success - + assert_match export1.filename, response.body assert_match "Exporting...", response.body end - + test "admin can download completed export" do export = @family.family_exports.create!(status: "completed") export.export_file.attach( @@ -58,16 +58,16 @@ class FamilyExportsControllerTest < ActionDispatch::IntegrationTest filename: "test.zip", content_type: "application/zip" ) - + get download_family_export_path(export) assert_redirected_to(/rails\/active_storage/) end - + test "cannot download incomplete export" do export = @family.family_exports.create!(status: "processing") - + get download_family_export_path(export) assert_redirected_to settings_profile_path assert_equal "Export not ready for download", flash[:alert] end -end \ No newline at end of file +end diff --git a/test/jobs/family_data_export_job_test.rb b/test/jobs/family_data_export_job_test.rb index 38726b66..6aae26e8 100644 --- a/test/jobs/family_data_export_job_test.rb +++ b/test/jobs/family_data_export_job_test.rb @@ -5,28 +5,28 @@ class FamilyDataExportJobTest < ActiveJob::TestCase @family = families(:dylan_family) @export = @family.family_exports.create! end - + test "marks export as processing then completed" do assert_equal "pending", @export.status - + perform_enqueued_jobs do FamilyDataExportJob.perform_later(@export) end - + @export.reload assert_equal "completed", @export.status assert @export.export_file.attached? end - + test "marks export as failed on error" do # Mock the exporter to raise an error Family::DataExporter.any_instance.stubs(:generate_export).raises(StandardError, "Export failed") - + perform_enqueued_jobs do FamilyDataExportJob.perform_later(@export) end - + @export.reload assert_equal "failed", @export.status end -end \ No newline at end of file +end diff --git a/test/models/family/data_exporter_test.rb b/test/models/family/data_exporter_test.rb index 07e3786b..fe56fb08 100644 --- a/test/models/family/data_exporter_test.rb +++ b/test/models/family/data_exporter_test.rb @@ -5,7 +5,7 @@ class Family::DataExporterTest < ActiveSupport::TestCase @family = families(:dylan_family) @other_family = families(:empty) @exporter = Family::DataExporter.new(@family) - + # Create some test data for the family @account = @family.accounts.create!( name: "Test Account", @@ -13,72 +13,72 @@ class Family::DataExporterTest < ActiveSupport::TestCase balance: 1000, currency: "USD" ) - + @category = @family.categories.create!( name: "Test Category", color: "#FF0000" ) - + @tag = @family.tags.create!( name: "Test Tag", color: "#00FF00" ) end - + test "generates a zip file with all required files" do zip_data = @exporter.generate_export - + assert zip_data.is_a?(StringIO) - + # Check that the zip contains all expected files - expected_files = ["accounts.csv", "transactions.csv", "trades.csv", "categories.csv", "all.ndjson"] - + expected_files = [ "accounts.csv", "transactions.csv", "trades.csv", "categories.csv", "all.ndjson" ] + Zip::File.open_buffer(zip_data) do |zip| actual_files = zip.entries.map(&:name) assert_equal expected_files.sort, actual_files.sort end end - + test "generates valid CSV files" do zip_data = @exporter.generate_export - + Zip::File.open_buffer(zip_data) do |zip| # Check accounts.csv accounts_csv = zip.read("accounts.csv") assert accounts_csv.include?("id,name,type,subtype,balance,currency,created_at") - + # Check transactions.csv transactions_csv = zip.read("transactions.csv") assert transactions_csv.include?("date,account_name,amount,name,category,tags,notes,currency") - + # Check trades.csv trades_csv = zip.read("trades.csv") assert trades_csv.include?("date,account_name,ticker,quantity,price,amount,currency") - + # Check categories.csv categories_csv = zip.read("categories.csv") assert categories_csv.include?("name,color,parent_category,classification") end end - + test "generates valid NDJSON file" do zip_data = @exporter.generate_export - + Zip::File.open_buffer(zip_data) do |zip| ndjson_content = zip.read("all.ndjson") lines = ndjson_content.split("\n") - + lines.each do |line| assert_nothing_raised { JSON.parse(line) } end - + # Check that each line has expected structure first_line = JSON.parse(lines.first) assert first_line.key?("type") assert first_line.key?("data") end end - + test "only exports data from the specified family" do # Create data for another family that should NOT be exported other_account = @other_family.accounts.create!( @@ -87,29 +87,29 @@ class Family::DataExporterTest < ActiveSupport::TestCase balance: 5000, currency: "USD" ) - + other_category = @other_family.categories.create!( name: "Other Family Category", color: "#0000FF" ) - + zip_data = @exporter.generate_export - + Zip::File.open_buffer(zip_data) do |zip| # Check accounts.csv doesn't contain other family's data accounts_csv = zip.read("accounts.csv") assert accounts_csv.include?(@account.name) refute accounts_csv.include?(other_account.name) - + # Check categories.csv doesn't contain other family's data categories_csv = zip.read("categories.csv") assert categories_csv.include?(@category.name) refute categories_csv.include?(other_category.name) - + # Check NDJSON doesn't contain other family's data ndjson_content = zip.read("all.ndjson") refute ndjson_content.include?(other_account.id) refute ndjson_content.include?(other_category.id) end end -end \ No newline at end of file +end