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

Comments

larowlan’s picture

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

cause I can't face comment field

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned
Status: Active » Needs review
StatusFileSize
new custom-block-multiple-2064591.pass_.patch3.82 KB
new 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
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
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
commented

1. we can try without it

larowlan’s picture

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

fixing

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned
StatusFileSize
new custom-block-multiple-2064591.7.patch3.89 KB
new 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
commented
Status: Needs review » Reviewed & tested by the community

Fair point! Thanks so much.

catch’s picture

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

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

Committed/pushed to 8.x, thanks!

xjm’s picture

xjm
she/her
Primary language English
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
commented
Category: task » bug
xjm’s picture

xjm
she/her
Primary language English
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
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.