fix: resolve pr remarks
This commit is contained in:
parent
a92f503752
commit
b4657cae23
GPG key ID:
40E7A58C4AA1EDB2
6 changed files with 139 additions and 83 deletions
|
|
@ -9,7 +9,8 @@ defmodule MvWeb.Components.MemberFilterComponent do
|
|||
|
||||
- Uses `div` panel instead of `ul.menu/li` structure to avoid DaisyUI menu styles
|
||||
(padding, display, hover, font sizes) that would interfere with form controls.
|
||||
- Filter controls are form elements (fieldset, radio inputs), not menu items.
|
||||
- Filter controls are form elements (fieldset with legend, radio inputs), not menu items.
|
||||
Uses semantic `<fieldset>` and `<legend>` for proper accessibility and form structure.
|
||||
- Dropdown stays open when clicking filter segments to allow multiple filter changes.
|
||||
- Uses `phx-change` on form for radio inputs instead of individual `phx-click` events.
|
||||
|
||||
|
|
@ -88,7 +89,8 @@ defmodule MvWeb.Components.MemberFilterComponent do
|
|||
<!--
|
||||
NOTE: We use a div panel instead of ul.menu/li structure to avoid DaisyUI menu styles
|
||||
(padding, display, hover, font sizes) that would interfere with our form controls.
|
||||
Filter controls are form elements (fieldset, radio inputs), not menu items.
|
||||
Filter controls are form elements (fieldset with legend, radio inputs), not menu items.
|
||||
We use semantic fieldset/legend structure for proper accessibility.
|
||||
We use relative/absolute positioning instead of DaisyUI dropdown classes to have
|
||||
full control over the open/close state via LiveView.
|
||||
-->
|
||||
|
|
@ -107,11 +109,11 @@ defmodule MvWeb.Components.MemberFilterComponent do
|
|||
<div class="text-xs font-semibold opacity-70 mb-2 uppercase tracking-wider">
|
||||
{gettext("Payments")}
|
||||
</div>
|
||||
<div class="grid grid-cols-[1fr_auto] items-center gap-3 py-2">
|
||||
<label class="text-sm font-medium" for="payment-filter">
|
||||
<fieldset class="grid grid-cols-[1fr_auto] items-center gap-3 py-2 border-0 p-0 m-0 min-w-0">
|
||||
<legend class="text-sm font-medium col-start-1 float-left w-auto">
|
||||
{gettext("Payment Status")}
|
||||
</label>
|
||||
<fieldset class="join" aria-label={gettext("Payment status filter")}>
|
||||
</legend>
|
||||
<div class="join col-start-2">
|
||||
<label
|
||||
class={"#{payment_filter_label_class(@cycle_status_filter, nil)} has-[:focus-visible]:ring-2 has-[:focus-visible]:ring-primary"}
|
||||
for="payment-filter-all"
|
||||
|
|
@ -156,8 +158,8 @@ defmodule MvWeb.Components.MemberFilterComponent do
|
|||
<.icon name="hero-x-circle" class="h-5 w-5" />
|
||||
<span class="text-xs">{gettext("Unpaid")}</span>
|
||||
</label>
|
||||
</fieldset>
|
||||
</div>
|
||||
</div>
|
||||
</fieldset>
|
||||
</div>
|
||||
|
||||
<!-- Custom Fields Group -->
|
||||
|
|
@ -166,20 +168,14 @@ defmodule MvWeb.Components.MemberFilterComponent do
|
|||
{gettext("Custom Fields")}
|
||||
</div>
|
||||
<div class="max-h-60 overflow-y-auto pr-2">
|
||||
<div
|
||||
<fieldset
|
||||
:for={custom_field <- @boolean_custom_fields}
|
||||
class="grid grid-cols-[1fr_auto] items-center gap-3 py-2 border-b border-base-200 last:border-0"
|
||||
class="grid grid-cols-[1fr_auto] items-center gap-3 py-2 border-b border-base-200 last:border-0 border-0 p-0 m-0 min-w-0"
|
||||
>
|
||||
<label
|
||||
class="text-sm font-medium"
|
||||
for={"custom-boolean-filter-#{custom_field.id}"}
|
||||
>
|
||||
<legend class="text-sm font-medium col-start-1 float-left w-auto">
|
||||
{custom_field.name}
|
||||
</label>
|
||||
<fieldset
|
||||
class="join"
|
||||
aria-label={gettext("Filter by %{name}", name: custom_field.name)}
|
||||
>
|
||||
</legend>
|
||||
<div class="join col-start-2">
|
||||
<label
|
||||
class={"#{boolean_filter_label_class(@boolean_filters, custom_field.id, nil)} has-[:focus-visible]:ring-2 has-[:focus-visible]:ring-primary"}
|
||||
for={"custom-boolean-filter-#{custom_field.id}-all"}
|
||||
|
|
@ -228,8 +224,8 @@ defmodule MvWeb.Components.MemberFilterComponent do
|
|||
<.icon name="hero-x-circle" class="h-5 w-5" />
|
||||
<span class="text-xs">{gettext("No")}</span>
|
||||
</label>
|
||||
</fieldset>
|
||||
</div>
|
||||
</div>
|
||||
</fieldset>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
|
|
|
|||
|
|
@ -778,9 +778,32 @@ defmodule MvWeb.MemberLive.Index do
|
|||
|> Ash.Query.new()
|
||||
|> Ash.Query.select(@overview_fields)
|
||||
|
||||
# Load custom field values for visible custom fields (based on user selection)
|
||||
# Load custom field values for visible custom fields AND active boolean filters
|
||||
# This ensures boolean filters work even when the custom field is not visible in overview
|
||||
visible_custom_field_ids = socket.assigns[:visible_custom_field_ids] || []
|
||||
query = load_custom_field_values(query, visible_custom_field_ids)
|
||||
|
||||
# Get IDs of active boolean filters (whitelisted against boolean_custom_fields)
|
||||
# Convert boolean_custom_fields list to map for efficient lookup (consistent with maybe_update_boolean_filters)
|
||||
boolean_custom_fields_map =
|
||||
socket.assigns.boolean_custom_fields
|
||||
|> Map.new(fn cf -> {to_string(cf.id), cf} end)
|
||||
|
||||
active_boolean_filter_ids =
|
||||
socket.assigns.boolean_custom_field_filters
|
||||
|> Map.keys()
|
||||
|> Enum.filter(fn id_str ->
|
||||
# Validate UUID format and check against whitelist
|
||||
String.length(id_str) <= @max_uuid_length &&
|
||||
match?({:ok, _}, Ecto.UUID.cast(id_str)) &&
|
||||
Map.has_key?(boolean_custom_fields_map, id_str)
|
||||
end)
|
||||
|
||||
# Union of visible IDs and active filter IDs
|
||||
ids_to_load =
|
||||
(visible_custom_field_ids ++ active_boolean_filter_ids)
|
||||
|> Enum.uniq()
|
||||
|
||||
query = load_custom_field_values(query, ids_to_load)
|
||||
|
||||
# Load membership fee cycles for status display
|
||||
query = MembershipFeeStatus.load_cycles_for_members(query, socket.assigns.show_current_cycle)
|
||||
|
|
@ -1233,35 +1256,43 @@ defmodule MvWeb.MemberLive.Index do
|
|||
|> Map.new(fn cf -> {to_string(cf.id), cf} end)
|
||||
|
||||
# Parse all boolean filter parameters
|
||||
# Security: Use reduce_while to abort early after @max_boolean_filters to prevent DoS attacks
|
||||
# This protects CPU/Parsing costs, not just memory/state
|
||||
# We count processed parameters (not just valid filters) to protect against parsing DoS
|
||||
prefix_length = String.length(@boolean_filter_prefix)
|
||||
|
||||
filters =
|
||||
{filters, total_processed} =
|
||||
params
|
||||
|> Enum.filter(fn {key, _value} -> String.starts_with?(key, @boolean_filter_prefix) end)
|
||||
|> Enum.reduce(%{}, fn {key, value_str}, acc ->
|
||||
process_boolean_filter_param(
|
||||
key,
|
||||
value_str,
|
||||
prefix_length,
|
||||
boolean_custom_fields,
|
||||
acc
|
||||
)
|
||||
|> Enum.reduce_while({%{}, 0}, fn {key, value_str}, {acc, count} ->
|
||||
if count >= @max_boolean_filters do
|
||||
Logger.warning(
|
||||
"Too many boolean filter parameters in request (#{count} processed), limiting to #{@max_boolean_filters} to prevent DoS"
|
||||
)
|
||||
|
||||
{:halt, {acc, count}}
|
||||
else
|
||||
new_acc =
|
||||
process_boolean_filter_param(
|
||||
key,
|
||||
value_str,
|
||||
prefix_length,
|
||||
boolean_custom_fields,
|
||||
acc
|
||||
)
|
||||
|
||||
# Increment counter for each processed parameter (DoS protection)
|
||||
# Note: We count processed params, not just valid filters, to protect parsing costs
|
||||
{:cont, {new_acc, count + 1}}
|
||||
end
|
||||
end)
|
||||
|
||||
# Security: Limit number of filters to prevent DoS attacks
|
||||
# Maximum @max_boolean_filters boolean filters allowed per request
|
||||
filters =
|
||||
if map_size(filters) > @max_boolean_filters do
|
||||
Logger.warning(
|
||||
"Too many boolean filters requested: #{map_size(filters)}, limiting to #{@max_boolean_filters}"
|
||||
)
|
||||
|
||||
filters
|
||||
|> Enum.take(@max_boolean_filters)
|
||||
|> Enum.into(%{})
|
||||
else
|
||||
filters
|
||||
end
|
||||
# Log additional warning if we hit the limit
|
||||
if total_processed >= @max_boolean_filters do
|
||||
Logger.warning(
|
||||
"Boolean filter limit reached: processed #{total_processed} parameters, accepted #{map_size(filters)} valid filters (max: #{@max_boolean_filters})"
|
||||
)
|
||||
end
|
||||
|
||||
assign(socket, :boolean_custom_field_filters, filters)
|
||||
end
|
||||
|
|
@ -1340,8 +1371,8 @@ defmodule MvWeb.MemberLive.Index do
|
|||
# This ensures no raw user input is ever passed to filter functions.
|
||||
#
|
||||
# Returns:
|
||||
# - `:true` for "true" string
|
||||
# - `:false` for "false" string
|
||||
# - `true` for "true" string
|
||||
# - `false` for "false" string
|
||||
# - `nil` for any other value
|
||||
defp determine_boolean_filter("true"), do: true
|
||||
defp determine_boolean_filter("false"), do: false
|
||||
|
|
|
|||
|
|
@ -1925,11 +1925,6 @@ msgstr "Validierung fehlgeschlagen: %{message}"
|
|||
msgid "Close"
|
||||
msgstr "Schließen"
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Filter by %{name}"
|
||||
msgstr "Filtern nach %{name}"
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Filter members"
|
||||
|
|
@ -1945,12 +1940,17 @@ msgstr "Mitgliedsfilter"
|
|||
msgid "Payment Status"
|
||||
msgstr "Bezahlstatus"
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format, fuzzy
|
||||
msgid "Payment status filter"
|
||||
msgstr "Bezahlstatusfilter"
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Reset"
|
||||
msgstr "Zurücksetzen"
|
||||
|
||||
#~ #: lib/mv_web/live/components/member_filter_component.ex
|
||||
#~ #, elixir-autogen, elixir-format
|
||||
#~ msgid "Filter by %{name}"
|
||||
#~ msgstr "Filtern nach %{name}"
|
||||
|
||||
#~ #: lib/mv_web/live/components/member_filter_component.ex
|
||||
#~ #, elixir-autogen, elixir-format, fuzzy
|
||||
#~ msgid "Payment status filter"
|
||||
#~ msgstr "Bezahlstatusfilter"
|
||||
|
|
|
|||
|
|
@ -1926,11 +1926,6 @@ msgstr ""
|
|||
msgid "Close"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Filter by %{name}"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Filter members"
|
||||
|
|
@ -1946,11 +1941,6 @@ msgstr ""
|
|||
msgid "Payment Status"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Payment status filter"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Reset"
|
||||
|
|
|
|||
|
|
@ -780,6 +780,7 @@ msgstr ""
|
|||
msgid "Payment Data"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#: lib/mv_web/live/member_live/form.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Payments"
|
||||
|
|
@ -1925,11 +1926,6 @@ msgstr ""
|
|||
msgid "Close"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Filter by %{name}"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Filter members"
|
||||
|
|
@ -1940,21 +1936,11 @@ msgstr ""
|
|||
msgid "Member filter"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format, fuzzy
|
||||
msgid "Payment"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format, fuzzy
|
||||
msgid "Payment Status"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format, fuzzy
|
||||
msgid "Payment status filter"
|
||||
msgstr ""
|
||||
|
||||
#: lib/mv_web/live/components/member_filter_component.ex
|
||||
#, elixir-autogen, elixir-format
|
||||
msgid "Reset"
|
||||
|
|
@ -1986,6 +1972,11 @@ msgstr ""
|
|||
#~ msgid "Regular"
|
||||
#~ msgstr ""
|
||||
|
||||
#~ #: lib/mv_web/live/components/member_filter_component.ex
|
||||
#~ #, elixir-autogen, elixir-format, fuzzy
|
||||
#~ msgid "Payment"
|
||||
#~ msgstr ""
|
||||
|
||||
#~ #: lib/mv_web/live/contribution_period_live/show.ex
|
||||
#~ #, elixir-autogen, elixir-format
|
||||
#~ msgid "Current"
|
||||
|
|
@ -2207,6 +2198,11 @@ msgstr ""
|
|||
#~ msgid "Contributions for %{name}"
|
||||
#~ msgstr ""
|
||||
|
||||
#~ #: lib/mv_web/live/components/member_filter_component.ex
|
||||
#~ #, elixir-autogen, elixir-format, fuzzy
|
||||
#~ msgid "Payment status filter"
|
||||
#~ msgstr ""
|
||||
|
||||
#~ #: lib/mv_web/live/contribution_type_live/index.ex
|
||||
#~ #, elixir-autogen, elixir-format
|
||||
#~ msgid "Family"
|
||||
|
|
@ -2251,3 +2247,8 @@ msgstr ""
|
|||
#~ #, elixir-autogen, elixir-format
|
||||
#~ msgid "About Contribution Types"
|
||||
#~ msgstr ""
|
||||
|
||||
#~ #: lib/mv_web/live/components/member_filter_component.ex
|
||||
#~ #, elixir-autogen, elixir-format
|
||||
#~ msgid "Filter by %{name}"
|
||||
#~ msgstr ""
|
||||
|
|
|
|||
|
|
@ -1526,6 +1526,44 @@ defmodule MvWeb.MemberLive.IndexTest do
|
|||
refute html =~ "FalseMember"
|
||||
end
|
||||
|
||||
test "boolean filter works even when custom field is not visible in overview", %{conn: conn} do
|
||||
conn = conn_with_oidc_user(conn)
|
||||
|
||||
# Create boolean field with show_in_overview: false
|
||||
boolean_field = create_boolean_custom_field(%{show_in_overview: false})
|
||||
|
||||
_member_with_true =
|
||||
create_member_with_boolean_value(%{first_name: "TrueMember"}, boolean_field, true)
|
||||
|
||||
_member_with_false =
|
||||
create_member_with_boolean_value(%{first_name: "FalseMember"}, boolean_field, false)
|
||||
|
||||
{:ok, _member_without_value} =
|
||||
Mv.Membership.Member
|
||||
|> Ash.Changeset.for_create(:create_member, %{
|
||||
first_name: "NoValue",
|
||||
last_name: "Member",
|
||||
email: "novalue.member.#{System.unique_integer([:positive])}@example.com"
|
||||
})
|
||||
|> Ash.create()
|
||||
|
||||
# Test that filter works even though field is not visible in overview
|
||||
{:ok, _view, html_true} =
|
||||
live(conn, "/members?bf_#{boolean_field.id}=true")
|
||||
|
||||
assert html_true =~ "TrueMember"
|
||||
refute html_true =~ "FalseMember"
|
||||
refute html_true =~ "NoValue"
|
||||
|
||||
# Test false filter
|
||||
{:ok, _view, html_false} =
|
||||
live(conn, "/members?bf_#{boolean_field.id}=false")
|
||||
|
||||
assert html_false =~ "FalseMember"
|
||||
refute html_false =~ "TrueMember"
|
||||
refute html_false =~ "NoValue"
|
||||
end
|
||||
|
||||
test "boolean custom field appears in filter dropdown after being added", %{conn: conn} do
|
||||
conn = conn_with_oidc_user(conn)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue