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

proposal: Ensure action fails when sender fails #894

Open
nallwhy opened this issue Jan 23, 2025 · 3 comments
Open

proposal: Ensure action fails when sender fails #894

nallwhy opened this issue Jan 23, 2025 · 3 comments

Comments

@nallwhy
Copy link
Contributor

nallwhy commented Jan 23, 2025

Description

In the current implementation, when the sender fails to execute send, the action is still considered a success. This can lead to issues where critical failures go unnoticed because the system doesn’t propagate or handle the error effectively.

For example:
• A user requests a password reset.
• The system attempts to send a password reset email.
• If send fails, the action completes successfully without any indication of failure.

e.g.

sender.send(to_string(identity), token, Keyword.put(send_opts, :tenant, context.tenant))

This behavior can result in users not receiving important notifications, while the system falsely assumes the operation was successful.

Proposed Solution

Introduce an option to control whether the action’s success depends on the result of send. Specifically:
1. Default Behavior: Maintain the current behavior for backward compatibility.
2. New Option: Add a configuration option (e.g., ensure_sender_success) that forces the action to fail if send fails.

@nallwhy nallwhy changed the title Ensure action fails when sender fails proposal: Ensure action fails when sender fails Jan 23, 2025
@zachdaniel
Copy link
Collaborator

Works for me 👍 PRs welcome! There are multiple senders that will need to be configurable in this way.

@nallwhy
Copy link
Contributor Author

nallwhy commented Jan 31, 2025

@zachdaniel How do you feel about making this a breaking change in v5, setting the default of option to true? I’ve seen similar planned changes in Ash as well.

@zachdaniel
Copy link
Collaborator

We can discuss it, but it would make sense to me.

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

No branches or pull requests

2 participants