From 1157ea8f201336ca4759bcd2efac37a7b996cb8a Mon Sep 17 00:00:00 2001 From: dripsmvcp <138900956+dripsmvcp@users.noreply.github.com> Date: Thu, 11 Jun 2026 07:56:35 -0700 Subject: [PATCH] fix(sharing): scope import account selects to accessible_by (#1803) (#2194) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sharing): scope import account selects to accessible_by (#1803) Three CSV / QIF account selects in import/uploads/show.html.erb and one PDF-import account select in imports/_pdf_import.html.erb pulled their options from `@import.family.accounts`. That listed every account in the family — including the family admin's unshared personal accounts — in the dropdown shown to any member running an import. Swap each call site to `Current.user.accessible_accounts` (owned + explicitly shared accounts only), matching the existing scoping used by the dashboard sidebar, transactions controller, transfers controller, etc. Adds a regression test that signs in as family_member and asserts the unshared-account names from the dylan_family fixtures never appear in the rendered upload page. * test(import): scope leak assertions to account select (#2194 CodeRabbit) CodeRabbit nitpick: assert_match on response.body could pass/fail on text outside the account dropdown (sidebar, breadcrumb, error message, etc.) and gave false confidence in the refute_match exclusions. Switch to assert_select 'select[name="import[account_id]"] option', text: … so the assertions only see the option nodes the leak test actually cares about. * test(import): cover PDF account-select scoping; pluck PDF partial (#2194 review) jjmata: the _pdf_import.html.erb scoping change was not covered by the existing test (which only hit /import/uploads). Add a regression test hitting GET /imports/:id with a PdfImport fixture, asserting the account dropdown options match accessible accounts only. Also swap the PDF partial's accounts.map { |a| [a.name, a.id] } for .pluck(:name, :id) to match the .pluck pattern the other three CSV/QIF selects already use. * test(import): stub pdf_uploaded? on PDF leak test (#2194 ci) The new regression test hit ImportsController#show which redirects to the upload page when @import.pdf_uploaded? is false. The pdf_with_rows fixture has neither a pdf_file attached nor a statement, so the redirect fired before the partial under test ever rendered, failing with 302 in CI. Stub PdfImport#pdf_uploaded? to true so the test exercises the account-select scoping path it was written to cover. * fix(import): scope PDF form to :import so field names match (#2194 ci) The PDF-import account-select form was `form_with model: import` with no explicit scope. Because the model is a PdfImport, Rails derived the param namespace from the class name, so the rendered field was named `pdf_import[account_id]` — not `import[account_id]`. The ImportsController#update action accepts either via `params.dig(:pdf_import, :account_id) || params.dig(:import, :account_id)` so live submissions still worked, but the regression test added in 0685bbdf asserted on `select[name="import[account_id]"]` and matched zero options. Add `scope: :import` to align the rendered name with both the test selector and the convention used by the CSV/QIF forms on the upload page (which all use `scope: :import`). --- app/views/import/uploads/show.html.erb | 6 +++--- app/views/imports/_pdf_import.html.erb | 6 +++--- .../import/uploads_controller_test.rb | 12 ++++++++++++ test/controllers/imports_controller_test.rb | 18 ++++++++++++++++++ 4 files changed, 36 insertions(+), 6 deletions(-) diff --git a/app/views/import/uploads/show.html.erb b/app/views/import/uploads/show.html.erb index 7cf51d651..7d19818c6 100644 --- a/app/views/import/uploads/show.html.erb +++ b/app/views/import/uploads/show.html.erb @@ -56,7 +56,7 @@ <%= styled_form_with model: @import, scope: :import, url: import_upload_path(@import), multipart: true, class: "space-y-4" do |form| %> <%= form.select :account_id, - @import.family.accounts.visible.alphabetically.pluck(:name, :id), + Current.user.accessible_accounts.visible.alphabetically.pluck(:name, :id), { label: t(".qif_account_label"), include_blank: t(".qif_account_placeholder"), selected: @import.account_id }, required: true %> @@ -112,7 +112,7 @@ <%= form.select :col_sep, Import.separator_options, label: true %> <% if @import.type == "TransactionImport" || @import.type == "TradeImport" %> - <%= form.select :account_id, @import.family.accounts.visible.alphabetically.pluck(:name, :id), { label: t(".account_optional_label"), include_blank: t(".multi_account_import"), selected: @import.account_id } %> + <%= form.select :account_id, Current.user.accessible_accounts.visible.alphabetically.pluck(:name, :id), { label: t(".account_optional_label"), include_blank: t(".multi_account_import"), selected: @import.account_id } %> <% end %>