In the current Auth code, the email must change to be valid. This may be true for the two situations the author has in mind: Registering a user and changing one’s email address. But if you change a user one little bit, let’s say you want to add a username field, that code doesn’t work any more. You’re forced to change the email address even if you just want to change your username. Logically, an email address shouldn’t have to change to be valid. Also, the email validations should happen on the field itself, not the whole form. This way, more of the existing code won’t have to change if a user adds other fields.
In my situation, I’m creating a user system where only an administrator can add users. They can’t self-register. This Auth code is making me rewrite code that I don’t feel comfortable rewriting. I know that it could blow up some security concerns the original author had in mind.
I understand it’s normal and expected to have multiple “changeset” helper functions for the different use cases you might have.
Validation is encapsulated in those helpers. If you need to share validations across multiple changeset helpers, create reusable defp validate_foo(changeset, ...) functions (they might group together the validations for one or more fields.
For example, imagine that when an admin creates a user they can set all 10 fields, but if an admin wants to edit a user they are limited to changing 2 fields. The users themselves can edit 6 fields of their accounts, but can never register themselves. This situation I described calls for 3 separate changeset helpers:
create_changeset
admin_edit_changeset
self_edit_changeset
Within each, you only cast and validate the fields that are allowed to change in those specific situations.
Indeed, those are the only use cases we had in mind for that changeset. For any other cases where you want to change other fields of a user, a separate changeset is the way to go! That’s also why there are separate changesets for confirmation and password change. For an admin interface, you definitely want a custom changeset
Indeed, those are the only use cases we had in mind for that changeset. For any other cases where you want to change other fields of a user, a separate changeset is the way to go! That’s also why there are separate changesets for confirmation and password change. For an admin interface, you definitely want a custom changeset
Maybe so, but because you included code which requires the email to change to be valid, you violated common sense, and you probably introduced a bug in the process. If the person doesn’t want to enforce unique emails, it doesn’t run the validate_email_changed method there.
Why not do this, instead:
def email_changeset(user, attrs, opts \\ []) do
user
|> cast(attrs, [:email])
|> validate_email(opts)
|> validate_email_changed()
end
But I’m not sure I agree with your original premise here. The thing about updating the email field is that it has side-effects (sending a validation message etc) which complicate its behavior within a form. I think it’s unwise to mix the email fields with other fields in your forms at all. In your example it would be better to split the username change off into its own form instead, for both UX and implementation reasons.
When you have a method called validate_email(), it’s one job should be to validate an email address – not to see if it’s also different. If changing an email address has consequences (it does indeed), then I would call a separate method, like change_email_address(:existing_email, :new_email), or something like that. It’s far cleaner and more obvious. If the existing email is valid – which it should be already, you’d send a notification to that account that the email was being changed to new_email@sample.com. If you didn’t request this change, then… If you’re creating an account, :existing_email would be nil and you’d get the standard verification email at the :new_email address.
We could bikeshed about the name of the function forever, I’m not interested in that.
My point was that in practice the only time you call that function, validate_email(), is when you’re changing the address. I can’t think of any other situation in which you would need to validate an address on a User. But if you can then feel free to share it; I am not omniscient.
I’m just trying to help the codebase. It’s not bikeshedding. email@sample.com is a valid email address. It shouldn’t only be considered a valid email address if it changed to that from some other email address (or nil)! That’s an implementation concern that triggers a notification with a validation link. It has NOTHING to do with the validity of the email address. And by the way, it is a bug, that validate_email_changed() is called inside the if Keyword.get(opts, :validate_unique, true) conditional. Since that bug needs to be fixed anyway, I would suggest just moving it to email_changeset(), and rerun your tests.
I was not disagreeing with you about the name, it’s just that neither of your replies have actually addressed what I wrote. Which is that this function is only ever used when the address is changed, and I don’t see any other reason it would be used.
I wrote this not because I want to have a conversation about naming functions (I seriously do not care), but because you indicated in your original post that you were validating the email address as part of a username change and I was trying to let you know that you probably shouldn’t do that.
But since this is apparently inescapable, the function is called User.validate_email(), not EmailValidator.validate_email(). Its purpose is to validate the address within the context of a User, not just to determine if the address is a valid address. There is only one line dedicated to checking if the address is valid (the regex). The function is also private, it cannot be used for anything else.
Also, I’m not entirely sure that’s a bug. The intention of the author may have been to only show the address change error on submit so as not to preemptively annoy the user.
It’s not a bug. If you look at the usage of Accounts.change_user_email in the default generated code, you’ll see it usually comes in pairs. For example in the Settings live view, it’s used like this in the validate_email event handler:
Then it’s used in the update_email event handler without validate_unique: false.
That means you can trigger the validate_email event with every keystroke (providing the required, format, and max length feedback instantly) without hitting the db for unique constraint checks every time.
The unique constraint checks only happen during the update_email event triggered by the form submission. As a bonus, if you submit the form without changing the value, you get this helpful feedback:
The email technically passes all the validations. That’s how I got to this page in the first place. Since it didn’t change, validate_email_changed() is there to prevent this user from going through the update email confirmation steps on the same email they’re already logged in with.
So, just to be clear, you’re saying it’s not a bug that validate_email_changed() is only executed in applications where the email should be unique?
In my head, these validate_[attribute] methods are simply validating that the value conforms to specific characteristics, like these:
It’s required (or not)
It has a certain minimum (and maximum?) length
It conforms to a certain pattern
It’s unique (based on the application)
It’s one of a list of possible values
In my head (and based on my 34 years of programming experience), the fact that a value has to change from some other value in order to be valid, makes no sense. That’s not at all like the things listed above. To me, this check doesn’t belong inside the validation routine, it belongs alongside it. This way, I can reuse the validate_email() method in some other form, where a person’s email might change, but won’t be flagged if it’s not changed. But that’s just me. What do I know? Apparently, I need more training in this new form of logic.
Someone tell me why this wouldn’t work. I’m not in a position to test it myself.
def email_changeset(user, attrs, opts \\ []) do
user
|> cast(attrs, [:email])
|> validate_email(opts)
|> validate_email_changed()
end
I think by having a special purpose form that’s just for a user to change their email, which uses a special purpose changeset function that’s only used when someone is changing their email, makes for a simpler system design. If instead you have multiple forms where a user might change their email address then in all of those places you need to check if they are trying to change it and afterwards check if the side effects need to be run, makes for a complicated design.
It is your codebase so if you want the more complicated setup then that’s totally reasonable, but I also think it’s totally reasonable for the starting place that the generator provides is the more straightforward architecture
You are misunderstanding the generated code. This is what the docs right on top of email_changeset says as of the latest RC:
@doc """
A user changeset for registering or changing the email.
It requires the email to change otherwise an error is added.
## Options
* `:validate_unique` - Set to false if you don't want to validate the
uniqueness of the email, useful when displaying live validations.
Defaults to `true`.
"""
The changeset function you’re complaining about serves only 2 use cases that are generated along with it: user registration and users updating their own email in the settings.
During registration, the email will go from nothing to something. On the settings page, the update email form submit event handler makes sure the email being submitted is different from the current email. This is why I say it’s not a bug.
