QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-07-12 12:55:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(sharing): scope import account selects to accessible_by (#1803) (#2194)

Browse Source 
* 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`).
This commit is contained in:
dripsmvcp
2026-06-11 07:56:35 -07:00
committed by GitHub
parent 0e1b3f8396
commit 1157ea8f20
4 changed files with 36 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
app/views/import/uploads/show.html.erb
View File

@@ -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 %>
<label for="import_import_file_csv" class="flex flex-col items-center justify-center w-full h-64 border border-secondary border-dashed rounded-xl cursor-pointer" data-controller="file-upload" data-file-upload-target="uploadArea">
@@ -144,7 +144,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 %>
<%= form.text_area :raw_file_str,

6
app/views/imports/_pdf_import.html.erb
View File

@@ -39,10 +39,10 @@
<div class="space-y-2">
<h2 class="font-medium text-primary"><%= t("imports.pdf_import.select_account", default: "Import to Account") %></h2>
<%= form_with model: import, url: import_path(import), method: :patch, class: "space-y-2" do |f| %>
<% accounts = import.family.accounts.manual.alphabetically %>
<%= form_with model: import, scope: :import, url: import_path(import), method: :patch, class: "space-y-2" do |f| %>
<% accounts = Current.user.accessible_accounts.manual.alphabetically.pluck(:name, :id) %>
<% if accounts.any? %>
<%= f.select :account_id, options_for_select(accounts.map { |a| [a.name, a.id] }, import.account_id), { include_blank: t("imports.pdf_import.select_account_placeholder", default: "Select an account...") }, class: "form-field__input" %>
<%= f.select :account_id, options_for_select(accounts, import.account_id), { include_blank: t("imports.pdf_import.select_account_placeholder", default: "Select an account...") }, class: "form-field__input" %>
<% if import.account.nil? %>
<p class="text-xs text-secondary"><%= t("imports.pdf_import.select_account_hint", default: "Choose which account to import these transactions into.") %></p>
<% end %>

12
test/controllers/import/uploads_controller_test.rb
View File

@@ -35,6 +35,18 @@ class Import::UploadsControllerTest < ActionDispatch::IntegrationTest
assert_equal "CSV uploaded successfully.", flash[:notice]
end
test "account select does not leak unshared family accounts (#1803)" do
sign_in users(:family_member)
get import_upload_url(@import)
assert_response :success
assert_select 'select[name="import[account_id]"] option', text: "Checking Account"
assert_select 'select[name="import[account_id]"] option', text: "Collectable Account", count: 0
assert_select 'select[name="import[account_id]"] option', text: "IOU (personal debt to friend)", count: 0
assert_select 'select[name="import[account_id]"] option', text: "Plaid Depository Account", count: 0
end
test "invalid csv cannot be uploaded" do
patch import_upload_url(@import), params: {
import: {

18
test/controllers/imports_controller_test.rb
View File

@@ -363,4 +363,22 @@ class ImportsControllerTest < ActionDispatch::IntegrationTest
assert_redirected_to imports_path
end
test "PDF import account select does not leak unshared family accounts (#1803)" do
sign_in users(:family_member)
pdf_import = imports(:pdf_with_rows)
# The fixture has no attached pdf_file and no statement, so
# ImportsController#show would redirect to the upload page. The
# partial under test only renders for an uploaded PDF — stub the
# state so we exercise the actual account-select scoping path.
PdfImport.any_instance.stubs(:pdf_uploaded?).returns(true)
get import_url(pdf_import)
assert_response :success
assert_select 'select[name="import[account_id]"] option', text: "Checking Account"
assert_select 'select[name="import[account_id]"] option', text: "Collectable Account", count: 0
assert_select 'select[name="import[account_id]"] option', text: "IOU (personal debt to friend)", count: 0
assert_select 'select[name="import[account_id]"] option', text: "Plaid Depository Account", count: 0
end
end