Support from Acquia helps fund testing for Drupal Acquia logo

Comments

barbun’s picture

And here's the patch.

Status: Needs review » Needs work

The last submitted patch, 1807266-user-cancel-method-2.patch, failed testing.

barbun’s picture

Status: Needs review » Needs work
FileSize
9.21 KB

Ok, added small tweak for default value.

Btw, I built my patch over this:
http://git.drupal.org/sandbox/heyrocker/1145636.git

is that ok?

barbun’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1807266-user-cancel-method-3.patch, failed testing.

barbun’s picture

Status: Needs work » Needs review

#3: 1807266-user-cancel-method-3.patch queued for re-testing.

Cameron Tod’s picture

This patch looks basically fine, just a few whitespace issues:

+++ b/core/modules/statistics/lib/Drupal/statistics/Tests/StatisticsAdminTest.phpundefined
@@ -137,8 +137,8 @@ class StatisticsAdminTest extends WebTestBase {
-    variable_set('user_cancel_method', 'user_cancel_delete');
+    ¶

There's a few leading whitespace issues like this.

+++ b/core/modules/user/user.admin.incundefined
@@ -654,6 +655,7 @@ function user_admin_settings_submit($form, &$form_state) {
     ->set('signatures', $form_state['values']['user_signatures'])
+    ->set('cancel_method', $form_state['values']['user_cancel_method'])      ¶

Trailing whitespace here.

I will post a patch shortly...

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
8.64 KB

Cleaned up the whitespace stuff.

Berdir’s picture

It probably wouldn't hurt to add some basic test coverage for this, see SystemUpgradePathTest.php (we should rename that to ConfigUpgradePathTest in another issue). Then we don't have to test it manually to RTBC this.

Cameron Tod’s picture

Ok so I have added an upgrade path, and set the default for the value in user.settings.yml instead of using a ternary.

There's an unused variable, $default_method, in user_cancel_confirm_form(), which is in D7 as well, so I wil create an issue for that once this gets committed.

Cameron Tod’s picture

Berdir’s picture

Not sure if we need a separate test method for this, because it's the same database dump anyway. Other than that, this looks good to me.

Cameron Tod’s picture

Moved tests into same method, renamed, and updated comment.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, I think this is fine like this.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks good. Committed/pushed to 8.x.

Cameron Tod’s picture

Great stuff. I have posted a patch over in #1825648: Rename SystemUpgradePathTest to ConfigUpgradePathTest, so we can have tests for the CMI upgrade path, if we can get that or something similar in it will be a good conduit to increasing test coverage of the conversion effort.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.