Updated: Comment #0

Problem/Motivation

Similar to the /node/add page, if there is only one custom block type, /block/add skips right to the form for that type.
However, all of the UI test coverage for the custom block type workflows assume only one block.

This leads to problems in issues like #2062439: Provide listing of custom block entities, where this workflow is left untouched.

Proposed resolution

Add full test coverage, with clickLink() not drupalGet() to ensure everything actually works

Remaining tasks

Write tests

User interface changes

N/A

API changes

N/A

N/A

CommentFileSizeAuthor
#7 custom-block-multiple-2064591.7.interdiff.txt4.02 KBlarowlan
#7 custom-block-multiple-2064591.7.patch3.89 KBlarowlan
#2 custom-block-multiple-2064591.fail_.patch2.81 KBlarowlan
#2 custom-block-multiple-2064591.pass_.patch3.82 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Assigned: Unassigned » larowlan

cause I can't face comment field

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Assigned: larowlan » Unassigned
Status: Active » Needs review
FileSize
custom-block-multiple-2064591.pass_.patch3.82 KB
custom-block-multiple-2064591.fail_.patch2.81 KB

hoping for red/green

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
  1. +++ b/core/modules/block/custom_block/custom_block.pages.inc
@@ -24,7 +24,12 @@ function template_preprocess_custom_block_add_list(&$variables) {
+    if (($destination = drupal_get_destination()) && $destination['destination'] !== current_path()) {
+      // A destination parameter is set other than the current path.

    I'm surprised we have to check this! Hm. 

  2. +++ b/core/modules/block/custom_block/custom_block.pages.inc
@@ -24,7 +24,12 @@ function template_preprocess_custom_block_add_list(&$variables) {
+    $variables['types'][$type->id]['link'] = l($type->label(), 'block/add/' . $type->id(), array('query' => $query));

    Partially out of scope, but this would look way better as a render array with '#type' => 'link'. 

  3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTypeTest.php
@@ -137,4 +137,61 @@ public function testCustomBlockTypeDeletion() {
+    // Create a block type programmaticaly.
...
+    // Create a block type programmaticaly.

    Can we change this to// Creates two block types programmatically.? There is a typo, and the important part here is that there are two. 

  4. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTypeTest.php
@@ -137,4 +137,61 @@ public function testCustomBlockTypeDeletion() {
+    $blocks = $this->container
+      ->get('plugin.manager.entity')
+      ->getStorageController('custom_block')
...
+    $blocks = $this->container
+      ->get('plugin.manager.entity')
+      ->getStorageController('custom_block')

    Might as well put this in a local variable 

  5. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTypeTest.php
@@ -137,4 +137,61 @@ public function testCustomBlockTypeDeletion() {
+      $this->assertEqual($this->getUrl(), url($destination, array(
+        'absolute' => TRUE
+      )));
...
+      $this->assertEqual($this->getUrl(), url('admin/structure/custom-blocks', array(
+        'absolute' => TRUE
+      )));

    Can't these be $this->assertUrl()? 

  6. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Tests/CustomBlockTypeTest.php
@@ -137,4 +137,61 @@ public function testCustomBlockTypeDeletion() {
+    }
+  }
 }

    Missing blank line

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Partially out of scope, but this would look way better as a render array with '#type' => 'link'.

Commented on #2025669: Introduce #type 'custom_block_add_list', agree out of scope.
3-6 yep will fix

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

1. we can try without it

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Assigned: Unassigned » larowlan

fixing

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Assigned: larowlan » Unassigned
FileSize
custom-block-multiple-2064591.7.patch3.89 KB
custom-block-multiple-2064591.7.interdiff.txt4.02 KB

Fixes issues at #3 except 1.
We don't want the destination to be set as /block/add because the form redirects back there instead. I.e. we only want drupal_get_destination() to give us the ?destination= bit, the fallback is to use the current page and this sends us back to /block/add.
Have updated the comment so its clear.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Fair point! Thanks so much.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Category: bug » task
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Fixed » Needs work

See #2068053: Custom block creation form redirect is inconsistent. If this issue was intended to fix that bug, it is (a) mis-titled and (b) not successful. :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Category: task » bug
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Missing test coverage for multiple custom block types » Custom block creation form redirect is inconsistent (was: missing test coverage for multiple custom block types)
Issue tags: +Usability, +Block UI overhaul
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Title: Custom block creation form redirect is inconsistent (was: missing test coverage for multiple custom block types) » Missing test coverage for multiple custom block types
Category: bug » task
Status: Needs work » Fixed

I'm reopening that one then. This did what it said.

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