Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support strictly hierarchical product views and URLs #101

Open
mik3y opened this issue Apr 16, 2013 · 14 comments
Open

Support strictly hierarchical product views and URLs #101

mik3y opened this issue Apr 16, 2013 · 14 comments

Comments

@mik3y
Copy link
Contributor

mik3y commented Apr 16, 2013

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:

  • No change to existing models.
  • Backwards compatible.

Con:

  • Extra DB lookups (joins against M2M table, I guess) for get_absolute_url() and product view.

Option 2

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:

  • Explicit, avoids M2M lookup in get_absolute_url.

Con:

  • Creates a Product -> Category dependency, a circular relationship (due to Category -> Products).

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
Copy link
Contributor Author

mik3y commented Apr 16, 2013

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
Copy link
Contributor Author

mik3y commented Apr 16, 2013

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

@stephenmcd
Copy link
Owner

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

@mik3y
Copy link
Contributor Author

mik3y commented Apr 17, 2013

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:

  • In the category_product view, add a fallback to lookup and render a matching Page on product slug miss. Suboptimal DB performance for these pages, but maybe enough of a special case not to care..
  • Tack something on to product urls, ie match the regex store/<category_slug>/<product_slug>/detail. Just reduces the size of the conflicting namespace (to store///detail). This token could be anywhere in the URL, ie store/detail// instead.
  • Make products Displayable (/Page?). However the category then becomes a static property of the product's slug (no longer changeable); there are probably other good reasons to avoid such a change.

Other ideas...?

@mik3y
Copy link
Contributor Author

mik3y commented Apr 17, 2013

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

@mik3y
Copy link
Contributor Author

mik3y commented Apr 17, 2013

(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
Copy link
Owner

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
Copy link
Contributor Author

mik3y commented Apr 18, 2013

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:

  • shop/product/<product-slug> -- if the feature is disabled or the product does not correspond to exactly one Category (existing behavior), or
  • shop/<category-slug>/<product-slug>/<product-id> otherwise.

(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:

  • Small stores: those with a limited number of products, and correspondingly limited number of "1-deep" categories. Examples: me!, "The Peculiar Store"
  • Large stores: those with large inventories and multivariate Categories, eg {men's, women's} x {shirts, jackets} x {sale, not sale}. Examples: "Adrenaline", "Cotton On".

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
Copy link
Contributor Author

mik3y commented Apr 19, 2013

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

@stephenmcd
Copy link
Owner

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
Copy link
Contributor Author

mik3y commented Apr 24, 2013

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

@mik3y
Copy link
Contributor Author

mik3y commented May 6, 2013

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

@mik3y
Copy link
Contributor Author

mik3y commented Jun 7, 2013

Rebased change for anyone still following along: 655154c

@andrewkoltsov
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants