An email shouldn't have to change to be valid

Any code that is generated is your code.

Feel free to add new changesets etc.

If you don’t feel comfortable writing this code use an existing library instead.

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

Could also add an option like the existing validate_unique.

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:

Accounts.change_user_email(user_params, validate_unique: false)

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:

image408×340 28.9 KB

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.