Contrib.social

Decide whether 'status' and 'close' validation errors should be shown at same time

Open on Drupal.org →
Created on 6 January 2025, 2 months ago

Problem/Motivation

Currently we do this:

        // The host entity is not enabled. Only add a violation if the host
        // is not already disabled for registration by the open or close date,
        // as this would result in redundant messaging.
        $result = $this->context->getResult();
        if (!$result->hasViolationWithCode('open') && !$result->hasViolationWithCode('close')) {

Which may not be right. Discussion in 📌 Replace HostEntity::isEnabledForRegistration Active :

Jonathan: I don't think we should do this. An event subscriber is perfectly within its rights to remove the 'open' or 'close' violations if it has its own open or close logic, but still expect the status settings to be respected.
I can see why you see it as undesirable to show both 'disabled' and 'close', because sometimes we have the setting to disable hosts after their closed date. But I'd also argue for the opposite: if I manually disable a host, I expect that I no longer have to worry about keepings its close date accurate, that all settings apart from disabled are basically ignored.
I think this shows how difficult it is to cover all use cases here. I suggest we're best off being as simple as possible, prioritising the basic integrity of the validation (duplicate-like violations are less of a problem than missing ones), and treat suppressing duplicate-like violations at the point of display long after the event subscribers have had their say.

John: Good points. When deciding which way to go on issues like this, I tend to weigh "avoid confusing users" over "avoid confusing developers" when those goals conflict. Let's mull this over. I could easily be convinced to either go back to the status quo (close date ignored if disabled) or the "simple" approach, the latter making more sense from a development point of view.

Jonathan: One way to support both developers and users here would be to handle this in the validator, after the event is dispatched. That solves the worst of data integrity problem, but still allows us to optimise for users. However, it still interferes with the ability of custom host entity handlers to fully customise their validation results.
A better approach might be to add a getViolationsForDisplay() method that returns the same as getViolations() but with the undesired duplicate-like violations filtered out. Event subscribers and other code that needs to understand the granular logic would call getViolations(), but code that is closer to displaying the messages (RegisterForm and RegistrationConstraintValidator) would call getViolationsForDisplay().
The hardest thing is that I don't know which confuses users less:

showing Disabled instead of Closed if status has been set automatically because the close deadline has passed
showing Closed instead of Disabled when the admin has manually disabled the host status and (reasonably) assumed that close dates no longer mattered then

One possible solution would be to check the set_and_forget setting and make a decision based on that.

John: I went back and forth on this. I don't want to over engineer with a new method. For now let's leave as-is and ponder for a possible follow up.

If we do 📌 The set_and_forget setting is no longer needed Active this situation get a bit clearer.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

📌 Task
Status

Active

Version

3.3

Component

Registration Core

Created by

🇬🇧United Kingdom jonathanshaw Stroud, UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024