zHaytam / SmartBreadcrumbs

A utility library for ASP.NET Core (both MVC and Razor Pages) websites to easily add and customize breadcrumbs.
https://blog.zhaytam.com/2018/06/24/asp-net-core-using-smartbreadcrumbs/
MIT License
161 stars 77 forks source link

DefaultBreadcrumb on Controller throws InvalidOperationException #24

Closed ConnerOrth closed 5 years ago

ConnerOrth commented 5 years ago

Create default project, add [DefaultBreadcrumb] to any Controller. Run project. See "Sequence contains no matching element" exception message.

Stacktrace:

  Name Value Type
  StackTrace " at System.Linq.Enumerable.Single[TSource](IEnumerable1 source, Func2 predicate)\r\n at SmartBreadcrumbs.BreadcrumbManager.GenerateHierarchy(Dictionary`2 entries) in \SmartBreadcrumbs\src\BreadcrumbManager.cs:line 67\r\n at SmartBreadcrumbs.BreadcrumbManager.Initialize(Assembly assembly) in SmartBreadcrumbs\src\BreadcrumbManager.cs:line 55\r\n at SmartBreadcrumbs.Extensions.ServiceCollectionExtensions.AddBreadcrumbs(IServiceCollection services, Assembly assembly, BreadcrumbOptions options) in \SmartBreadcrumbs\src\Extensions\ServiceCollectionExtensions.cs:line 27\r\n at SmartBreadcrumbs.Extensions.ServiceCollectionExtensions.AddBreadcrumbs(IServiceCollection services, Assembly assembly) in

I think this error is being thrown because the BreadcrumbManager.Initialize method. Does not consider attributes on controller class only on controller methods.

My suggestion would be to also check for BreadcrumbAttribute on controller class level just like with razor page model class level.

zHaytam commented 5 years ago

Hello, why would we check the attribute on the whole controller?

ConnerOrth commented 5 years ago

For the same reason you check the attribute on a razor page.

If I have a HomeController, then placing a DefaultBreadcrumb would make sense that we generate a "Primary" breadcrumb: "Home".

MvcRouteTemplate should be able to handle the generation of the url if no action is specified.

// Add MVC to the request pipeline.
app.UseMvc(routes =>
{
    routes.MapRoute(
        name: "default",
        template: "{controller=Home}/{action=Index}/{id?}");
});

For instance, creating the following Breadcrumbs: "Home > Create".

Could be done with just 2 attributes. [DefaultBreadcrumb] on HomeController class. [Breadcrumb("Create")] on HomeController.Create Method. The "Home > Create" <- the home section should link to Home/Index because of the MapRoute template. There is now no need for an attribute at the default action method.

As a sidenote perhaps make the Breadcrumb title, default to the method name if no title/name has been passed. This would result in less "noice/verbosity" when creating the breadcrumbAttributes.

zHaytam commented 5 years ago

But there is no overload of Url.Action that expects a controller without an action, no?

ConnerOrth commented 5 years ago

You can call _urlHelper.Action(new UrlActionContext() { Action = null, Controller = route["controller"] }); this will generate an url just fine.

Allowing breadcrumb at the controller class level, would however require a few changes in the registration of the nodekeys. Also the TryExtractingEntries will probably need some work aswell then.

I could try to see if in the coming weeks, I'm able to get a PR up, if that's ok with you?

zHaytam commented 5 years ago

Of course it's okay with me!

ConnerOrth commented 5 years ago

Update on my progress so far, seems to be going well. Just want to make sure I'm not going to break smartbreadcrumbs for anyone else. ;)

ConnerOrth commented 5 years ago

@zHaytam I got my PR ready, all tests passed and also added a few new tests.

zHaytam commented 5 years ago

Fixed in #27