From 7da037d81d2d49f7b0345f29dc9e5eb951d74884 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 19 Jan 2026 11:43:51 +0100 Subject: [PATCH] refactor: adds schemales changeset and validation constant --- docs/email-validation.md | 62 ++++++++++++ lib/accounts/user.ex | 4 +- lib/membership/member.ex | 4 +- lib/mv/constants.ex | 21 ++++ lib/mv/membership/import/member_csv.ex | 99 +++++++++++++------ test/mv/membership/import/member_csv_test.exs | 2 + 6 files changed, 158 insertions(+), 34 deletions(-) create mode 100644 docs/email-validation.md diff --git a/docs/email-validation.md b/docs/email-validation.md new file mode 100644 index 0000000..74a1ffd --- /dev/null +++ b/docs/email-validation.md @@ -0,0 +1,62 @@ +# Email Validation Strategy + +We use `EctoCommons.EmailValidator` with both `:html_input` and `:pow` checks, defined centrally in `Mv.Constants.email_validator_checks/0`. + +## Checks Used + +- `:html_input` - Pragmatic validation matching browser `` behavior +- `:pow` - Stricter validation following email spec, supports internationalization (Unicode) + +## Rationale + +Using both checks ensures: +- **Compatibility with common email providers** (`:html_input`) - Matches what users expect from web forms +- **Compliance with email standards** (`:pow`) - Follows RFC 5322 and related specifications +- **Support for international email addresses** (`:pow`) - Allows Unicode characters in email addresses + +This dual approach provides a balance between user experience (accepting common email formats) and technical correctness (validating against email standards). + +## Usage + +The checks are used consistently across all email validation points: + +- `Mv.Membership.Import.MemberCSV.validate_row/3` - CSV import validation +- `Mv.Membership.Member` validations - Member resource validation +- `Mv.Accounts.User` validations - User resource validation + +All three locations use `Mv.Constants.email_validator_checks()` to ensure consistency. + +## Implementation Details + +### CSV Import Validation + +The CSV import uses a schemaless changeset for email validation: + +```elixir +changeset = + {%{}, %{email: :string}} + |> Ecto.Changeset.cast(%{email: Map.get(member_attrs, :email)}, [:email]) + |> Ecto.Changeset.update_change(:email, &String.trim/1) + |> Ecto.Changeset.validate_required([:email]) + |> EctoCommons.EmailValidator.validate_email(:email, checks: Mv.Constants.email_validator_checks()) +``` + +This approach: +- Trims whitespace before validation +- Validates email is required +- Validates email format using the centralized checks +- Provides consistent error messages via Gettext + +### Resource Validations + +Both `Member` and `User` resources use similar schemaless changesets within their Ash validations, ensuring consistent validation behavior across the application. + +## Changing the Validation Strategy + +To change the email validation checks, update the `@email_validator_checks` constant in `Mv.Constants`. This will automatically apply to all validation points. + +**Note:** Changing the validation strategy may affect existing data. Consider: +- Whether existing emails will still be valid +- Migration strategy for invalid emails +- User communication if validation becomes stricter + diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index ceedeae..9598b76 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -290,7 +290,9 @@ defmodule Mv.Accounts.User do changeset2 = {%{}, %{email: :string}} |> Ecto.Changeset.cast(%{email: email_string}, [:email]) - |> EctoCommons.EmailValidator.validate_email(:email, checks: [:html_input, :pow]) + |> EctoCommons.EmailValidator.validate_email(:email, + checks: Mv.Constants.email_validator_checks() + ) if changeset2.valid? do :ok diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 41e02b1..5a4d01c 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -453,7 +453,9 @@ defmodule Mv.Membership.Member do changeset2 = {%{}, %{email: :string}} |> Ecto.Changeset.cast(%{email: email}, [:email]) - |> EctoCommons.EmailValidator.validate_email(:email, checks: [:html_input, :pow]) + |> EctoCommons.EmailValidator.validate_email(:email, + checks: Mv.Constants.email_validator_checks() + ) if changeset2.valid? do :ok diff --git a/lib/mv/constants.ex b/lib/mv/constants.ex index 82a8400..73bfcd9 100644 --- a/lib/mv/constants.ex +++ b/lib/mv/constants.ex @@ -19,6 +19,8 @@ defmodule Mv.Constants do @custom_field_prefix "custom_field_" + @email_validator_checks [:html_input, :pow] + def member_fields, do: @member_fields @doc """ @@ -30,4 +32,23 @@ defmodule Mv.Constants do "custom_field_" """ def custom_field_prefix, do: @custom_field_prefix + + @doc """ + Returns the email validator checks used for EctoCommons.EmailValidator. + + We use both `:html_input` and `:pow` checks: + - `:html_input` - Pragmatic validation matching browser `` behavior + - `:pow` - Stricter validation following email spec, supports internationalization (Unicode) + + Using both ensures: + - Compatibility with common email providers (html_input) + - Compliance with email standards (pow) + - Support for international email addresses (pow) + + ## Examples + + iex> Mv.Constants.email_validator_checks() + [:html_input, :pow] + """ + def email_validator_checks, do: @email_validator_checks end diff --git a/lib/mv/membership/import/member_csv.ex b/lib/mv/membership/import/member_csv.ex index 6a1ba0f..ec729cd 100644 --- a/lib/mv/membership/import/member_csv.ex +++ b/lib/mv/membership/import/member_csv.ex @@ -334,45 +334,80 @@ defmodule Mv.Membership.Import.MemberCSV do member_attrs = Map.get(row_map, :member, %{}) custom_attrs = Map.get(row_map, :custom, %{}) - # Trim all string values in member map - trimmed_member = trim_string_values(member_attrs) + # Validate email using schemaless changeset + changeset = + {%{}, %{email: :string}} + |> Ecto.Changeset.cast(%{email: Map.get(member_attrs, :email)}, [:email]) + |> Ecto.Changeset.update_change(:email, &String.trim/1) + |> Ecto.Changeset.validate_required([:email]) + |> EctoCommons.EmailValidator.validate_email(:email, + checks: Mv.Constants.email_validator_checks() + ) - # Validate email presence (after trim) - email = Map.get(trimmed_member, :email) - - cond do - is_nil(email) or email == "" -> - {:error, - %Error{ - csv_line_number: csv_line_number, - field: :email, - message: gettext("Email is required.") - }} - - not valid_email_format?(email) -> - {:error, - %Error{ - csv_line_number: csv_line_number, - field: :email, - message: gettext("Email is invalid.") - }} - - true -> - {:ok, %{member: trimmed_member, custom: custom_attrs}} + if changeset.valid? do + # Apply trimmed email back to member_attrs + trimmed_email = Ecto.Changeset.get_change(changeset, :email) + trimmed_member = Map.put(member_attrs, :email, trimmed_email) |> trim_string_values() + {:ok, %{member: trimmed_member, custom: custom_attrs}} + else + # Extract first error + error = extract_changeset_error(changeset, csv_line_number) + {:error, error} end end - # Validates email format using EctoCommons.EmailValidator - defp valid_email_format?(email) when is_binary(email) do - changeset = - {%{}, %{email: :string}} - |> Ecto.Changeset.cast(%{email: email}, [:email]) - |> EctoCommons.EmailValidator.validate_email(:email, checks: [:html_input, :pow]) + # Extracts the first error from a changeset and converts it to a MemberCSV.Error struct + defp extract_changeset_error(changeset, csv_line_number) do + case Ecto.Changeset.traverse_errors(changeset, fn {msg, opts} -> + Enum.reduce(opts, msg, fn {key, value}, acc -> + String.replace(acc, "%{#{key}}", to_string(value)) + end) + end) do + %{email: [message | _]} -> + # Email-specific error + %Error{ + csv_line_number: csv_line_number, + field: :email, + message: gettext_error_message(message) + } - changeset.valid? + errors when map_size(errors) > 0 -> + # Get first error (any field) + {field, [message | _]} = Enum.at(Enum.to_list(errors), 0) + + %Error{ + csv_line_number: csv_line_number, + field: String.to_existing_atom(to_string(field)), + message: gettext_error_message(message) + } + + _ -> + # Fallback + %Error{ + csv_line_number: csv_line_number, + field: :email, + message: gettext("Email is invalid.") + } + end end - defp valid_email_format?(_), do: false + # Maps changeset error messages to appropriate Gettext messages + defp gettext_error_message(message) when is_binary(message) do + cond do + String.contains?(String.downcase(message), "required") or + String.contains?(String.downcase(message), "can't be blank") -> + gettext("Email is required.") + + String.contains?(String.downcase(message), "invalid") or + String.contains?(String.downcase(message), "not a valid") -> + gettext("Email is invalid.") + + true -> + message + end + end + + defp gettext_error_message(_), do: gettext("Email is invalid.") # Processes a single row and creates member with custom field values defp process_row( diff --git a/test/mv/membership/import/member_csv_test.exs b/test/mv/membership/import/member_csv_test.exs index 5482354..6edc9d8 100644 --- a/test/mv/membership/import/member_csv_test.exs +++ b/test/mv/membership/import/member_csv_test.exs @@ -403,6 +403,7 @@ defmodule Mv.Membership.Import.MemberCSVTest do }, custom: %{} } + csv_line_number = 2 opts = [] @@ -417,6 +418,7 @@ defmodule Mv.Membership.Import.MemberCSVTest do member: %{email: "john@example.com"}, custom: %{"field1" => "value1"} } + csv_line_number = 2 opts = []