Allow user admins to choose the user cancellation method on the user deletion confirmation form (rather than having a single sitewide setting)

Ready to review, It needs tests, but we would like to confirm if this solution is good for Drupal and then we will add the coverage.

Upload patch with the right Schema

The last submitted patch, 4: core-3384789-cancel-options-4.patch, failed testing. View results →

The last submitted patch, 6: core-3384789-cancel-options-6.patch, failed testing. View results →

Tests fixed, pending to cover with tests the new functionality.

Created test and solved failed PHPUnit tests, ready to review.

Tests are looking good.

Think we will need a simple update test though. Something simple like

Check values are null
Run updates
Check values are false.

Rest looks good!

Added upgrade test checking the new values on user.settings.

Reviewing failed tests on Drupal 11.x

@smustgrave Both MR tests passed.

Thanks for working on this @Eduardo Morales Alberti!

I am not sure, that the code is doing what the field description says:

Choose the method that will be available for the user when canceling the account

Looking at the changed code:

@@ -765,7 +766,11 @@ function user_cancel_methods() {
-    if (isset($method['access'])) {
+
+    if (isset($cancel_methods_settings[$name]) && !($cancel_methods_settings[$name])) {
+      $form[$name]['#access'] = $cancel_methods_settings[$name];
+    }
+    elseif (isset($method['access'])) {
       $form[$name]['#access'] = $method['access'];
     }

It seems to me, that the new field can only restrict access in addition to the already set #access property. It cannot explicitelly allow an access to the option if #access is already disabled (so the option will not be displayed). Therefore the field name and description seem to me a bit confusing.

Other thing is that in case you have disabled some options (via hook_user_cancel_methods_alter), these options will still display in the new field because of this:

+    // Do not hide options on user cancel methods.
+    foreach (array_keys($form['registration_cancellation']['user_cancel_methods']['#options']) as $method_name) {
+      if (isset($form['registration_cancellation']['user_cancel_methods'][$method_name]['#access'])) {
+        unset($form['registration_cancellation']['user_cancel_methods'][$method_name]['#access']);
+      }
+    }

I think, that this is not good from the usability point of view (and it has no point to allow a user to check/uncheck them, because these will not display regardless of this setting).

Other point is, what will happen if you select a default option in the user_cancel_method, but you will not enable that method in user_cancel_methods? Should a new validation be added?

Available options can be altered via contrib modules, so I would consider if this change would fit to the core. In case yes, I think that we need a Usability review to check the new field (and its title / description), so we do not create a confusion among users. Thanks!

So after reading the comment from @poker10

The pending tasks are:

• If by settings the option is disabled, do not allow to alter the access to other modules.
• Do not show the cancel options in the checkbox list if some modules have already disabled them.
• Set the available cancel options before the default options, and set the states to hide the disabled options, also add a validation to check that the user does not choose a disabled option as a default cancel option.

@poker10 Did I forget any point?

If we change the description of the field to "Disable the following cancel methods:", does this fit better for you?

Changing from:

    if (isset($cancel_methods_settings[$name]) && !($cancel_methods_settings[$name])) {
      $form[$name]['#access'] = $cancel_methods_settings[$name];
    }
    elseif (isset($method['access'])) {
      $form[$name]['#access'] = $method['access'];
    }

To

    if ($cancel_methods_settings[$name]) {
      $form[$name]['#access'] = FALSE;
    }
    elseif (isset($method['access'])) {
      $form[$name]['#access'] = $method['access'];
    }

Ready to review.

Now the options to disable the cancel options are like this:
admin/config/people/accounts

We also added a validation to avoid choosing a disabled user cancel method.

Probably can close MR 4683 I don't think non bugs are being backported to D9 anymore.

Reviewing 4684
Applied the patch
Ran updb which ran without issue
Verified the config changes appear when doing a config export
Verified test coverage for post_update is there

Was checking the settings and think a good follow up would be to add states to the cancel methods. For example if I try to check "Disable the account and keep its content." I get an error in "When cancelling a user account" with message "The default user cancel method can not be a disabled method." Tagging for follow up if someone could open that.

But configuration updates work.

Was also still able to cancel an account.

Hiding file attachments since the MR work seems most recent.

xjm → closed merge request !4683

Thanks for your work on this!

It took me a couple reads to understand what this is actually proposing. Posted a few remarks on the MR.

I think #21 needs to be fixed here (with test coverage), not in a followup. Otherwise, we're closing a feature request but opening a bug instead.

Do users see these options when canceling their own accounts, or do they just follow the default option? And if so, should the user be told that?

I've more concerns aside from those I already expressed but out of time for the moment. I think this issue needs framework manager feedback, including on whether this is a suitable feature for core.

I was thinking about the architectural implications of whether core should allow this configuration, but we could also use product manager feedback on whether this is a suitable feature for core. There's a usability cost to having an extra field every time a user is canceled, as well.

Working on xjm comments

All issues were solved, added more testing coverage and validations, ready to review.

@Eduardo Morales Alberti yes, I think the descriptions / labels are better now, thanks!

The MR proposes to hardcode the original four cancellation methods in the cancel_method_options (in user.settings.yml and user.schema.yml).

But it is still possible to modify these methods via hook_user_cancel_methods_alter, so it is possible to add a new method for example. What would happen in that case - would it be possible to disable also such a new method added via that hook?

@poker10 Maybe the property should be a sequence instead of a mapping, reading the documentation about schema I saw this https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →

Mappings and sequences

There are two base list types defined by
configuration schema. If all keys can be predefined
in the schema structure, then you should use a
mapping, otherwise, you should use a sequence (even
if the list’s keys are strings).

#3384789-28: Allow user admins to choose the user cancellation method on the user deletion confirmation form (rather than having a single sitewide setting) →

would it be possible to disable also such a new method added via that hook?

The Account Settings form will also load the custom methods because it calls to user_cancel_methods, if they are not configured, will be shown as disabled, in those cases, we can show them as "Available" to avoid lost methods on the form save, and then let the user chose if checks or not those methods. What do you think about it?
https://git.drupalcode.org/project/drupal/-/merge_requests/4684/diffs#0f...

$form['registration_cancellation']['user_cancel_method_options'] += user_cancel_methods();

If the custom method is not configured by the user.settings then the method will be available.
https://git.drupalcode.org/project/drupal/-/merge_requests/4684/diffs#b2...

    // Allow other modules to alter the access if the method is available.
    if (isset($cancel_methods_settings[$name]) && !($cancel_methods_settings[$name])) {
      $form[$name]['#access'] = FALSE;
    }
    elseif (isset($method['access'])) {
      $form[$name]['#access'] = $method['access'];
    }

Can this be rebased? Gitlab is saying this is unmergable

Ran into this on two separate sites this week, where content has been deleted in error because users with "administer users" did not understand the difference between the options. Allowing this to be configured in core would be a good step.

However I think the title of this issue is incorrect? Admins can already choose the cancellation method on the user cancel form; this issue proposes a separate option to restrict the set of available options?