backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

User module still sends messages TO `site_mail`, the FROM email address #4428

Open jenlampton opened 4 years ago

jenlampton commented 4 years ago

Follow up to https://github.com/backdrop/backdrop-issues/issues/4403.

Currently, when user registration with admin approval is enabled, the site_mail email address is used as a recipient of pending user accounts.

The site_mail should not be used as to receive messages of pending user accounts. (See https://github.com/backdrop/backdrop-issues/issues/2365)

I would like to see this be configurable! Perhaps on the same page where the text for the emails can be configured.


PR: https://github.com/backdrop/backdrop/pull/3157

indigoxela commented 4 years ago

Any idea, why so many (seemingly unrelated) tests were failing?

Here's a screenshot from the sandbox: screenshot-form

I think, a little description would be helpful here, what do you think @jenlampton ?

jenlampton commented 4 years ago

Can we think of a better label for the field instead? I wasn't super happy with Email address to notify either, but it was the best I could come up with.

Some ideas:

I'm not a fan of help text in general, as it clutters the form creating a more overwhelming feeling, and most people won't read it anyway. We may need it here though, if we can't get a brief label that does a good enough job of explaining what this is for.

Ideas for description text:

indigoxela commented 4 years ago

Regarding the label: "Email address to notify of new accounts" is currently my personal favorite.

Regarding the help text: Shouldn't we also note that if this field is left empty, the email of user/1 is used (in user_get_approval_email())? But what if user/1 is disabled or has been cancelled? And what if someone doesn't want this sort of notification? For instance, if they do something more complex using custom actions/rules? How to turn it off then?

jenlampton commented 4 years ago

Shouldn't we also note that if this field is left empty, the email of user/1 is used

What if we make the field required, so that it can never be left empty. The fall-back to user/1 should only be used for people who have not specified an email address.

But what if user/1 is disabled or has been cancelled?

The user/1 account cannot be cancelled, but the email address will not be used if the account has been blocked. Contact module has this problem too, and does not provide a second fall-back email address when this is the case.

I see three scenarios when this could happen:

For scenarios other than when this form is being used, we could set a message letting people know they need to set an email address.

And what if someone doesn't want this sort of notification? For instance, if they do something more complex using custom actions/rules? How to turn it off then?

I saw your comment about this on the global mailto address issue after I made this PR. We could provide a disable option here, but I'm tempted to leave that to contrib. Is that an 80% use-case? My guess is no.

indigoxela commented 4 years ago

What if we make the field required

I belief that would simplify things, let's not do too much "magic" in background.

We could provide a disable option here, but I'm tempted to leave that to contrib

They'd probably use hook_mail_alter() then, so that should be OK. No, I don't think this is an 80% use-case.

Anyway, we probably need an update hook to populate user_approval_email, as it's a new config item.

klonos commented 4 years ago

How about "Send account approval emails to" as a label?

But what if user/1 is disabled or has been cancelled?

This issue here so timely btw 😅 ... in the GovCMS distro, we've been locking user1 (also changing its email address to a no_reply, generic one) for SaaS customers. Yet there are some use cases where gov sites require email notifications to be sent out to specific people, in order to approve new accounts. Setting the user1 email to a specific email owned by the gov agency would allow them to use the password reset form, and gain access to user1 by reseting the password. If this feature here was available, we would be having this issue. So glad to see this sorted in Backdrop, to account for such use cases 👍

A few notes/suggestions:

  1. Why not allow multiple email addresses to be specified?
  2. Why not allow selecting specific (multiple) user accounts, via an autocomplete. Then send emails to these accounts. This way, admins configuring this setting do not have to look up a person's email to add it here + if a person updates their email, this setting would not need updating. (we need entity reference in core for this to happen, so follow-up)
  3. Why not also allow a user role to be specified as "account approvers"? I was thinking perhaps add a "Roles to send account approval emails to" drop-down menu, that lists user roles with the "Administer user accounts" permission. This would cover the use case of community sites, where the account approval/management is delegated to a group of people, rather than a specific person.