Task: Restructure ECK's admin URLs so that the breadcrumbs make more sense, providing better UX.

# Old New Status What the heck is it
1 admin/structure/eck/entity_type admin/structure/eck Done Entity type list
15 admin/structure/eck/entity_type/add admin/structure/eck/add Done Add an entity type
2 admin/structure/eck/entity_type/manage/eck_type admin/structure/eck/eck_type Done Entity type edit-form link
9 admin/structure/eck/entity_type/manage/eck_type/delete admin/structure/eck/eck_type/delete Done Entity type delete-form link
4 admin/structure/eck/entity/eck_type admin/content/eck_type Done List of entities of a type
16 admin/structure/eck/eck_entity_type/content/add admin/content/eck_entity_type/add Done List of bundles for an ECK entity type
5 admin/structure/eck/eck_entity_bundle/content/add admin/content/eck_entity_bundle/add Done Create a new entity of a specific bundle and type
8 admin/structure/eck/entity/eck_type/id /eck_type/id Done entity canonical link
6 admin/structure/eck/entity/eck_type/id/edit /eck_type/id/edit Done entity edit-form link
7 admin/structure/eck/entity/eck_type/id/delete /eck_type/id/delete Done entity delete-form link
3 admin/structure/eck/entity/eck_type/types admin/structure/eck/eck_type/bundles Done List of bundles of a type
17 admin/structure/eck/entity/eck_type/types/add admin/structure/eck/eck_type/bundles/add Done Add a bundle of a type
10 admin/structure/eck/entity/eck_type/types/manage/eck_bundle admin/structure/eck/eck_type/bundles/eck_bundle/edit Done entity bundle edit-form link
11 admin/structure/eck/entity/eck_type/types/manage/eck_bundle/delete admin/structure/eck/eck_type/bundles/eck_bundle/delete Done entity bundle delete-form link
12 admin/structure/eck/entity/eck_type/types/manage/eck_bundle/fields admin/structure/eck/eck_type/bundles/eck_bundle/fields automatic? needs tests? Bundle fields
13 admin/structure/eck/entity/eck_type/types/manage/eck_bundle/form-display admin/structure/eck/eck_type/bundles/eck_bundle/form-display automatic? needs tests? Bundle form display
14 admin/structure/eck/entity/eck_type/types/manage/eck_bundle/display admin/structure/eck/eck_type/bundles/eck_bundle/display automatic? needs tests? Bundle display

Remaining Tasks:

  • Paths
  • Breadcrumbs
  • Routes

Original report by legolasbo:

The entity listings only show breadcrumbs up until the 'Structure' page, instead of eck's parent entity type listing.

CommentFileSizeAuthor
#60 properly_sort_out_eck-2624784-60.patch20.27 KBakalata
#57 properly_sort_out_eck-2624784-57.patch152.95 KBlegolasbo
#57 interdiff-51-57.txt849 byteslegolasbo
#51 properly_sort_out_eck-2624784-51.patch20.27 KBlegolasbo
#48 properly_sort_out_eck-2624784-48.patch11.62 KBlegolasbo
#47 interdiff-44-47.txt7.86 KBlegolasbo
#47 properly_sort_out_eck-2624784-47.patch13.51 KBlegolasbo
#44 properly_sort_out_eck-2624784-43.patch14.88 KBlegolasbo
#40 properly_sort_out_eck-2624784-40.patch11.63 KBlegolasbo
#39 update_admin_paths_for-2624784-17.patch11.76 KBlegolasbo
#36 properly_sort_out_eck-2624784-36.patch34.97 KBlegolasbo
#33 properly_sort_out_eck-2624784-33.patch34.07 KBlegolasbo
#27 interdiff-23-26.txt13.8 KBlegolasbo
#26 properly_sort_out_eck-2624784-26.patch36.11 KBlegolasbo
#23 interdiff-20-23.txt16.48 KBlegolasbo
#23 update_admin_paths_for-2624784-23.patch27.72 KBlegolasbo
#20 interdiff-17-20.txt13.04 KBlegolasbo
#20 update_admin_paths_for-2624784-20.patch11.24 KBlegolasbo
#20 eck sitemap.png153.37 KBlegolasbo
#17 update_admin_paths_for-2624784-17.patch11.76 KBlegolasbo
#11 2624784-11.patch16.48 KBakalata
#8 2624784-8.patch16.51 KBakalata
#5 2624784-5.patch7.79 KBakalata
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

legolasbo created an issue. See original summary.

akalata’s picture

Based on the URL structure, it looks like the list of entities of a particular type is a parent of the type itself.

Page title: My ECK Entity Type content
Listing page: admin/structure/eck/entity/my_eck_entity_type
Breadcrumbs: Home > Admin > Structure

Page title: My ECK Entity Type bundles
Bundle listing page: admin/structure/eck/entity/my_eck_entity_type/types
Breadcrumbs: Home > Admin > Structure > "My ECK Entity Type" content

Meanwhile, the Structure -> ECK Entity Types listing is: admin/structure/eck/entity_type -- so I think the issue is more that the other Structure/config items don't follow that convention.

The entity lists and "add content" pages should probably live under Admin > Content instead, with the "add" pages following the [entity_type]/add/[bundle] convention (ala node/add/page).

My suggestion would be to update the paths as follows:

Old New
admin/structure/eck/entity_type admin/structure/eck
admin/structure/eck/entity_type/manage/my_eck_entity_type admin/structure/eck/my_eck_entity_type
admin/structure/eck/entity/my_eck_entity_type/types admin/structure/eck/my_eck_entity_type/types
admin/structure/eck/entity/my_eck_entity_type admin/content/my_eck_entity_type
admin/structure/eck/my_eck_entity_type/add/my_eck_entity_bundle my_eck_entity_type/add/my_eck_entity_bundle

Would like to get some feedback before I start in on those changes, though!

legolasbo’s picture

I think your suggestions make a lot of sense and I would gladly welcome a patch to implement them.

legolasbo’s picture

Issue tags: +Needs tests
akalata’s picture

Here's a start on adjusting paths to match the proposed new structure.

I started trying to figure out how to add the entity lists dynamically to admin/content as tabs in the menu. I keep getting distracted here at DrupalCon, but also I'm not sure if that's the "correct" approach -- ECK entities aren't necessarily "content".

On the other hand, if we look at implementing #1434224: Change entity admin pages to use Views in 8.x-1.x, and create default Views to function as the content lists, a site builder can change the path to be whatever they would like -- but we would still want an appropriate default.

Tests still need updating, but I wanted to post progress.

Status: Needs review » Needs work

The last submitted patch, 5: 2624784-5.patch, failed testing.

The last submitted patch, 5: 2624784-5.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
16.51 KB

Posting progress - a few tests fixed.

Status: Needs review » Needs work

The last submitted patch, 8: 2624784-8.patch, failed testing.

The last submitted patch, 8: 2624784-8.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
16.48 KB
gadaniels72’s picture

Status: Needs review » Needs work

Will be a big UX improvement but a few issues remain. I ran each of the before and after URLs in the issue summary, compared before and after patch displays and reviewed the breadcrumbs.

Test 1: admin/structure/eck/entity_type to admin/structure/eck
This passed completely. Expected and got breadcrumbs of Home Administration Structure.

Test 2: admin/structure/eck/entity_type/manage/my_eck_entity_type to admin/structure/eck/my_eck_entity_type
I get the page here that I would expect for the next test and not the page to edit/manage the entity type's settings.

Test 3: admin/structure/eck/entity/my_eck_entity_type/types to admin/structure/eck/my_eck_entity_type/types;
I get a page not found error. This should go to the page that is currently mapped to admin/structure/eck/my_eck_entity_type; the breadcrumbs I get on admin/structure/eck/my_eck_entity_type of Home Administration Structure ECK entities is what I would expect for the page that lists the bundles in a specific entity.

Test 4: admin/structure/eck/entity/my_eck_entity_type to admin/content/my_eck_entity_type.
I get a page not found error here but can reach the expected page at admin/entity-type. Given that not all ECK entities are content (and I would question instances where if it were content why ECK was used instead of a content type) this seems like a better result than using admin content but does not match the issue summary above. Would recommend changing the issue summary here. Also is it better to have it point to /admin/eck/my_eck_entity_type versus just admin/my_eck_entity_type?

Test 5: admin/structure/eck/my_eck_entity_type/add/my_eck_entity_bundle to my_eck_entity_type/add/my_eck_entity_bundle
I get a page not found error.

The URL is pointing to /eck/my_eck_entity_type/add/my_eck_entity_bundle instead of /my_eck_entity_type/add/my_eck_entity_bundle. If this is correct, the issue summary needs updated.

The breadcrumb reads Home -> Add [entity type] content. If you are using admin/entity-type instead of admin/content/entity type, should we drop the word content from the breadcrumb?

If you are changing the Add path, should the edit and delete paths also be updated? So if you have /eck/[entity type]/add/[entity bundle] should the edit path be similar (/eck/[entity type]/id/edit vs admin/structure/eck/[entity_type]/id that it currently has?

akalata’s picture

Title: Entity listing shows the wrong breadcrumbs » Update admin paths for better breadcrumb UX
Issue summary: View changes

Thanks Gwen for your review! Really great to have another pair of eyes on this.

I've updated the issue summary, adding numbers for clarity.

Agree that #2 got missed completely and confused with #3.

#2 is the implicit eck_type "edit", and should be consistent with the eck_type "delete" (new #9).

The patch did change #4 to "admin/[eck_type]", issue summary was out-of-date. I will elaborate on my "ECK entities are not necessarily content" in a follow-up comment. Maybe switch to "eck/[eck_type]" for the entity lists? This way we start to standardize on "/admin/structure/eck" for the entity building/configuration paths, and "/eck" for listing and working with the created entities.

#5 the reconfigured path should have been "eck/[eck_type]/add/[eck_bundle]" -- the initial "eck" needed to be added since paths are prohibited from having a dynamic value as the first item. This follows the node/add/bundle pattern as closely as possible.

Added #s 6 and 7 for entity "edit" and "delete", also 8 for the implicit entity "view".

Also noting that bundle-specific management changes were undocumented (and probably missing) as well.

akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
legolasbo’s picture

Title: Update admin paths for better breadcrumb UX » Properly sort out ECK routing, paths and breadcrumbs.
FileSize
11.76 KB

I've been working on tests to get all of this covered. Attached patch adds a new test class (NavigationalStructureTest) and tests all routes for their Title, Url and Breadcrumbs. Currently it tests the current situation. Next step is to change it to test the desired situation (and make testbot fail), then we can add @akatala's patch to it and fix the remaining issues.

Things I want to address in this issue are the following:

  • Make all routes follow the same naming pattern
  • Change the generated Urls to be more sensible (i.e. make entities accessible on {entityTypeName}/{entityId} like node/{nodeId} and other core entities)
  • Ensure breadcrumbs match the urls
  • Ensure titles are correct (many of them have issues with caps/non-caps letters)
legolasbo’s picture

Status: Needs work » Needs review

Setting needs review to see what testbot thinks of this patch that should pass testing.

legolasbo’s picture

Assigned: Unassigned » legolasbo
Status: Needs review » Needs work

Ok, so that passes.

Next up: Sitemap (as far as ECK is concerned) detailing expected routes, urls, breadcrumbs, etc. for each possibility and updated patch according to those specifications.

legolasbo’s picture

I've created a sitemap for eck's pages and updated the tests accordingly. The tests will obviously fail miserably, but let's see how badly they'll fail.

Status: Needs review » Needs work

The last submitted patch, 20: update_admin_paths_for-2624784-20.patch, failed testing.

The last submitted patch, 20: update_admin_paths_for-2624784-20.patch, failed testing.

legolasbo’s picture

Status: Needs review » Needs work

The last submitted patch, 23: update_admin_paths_for-2624784-23.patch, failed testing.

The last submitted patch, 23: update_admin_paths_for-2624784-23.patch, failed testing.

legolasbo’s picture

I've got the tests related to entity types passing. Unfortunately the route names for entities follow a convention of entity.[entityTypeName].foo where foo is foo_form when it comes to links generated by LinkGeneratorTrait. I had to adjust the tests accordingly to match this convention instead of the one I proposed in the sitemap on #20.

legolasbo’s picture

FileSize
13.8 KB

For some reason the interdiff was not added.

Status: Needs review » Needs work

The last submitted patch, 26: properly_sort_out_eck-2624784-26.patch, failed testing.

The last submitted patch, 26: properly_sort_out_eck-2624784-26.patch, failed testing.

  • legolasbo committed 0cf863c on 8.x-1.x
    Issue #2624784 by legolasbo: Add tests for routing, paths and...
legolasbo’s picture

Committed patch #17 to prevent regressions because it successfully tests untested parts of the current state and other issues might break those.

  • legolasbo committed ee7cf08 on 8.x-1.x
    Issue #2624784 by legolasbo: Add tests for routing, paths and...
legolasbo’s picture

Attached patch sorts out the paths.

Status: Needs review » Needs work

The last submitted patch, 33: properly_sort_out_eck-2624784-33.patch, failed testing.

The last submitted patch, 33: properly_sort_out_eck-2624784-33.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 36: properly_sort_out_eck-2624784-36.patch, failed testing.

The last submitted patch, 36: properly_sort_out_eck-2624784-36.patch, failed testing.

legolasbo’s picture

It seems #17 made the branch tests stall earlier so I reverted the commit. Re-uploading the patch now to see what happens.

*edit typo

legolasbo’s picture

hmm, test isn't being picked up.

let's try that again, but now with a patch from git diff instead of phpstorm.

Status: Needs review » Needs work

The last submitted patch, 40: properly_sort_out_eck-2624784-40.patch, failed testing.

The last submitted patch, 40: properly_sort_out_eck-2624784-40.patch, failed testing.

legolasbo’s picture

Ok, so after having a chat with dawehner on IRC I think I've found the problem with the test. Let's give this another go.

Status: Needs review » Needs work

The last submitted patch, 44: properly_sort_out_eck-2624784-43.patch, failed testing.

The last submitted patch, 44: properly_sort_out_eck-2624784-43.patch, failed testing.

legolasbo’s picture

  • legolasbo committed bc9f310 on 8.x-1.x
    Issue #2624784 by legolasbo, dawehner: Properly sort out ECK routing,...
legolasbo’s picture

Committed and pushed #48 to 8.x-1.x. Rerolling #36.

legolasbo’s picture

legolasbo’s picture

Ok, so the patch in #51 improves the paths. Any changes to breadcrumb assertions in the test are just there to account for the changed paths.

I'd like this patch to be reviewed and committed before we continue with making sure the breadcrumbs line up.

akalata’s picture

Issue summary: View changes

Updating issue summary while reviewing #51.

akalata’s picture

Issue summary: View changes

One more round of saving this crazy table.

akalata’s picture

Issue summary: View changes

I lied, this should be the last edit.

akalata’s picture

Issue summary: View changes
Status: Needs review » Needs work
  1. @@ -137,8 +137,8 @@ function eck_entity_type_build(array &$entity_types) {
               'group' => 'configuration',
               'group_label' => t('Configuration'),
               'links' => array(
    -            'edit-form' => '/admin/structure/eck/entity/' . $eck_type->id . '/types/manage/{' . $eck_type->id . '_type}/edit',
    -            'delete-form' => '/admin/structure/eck/entity/' . $eck_type->id . '/types/manage/{' . $eck_type->id . '_type}/delete',
    +            'edit-form' => '/admin/structure/eck/entity/' . $eck_type->id . '/types/{' . $eck_type->id . '_type}/edit',
    +            'delete-form' => '/admin/structure/eck/entity/' . $eck_type->id . '/types/{' . $eck_type->id . '_type}/delete',
               ),
             );
    

    "types" should be "bundles" to match the changes in createEditBundleRoute() and createDeleteBundleRoute()

  2. I'm guessing we don't need to add tests for 12, 13, and 14 (bundle fields, form display, and display), since those are created by virtue of the module defining a fieldable entity?

Sorry the issue summary table is now even more cramped; I wanted to record human-understandable labels, and we might want it available for early adopters (I'm sure I'll reference it a few times!).

legolasbo’s picture

Thanks for the review @akalata!

#56.1: Good catch! This is fixed in the attached patch.
#56.2: Yes, those are automatic by virtue of the module defining a fieldable entity.

Status: Needs review » Needs work

The last submitted patch, 57: properly_sort_out_eck-2624784-57.patch, failed testing.

The last submitted patch, 57: properly_sort_out_eck-2624784-57.patch, failed testing.

akalata’s picture

In #57, the interdiff was correct but the patch contained lots of extra stuff.

legolasbo’s picture

So @akatala,

No comments other than the misformatted patch? I've doublechecked your patch and it exactly 51 + the interdiff in 57. If you found no other issues I'll commit this so we can continue with the breadcrumbs.

  • legolasbo committed 90e7d06 on 8.x-1.x
    Issue #2624784 by legolasbo, akalata: Properly sort out ECK routing,...
legolasbo’s picture

kelly.m.jacobs’s picture

I've gone through all the paths listed in this ticket, and the breadcrumbs all seem to match the path structure. I'm not sure what was considered still left to do in terms of breadcrumbs, but unless I'm missing something, this issue seems Fixed to me.