Allow non-admins to edit own registration information

The use case makes sense and I am good with this as long as existing installs are not affected. Meaning it is done with a checkbox setting on the per-host-entity Registration Settings form (a new base field), the Registration type, or global Registration settings, and an update hook sets the box to unchecked for existing sites. If you want new sites to default to checked, no problem. If putting in global settings, https://www.drupal.org/project/registration/issues/3490561 🐛 From address in registration setting is overwritten Active provides an example.

This should definitely wait until the related issue #3464736 is Fixed, so as to avoid rework.

Makes sense re existing installs.

What's tricky with regard to 📌 Replace HostEntity::isEnabledForRegistration Active the behabior we give to the new isRegistrationValidForHost() we are introducing. I'm not fond of the idea we bake into it the legacy idea that any existing registration is invalid for the host if the host is not longer available in the sense defined by isAvailableForRegistration().

But if we don't keep the legacy behavior in isRegistrationValidForHost(), then we have to deliberately implement the legacy behavior elsewhere in addition alongside calls to the new isRegistrationValidForHost().

Here's a proposal that gives us maybe the best of both worlds:

In 📌 Replace HostEntity::isEnabledForRegistration Active :
(1) isRegistrationValidForHost() only checks isAvailableForRegistration() if the registration is new or changes the number of spaces. It doesn't have the legacy behavior.
(2) The logic in RegistrationAccessHandler added in 🐛 Access & validation mismatch creating/editing a registration on a disabled host Active for 'update' is changed to consider isAvailableForRegistration(), keeping current behavior that blocks access for non-admins to edit registrations on closed hosts.
(3) The RegistrationConstraintValidator is changed to consider only isRegistrationValidForHost(). This looks like a behavior change for ordinary users, but actually its not because they can't access the registration edit form to edit it anyway if the host is closed. This will have the consequence that admins are able to edit registration information (but not spaces etc.) on closed hosts. So this is a behavior change, but probably beneficial and only for admins.

In this issue:
(4) We add a setting that give site builders control over whether RegistrationAccessHandler should or should not block access to edit depending on isAvailableForRegistration(). I suggest a global setting to keep it simple, as I think sites needs here are so various that I don't have a lot of confidence its that widely useful for more than BC.

Whatever happens here should take the comments on 📌 RegistrationAccessControlHandler should rely on RegistratonHostEntityHandler to consider the validation for edit access Active into account.

Elsewhere you have decided to not block access to register of the host is not available, for performance reasons. So we're currently inconsistent if we block access to edit using the validation mechanism, but don't block access to register.

I think the best solution might be to do what I suggested in comment 5 of 📌 RegistrationAccessControlHandler should rely on RegistratonHostEntityHandler to consider the validation for edit access Active : to use the host access handler but not the validation mechanism. But when the host access handler checks close and status directly, have it check a global setting also about whether or not those should block access.

Sorry, I created an issue fork before seeing it was assigned to you.

So we're currently inconsistent if we block access to edit using the validation mechanism, but don't block access to register.

We are inconsistent, but we can afford to block edit, because the performance issue does not exist - the Register tab is on the host page, where performance is paramount, but the Edit tab is on the registration, where performance is not a concern. Apples and oranges. That said, I'm very likely to use a global setting and generally do what you are suggesting.

there are plenty of uses cases for benign updates, like changing their dietary needs or any other information on custom fields

This is committed. As a bonus, any user with update access can edit registrations for closed or disabled hosts now, if the site config is set appropriately, not just users editing their own registrations.

I suggest a global setting to keep it simple

Agreed, this is complete.

It's hard to know when sites might want users to stop being able to edit this additional info: I think making this unrestricted is a better default.

I did not do this because I always choose safety when in doubt. Way easier for a site builder to enable a feature later, than to cleanup data corruption caused by a setting they didn't realize was active. So the global settings page has to be visited to enable this, even for new installs of the module. I want site builders to think hard about this before enabling.

From #3497736:

I'd just check the status and close directly in the Host access handler and not use validation for this.

I know you've been lobbying @jonathanshaw to remove isEditableRegistration and its constraint. It was not possible because depending on the site config, the checks still need to happen. And I couldn't do them in the host access because in most cases the host is not even consulted when checking update access. There are a myriad of ways a user can have update access, and to avoid duplicating logic in various code paths, I needed to keep the "is editable" check exactly where it is, in the RACH, there is no way around it. I also had to add a new constraint to ensure non-admins cannot change key properties of the registration - namely spaces, status and registrant. I also don't mind the symmetry of having one host entity method for new registrations (isAvailableForRegistration) and one for existing (isEditableRegistration). Also the logic for checking "is editable" is now even more complex, so having a helper method to handle that is very useful, since I need to invoke that logic from both RACH and the RegisterForm. I considered removing it from RegisterForm but that class can be called outside of a context controlled by RACH, for example somebody editing a cart with registrations in it.

having a helper method to handle that is very useful, since I need to invoke that logic from both RACH and the RegisterForm. I considered removing it from RegisterForm but that class can be called outside of a context controlled by RACH, for example somebody editing a cart with registrations in it.

Makes sense.

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