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

Update documentation for HashPasswordChange #842

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

LannyBose
Copy link

@LannyBose LannyBose commented Nov 17, 2024

When referencing how to make a manual password change, I felt the included documentation could use some sprucing up.

Changes include:

  • Ash.Changeset.new/2 seems to be gone
  • Setting the context via set_context didn't work as expected when handed to HashPasswordChange.change. It kept resulting in BadMapError
  • Providing explicit direction how to set an argument within the update Resource DSL

It's very possible my first chunk of code isn't idiomatic, but it did work. Happy to receive adjusting feedback there!

Provide clearer pseudo-code, which reflects the removal of Ash.Changeset.new/2 and shows the interaction between arguments and the `change` in the Resource.DSL
Copy link
Collaborator

@jimsynz jimsynz left a comment

Choose a reason for hiding this comment

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

Hi @LannyBose 👋

Thanks for the contribution. Unfortunately they're not quite right. If you can update your PR I'll be happy to merge it, otherwise I'll swing back in a day or two and make the changes myself.

Comment on lines -14 to -17
Changeset.new(user, %{})
|> Changeset.set_context(%{strategy_name: :password})
|> Changeset.for_update(:update, params)
|> Accounts.update()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right that Ash.Changeset.new/2 is gone, however the rest of your example is incorrect.

It should probably be more like:

Changeset.new(user)
|> Changeset.set_context(%{strategy_name: :password})
|> Changeset.for_update(:change_password, params)
|> Accounts.update()

@@ -30,6 +32,8 @@ defmodule AshAuthentication.Strategy.Password.HashPasswordChange do

```elixir
update :change_password do
argument :your_unencrypted_password_field, :string, sensitive?: true

change {AshAuthentication.Strategy.Password.HashPasswordChange, strategy_name: :password}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that these examples should be updated to match the action that the add strategy igniter generates.

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.

2 participants