property values as maps closes #53 #56

moritz · 2025-05-22T02:01:26+02:00

moritz commented

2025-05-22 02:01:26 +02:00

boolean type does not work for any reason.

moritz added this to the Ich kann einen neuen Kontakt anlegen. milestone 2025-05-22 02:01:26 +02:00

moritz self-assigned this 2025-05-22 02:01:26 +02:00

moritz added 2 commits 2025-05-22 02:01:28 +02:00

chore: add regen_migrations script and seed-database to Justfile c91f65d04c

feat: property value as Union type

continuous-integration/drone/push Build is passing

Details

d085d80f74

moritz added this to the Sprint 2 - 07.05. - 28.05 project 2025-05-22 02:01:29 +02:00

requested reviews from simon, carla, rafael

2025-05-22 02:01:38 +02:00

moritz changed title from ~~WIP: property values as maps~~ to WIP: property values as maps closes #53

2025-05-22 02:02:25 +02:00

rafael reviewed 2025-05-22 11:59:41 +02:00

rafael left a comment

Excellent work :) I think we can even build SQL expressions for individual property values in maps stored as json.

Excellent work :) I think we can even build SQL [expressions for individual property values in maps stored as json](https://hexdocs.pm/ash/expressions.html#primitives).

lib/mv_web/member_live/form_component.ex Outdated

					
				@ -11,2 +11,3 @@

				          "property_type_id" => pt.id,

				          "value" => nil

				          "value" => %{

				            "type" => String.to_existing_atom(pt.type),

rafael commented

2025-05-22 11:38:14 +02:00

We could make this more convenient by storing PropertyType.type as an atom or an enum :)

https://hexdocs.pm/ash/Ash.Type.Atom.html

https://hexdocs.pm/ash/Ash.Type.Enum.html

We could make this more convenient by storing `PropertyType.type` as an atom or an enum :) https://hexdocs.pm/ash/Ash.Type.Atom.html https://hexdocs.pm/ash/Ash.Type.Enum.html

moritz marked this conversation as resolved

lib/mv_web/member_live/form_component.ex Outdated

					
				@ -36,2 +40,3 @@

				          <% type = Enum.find(@property_types, &(&1.id == f_property[:property_type_id].value)) %>

				          <.input field={f_property[:value]} label={type && type.name} />

				          <.inputs_for :let={value_form} field={f_property[:value]}>

				            <.input field={value_form[:value]} label={type && type.name} />

rafael commented

2025-05-22 11:56:25 +02:00

Adding a type="checkbox" parameter for boolean properties should make booleans correctly show up in the update form here.

Adding a `type="checkbox"` parameter for boolean properties should make booleans correctly show up in the update form here.

moritz marked this conversation as resolved

regen_migrations.sh Outdated

					
				@ -0,0 +1,19 @@

				#!/bin/bash

rafael commented

2025-05-22 11:27:13 +02:00

You can embed this script as a task in the Justfile: https://github.com/casey/just/?tab=readme-ov-file#shebang-recipes

moritz marked this conversation as resolved

moritz modified the project from Sprint 2 - 07.05. - 28.05 to Sprint 3 - 28.05 - 09.07

2025-05-28 16:50:42 +02:00

moritz force-pushed property_values from d085d80f74 to 83d6b1173d

2025-05-28 19:02:53 +02:00

Compare

moritz added 2 commits 2025-05-28 23:32:20 +02:00

chore(Justfile): allow regenerating migrations by commit hash 8b0a7c76ff

feat: add custom email type for validation 74f980414a

moritz force-pushed property_values from 74f980414a to 780efc3e91

2025-05-28 23:38:08 +02:00

Compare

moritz referenced this pull request

2025-05-28 23:48:23 +02:00

Create member validation #49

moritz force-pushed property_values from 780efc3e91 to 6a22efce1a

2025-05-28 23:53:16 +02:00

Compare

rafael approved these changes 2025-05-29 11:24:30 +02:00

rafael left a comment

Works like a charm, just had some minor comments but I'd say this is ready to merge :)

Justfile Outdated

					
				@ -40,0 +49,4 @@

				  #!/usr/bin/env bash

				  set -euo pipefail

				  # Pick migrations either from the given commit or untracked files

				  if [ -n "{{commit_hash}}" ]; then

rafael commented

2025-05-29 11:19:31 +02:00

Very nice 👌

lib/membership/email.ex Outdated

					
				@ -0,0 +1,34 @@

				defmodule Mv.Membership.Email do

				  @constraints [

				    match: ~r/^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$/,

rafael commented

2025-05-29 11:07:12 +02:00

For things like this, we can also integrate validations from ecto, with some duct-tape: By creating an ad-hoc ecto changeset and applying Ecto's validate_email function.

I don't think it's worth it to do this right now, but we should be aware of the opportunity to do this in the future, as Ecto's validations seem more battle-tested and feature-rich to me.

For things like this, we can also integrate [validations from ecto](https://hexdocs.pm/ecto_commons/EctoCommons.EmailValidator.html), with some duct-tape: By creating an ad-hoc ecto changeset and applying Ecto's `validate_email` function. I don't think it's worth it to do this right now, but we should be aware of the opportunity to do this in the future, as Ecto's validations seem more battle-tested and feature-rich to me.

lib/membership/email.ex Outdated

					
				@ -0,0 +11,4 @@

				    constraints: @constraints

				  @impl true

				  def cast_input(value, _) when is_binary(value) do

rafael commented

2025-05-29 11:08:41 +02:00

Why does this function get a binary value, is json stored as binary data in the DB? This is confusing to me because you use String functions with value below 🤔

Why does this function get a binary value, is json stored as binary data in the DB? This is confusing to me because you use String functions with `value` below 🤔

moritz commented

2025-06-02 14:23:35 +02:00

value is a string: https://hexdocs.pm/elixir/main/binaries-strings-and-charlists.html

`value` is a string: https://hexdocs.pm/elixir/main/binaries-strings-and-charlists.html

moritz marked this conversation as resolved

lib/membership/email.ex Outdated

					
				@ -0,0 +16,4 @@

				    cond do

				      @constraints[:min_length] && String.length(value) < @constraints[:min_length] ->

				        :error

rafael commented

2025-05-29 11:23:20 +02:00

Currently, this approach always shows an "is invalid" error message when validation fails. I think we should merge this as-is and move on as it's a minor detail, but in the future we could see if we can leverage ash's validations to provide more detailed error messages.

Currently, this approach always shows an "is invalid" error message when validation fails. I think we should merge this as-is and move on as it's a minor detail, but in the future we could see if we can leverage [ash's validations](https://hexdocs.pm/ash/validations.html) to provide more detailed error messages.