search

stephenmcd / cartridge

Ecommerce for Mezzanine
http://cartridge.jupo.org
BSD 2-Clause "Simplified" License
709 stars 299 forks source link

Support strictly hierarchical product views and URLs #101

Open mik3y opened 11 years ago

mik3y commented 11 years ago

Hi Stephen,

I'd like to support a strict hierarchy of products. Perhaps an illustration of the desired outcome will explain best.

Here's what the store's left nav and URLs would ideally look like:

My Store                           /store/
  |- Hardware (category)           /store/hardware/
      |--- Product 1               /store/hardware/product-1/
      |--- Product 2               /store/hardware/product-2/
  |- Accessories                   /store/accessories/
      |--- Product 3               /store/accessories/product-3/
      |--- Product 4               /store/accessories/product-4/

On each product page, I'd like to render the nav bar and breadcrumbs in terms of each product's one (and only) category, ie:

[Home] / [My Store] / [Hardware] / Product 1

Today each product has a ManyToMany relationship to zero or more Categories. I see two ways to support this:

Option 1

Modify Product.get_absolute_url() to inspect Product.categories to find the Product's canonical category.

When len(self.categories) == 1, return a different view mapped to the URL /<category_slug>/<slug>/ instead of /product/<slug>/.

Pro:

Con:

Add a new field to Product indicating the "primary" category:

    primary_category = models.ForeignKey("Category", _("Primary category"),
            blank=True, null=True, on_delete=models.SET_NULL)

Pro:

Con:

Perhaps you have other ideas. I am leaning towards option 1; it could be gated by a feature flag (default off) out of concern for extra DB load.

One other thought: A view serving /<category_slug>/<product_slug>/ should be forgiving in the event that category_slug does not match the product's category.

Since product_slug unambiguously identifies the product, the view should issue a 301 redirect to product.get_absolute_url(). This keeps hierarchical URLs while allowing categories to be renamed/changed in the future.

mik3y commented 11 years ago

Taking a look at option 1, I've run into a small problem: Category is a Page, but Product is not.

So Category has a "fully qualified" slug ("store/hardware") served by a mezzanine view, but Product has a relative slug ("product-1") served by a cartridge view.

Simply joining these almost works, except it only makes sense to do so in shop.urls, which is presumably rooted at ^store/ -- so the resolved URL becomes store/store/hardware/product-1. Hrm..

mik3y commented 11 years ago

See above quick-and-dirty hack, which seems to work for my purposes..

stephenmcd commented 11 years ago

Won't your new urlpattern prevent regular Mezzanine pages from being matched?

mik3y commented 11 years ago

Yes, I suppose it would, for any Page created within the ^store/ url path (and containing another slash). Not an issue in my case but I can certainly see how it might be.

Some possible remedies, all a bit icky:

Other ideas...?

mik3y commented 11 years ago

Here's a variation on the second bullet, which on balance doesn't seem very nasty: ac345f4187eb9041de72be1806f862fc4fc78f76. See the justification there.

mik3y commented 11 years ago

(Bizarre that the commit hash auto-linked & renders within this repo rather than mine. I think this is a github bug, shot it over to support.)

stephenmcd commented 11 years ago

Firstly I just wanna say that I really do appreciate what you're trying to solve here. I've had the exact same problem in the past with ecommerce sites, and working out what the canonical category is for the sake of a nicer URL, and more importantly for breadcrumb navigation. Interestingly, it's actually a broader problem that comes up in other cases as well, such as news sites where stories can appear in multiple sections of a site.

Let's forget about breadcrumbs for a moment and just think about the URL structure.

I think with all the options you've come up with (which are all really clever, and it's great they actually work), you've basically shown that no matter which way we wrangle this, we're gonna end up with an ugly overall design whereby we end up with multiple code paths for handling the category and product views. Categories are Mezzanine pages - they can pretty much fall under any urlpattern - this is a great feature I think, it allows people to set up site hierarchies however they choose to. But products aren't pages, and I'd argue rightly so - they don't belong in the navigation structure of the site.

So taking Mezzanine pages into account, taking Django urlpatterns into account, and specifically the need for a specific urlpattern that matches products (eg /product/some-slug/), I think the whole idea of trying to include category slugs in the URLs for products really just falls apart. I guess we could somehow have product slugs actually include the slug of their canonical category, but we'd still need the urlpattern prefixed with the unique /product/ part, so we wouldn't end up with perfect URLs anyway.

To top all that off - if someone really wants this, and can actually get this working in the context of their own project, then that's great and I'd totally suggest doing it. Perhaps we can take some of the stuff you've put together here and put it into a separate app, cartridge-canonical-categories or some such. I just think that if we try and implement this in Cartridge itself, it's a slippery slope downward towards corrupting the overall flow of how each of the urlpatterns and views work together. We'd lose a great deal of simplicity at the cost of changing the URLs slightly in some cases. I just don't think it's worth it.

Now for my mind, I think the urlpatterns are of little relative importance when compared to the breadcrumb issue - I think in the case of each product only belonging to single category, having the breadcrumb nav in the product template reflect this would actually be an extremely useful feature. So I'd actually really like to solve that, and I think we can do it in a pretty clean way, clean at least when compared to all the issues that come up with trying to make this happen for URLs.

Not sure what the end result for getting breadcrumbs working might look like - it could be a setting like you described, and that controls in the template whether we use a special template tag that handles rendering the breadcrumbs for a product and its one and only category.

mik3y commented 11 years ago

Thanks for your thoughts! I appreciate the consideration. More inline.

you've basically shown that no matter which way we wrangle this, we're gonna end up with an ugly overall design whereby we end up with multiple code paths for handling the category and product views.

Agreed; I think this is the status quo and not worth changing.

Categories are Mezzanine pages - they can pretty much fall under any urlpattern - this is a great feature I think, it allows people to set up site hierarchies however they choose to. But products aren't pages, and I'd argue rightly so - they don't belong in the navigation structure of the site.

I think I've got my head around the distinction now, thanks.

So taking Mezzanine pages into account, taking Django urlpatterns into account, and specifically the need for a specific urlpattern that matches products (eg /product/some-slug/), I think the whole idea of trying to include category slugs in the URLs for products really just falls apart.

I think I see now.

It specifically breaks down if a Category is rooted outside of the shop, ie, the Category's slug is not a child of / does not begin with shop/ (or wherever cartridge.shop.urls is rooted). In this case my change borrow's the Category's slug to create shop/<category>/<product>, for a category that actually lives at anywhere/<category> -- shop/<category>/ does not exist..

Curious, do you think this is common? Though Categories have all the flexibility of Pages and can be scattered anywhere, do many cartridge sites take advantage of this this? I couldn't find any examples in spot checking a few of the sites on the list: for example, Cotton On (by all accounts an impressively customized site!) seems to use shop/ for all categories.

This issue aside, the resulting logic should be straightforward: A product's URL is either:

(I would separately propose tacking product-id on to the shop/product/<product-slug> URL, for symmetry and resilience to product slug changes -- but slugs aren't editable, and this orthogonal to the matter at hand.)

I guess we could somehow have product slugs actually include the slug of their canonical category, but we'd still need the urlpattern prefixed with the unique /product/ part, so we wouldn't end up with perfect URLs anyway.

It's probably a bad idea; the product's slug would be brittle in the face of changes to its canonical category.

To top all that off - if someone really wants this, and can actually get this working in the context of their own project, then that's great and I'd totally suggest doing it. Perhaps we can take some of the stuff you've put together here and put it into a separate app, cartridge-canonical-categories or some such.

I do like cartridge's philosophy of intentionally minimizing bundled functionality, though I'm not quite sure how to package this separately (it would no doubt involve monkey-patching Product.get_absolute_url).. I mean, I can always maintain and deploy from my fork, but that's not why I'm here :-)

We'd lose a great deal of simplicity at the cost of changing the URLs slightly in some cases. I just don't think it's worth it.

Acknowledged. From my perspective -- which is very limited -- perhaps there are at least two broad "styles" of stores that could be supported in cartridge, with slightly different requirements:

In my case, I don't ever envision needing (a) categories outside of shop/, nor (b) products corresponding to more than one category (or nested categories). So achieving descriptive URLs and breadcumbs feels within reach -- though maybe it's a lost cause for larger multi-category operations like "Cotton On".

Now for my mind, I think the urlpatterns are of little relative importance when compared to the breadcrumb issue - I think in the case of each product only belonging to single category, having the breadcrumb nav in the product template reflect this would actually be an extremely useful feature.

I'm certainly fine for separating the issues! The loss of context in the breadcrumbs (for example, on "Adrenaline") once you drill down into a product is jarring. I can live with opaque URLs, but loss of context while browsing my store is what originally sent me down this route..

FWIW and to my surprise, breadcrumbs "just worked" with my change -- the store and category breadcrumbs are correctly displayed and linked on the (category-slugified) product page. I suspect it may break with sub-categories, but I haven't tried. This also relies on the earlier assumption that categories are children of the main shop/ category.

Whew, that was longer than expected, sorry for the verbosity..

[edit: typos]

mik3y commented 11 years ago

FYI: Not adding anything new to the discussion, but here's a consolidated change: 51e5f7db97880e382f0a095712c66f2bb8f518c3

stephenmcd commented 11 years ago

Sorry about the delay on this - I'm just giving it a lot of thought. The last commit you linked to is really useful and with everything boiled down into that, I think we're on the right track. We might be able to achieve it even a bit more simply, but still trying to flesh that out in my head :-)

mik3y commented 11 years ago

No worries! I'm on vacation this week, take your time. Glad I at least have you intrigued :)

mik3y commented 11 years ago

FYI (again nothing new, just for illustration) my store is now online with this change: https://kegbot.org/store/

mik3y commented 11 years ago

Rebased change for anyone still following along: 655154c55ab840e4408619

andrewkoltsov commented 11 years ago

will it be included in cartridge, or I should patch it my self?