Support for custom Product models #57

Closed
wants to merge 16 commits into
from

Projects

None yet
@AlexHill
Collaborator

This is not intended to be merged immediately.

This adds support for subclassing Product, using the same strategy used in Mezzanine for subclassing Page:

  • A content_model field and corresponding method Product.get_content_model() are added.
  • ProductAdmin.change_view() is added which redirects to the subclass's change view.
  • The product view looks for a template called "shop/products/customproduct.html", where customproduct is the lower-cased name of the Product subclass.

Things that need doing:

  • tests
  • the names of content_model and get_content_model() are taken directly from Mezzanine and don't really make sense here. I think they should either be changed, or this functionality should be factored out into a mixin in Mezzanine core.
  • the template names and directory structure might warrant some attention
  • we should think about variations and how that's going to work

In the long term I'd like Category models to be able to be limited to a single Product subclass. That would allow templates, sort options, and custom page processors that are specific to a single Product type.

@travisbot

This pull request fails (merged 6ec055d into 635f267).

@travisbot

This pull request passes (merged 14d7866 into 635f267).

@joshcartme
Collaborator

I merged your changes into a fork of Cartridge of mine and I'm giving them a try. A few questions:

How is an admin user supposed to access the custom products since they are removed from the menus? If I type in the correct URL it works, but this doesn't show in any menus.

When accessing a custom product how do you access custom fields, or would a new admin class need to be created?

@AlexHill AlexHill closed this Sep 5, 2012
@AlexHill AlexHill reopened this Sep 5, 2012
@AlexHill
Collaborator
AlexHill commented Sep 5, 2012

Hey Josh,

I've overridden change_view in ProductAdmin, so when you click on a subclassed product in /admin/shop/product/, you should be redirected to the custom product's change view. This works the same way as custom content types in Mezzanine.

As yet there's no support for automatic detection of custom fields in ProductAdmin - as you say you need to create a new admin class.

@travisbot

This pull request passes (merged 14d7866 into 635f267).

@AlexHill AlexHill commented on the diff Sep 5, 2012
cartridge/shop/admin.py
@@ -227,6 +236,21 @@ def save_formset(self, request, form, formset, change):
# variation to the product.
self._product.copy_default_variation()
+ def change_view(self, request, object_id, extra_context=None):
+ """
+ As in Mezzanine's ``Page`` model, check ``product.get_content_model()``
+ for a subclass and redirect to its admin change view.
+ """
+ if self.model is Product:
+ product = get_object_or_404(Product, pk=object_id)
+ content_model = product.get_content_model()
+ if content_model is not None:
+ change_url = admin_url(content_model.__class__, "change",
+ content_model.id)
+ return HttpResponseRedirect(change_url)
@AlexHill
AlexHill Sep 5, 2012 collaborator

Here's where the redirection happens.

@travisbot

This pull request fails (merged 9e06c69 into 635f267).

@joshcartme
Collaborator

Thanks for the reply Alex, that bit of redirection works great!

I am wondering though, how would you recommend going about creating a new object that is a subclass of Product. Since the subclasses are removed from the menus I can't get to the screen to add a new subclass, unless I know and construct the URL myself. As far as I can tell there is no dropdown similar to pages (or other method) that allows you to pick the type of subclass you want.

Once I create an object that is of a type that is a subclass of Product it shows up under products and works great, it is just getting to that point that is proving a bit difficult.

Also I modified ProductAdmin so that it automatically pulls custom fields if custom fieldsets aren't specified (I took what PageAdmin does and modified it to work with products). You can see that here: https://bitbucket.org/joshcartme/cartridge_custom_product/changeset/6cc3adaf18b66cc982a31db6b8bfbd0c Feel free to pull it into your repo if you like it.
edit: just noticed that you already pulled in my ProductAdmin changes, I think I was typing when you committed it and didn't notice it in the thread =)

Sorry that it's on bitbucket, I haven't jumped on the Github train yet.

@travisbot

This pull request passes (merged 75004e2 into 635f267).

@AlexHill
Collaborator
AlexHill commented Sep 5, 2012

Thanks for the init code Josh. For now I'm happy to copy and paste as long as you are.

You're right, adding custom products is completely missing at the moment. The simple reason is that for the project I'm working on, I import everything automatically from another database.

Any ideas as to how we could fix this? I think a drop-down box next to the "add product" button on the product changelist page would work well, but what about on the dashboard?

@AlexHill
Collaborator
AlexHill commented Sep 5, 2012

I also don't necessarily think that having all products lumped into one in the admin is the right way to go. It might be good to have this as a per-product-type option somehow.

@AlexHill
Collaborator
AlexHill commented Sep 5, 2012

Latest commit adds a get_content_models class method and a models_for_products templatetag. We should be able to use those to insert a selection box on the product changelist page.

@joshcartme
Collaborator

It appears that we think alike, I just did something very similar here:
https://bitbucket.org/joshcartme/cartridge_custom_product/changeset/f3e24965c0dd80973cf959365898e2aa

I also accidentally change Product to Page in a comment, which I fixed in
the next commit

On Tue, Sep 4, 2012 at 8:51 PM, Alex Hill notifications@github.com wrote:

Latest commit adds a get_content_models class method and a
models_for_products templatetag. We should be able to use those to insert a
selection box on the product changelist page.


Reply to this email directly or view it on GitHubhttps://github.com/stephenmcd/cartridge/pull/57#issuecomment-8286928.

@travisbot

This pull request fails (merged 4dc2df7 into 635f267).

@travisbot

This pull request fails (merged ccc3ed7 into 635f267).

@AlexHill
Collaborator
AlexHill commented Sep 5, 2012

Hah - that's uncanny!

If we feel that this is a good way forward for Cartridge, I think it would be best if Steve pulled these changes into a branch on his repo. Then we can submit smaller pull requests to that branch from either Mercurial or Git.

@travisbot

This pull request passes (merged f62da50 into 635f267).

@joshcartme
Collaborator

I agree that's a good way to move forward. I'll post in the thread in the
Google Group again and ask Steve to take a look.

On Tue, Sep 4, 2012 at 9:16 PM, Alex Hill notifications@github.com wrote:

Hah - that's uncanny!

If we feel that this is a good way forward for Cartridge, I think it would
be best if Steve pulled these changes into a branch on his repo. Then we
can submit smaller pull requests to that branch from either Mercurial or
Git.


Reply to this email directly or view it on GitHubhttps://github.com/stephenmcd/cartridge/pull/57#issuecomment-8287285.

@joshcartme
Collaborator

Here is a final commits that create the drop down which works like the one on the Page change_list.
https://bitbucket.org/joshcartme/cartridge_custom_product/changeset/9594fc8da885cab1b5b168852d11d2847a8d89f7

@joshcartme
Collaborator

I decided to give git a try. Here is a branch that has the custom change_list.html https://github.com/joshcartme/cartridge/compare/custom_product

@AlexHill
Collaborator
AlexHill commented Sep 5, 2012

Cheers Josh. I took your change_list.html and tweaked it slightly so that the select box replaces the "Add Product" button.

@travisbot

This pull request passes (merged 53d1b0a into 635f267).

@electroniceagle

Hey guys, what is the status of this pull request? I'm in a refactoring mode (mood?) for the next few days for our own internal digital product subclasses and was thinking about using this instead. Are you using it actively in your sites? Cheers, Brian.

@stephenmcd
Owner

Hey Brian,

The intention is to merge it in - it's a big change so I need to go over it very thoroughly though, which I haven't had time to do.

If you'd like to run with it that'd at least go towards verifying that it works well.

@electroniceagle

Cool. I'll work that this week and report back.

Brian

@joshcartme
Collaborator

Just fyi, I am using it in production, and so far it works well for me.
One issue that I had was that I was also using Ken's cartridge-tax (which
works great). Since cartridge-tax implements it's own product admin extra
model fields were no longer being picked up which lead me to do this:
https://bitbucket.org/joshcartme/cartridge_custom_product/changeset/0488c0ba6ac9d2a1c7f7dc483a686344807a1db0

That's a bit hackish and probably not the best way to go about things, but
it got things working again for me quickly (a better approach would
probably be to unregister product form the admin and then re register a one
that include the proper fields).

On Tue, Oct 23, 2012 at 1:03 PM, Brian Schott notifications@github.comwrote:

Cool. I'll work that this week and report back.

Brian


Reply to this email directly or view it on GitHubhttps://github.com/stephenmcd/cartridge/pull/57#issuecomment-9716038.

@joshcartme
Collaborator

Sorry for the double post, but I just thought of something else.

Some work would probably still need to go into making it easy to override
how prices are calculated (at least in templates). Templates still
received an instance of Product so I ended up creating a template tag that
checked if the product had an instance of a particular subproduct and then
I returned it's price. It would be much better if this could be automated
somehow.

I'm curious to hear how it goes for you Brian.

On Tue, Oct 23, 2012 at 1:53 PM, Josh Cartmell joshcartme@gmail.com wrote:

Just fyi, I am using it in production, and so far it works well for me.
One issue that I had was that I was also using Ken's cartridge-tax (which
works great). Since cartridge-tax implements it's own product admin extra
model fields were no longer being picked up which lead me to do this:

https://bitbucket.org/joshcartme/cartridge_custom_product/changeset/0488c0ba6ac9d2a1c7f7dc483a686344807a1db0

That's a bit hackish and probably not the best way to go about things, but
it got things working again for me quickly (a better approach would
probably be to unregister product form the admin and then re register a one
that include the proper fields).

On Tue, Oct 23, 2012 at 1:03 PM, Brian Schott notifications@github.comwrote:

Cool. I'll work that this week and report back.

Brian


Reply to this email directly or view it on GitHubhttps://github.com/stephenmcd/cartridge/pull/57#issuecomment-9716038.

@AlexHill
Collaborator

Hey Brian et al,

I haven't had much time to add more feature-wise to this, but IIRC the last
email I sent to the list had a few areas for discussion, particularly
surrounding variations.

https://groups.google.com/d/msg/mezzanine-users/XPIUkII3Bkc/nE2xCL_Eo4oJ

I'd like to hear everyone's thoughts!

Alex

On Wed, Oct 24, 2012 at 12:24 AM, Brian Schott notifications@github.comwrote:

Hey guys, what is the status of this pull request? I'm in a refactoring
mode (mood?) for the next few days for our own internal digital product
subclasses and was thinking about using this instead. Are you using it
actively in your sites? Cheers, Brian.


Reply to this email directly or view it on GitHubhttps://github.com/stephenmcd/cartridge/pull/57#issuecomment-9708317.

@electroniceagle

Alex,

I'm going to reply to your original email and restart the discussion. I missed that whilst playing whack-a-mole on another project. I'm going to try your branch today and will hopefully have something intelligent to say (maybe :-).

Brian

On Oct 23, 2012, at 8:41 PM, Alex Hill notifications@github.com wrote:

Hey Brian et al,

I haven't had much time to add more feature-wise to this, but IIRC the last
email I sent to the list had a few areas for discussion, particularly
surrounding variations.

https://groups.google.com/d/msg/mezzanine-users/XPIUkII3Bkc/nE2xCL_Eo4oJ

I'd like to hear everyone's thoughts!

Alex

On Wed, Oct 24, 2012 at 12:24 AM, Brian Schott notifications@github.comwrote:

Hey guys, what is the status of this pull request? I'm in a refactoring
mode (mood?) for the next few days for our own internal digital product
subclasses and was thinking about using this instead. Are you using it
actively in your sites? Cheers, Brian.


Reply to this email directly or view it on GitHubhttps://github.com/stephenmcd/cartridge/pull/57#issuecomment-9708317.


Reply to this email directly or view it on GitHub.

@electroniceagle

We're using this on our internal alpha site and it is working well. One thing I want to add is a ProductBase similar to PageBase that defines the DisplayableManager in an abstract class so it gets inherited. Otherwise, search breaks unless the user defines the manager manually. I'll try to post some code.

@lorin
lorin commented Nov 8, 2012

We also have a different version that supports multiple levels of inheritance (if you're curious, see https://github.com/lorin/django-model-utils)

Unfortunately, the multi-level-inheritance solution doesn't work with the stock version of Django because of bug #16572. There's a proposed fix attached to that ticket that is working for us, but it would involve patching your local version of Django.

AlexHill added some commits Jan 10, 2013
@AlexHill AlexHill Merge remote-tracking branch 'upstream/master' into custom_products 542689c
@AlexHill AlexHill Added abstract base class for Product.
Defining the manager on an abstract base class means subclasses will
inherit it instead of having to explicitly define their own.
2941f55
@AlexHill
Collaborator

@electroniceagle I've added an abstract BaseProduct mirroring Mezzanine's BasePage as you suggested.

@AlexHill
Collaborator

@joshcartme regarding getting the subclass instance into templates: I agree this would be nice, and all it would take is a call to get_product_model in the product view. That means an extra trip to the database. On the one hand it seems wrong to do that by default; on the other hand, a single PK lookup is very cheap. If the content_model approach is replaced by select_subclasses, that will work there too.

Also if your custom product has its own custom template, you can always use product.subclass in the template.

@electroniceagle

If you update mezzanine, you will come across this change to mezzanine jquery-ui suport. It has moved from 1.8.14 to 1.9.1.

diff --git a/cartridge/shop/templates/admin/shop/product/change_list.html b/cartridge/shop/templates/admin/shop/product/change_list.html
index f8b7adc..41e7e08 100644
--- a/cartridge/shop/templates/admin/shop/product/change_list.html
+++ b/cartridge/shop/templates/admin/shop/product/change_list.html
@@ -4,7 +4,8 @@
 {% block extrahead %}
 {{ block.super }}

-<script src="{{ STATIC_URL }}mezzanine/js/jquery-ui-1.8.14.custom.min.js"></script>
+<script src="{{ STATIC_URL }}mezzanine/js/jquery-ui-1.9.1.custom.min.js"></script>
+<script src="{{ STATIC_URL }}mezzanine/js/admin/jquery.mjs.nestedSortable.js"></script>
 <script src="{{ STATIC_URL }}mezzanine/js/admin/page_tree.js"></script>
 {% endblock %}

PS: sorry for the multiple edits if that cause multiple emails. I still think entering obscure varying markup standards like trying to memorize wordstar and epson printer codes. If you don't know what that is, ask your father :-)

@stepmr
stepmr commented Aug 14, 2013

Any updates on this?

@electroniceagle

Our intention is to try to push our inheritance model approach after we migrate to Django 1.6.  The Django bug fix is in 1.6.

Sent from Mailbox for iPad

On Wed, Aug 14, 2013 at 1:00 PM, stepmr notifications@github.com wrote:

Any updates on this?

Reply to this email directly or view it on GitHub:
#57 (comment)

@jaywink
jaywink commented Jan 18, 2014

Any idea if this is still intended to land in master at some point in the future? Now that Django 1.6 is supported by Cartridge.

@pwalsh
pwalsh commented Feb 26, 2014

I'm interested in this. Have there been any more developments with the inheritance model approach + django 1.6?

@stephenmcd
Owner

No, nothing has really changed architecturally since this was done, so it should still apply at a high level - at a low level it looks like there are probably some code conflicts that need resolving still, given how long it's been.

@aleksey-zhigulin

Does ratings work with this feature?

@maxshilov

Hello,
any plans to merge this branch to mainstream?

@procmail
procmail commented Feb 2, 2016

Hi,

Any plans to merge this into the main branch?

@stephenmcd
Owner

Evolving in #293

@stephenmcd stephenmcd closed this Jun 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment