Allow admins to change host entity for existing registration

Hi, I would really appreciate this feature.

I have working code for this, as a new submodule of registration, I will open an MR here in the next few weeks.

@jonathanshaw: Great, thanks you!

So, how is it, does it work in v. 3.1.1?

Sorry this has been slow, but it really is coming. Should be doing the final touches very soon.

I think it's worth considering ✨ Establish host variations as an architectural concept Active as it makes this easier.

Chiming in - in my view, the candidate list for the new host entity should be any host entity matching the registration type of the registration that wants to change its host - it does not need to be limited to just those hosts for the same "event". For example, in a Commerce context, I do not think it needs to be limited to only those product variations that are "siblings" (attached to the same product as the original host entity). In the real world, people often want to change from one "event" to another - for example, I have an appointment for a class, and want to switch classes but keep the other registration data intact.

Created a new MR with a new submodule to change the host entity.

Thanks Victor for your contribution! I tried this out on a local test system - from what I can tell, in order to engage your module, you have to write an event subscriber in a custom module to identify which host entities can be used as the new host - is that accurate? I can give more feedback but wanted to start with that question. (I like the idea of using an event for this BTW.)

In order to engage your module, you have to write an event subscriber in a custom module to identify which host entities can be used as the new host - is that accurate?

Yes. I had been concerned that showing all hosts would be a bad choice in almost all use cases, although now that I think of it I realise that showing all hosts open for registration currently could be alright for staff in many use cases, although again unlikely to be adequate for users in most use cases.

I plan to provide an event subscriber allowing to change to other variations of the same product for commerce_registration, which is the main use case I've been thinking of.

Re #12 I definitely think we should *allow* changing host without hardcoded restrictions. But I'm expecting that every site would need to make it's own decisions about which changes to allow based on it's business logic - I don't see how we can really help with this. Hence the "highly flexible but completely opt in and code based" approach of the MR.

Thanks jonathan, didn't realize you and Viktor are working together, nice. Some great work here. Issues with the current MR:

* tests are failing and phpstan is reporting a warning
* after enabling the module I lost my ability to do a regular registration edit - needs to be some way to trigger the change host action - perhaps through a "Change Host" action button at the bottom of the View Registration page (see registration_workflow_preprocess_registration for example) or through a new "Change Host" tab for the registration, or both. The route should have a custom access checker that disables the route for the given registration unless there are at least 2 possible hosts. If you don't provide a custom event subscriber to provide possible hosts, then the admin UI experience should be as though the module were not enabled.
* The paths and verbiage should be more clear about what is happening. "Select registration type" (/type) should probably be "Select new host" (/change-host) and "Update registration" could be "Confirm and save new host" (/change-host/confirm) or something like that. Also, if the registration type isn't changing, then there is no need to launch step 2 for the field updates and it could simply be a confirmation page "You are about to change the host from X to Y" with Confirm and Cancel buttons.
* The selection interface for the new host is really neat - but it needs to exclude any possible host that is not configured for registration - and if too many possible hosts are returned, maybe it needs to have a pager (i could accept without a pager for now).

Setting this issue to "Needs work". If you need assistance with the legwork on the above, let me know.

Also - probably need a separate permission for changing host. Maybe "administer registration" gets access automatically, and then a new permission that can be added to roles for non-admins. On some sites, I could see a use case where some people can edit registrations and others can change the host but not edit.

Sorry for the delay here, I will get Victor to make these fixes.

The paths and verbiage should be more clear about what is happening. "Select registration type" (/type) should probably be "Select new host" (/change-host) and "Update registration" could be "Confirm and save new host" (/change-host/confirm) or something like that.

This makes "host" a user-facing concept. But its meaning is completely opaque to a site's user, certainly to a non-staff user.
I think hosts is ok for the path - people aren't surprised by weird urls.

Ideally for on-page text maybe we should use the label of the host bundle. But in practice this seems tricky as there could be multiple valid host entity bundles or even entity types. I suppose we could use the bundle label if all available hosts are from the same bundle, the entity type label if they're all of the same type but different bundles, or "host" if there are multiple types involved? That's probably the best UX.

The selection interface for the new host is really neat - but it needs to exclude any possible host that is not configured for registration

I've asked Victor to fix this in PossibleHostEntity::access(). This way event subscribers can return hosts to be listed, but displayed as unavailable. I'm imaging that some sites might sometimes want to list a certain option without making it available (e.g. marking it sold out) rather than simply hide the option altogether. template_preprocess_registration_change_host_list() only converts a list item to a link if its access is allowed.

Updated the MR.

Currently, there are warnings in cspell job but it's not related to the current issue.
There are also errors in tests but none of them are related to our submodule.

e.g.

Drupal\Tests\registration\Functional\RegistrationFieldTest::testRegistrationFieldAssignTypePermission
ElementNotFoundException: Button with id|name|label|value "Save and continue" not found.

FYI I had the same error for another project and fixed it by creating fields in another way described here - Button values and messages during field creation workflow have changed →

thanks

Awesome, looking forward to trying this out again. Will review within the next few days.

This is pretty ready for review, although I've asked Victor to tweak the hook_entity_access a bit.

Yeah, I see what you mean - the entity access hook is not setting the cache dependencies. The function "node_node_access" in core shows roughly what has to be done - caching either per user (for "own") or per permissions, and adding cache dependencies on both the registration and the existing host entity for the registration. Also, it should return forbidden unless the host can actually be changed (> 1 possible host), even if the user would otherwise have access. No sense in showing the Change Host tab for no reason. In my view the change host manager should actually remove the existing host if it's in the list returned from the event - that way you can just see if the possible hosts is empty in the access check.

Everything else is looking really good. Setting to needs work for these last tweaks. Thanks!

Also, it should return forbidden unless the host can actually be changed (> 1 possible host), even if the user would otherwise have access.

I believe Neutral is equivalent to Forbidden for route access, but Neutral would allow RegistrationAccessControlHandler to override and grant access if the user has administer registrations permissions, so I think that will be the better approach to DRY.

quick update: currently, I’m busy with some other things. so, will continue working on the issue in one or two weeks

thanks for understanding

No problem, thanks for the update Viktor

OK, finally this ready for review again.

The coding standards failures on CI are unrelated.

The one piece I am aware is missing is related to the event dispatching. The event can be used in many different places on a request: checking route access, checking entity access, building render array, adding cacheability to render array. Currently this leads to the event being dispatched multiple times on a single request. It got too complex trying to hack ways to avoid this so I ended up accepting it.

I'm not sure it's a significant performance issue. But if we do care about it, the best solution would be to implement a memory cache in RegistrationChangeHostManager and cache the dispatched event there. I've held off from implementing this (it's a bit esoteric) until the the rest of the code was agreed.

In #19 @john.oltman said:

The paths and verbiage should be more clear about what is happening. "Select registration type" (/type) should probably be "Select new host" (/change-host)

The path is now "/host".
The title is "Select registration for" because I'm sceptical of making "host" a user facing concept as explained in #21.

and "Update registration" could be "Confirm and save new host" (/change-host/confirm) or something like that. Also, if the registration type isn't changing, then there is no need to launch step 2 for the field updates and it could simply be a confirmation page "You are about to change the host from X to Y" with Confirm and Cancel buttons.

I didn't implement this. I'm not sure the proposed title is better, this form is identical to that for updating the registration, and does in fact update the registration, so having the same title seems sensible to me. This is more obvious for registration types that have many fields.

I see giving a chance to edit registration info at the same time as changing host to be a UX feature, not a bug (even when the new host is of the same bundle so this is not technically necessary). It forces people to reconsider all of the registration info when they change the host, which they probably should be. Changing the host is a significant change in the registration, typically one of the biggest changes possible, and I'd imagine it will very often require at least reconsidering answers given to other questions about the registration.

It's true that if the new registration type has no fields, then the UI here is a bit sparse. I propose we add some text to the page like "This was a registration for ... You are updating it to be a registration for ..."

Thanks for your latest @jonathan I am still testing but wanted to provide some status.

CI errors - I merged the 3.1.x branch back in to resolve
Access bug - I noticed an issue when flipping hosts between enabled for registration and disabled, so pushed up a fix
Admin routes - the change host routes should be admin so users with access to the admin theme have a consistent experience - pushed up a commit for that

I'll get back with more feedback soon. Overall looking good.

Getting closer. More feedback.

I see giving a chance to edit registration info at the same time as changing host to be a UX feature, not a bug... Changing the host is a significant change in the registration, typically one of the biggest changes possible, and I'd imagine it will very often require at least reconsidering answers given to other questions about the registration.

This makes sense and I'm on board with sending people through the update screen all the time, provided there are some minor UX changes per below.

I'm sceptical of making "host" a user facing concept as explained in #21.

I am not on board with this, because the point of the new submodule is to make "host" a user facing concept so site admins can change it, and the admin clicks on a tab saying "Change host" to initiate the process. So "host" is front and center and pretending it isn't user facing is making the UX more difficult than it needs to be. I have some other changes to recommend as well so I'll post all in one new comment.

Recommended changes below.

UX - Right now it is unclear what the state is after selecting a new host and getting to the update page. Has the new host been applied and saved already? Why am I on an update page - did I click the Edit tab by mistake? If I close my browser at this point is the new host set or lost? Recommended changes: Page 1 title is "Select new host", Page 2 title is "Confirm new host and registration data" and page 2 indicates the previous host and new host somehow (and that you are "about to change" to a new host but it hasn't happened yet). I do not think "Delete" should be a link on page 2 and the button text could be "Save Registration and Set New Host" to make clear that unless the button is pressed nothing has changed yet.

Existing host in possible hosts list - The existing host is always present in the displayed list - either hide it on the display side or add "(existing host)" after the title and make it not selectable. Otherwise it is confusing why it is there and people may wonder "do I have two entities with the same title?"

Possible hosts access vs. display - I think we may need to distinguish between possible hosts used for cacheability (in the list so if they become enabled for registration the Change hosts tab shows up) and possible hosts that are visible to the user. Perhaps add a new method "setVisible" on the PossibleHost object, only hosts that are visible are displayed - selectable if enabled for registration and not selectable if not enabled yet. If there are zero visible enabled hosts, the Change host tab is hidden.

Change host entity operation - this does not update like the Change host tab does, when hosts move between enabled and disabled for registration, due to a bug in Drupal core with entity operation cacheability. So if a possible new host becomes disabled and there are no longer any available hosts, clicking on it gives a 403. So I recommend we remove it until the core bug 🐛 Views entity operations lack cacheability support, resulting in incorrect dropbuttons Needs work is fixed in D11. (I realize I am the one who asked for this in the first place, my apologies, did not realize the core bug would affect this.)

Lack of a default event subscriber - I would like there to be a simple subscriber that is there by default, and easily overridden (we'd put instructions in the README), so that a non-developer can at least see the module in action (and in some cases it might be enough). Perhaps something as simple as putting every entity of the same type and bundle as the existing host into the PossibleHosts list (up to some limit perhaps), and making the first 10 that are enabled for registration visible. I agree that most site builders will need to add their own subscriber that overrides it, but I have to think about the non-developer use case as well - of which there are many based on other support requests I get. I don't see a downside to this as it doesn't create more work for the developer use case and it allows people to try the interface out and make a decision on whether they want to put the work into making it perfect for their site with a custom event subscriber.

README - We'll need this eventually once everything else is set.

Thanks Jonathan for all your work and as I have said before, I can do the legwork on anything you are not comfortable with or do not have time for.

I am not on board with this, because the point of the new submodule is to make "host" a user facing concept so site admins can change it, and the admin clicks on a tab saying "Change host" to initiate the process.

I'm personally happy to go along with 'host' as it doesn't touch on anything that concerns my project. But there's a few nuances here I think it's worth you considering:
(1) It's perfectly reasonable for sites to allow end users to change host; especially in something like commerce_registration where variations are the 'host' and allow different types of regisration for a single event. Naming matters a lot in this use case.
(2) Most uses of registration I imagine will happen in the context of some kind of event. And in the world of events, a "host" naturally means the person who is hosting the event. Using it in this very different way risks confusing even admins.

A step towards a solution would be to add a getPossibleHostLabel($registration) method to RegistrationChangeHostManager allowing anyone decorating that service to override the default 'host' and use their own approach to providing UI text here.

A further step would be to add a host_label setting to RegistrationItem allowing sitebuilders to configure the UI text to use in this context, defaulting to the name of the bundle the field is configured on.

And it'd not be unreasonable for you to think I'm over-engineering an edge case ...

Good points Jonathan and not over-engineering - on the View Registration page the name of the host entity bundle is used. We could probably use that same label everywhere I am proposing to use "host". So the tab would read "Change Event" if the host entity type is an Event content type and page 1 title would be "Select new Event" etc. The relevant function is right on the Registration entity with method name "getHostEntityTypeLabel" and it returns the bundle label if the type has bundles, so "Event" in my example. If you can give that a shot in your changes please do, otherwise I'm happy to add this when you are done with your other work.

My #34 cross-posted with your #33, thanks for looking so carefully!

Agreed in principle on all except:

Page 1 title is "Select new host"

Existing host in possible hosts list - The existing host is always present in the displayed list - either hide it on the display side or add "(existing host)" after the title and make it not selectable. Otherwise it is confusing why it is there and people may wonder "do I have two entities with the same title?"

A complication here is that I'm trying to make it easy to implement my preferred non-standard workflow where the user doesn't choose between "edit" and change host" but instead "edit" always takes them through the select host page, i.e. a single 2-step editing process.
I'd prefer "Select {host_type_label}" to "Select new {host_type_label}" for that reason, and I think that works well for either workflow.

Regarding the existing host being in the list, I think you're right, but I'd like to implement in a way that makes it easy to put the current host back in the list for the workflow where that is desired. I think having it in the list but not selectable is my preferred solution probably.

Possible hosts access vs. display - I think we may need to distinguish between possible hosts used for cacheability (in the list so if they become enabled for registration the Change hosts tab shows up) and possible hosts that are visible to the user.

I suspect this is a bigger issue actually. The registration settings data is really like a part of the host entity data and should be treated like that for cacheability purposes. Therefore I think RegistrationSettings::invalidateTagsOnSave() should be more aggressive. It should invalidate the list cache tags for the host entity. If we did this, the possible hosts cacheability would work fine as long as the subscriber added the appropriate list cache tag.

I'd prefer "Select {host_type_label}" to "Select new {host_type_label}" for that reason, and I think that works well for either workflow.

Sounds good, I'm on board.

It should invalidate the list cache tags for the host entity. If we did this, the possible hosts cacheability would work fine as long as the subscriber added the appropriate list cache tag.

Good idea, but can we use [registration_settings:list] instead of the host entity list. I'm concerned about forcing a rebuild of every node list block on the site when settings for one event are updated. Sites that want to couple the caching for hosts and settings can use the registration_inline_entity_form module to achieve that, or do custom code. I'm afraid to force it for all sites at this point given there are hundreds of existing installs.

Possible hosts access vs. display - I think we may need to distinguish between possible hosts used for cacheability (in the list so if they become enabled for registration the Change hosts tab shows up) and possible hosts that are visible to the user. Perhaps add a new method "setVisible" on the PossibleHost object, only hosts that are visible are displayed - selectable if enabled for registration and not selectable if not enabled yet. If there are zero visible enabled hosts, the Change host tab is hidden.

On further thought, I'm not clear on the need here. It's true that if a developer wants to only have hosts configured present in the list then in their event subscriber they need to load their possible host entities and filter by whether they are enabled for registration. But they could (and should) add the registration settings cache tags of any disabled host to the event at that point, ensuring that the possible hosts list is rebuilt if any of the disabled hosts get enabled.

This is fiddly, but I think that's unavoidable really. We could add a protected helper method on the event class (removeDisabledHosts()) that provides the 'best practice' logic for handling this.

On further thought, I'm not clear on the need here. It's true that if a developer wants to only have hosts configured present in the list then in their event subscriber they need to load their possible host entities and filter by whether they are enabled for registration. But they could (and should) add the registration settings cache tags of any disabled host to the event at that point, ensuring that the possible hosts list is rebuilt if any of the disabled hosts get enabled.

Wouldn't having the subscriber add a cache tag of [registration_settings:list] be all that is needed? Then any settings update triggers a rebuild of the possible hosts list/display. I thought this was your idea actually. Then the subscriber only needs to add the hosts it wants to display to the list.

Wouldn't having the subscriber add a cache tag of ['registration_settings_list'] be all that is needed? Then any settings update triggers a rebuild of the possible hosts list/display. I thought this was your idea actually. Then the subscriber only needs to add the hosts it wants to display to the list.

You're right, that works for the cacheability, although it will invalidate more often in some circumstances because it will respond to changes to any host, including ones not even considered by the subscriber. Not that this is important.

But this use case of needing to include only hosts enabled for registration is likely very common. So there may be merit to a helper anyway. And if we do have one, we might as well use it to add the cache tag for excluded hosts, so the subscriber doesn't have to remember the list tag and doesn't get invalidated so often. Maybe we should add as our helper an addHostIfEnabled(PossibleHostEntityInterface $host) method to RegistrationChangeHostPossibleHostsEvent in addition to addHost(PossibleHostEntityInterface $host).

Maybe we should add as our helper an addHostIfEnabled(PossibleHostEntityInterface $host) method to RegistrationChangeHostPossibleHostsEvent in addition to addHost(PossibleHostEntityInterface $host).

I like it, sounds like a plan. Please proceed (and thank you).

@jonathanshaw I am changing this to Needs Work. I saw an initial flurry of commits, but I am guessing there is a bit more to do. If I am wrong, please put this back to Needs Review and assign back to me. Thank you for all of your efforts.

Yes, sorry I stalled but plan to finish,!