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

ProductRetrieveView:param:with_direct_siblings if True return product_pre, product_next to the context and direct_siblings in serializer #801

Open
wants to merge 10 commits into
base: releases/1.2
Choose a base branch
from

Conversation

haricot
Copy link
Contributor

@haricot haricot commented Apr 7, 2020

Same things as the linked pull request #734 but reviewed in a simpler way.

@haricot
Copy link
Contributor Author

haricot commented Apr 7, 2020

Otherwise change ProductRetrieveView.as_view in myshop.cms_app.CatalogListApp should be sufficient and in this case this pull request can be closed.

@jrief
Copy link
Member

jrief commented Apr 7, 2020

Thanks for this pull request. Rendering the previous and next siblings of a product, may really be useful in some situations.
Just some points I would have done differently:

  • Instead of using a tag named one_product, I would use something such as with_direct_siblings.
  • Don't change the semantics of the method get_object(), this should always return one and only one product. Instead add another method named say get_object_with_direct_siblings() .

But in general looking at your code, how does this view function behave, if someone filters by a certain product tag, or uses the search input field? Where is that information kept while rendering the product's detail view?

For this we have three possible solutions. Add this filtering information to the session, or to a cookie or as a request parameter. The latter generates ugly URLs for the detail view. Session could be problematic, because in django-SHOP the session_id is generated only after adding the first product to the cart, or as logged in user. So the remaining possibility is a cookie.

Let's discuss this on a chat.

@haricot haricot changed the title ProductRetrieveView:param:one_product if False return pre,cur,nex products to the context ProductRetrieveView:param:with_direct_siblings if True return pre,cur,nex products to the context Apr 8, 2020
@haricot haricot force-pushed the github/one_product_or_pre_cur_nex branch from e851bff to 02ee0cc Compare April 9, 2020 12:35
@haricot haricot changed the title ProductRetrieveView:param:with_direct_siblings if True return pre,cur,nex products to the context ProductRetrieveView:param:with_direct_siblings if True return pre,nex products to the context and direct_siblings Apr 9, 2020
@haricot haricot changed the title ProductRetrieveView:param:with_direct_siblings if True return pre,nex products to the context and direct_siblings ProductRetrieveView:param:with_direct_siblings if True return pre,nex products to the context and direct_siblings in serializer Apr 9, 2020
@haricot haricot changed the title ProductRetrieveView:param:with_direct_siblings if True return pre,nex products to the context and direct_siblings in serializer ProductRetrieveView:param:with_direct_siblings if True return product_pre,product_next to the context and direct_siblings in serializer Apr 9, 2020
@haricot haricot changed the title ProductRetrieveView:param:with_direct_siblings if True return product_pre,product_next to the context and direct_siblings in serializer ProductRetrieveView:param:with_direct_siblings if True return product_pre, product_next to the context and direct_siblings in serializer Apr 9, 2020
@haricot haricot force-pushed the github/one_product_or_pre_cur_nex branch from c22018a to 2bdfc8b Compare April 10, 2020 15:22
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

Successfully merging this pull request may close these issues.

None yet

2 participants