#550 Split TaggedCategoryPage class #551

duker33 · 2018-08-27T11:53:57Z

It's the start of splitting CatalogPage's monolith code

Closes #550
Closes #548

ArtemijRodionov · 2018-08-28T02:12:29Z

shopelectro/views/catalog.py

+    def get_sorting_index(self):
+        return int(self.kwargs.get('sorting', 0))
+
+    def get_tags(self) -> typing.Union[models.TagQuerySet, None]:


typing.Optional[models.TagQuerySet] will be better

ArtemijRodionov · 2018-08-28T02:23:36Z

shopelectro/views/catalog.py

+#  See details at parent task.
+class TaggedCategoryPage(CategoryPage):
+
+    def get_sorting_index(self):


Seems the better place for it in CategoryPage class

@artemiy312 , the best place is SortedCategoryPage.
I moved all instead of CategoryPage's logic to TaggedCategoryPage.
Then i'll fork another class from TaggedCategoryPage and so on

ArtemijRodionov · 2018-08-28T02:26:47Z

shopelectro/views/catalog.py

-        page = context['page']
-        page.get_template_render_context = partial(
-            template_context, page, tag_titles, tags)
+        tags = self.get_tags()


You are calling it two times: here and in get_products(...). It will increase quantity of queries to db.

@artemiy312 , i thought, ORM caches such queries. I'll fix it

ArtemijRodionov · 2018-08-28T02:28:08Z

shopelectro/views/catalog.py

-        paginated_page = get_paginated_page_or_404(all_products, products_on_page, page_number)
-        total_products = all_products.count()
+        paginated_page = get_paginated_page_or_404(products, products_on_page, page_number)
+        total_products = products.count()
        products = paginated_page.object_list
        if not products:
            raise http.Http404('Page without products does not exist.')

        return {
            **context,
            'products_data': merge_products_context(products),


It will increase quantity of queries too, because you are already doing it in CategoryPage class

duker33 · 2018-09-03T15:13:49Z

@artemiy312 , review plz again

ArtemijRodionov

One moment with object composition

ArtemijRodionov · 2018-09-06T03:45:49Z

shopelectro/context.py

+        models.ProductPage.objects.filter(shopelectro_product__in=products)
+    )
+
+    # @todo #550:30m Create Product.get_brand method


We already have such issue

ArtemijRodionov · 2018-09-06T03:51:46Z

shopelectro/views/catalog.py

@@ -158,11 +159,14 @@ def get_context_data(self, **kwargs):
        }


+# TODO - split to ProductImagesContext and ProductBrandContext


Let's remove it or create well-formatted todoS

ArtemijRodionov · 2018-09-06T04:01:40Z

shopelectro/views/catalog.py

+        """Add sorting options and view_types in context."""
+        context_ = (
+            context.Category(self.kwargs, self.request)
+            | context.TaggedCategory()


Currently I do not understand why we need to use such syntax: context_obj | context_obj | ....
It has some critical cons:

We can not explicitly work with dependencies

We have to note, how some context depends on another one

I propose to use classic object composition, that has no any of these cons

e.g. of object composition:

category = context.SortedCategory(context.TaggedCategory(context.Category(self.kwargs, self.request))) pagination = context.PaginationContext(category) rendered_fields = context.RenderedFields(category) Contexts(category, pagination, rendered_fields).get_context()

ArtemijRodionov · 2018-09-06T07:41:33Z

shopelectro/context.py

+        return self.direction + self.field
+
+
+# @todo #539:60m Move PaginatorLinks to refarm-site.


This is no longer relevant todo, i am going to integrate implemented refarm_pagination after fidals/refarm-site#175

ArtemijRodionov · 2018-09-06T07:42:25Z

shopelectro/context.py

+from django.core.paginator import Paginator, InvalidPage
+from django_user_agents.utils import get_user_agent
+
+from images.models import Image


We should place RF related packages to single group, as i remember

ArtemijRodionov · 2018-09-07T02:03:31Z

shopelectro/context.py

+        #  Depends on updating to python3.7
+        view_type = self.request.session.get('view_type', 'tile')
+
+        # @todo #550:60m Create `Product.get_brand`


We already have the issue for it

ArtemijRodionov · 2018-09-07T02:05:05Z

shopelectro/context.py

+    def get_sorting_index(self):
+        return int(self.url_kwargs.get('sorting', 0))
+
+    # @todo #550:30m Test if context.TaggedCategory.get_tags cached.


What exactly should be cached here? DB queries?

@artemiy312 , y, i supposed them. But django ORM caches them by default.
So, i rm both lru_cache and task

ArtemijRodionov · 2018-09-07T02:07:17Z

@duker33 I left some comments

#550 Resolve branch inner conflicts #550 Apply linter rules #550 Review#2 fixes. Rm redundant pdd issue, fix imports #550 Rm redundant pdd issue #550 Pdd issue about context module improving #550 Merge fixes after hell #550 Minor self-review fixes #550 Add code example for creating context #550 Make context names shorten #550 Fork AbstractPageContext #550 Cleanup code #550 Fork PaginationCategoryContext class #550 Fork SortingCategoryContext class #550 Fork DBTemplateContext class. Improve pipe mech #550 Fork TaggedCategoryContext class #550 Implement CatalogContext #550 Fork SortingCategoryPage class #550 Fork PaginatedCatalogPage class #550 Fork DBContextCatalogPage class #548 Apply E305 linter rule #550 Review#1 fixes. Cache in memory db queries #548 Rm E305 pycodestyle rule #550 Add pdd issue for continue splitting #550 Create TaggedCategoryPage class

ArtemijRodionov · 2018-09-07T06:12:01Z

shopelectro/context.py

+Contains view context classes.
+
+Context is our own concept.
+It's part of the View abstract layer in MTV paradigm.


@artemiy312 , seems it's called MTV. Like one popular TV-channel

If you’re hungry for acronyms, you might say that Django is a “MTV” framework – that is, “model”, “template”, and “view.” That breakdown makes much more sense.

https://docs.djangoproject.com/en/2.1/faq/general/#django-appears-to-be-a-mvc-framework-but-you-call-the-controller-the-view-and-the-view-the-template-how-come-you-don-t-use-the-standard-names

duker33 self-assigned this Aug 27, 2018

duker33 requested a review from ArtemijRodionov August 27, 2018 11:53

duker33 changed the title ~~#550 Split catalog page~~ #550 Split TaggedCategoryPage class Aug 27, 2018

ArtemijRodionov requested changes Aug 28, 2018

View reviewed changes

duker33 force-pushed the 550_split_catalog_page branch from 541168e to 67ccf88 Compare September 3, 2018 15:13

duker33 requested a review from ArtemijRodionov September 3, 2018 15:13

ArtemijRodionov requested changes Sep 6, 2018

View reviewed changes

ArtemijRodionov reviewed Sep 6, 2018

View reviewed changes

duker33 requested a review from ArtemijRodionov September 7, 2018 01:18

ArtemijRodionov reviewed Sep 7, 2018

View reviewed changes

duker33 force-pushed the 550_split_catalog_page branch from cba1d81 to b0d5151 Compare September 7, 2018 02:04

ArtemijRodionov reviewed Sep 7, 2018

View reviewed changes

duker33 added 2 commits September 7, 2018 08:12

#550 Merge fixes

dd55ab6

duker33 force-pushed the 550_split_catalog_page branch from b0d5151 to dd55ab6 Compare September 7, 2018 05:22

duker33 requested a review from ArtemijRodionov September 7, 2018 05:22

ArtemijRodionov approved these changes Sep 7, 2018

View reviewed changes

ArtemijRodionov reviewed Sep 7, 2018

View reviewed changes

duker33 added 2 commits September 7, 2018 12:21

#550 Fix shadowed var

002d1c4

#550 Apply linter rules after rebase

d6b6ab3

duker33 closed this Sep 7, 2018

duker33 reopened this Sep 7, 2018

duker33 merged commit 9399dd6 into master Sep 7, 2018

duker33 deleted the 550_split_catalog_page branch September 7, 2018 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#550 Split TaggedCategoryPage class #551

#550 Split TaggedCategoryPage class #551

duker33 commented Aug 27, 2018 •

edited

Loading

ArtemijRodionov Aug 28, 2018

ArtemijRodionov Aug 28, 2018

duker33 Aug 28, 2018

ArtemijRodionov Aug 28, 2018

duker33 Aug 28, 2018

ArtemijRodionov Aug 28, 2018

duker33 commented Sep 3, 2018

ArtemijRodionov left a comment

ArtemijRodionov Sep 6, 2018

ArtemijRodionov Sep 6, 2018 •

edited

Loading

ArtemijRodionov Sep 6, 2018 •

edited

Loading

ArtemijRodionov Sep 6, 2018

ArtemijRodionov Sep 6, 2018

ArtemijRodionov Sep 6, 2018

ArtemijRodionov Sep 7, 2018

ArtemijRodionov Sep 7, 2018 •

edited

Loading

duker33 Sep 7, 2018

ArtemijRodionov commented Sep 7, 2018

ArtemijRodionov Sep 7, 2018

duker33 Sep 7, 2018

		@@ -158,11 +159,14 @@ def get_context_data(self, **kwargs):
		}


		# TODO - split to ProductImagesContext and ProductBrandContext

		return self.direction + self.field


		# @todo #539:60m Move PaginatorLinks to refarm-site.

#550 Split TaggedCategoryPage class #551

#550 Split TaggedCategoryPage class #551

Conversation

duker33 commented Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duker33 commented Sep 3, 2018

ArtemijRodionov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtemijRodionov Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

ArtemijRodionov Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtemijRodionov Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtemijRodionov commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duker33 commented Aug 27, 2018 •

edited

Loading

ArtemijRodionov Sep 6, 2018 •

edited

Loading

ArtemijRodionov Sep 6, 2018 •

edited

Loading

ArtemijRodionov Sep 7, 2018 •

edited

Loading