Improve manual account creation process

Nice. If it can be done, let's add it to either the drupal_cms_starter or drupal_cms_authentication recipes. :)

I'm on a slow come back and started working on this. The user register form looks already like the animated gif in the related core issue. I'll keep working on the submission and creating a random password next.

This is now ready for a first review. It works as described in ✨ Allow per-site configuration of 'Notify user of new account' default setting Active with one extra that was unnoticed so far: to make this work, the password field can no longer be required; otherwise the form couldn't be submitted as the random password will be set after submission and will be empty before then.

To resolve this, the ECA model adds a form validation for an empty password in case that the notification is turned off. Because then we expect a password to be set. So, we would now get a traditional form validation error in this scenario, which is less user-friendly than the inline form validation that we have with the required password field.

An alternative solution would be to accept an empty password, since nobody could log in to such an account without using a password reset.

Please have a look how this works for you. Once we have the final version of this, writing tests should be done afterwards.

Thanks @jurgenhaas. Assigning to Pam for manual testing and review.

This is awesome!

I think we should accept an empty password, it is quite confusing to get an error that the field is required when it's not marked as such. If the password isn't provided, presumably the creator has some other plans for how to get the person access?

Great you like it. I've remove the empty password validation part and it's ready for review again.

@phenaproxima the test is failing with a 500 return code when the redirect test goes to /admin, but I can't reproduce that locally, neither with Drupal CMS nor with standard profile. Also, what's the scope for tests here?

Oh, and another note that I've updated the bpmn_io dependency to 2.1.x as this branch comes with some exciting new features and we'll publish a stable release together with ECA 2.1 before the end of November.

@jurgenhaas just one question, do you have a sense of what would happen here when this gets added to core? Will something break on people's sites? I guess it's hard to say without knowing the specifics, just wondering if we need to think about this.

@pameeela, we can't tell for sure. But what the model firs is similar to any hook implemented by any of the installed modules. Nothing special, though.

In this case, we just alter the weights, a label and state conditions to some fields. Worst case, that should create an unexpected field order and label, but remain unnoticed. Wouldn't classify that as breaking a site, so the risk seems low.

OK cool I think this is over to @phenaproxima for final review but I'm happy so marking RTBC.

Oh, failing tests, but still over to Adam from here.

No technical objections but this absolutely needs functional tests. I am guessing it should be a Cypress test since the functionality depends on JavaScript.

I guess I will have to take a look at that since there are some unknowns here…

Hey @pameeela regarding your last commit for the checkbox label:

The original label from Drupal core is "Notify user of new account". As this is so close, I'd suggest to not alter the label at all, because then also translations from Drupal core will continue to apply.

In any event, just changing the label in the eca config wouldn't last long, we would also have to update the eca model. The former is derived from the latter.

But I hold off on this waiting for your choice whether we should just leave the label as is. I can then make that change.

Great point re: translations! I think that is a good idea to match what's in core. After thinking about it further I didn't feel that it needed to explain the password aspect since it doesn't now. Also noted re: the model, oops.

OK, I've removed the label change entirely so that we stick with the original label from core.

@phenaproxima I've also rebased the MR to include your changes from yesterday. Leaving this in your hands for the tests now.

Wrote a Cypress test for this, so assigning to @pameeela for final review.

Actually, wait. Pam already signed off on this. It's going in if the test passes.

Merged into 0.x - a worthy improvement, and if you look at the email sent from the form (ddev mailpit), it looks great and works nicely!

Automatically closed - issue fixed for 2 weeks with no activity.