Explore alternatives to passing existing registration into hasRoom

Where I've got to with this is:
(1) The first step is to add a Registration::getAdditionalSpacesRequested() method.
(2) This constraint validators benefit from this I think. It solves 📌 Improve maximum spaces validation Active .
(3) This will fail a waitlist RegistrationAdminTest, but I think that's because somehow it exposes the bug fixed in 📌 Refactor registration_waitlist RegistrationValidationEventSubscriber Active .

If you like this new method, then we should discuss how/whether to deprecate passing the registration into hasRoom() and its (numerous) friends. I fear this would require us to create new methods and deprecate the old ones, because changing parameters would break the interface. However, as we only recently made it possible to swap in custom host handlers, there may be next to no custom implementations in the wild that customise the changed methods so that may be an acceptable risk.

Adding the new method here may be worthwhile even if we don't deprecate passing the registration to hasRoom().

There's a catch I was overlooking ... Needs a rethink.

I have managed to get working a method Registration::getSpacesRequested().

This basically does what HostEntity::needsCapacityCheck() used to do.

What's good about Registration::getSpacesRequested() is that it will help us solve 📌 Improve maximum spaces validation Active . I think this suggests that it's probably a good API addition.

I've also added new test coverage that I created as part of trying to figure things out here. One of these tests currently fails because it exposes the bug in 📌 Refactor registration_waitlist RegistrationValidationEventSubscriber Active .

I'm still interested in clarifying or improving what is going on when we pass a registration into hasRoom() and friends, but it's an extremely intricate and delicate web, tied up with what really things like active or held mean, and how waitlist works. So I'm not going to do more on that immediately if at all.

I removed a test I had also added in 📌 Refactor registration_waitlist RegistrationValidationEventSubscriber Active , and that should remove the test failure here.

This was a valiant effort, and the goal is honorable, but it doesn't simplify things, so I am postponing until we have consensus on how to proceed. There are a few problems. The function name getSpacesRequested is quite similar to getSpacesReserved in everyday meaning, and leads to confusion. The method description in the interface file is also quite difficult - along with you, I am the foremost expert on the module and it still took me an hour. to confirm that there wasn't a typo in the description around the state change. It also exposes publicly the complexity of when a capacity check is needed - I understand why, but there was an advantage to having that behind the curtain before, as the concepts are quite difficult to glean.

Let's regroup on this. We could possibly close it and address the overall goal as part of 📌 Improve maximum spaces validation Active , but I will leave as postponed for now. Perhaps one of us will have a breakthrough idea that we can agree on. Whatever we come up with, it seems like the question being asked is "could this registration need more spaces from the active registration pool" - it isn't even "does this registration need more spaces" since a state change with no change to the spaces field is still resulting in a yes answer. Which is why "needsCapacityCheck" worked previously. Obviously needsCapacityCheck is not appropriate for a public interface, thus the conundrum.

Actually, I wonder if we could simply move the existing function name to the registration interface. That way the method description is very simple, and it can be used by external callers like the max spaces check:

  /**
   * Determines if a registration requires a capacity check before it can be saved.
   *
   * Returns FALSE for canceled registrations. Otherwise returns TRUE for new registrations
   * and existing registrations changing state or adding spaces.
   *
   * @return bool
   *   TRUE if a capacity check is needed, FALSE otherwise.
   */
  public function needsCapacityCheck(): bool;

I like this because it moves the complexity out of the API surface and into the implementation. Not sold on it yet though because it is less flexible than "getSpacesRequested", as it returns less information. Although I'm not sure we need the extra information - I don't think max spaces constraint will need to know anything other than True/False, and hasRoom doesn't.

I'll push up a different MR than yours on 📌 Improve maximum spaces validation Active showing how this would work.

I share your unimpressedness with where I got to with this. And I think there's a fair argument that requiresCapacityCheck() is actually more accurate than getSpacesRequested() because of the unintuitive intricacies of the method.

Looking at this whole issue made me less positive with the whole way that states and capacity and capacity methods are working. It's very difficult to understand the meaning of the different states and their properties, how registrations move between them, and how this affects capacity, and how different methods and extension points are involved. However, it's a super hard problem space and likely impossible to fix with backwards compatability. I assume that the system has evolved over time - waitlist in particular is very tricky to understand.

Worth considering an overhaul for a breaking 4.x, but also likely works well enough as is. That the tests pass is kind of amazing ... but there's plenty of them.

john.oltman → closed merge request !84