Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ash destroy action #666

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Flo0807
Copy link
Collaborator

@Flo0807 Flo0807 commented Nov 14, 2024

No description provided.

@Flo0807 Flo0807 self-assigned this Nov 14, 2024
@Flo0807 Flo0807 marked this pull request as draft November 14, 2024 15:01
|> Ash.Query.filter(^Ash.Expr.ref(primary_key) in ^ids)
|> Ash.bulk_destroy!(:destroy, %{})

:ok
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we want to catch errors? Currently, the item action, e.g., Delete, uses try rescue and pattern matches on the error. I don't like this, as the item action itself should not figure out why the deletion failed. In the case of Ash, the bulk delete might contain errors (in the result struct), even if it did not raise an error. I suggest moving the error handling to the adapter. @pehbehbeh

@@ -22,6 +22,8 @@ defmodule Backpex.ItemAction do
@callback fields() :: list()

@doc """
TODO: rename to `init_schema`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to rename init_change. You might think that this function passes the result as the change / params / attrs into the changeset, but it is used as the source changeset data. WDYT, is init_schema a good name? @pehbehbeh

Comment on lines +123 to +139
@doc """
Checks whether item action has confirmation modal.
"""
def has_confirm_modal?(item_action) do
module = Map.fetch!(item_action, :module)

function_exported?(module, :confirm, 1)
end

@doc """
Checks whether item action has form.
"""
def has_form?(item_action) do
module = Map.fetch!(item_action, :module)

module.fields() != []
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about moving this functions in the item action use block so we can call item_action.has_form?, but I guess we should stick with a simple function inside Backpex.ItemAction. What do you think? @pehbehbeh

Comment on lines +89 to +91
init_schema
|> changeset_function.(change, metadata)
|> Map.put(:action, :validate)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure where to place the code that calls the changeset function and if we should move it to another module 🤔 I also did not include special calls like before_changeset. We even state that we do not support advanced fields like HasMany in item actions in our docs (of course, we should change this in the future).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant