local-it/mitgliederverwaltung
2
3
Fork 0
Code Issues 51 Pull requests 13 Projects 1 Releases Packages 1 Activity Actions

Default Memberfields closes #74 #48 #49 #50 #81

Merged
carla merged 6 commits from feature/74_memberfields into main 2025-06-25 14:28:07 +02:00
Conversation 4 Commits 6 Files changed 16 +730 -20
moritz commented 2025-06-17 16:01:38 +02:00
Owner
Copy link

#48
#49
#74
#50

https://git.local-it.org/local-it/mitgliederverwaltung/issues/48 https://git.local-it.org/local-it/mitgliederverwaltung/issues/49 https://git.local-it.org/local-it/mitgliederverwaltung/issues/74 https://git.local-it.org/local-it/mitgliederverwaltung/issues/50
moritz self-assigned this 2025-06-17 16:01:38 +02:00
moritz added 4 commits 2025-06-17 16:01:40 +02:00
fix member deletion: property delete on cascade
Some checks failed
continuous-integration/drone/push Build is failing
Details
6b770a98ef
moritz added this to the Sprint 3 - 28.05 - 09.07 project 2025-06-17 16:01:41 +02:00
requested reviews from simon, carla, rafael 2025-06-17 16:01:49 +02:00
moritz added 1 commit 2025-06-17 16:15:28 +02:00
chore: fix linting
Some checks failed
continuous-integration/drone/push Build is failing
Details
d771e5b579
carla approved these changes 2025-06-17 16:38:12 +02:00
carla left a comment
Owner
Copy link

Nice :)

Nice :)
moritz force-pushed feature/74_memberfields from d771e5b579 to 03da80cca5 2025-06-18 17:44:25 +02:00 Compare
moritz force-pushed feature/74_memberfields from 03da80cca5 to 2ab3332941 2025-06-18 23:35:34 +02:00 Compare
moritz commented 2025-06-18 23:48:31 +02:00
Author
Owner
Copy link

The last main rebase created this error:

Could not find "rebar3", which is needed to build dependency :telemetry
** (Mix) Could not find "rebar3" to compile dependency :telemetry, please ensure "rebar3" is available
Shall I install rebar3? (if running non-interactively, use "mix local.rebar --force") [Yn]

https://drone.dev.local-it.cloud/local-it/mitgliederverwaltung/284/1/7

The last main rebase created this error: ``` Could not find "rebar3", which is needed to build dependency :telemetry ** (Mix) Could not find "rebar3" to compile dependency :telemetry, please ensure "rebar3" is available Shall I install rebar3? (if running non-interactively, use "mix local.rebar --force") [Yn] ``` https://drone.dev.local-it.cloud/local-it/mitgliederverwaltung/284/1/7
rafael commented 2025-06-19 16:33:13 +02:00
Collaborator
Copy link

@moritz wrote in #81 (comment):

The last main rebase created this error:

Could not find "rebar3", which is needed to build dependency :telemetry
** (Mix) Could not find "rebar3" to compile dependency :telemetry, please ensure "rebar3" is available
Shall I install rebar3? (if running non-interactively, use "mix local.rebar --force") [Yn]

https://drone.dev.local-it.cloud/local-it/mitgliederverwaltung/284/1/7

Seems like my commit d54b226b was a bit too eager. I've opened a MR for reverting it and will check if that fixes the issue.

@moritz wrote in https://git.local-it.org/local-it/mitgliederverwaltung/pulls/81#issuecomment-11691: > The last main rebase created this error: > > ```text > Could not find "rebar3", which is needed to build dependency :telemetry > ** (Mix) Could not find "rebar3" to compile dependency :telemetry, please ensure "rebar3" is available > Shall I install rebar3? (if running non-interactively, use "mix local.rebar --force") [Yn] > ``` > > https://drone.dev.local-it.cloud/local-it/mitgliederverwaltung/284/1/7 Seems like my commit d54b226b was a bit too eager. I've opened a MR for reverting it and will check if that fixes the issue.
rafael requested changes 2025-06-19 17:54:14 +02:00
Justfile Outdated
@ -9,6 +9,7 @@ migrate-database:
reset-database:
mix ash.reset
MIX_ENV=test mix ash.setup
rafael commented 2025-06-19 17:04:12 +02:00
Collaborator
Copy link

Why not use ash.reset here as well?

Why not use `ash.reset` here as well?
moritz commented 2025-06-19 22:36:58 +02:00
Author
Owner
Copy link

typo that works as well. but i change it

typo that works as well. but i change it
carla marked this conversation as resolved
lib/membership/member.ex Outdated
@ -19,1 +36,4 @@
destroy :destroy do
primary? true
change Ash.Resource.Change.Builtins.cascade_destroy(:properties)
rafael commented 2025-06-19 17:51:23 +02:00
Collaborator
Copy link

I think this will run using ash's "notifiers", and as such will run outside the transaction used for the destroy itself. This means that we could end up with errors if postgres detects that a property does not have an associated member.

Also, I think the "references" block in the property's postgres configuration should take make this line obsolete because it tells postgres to cascade delete properties when a member is deleted? 🤔

I think this will run using ash's "notifiers", and as such [will run outside the transaction used for the destroy itself](https://hexdocs.pm/ash/notifiers.html#transactions). This means that we could end up with errors if postgres detects that a property does not have an associated member. Also, I think the "references" block in the property's postgres configuration should take make this line obsolete because it tells postgres to cascade delete properties when a member is deleted? 🤔
moritz commented 2025-06-19 22:44:34 +02:00
Author
Owner
Copy link

I tried different approaches to get the cascade deletion working. I think in the end the postgres reference block has done the job and this is still a leftover. I will try to run it without this change.

I tried different approaches to get the cascade deletion working. I think in the end the postgres reference block has done the job and this is still a leftover. I will try to run it without this change.
carla commented 2025-06-20 07:35:21 +02:00
Owner
Copy link

Seems that properties are deleted using default :destroy with the reference block

Seems that properties are deleted using default :destroy with the reference block
carla marked this conversation as resolved
lib/membership/member.ex Outdated
@ -28,0 +87,4 @@
validate fn changeset, _ ->
join_date = Ash.Changeset.get_attribute(changeset, :join_date)
if join_date && Date.compare(join_date, Date.utc_today()) == :gt do
rafael commented 2025-06-19 17:28:39 +02:00
Collaborator
Copy link

I'm fine with doing this using custom validations as I think "real" code is easier to read than DSLs - but I think if we wanted to, we could achieve this in a more concise way using ash's built-in compare validation, e.g.:

validate compare(:join_date, less_than: &Date.utc_today/0),
  where: [present(:join_date)],
  message: "cannot be in the future"

Not sure if I got the syntax exactly right, but I hope you get what I mean. I think this approach would work for other custom validations in this file as well, e.g. using the built-in match validation for regexes.

One advantage of using the built-in validations is that they are "atomic", meaning they can run in the database directly, which can improve performance.

I'm fine with doing this using custom validations as I think "real" code is easier to read than DSLs - but I think if we wanted to, we could achieve this in a more concise way using ash's [built-in compare validation](https://hexdocs.pm/ash/Ash.Resource.Validation.Builtins.html#compare/2), e.g.: ```elixir validate compare(:join_date, less_than: &Date.utc_today/0), where: [present(:join_date)], message: "cannot be in the future" ``` Not sure if I got the syntax exactly right, but I hope you get what I mean. I think this approach would work for other custom validations in this file as well, e.g. using the built-in `match` validation for regexes. One advantage of using the built-in validations is that they are "atomic", meaning they can run in the database directly, which can improve performance.
moritz commented 2025-06-19 22:45:26 +02:00
Author
Owner
Copy link

Thank you for the hint, I will apply it and test if it works.

Thank you for the hint, I will apply it and test if it works.
carla commented 2025-06-20 07:44:33 +02:00
Owner
Copy link

Maybe not from a technical but from a user perspective: Why should it not be possible to add members already, where the membership starts maybe beginning of next month? Should we take the question to the Pilotvereine?

Maybe not from a technical but from a user perspective: Why should it not be possible to add members already, where the membership starts maybe beginning of next month? Should we take the question to the Pilotvereine?
👍 2
carla marked this conversation as resolved
lib/membership/member.ex Outdated
@ -28,0 +132,4 @@
validate fn changeset, _ ->
paid = Ash.Changeset.get_attribute(changeset, :paid)
if not is_nil(paid) and not is_boolean(paid) do
rafael commented 2025-06-19 17:25:01 +02:00
Collaborator
Copy link

I don't think this works the way it appears: If I remove the type="checkbox" attribute in the paid form field, and e.g. enter "not a boolean" instead of "true" or "false", I get back an "is invalid" error. This hints to me that the validate function does not get called if the input does not cast to the required attribute type. There's a hint buried in the ash docs:

Keep in mind that some types cast certain values to nil, and validations are applied after all inputs have been cast. For example, a :string type attribute with the default constraints will cast "" as nil, meaning an input of "" would pass the absent validation.

I don't think this works the way it appears: If I remove the `type="checkbox"` attribute in the paid form field, and e.g. enter "not a boolean" instead of "true" or "false", I get back an "is invalid" error. This hints to me that the validate function does not get called if the input does not cast to the required attribute type. There's a [hint buried in the ash docs](https://hexdocs.pm/ash/Ash.Resource.Validation.Builtins.html#absent/2): > Keep in mind that some types cast certain values to nil, and validations are applied after all inputs have been cast. For example, a :string type attribute with the default constraints will cast "" as nil, meaning an input of "" would pass the absent validation.
carla commented 2025-06-20 08:09:23 +02:00
Owner
Copy link

Should we omit that validation? Because we define "paid" as boolean and I think using type="checkbox" should not be removed anywhere or am I wrong?

Should we omit that validation? Because we define "paid" as boolean and I think using `type="checkbox"` should not be removed anywhere or am I wrong?
rafael commented 2025-06-20 11:32:31 +02:00
Collaborator
Copy link

Just realized that my comment was very unspecific about what to change :D I think we can omit the validation, because ash already ensures that the attribute is a boolean, because the attribute definition marks it as the type :boolean. We should not remove the type="checkbox", that was just what I did to be able to send invalid data to the server.

Just realized that my comment was very unspecific about what to change :D I think we can omit the validation, because ash already ensures that the attribute is a boolean, because the attribute definition marks it as the type `:boolean`. We should not remove the `type="checkbox"`, that was just what I did to be able to send invalid data to the server.
👍 1
carla marked this conversation as resolved
lib/membership/member.ex Outdated
@ -31,3 +217,3 @@
relationships do
has_many :properties, Mv.Membership.Property
has_many :properties, Mv.Membership.Property, destination_attribute: :member_id
rafael commented 2025-06-19 17:44:01 +02:00
Collaborator
Copy link

AFAIK :member_id is the default destination_attribute, why is this addition needed?

AFAIK `:member_id` is the default `destination_attribute`, why is this addition needed?
moritz commented 2025-06-19 22:46:17 +02:00
Author
Owner
Copy link

As mentioned above, this is also a leftover of getting the deletion to work.

As mentioned above, this is also a leftover of getting the deletion to work.
carla marked this conversation as resolved
carla added 1 commit 2025-06-20 12:48:20 +02:00
carla merged commit 4c117e1971 into main 2025-06-25 14:28:07 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
simon
carla
rafael
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
high priority

   
invalid

Something is wrong

  
L

5 h

  
low priority

   
M

3 h

  
medium priority

   
needs refinement

   
question

More information is needed

  
S

1 h

  
UX research

   
wontfix

This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
high priority
invalid
L
low priority
M
medium priority
needs refinement
question
S
UX research
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Sprint 3 - 28.05 - 09.07
Assignees
Clear assignees
No assignees
moritz
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: local-it/mitgliederverwaltung#81
No description provided.
Delete branch "feature/74_memberfields"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?