From cb330bc6e8875930a8b02a0a6824746eabaa404f Mon Sep 17 00:00:00 2001 From: smilerz Date: Tue, 28 Sep 2021 10:09:13 -0500 Subject: [PATCH 1/6] added Admin function to sort trees --- cookbook/admin.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cookbook/admin.py b/cookbook/admin.py index 2146629c3..726563c31 100644 --- a/cookbook/admin.py +++ b/cookbook/admin.py @@ -89,9 +89,18 @@ class SyncLogAdmin(admin.ModelAdmin): admin.site.register(SyncLog, SyncLogAdmin) +@admin.action(description='Sort tree by name') +def sort_tree(modeladmin, request, queryset): + modeladmin.model.node_order_by = ['name'] + with scopes_disabled(): + Keyword.fix_tree(fix_paths=True) + modeladmin.model.node_order_by = [] + + class KeywordAdmin(TreeAdmin): form = movenodeform_factory(Keyword) ordering = ('space', 'path',) + actions = [sort_tree] admin.site.register(Keyword, KeywordAdmin) @@ -138,6 +147,7 @@ admin.site.register(Unit) class FoodAdmin(TreeAdmin): form = movenodeform_factory(Keyword) ordering = ('space', 'path',) + actions = [sort_tree] admin.site.register(Food, FoodAdmin) From 658e04addbf5fc13b4e6af53cdcbffa7ba16ea1a Mon Sep 17 00:00:00 2001 From: smilerz Date: Tue, 28 Sep 2021 11:39:15 -0500 Subject: [PATCH 2/6] make node_sort_order an .env variable --- .env.template | 10 +++++++++- cookbook/admin.py | 2 +- cookbook/models.py | 10 +++++----- cookbook/tests/api/test_api_food.py | 10 +++++++--- cookbook/tests/api/test_api_keyword.py | 9 ++++++--- cookbook/views/api.py | 15 ++++++++++++--- recipes/settings.py | 1 + 7 files changed, 41 insertions(+), 16 deletions(-) diff --git a/.env.template b/.env.template index d7c44dfeb..9fb821922 100644 --- a/.env.template +++ b/.env.template @@ -120,4 +120,12 @@ REVERSE_PROXY_AUTH=0 # Django session cookie settings. Can be changed to allow a single django application to authenticate several applications # when running under the same database # SESSION_COOKIE_DOMAIN=.example.com -# SESSION_COOKIE_NAME=sessionid # use this only to not interfere with non unified django applications under the same top level domain \ No newline at end of file +# SESSION_COOKIE_NAME=sessionid # use this only to not interfere with non unified django applications under the same top level domain + + +# by default SORT_TREE_BY_NAME is enabled this will store all Keywords and Food in case sensitive order +# this setting makes saving new keywords and foods very slow, which doesn't matter in most usecases. +# however, when doing large imports of recipes that will create new objects, can increase total run time by 5-10x +# Disabling SORT_TREE_BY_NAME (setting value to 0) will store objects unsorted, but will substantially increase speed of imports. +# Keywords and Food can be manually sorted by name in Admin +# SORT_TREE_BY_NAME=0 \ No newline at end of file diff --git a/cookbook/admin.py b/cookbook/admin.py index 726563c31..e51741d7b 100644 --- a/cookbook/admin.py +++ b/cookbook/admin.py @@ -89,7 +89,7 @@ class SyncLogAdmin(admin.ModelAdmin): admin.site.register(SyncLog, SyncLogAdmin) -@admin.action(description='Sort tree by name') +@admin.action(description='Fix problems and sort tree by name') def sort_tree(modeladmin, request, queryset): modeladmin.model.node_order_by = ['name'] with scopes_disabled(): diff --git a/cookbook/models.py b/cookbook/models.py index 62c8dc9b3..867d5fd80 100644 --- a/cookbook/models.py +++ b/cookbook/models.py @@ -19,7 +19,7 @@ from treebeard.mp_tree import MP_Node, MP_NodeManager from django_scopes import ScopedManager, scopes_disabled from django_prometheus.models import ExportModelOperationsMixin from recipes.settings import (COMMENT_PREF_DEFAULT, FRACTION_PREF_DEFAULT, - STICKY_NAV_PREF_DEFAULT) + STICKY_NAV_PREF_DEFAULT, SORT_TREE_BY_NAME) def get_user_name(self): @@ -335,8 +335,8 @@ class SyncLog(models.Model, PermissionModelMixin): class Keyword(ExportModelOperationsMixin('keyword'), TreeModel, PermissionModelMixin): - # TODO add find and fix problem functions - # node_order_by = ['name'] + if SORT_TREE_BY_NAME: + node_order_by = ['name'] name = models.CharField(max_length=64) icon = models.CharField(max_length=16, blank=True, null=True) description = models.TextField(default="", blank=True) @@ -370,8 +370,8 @@ class Unit(ExportModelOperationsMixin('unit'), models.Model, PermissionModelMixi class Food(ExportModelOperationsMixin('food'), TreeModel, PermissionModelMixin): - # TODO add find and fix problem functions - # node_order_by = ['name'] + if SORT_TREE_BY_NAME: + node_order_by = ['name'] name = models.CharField(max_length=128, validators=[MinLengthValidator(1)]) recipe = models.ForeignKey('Recipe', null=True, blank=True, on_delete=models.SET_NULL) supermarket_category = models.ForeignKey(SupermarketCategory, null=True, blank=True, on_delete=models.SET_NULL) diff --git a/cookbook/tests/api/test_api_food.py b/cookbook/tests/api/test_api_food.py index 8e53c753a..a1b30d3c1 100644 --- a/cookbook/tests/api/test_api_food.py +++ b/cookbook/tests/api/test_api_food.py @@ -21,6 +21,10 @@ LIST_URL = 'api:food-list' DETAIL_URL = 'api:food-detail' MOVE_URL = 'api:food-move' MERGE_URL = 'api:food-merge' +if (Food.node_order_by): + node_location = 'sorted-child' +else: + node_location = 'last-child' @pytest.fixture() @@ -264,7 +268,7 @@ def test_integrity(u1_s1, recipe_1_s1): i_1.step_set.first().ingredients.remove(i_1) assert Food.objects.count() == 10 assert Ingredient.objects.count() == 10 - + # deleting food will succeed because its not part of recipe and delete will cascade to Ingredient r = u1_s1.delete( reverse( @@ -429,7 +433,7 @@ def test_root_filter(obj_1, obj_1_1, obj_1_1_1, obj_2, obj_3, u1_s1): assert len(response['results']) == 2 with scopes_disabled(): - obj_2.move(obj_1, 'last-child') + obj_2.move(obj_1, node_location) # should return direct children of obj_1 (obj_1_1, obj_2), ignoring query filters response = json.loads(u1_s1.get(f'{reverse(LIST_URL)}?root={obj_1.id}').content) assert response['count'] == 2 @@ -439,7 +443,7 @@ def test_root_filter(obj_1, obj_1_1, obj_1_1_1, obj_2, obj_3, u1_s1): def test_tree_filter(obj_1, obj_1_1, obj_1_1_1, obj_2, obj_3, u1_s1): with scopes_disabled(): - obj_2.move(obj_1, 'last-child') + obj_2.move(obj_1, node_location) # should return full tree starting at obj_1 (obj_1_1_1, obj_2), ignoring query filters response = json.loads(u1_s1.get(f'{reverse(LIST_URL)}?tree={obj_1.id}').content) assert response['count'] == 4 diff --git a/cookbook/tests/api/test_api_keyword.py b/cookbook/tests/api/test_api_keyword.py index 82cad211e..7afcbf8a1 100644 --- a/cookbook/tests/api/test_api_keyword.py +++ b/cookbook/tests/api/test_api_keyword.py @@ -20,7 +20,10 @@ LIST_URL = 'api:keyword-list' DETAIL_URL = 'api:keyword-detail' MOVE_URL = 'api:keyword-move' MERGE_URL = 'api:keyword-merge' - +if (Keyword.node_order_by): + node_location = 'sorted-child' +else: + node_location = 'last-child' @pytest.fixture() def obj_1(space_1): @@ -350,7 +353,7 @@ def test_root_filter(obj_1, obj_1_1, obj_1_1_1, obj_2, obj_3, u1_s1): assert len(response['results']) == 2 with scopes_disabled(): - obj_2.move(obj_1, 'last-child') + obj_2.move(obj_1, node_location) # should return direct children of obj_1 (obj_1_1, obj_2), ignoring query filters response = json.loads(u1_s1.get(f'{reverse(LIST_URL)}?root={obj_1.id}').content) assert response['count'] == 2 @@ -360,7 +363,7 @@ def test_root_filter(obj_1, obj_1_1, obj_1_1_1, obj_2, obj_3, u1_s1): def test_tree_filter(obj_1, obj_1_1, obj_1_1_1, obj_2, obj_3, u1_s1): with scopes_disabled(): - obj_2.move(obj_1, 'last-child') + obj_2.move(obj_1, node_location) # should return full tree starting at obj_1 (obj_1_1_1, obj_2), ignoring query filters response = json.loads(u1_s1.get(f'{reverse(LIST_URL)}?tree={obj_1.id}').content) assert response['count'] == 4 diff --git a/cookbook/views/api.py b/cookbook/views/api.py index 135f8a94b..5c623c407 100644 --- a/cookbook/views/api.py +++ b/cookbook/views/api.py @@ -189,9 +189,14 @@ class MergeMixin(ViewSetMixin): # TODO update Units to use merge API # a new scenario exists and needs to be handled raise NotImplementedError if isTree: + if self.model.node_order_by: + node_location = 'sorted-child' + else: + node_location = 'last-child' + children = source.get_children().exclude(id=target.id) for c in children: - c.move(target, 'last-child') + c.move(target, node_location) content = {'msg': _(f'{source.name} was merged successfully with {target.name}')} source.delete() return Response(content, status=status.HTTP_200_OK) @@ -232,6 +237,10 @@ class TreeMixin(MergeMixin, FuzzyFilterMixin): @decorators.renderer_classes((TemplateHTMLRenderer, JSONRenderer)) def move(self, request, pk, parent): self.description = f"Move {self.basename} to be a child of {self.basename} with ID of [int]. Use ID: 0 to move {self.basename} to the root." + if self.model.node_order_by: + node_location = 'sorted' + else: + node_location = 'last' try: child = self.model.objects.get(pk=pk, space=self.request.space) @@ -244,7 +253,7 @@ class TreeMixin(MergeMixin, FuzzyFilterMixin): if parent == 0: try: with scopes_disabled(): - child.move(self.model.get_first_root_node(), 'last-sibling') + child.move(self.model.get_first_root_node(), f'{node_location}-sibling') content = {'msg': _(f'{child.name} was moved successfully to the root.')} return Response(content, status=status.HTTP_200_OK) except (PathOverflow, InvalidMoveToDescendant, InvalidPosition): @@ -262,7 +271,7 @@ class TreeMixin(MergeMixin, FuzzyFilterMixin): try: with scopes_disabled(): - child.move(parent, 'last-child') + child.move(parent, f'{node_location}-child') content = {'msg': _(f'{child.name} was moved successfully to parent {parent.name}')} return Response(content, status=status.HTTP_200_OK) except (PathOverflow, InvalidMoveToDescendant, InvalidPosition): diff --git a/recipes/settings.py b/recipes/settings.py index bfd58dc4b..ccb23acb7 100644 --- a/recipes/settings.py +++ b/recipes/settings.py @@ -73,6 +73,7 @@ ACCOUNT_SIGNUP_FORM_CLASS = 'cookbook.forms.AllAuthSignupForm' TERMS_URL = os.getenv('TERMS_URL', '') PRIVACY_URL = os.getenv('PRIVACY_URL', '') IMPRINT_URL = os.getenv('IMPRINT_URL', '') +SORT_TREE_BY_NAME = os.getenv('IMPRINT_URL', 1) HOSTED = bool(int(os.getenv('HOSTED', False))) From 20c2c5cc40510210e3862404723bbe60a0368589 Mon Sep 17 00:00:00 2001 From: smilerz Date: Tue, 28 Sep 2021 11:41:58 -0500 Subject: [PATCH 3/6] maintain env config when admin sorting tree --- cookbook/admin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cookbook/admin.py b/cookbook/admin.py index e51741d7b..77fc06da7 100644 --- a/cookbook/admin.py +++ b/cookbook/admin.py @@ -91,10 +91,11 @@ admin.site.register(SyncLog, SyncLogAdmin) @admin.action(description='Fix problems and sort tree by name') def sort_tree(modeladmin, request, queryset): + orginal_value = modeladmin.model.node_order_by[:] modeladmin.model.node_order_by = ['name'] with scopes_disabled(): Keyword.fix_tree(fix_paths=True) - modeladmin.model.node_order_by = [] + modeladmin.model.node_order_by = orginal_value class KeywordAdmin(TreeAdmin): From 39255c93a5655ed35a469888df2e62c430fadb03 Mon Sep 17 00:00:00 2001 From: smilerz Date: Tue, 28 Sep 2021 12:51:49 -0500 Subject: [PATCH 4/6] allow admin to enable/disable sorting --- .env.template | 1 + cookbook/admin.py | 21 ++++++++++++++++++--- recipes/wsgi.py | 9 +++++++++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/.env.template b/.env.template index 9fb821922..bff198606 100644 --- a/.env.template +++ b/.env.template @@ -128,4 +128,5 @@ REVERSE_PROXY_AUTH=0 # however, when doing large imports of recipes that will create new objects, can increase total run time by 5-10x # Disabling SORT_TREE_BY_NAME (setting value to 0) will store objects unsorted, but will substantially increase speed of imports. # Keywords and Food can be manually sorted by name in Admin +# This value can also be temporarily changed in Admin, it will revert the next time the application is started # SORT_TREE_BY_NAME=0 \ No newline at end of file diff --git a/cookbook/admin.py b/cookbook/admin.py index 77fc06da7..f86232cb6 100644 --- a/cookbook/admin.py +++ b/cookbook/admin.py @@ -89,19 +89,34 @@ class SyncLogAdmin(admin.ModelAdmin): admin.site.register(SyncLog, SyncLogAdmin) +@admin.action(description='Temporarily ENABLE sorting on Foods and Keywords.') +def enable_tree_sorting(modeladmin, request, queryset): + Food.node_order_by = ['name'] + Keyword.node_order_by = ['name'] + with scopes_disabled(): + Food.fix_tree(fix_paths=True) + Keyword.fix_tree(fix_paths=True) + + +@admin.action(description='Temporarily DISABLE sorting on Foods and Keywords.') +def disable_tree_sorting(modeladmin, request, queryset): + Food.node_order_by = [] + Keyword.node_order_by = [] + + @admin.action(description='Fix problems and sort tree by name') def sort_tree(modeladmin, request, queryset): orginal_value = modeladmin.model.node_order_by[:] modeladmin.model.node_order_by = ['name'] with scopes_disabled(): - Keyword.fix_tree(fix_paths=True) + modeladmin.model.fix_tree(fix_paths=True) modeladmin.model.node_order_by = orginal_value class KeywordAdmin(TreeAdmin): form = movenodeform_factory(Keyword) ordering = ('space', 'path',) - actions = [sort_tree] + actions = [sort_tree, enable_tree_sorting, disable_tree_sorting] admin.site.register(Keyword, KeywordAdmin) @@ -148,7 +163,7 @@ admin.site.register(Unit) class FoodAdmin(TreeAdmin): form = movenodeform_factory(Keyword) ordering = ('space', 'path',) - actions = [sort_tree] + actions = [sort_tree, enable_tree_sorting, disable_tree_sorting] admin.site.register(Food, FoodAdmin) diff --git a/recipes/wsgi.py b/recipes/wsgi.py index a33e55a2b..474e7f413 100644 --- a/recipes/wsgi.py +++ b/recipes/wsgi.py @@ -10,9 +10,18 @@ https://docs.djangoproject.com/en/2.0/howto/deployment/wsgi/ import os from django.core.wsgi import get_wsgi_application +from django_scopes import scopes_disabled +from cookbook.models import Food, Keyword os.environ.setdefault("DJANGO_SETTINGS_MODULE", "recipes.settings") +# run fix tree here solves 2 problems: +# 1 if tree sorting is changed from unsorted to sorted ensures that objects are sorted +# 2 if any condition caused the tree to be in an inconsistent state this will fix most problems +with scopes_disabled(): + Food.fix_tree(fix_paths=True) + Keyword.fix_tree(fix_paths=True) + _application = get_wsgi_application() From b7b0b9c690df29f11366dcb7aeba6c2260351249 Mon Sep 17 00:00:00 2001 From: smilerz Date: Tue, 28 Sep 2021 15:45:54 -0500 Subject: [PATCH 5/6] fixed wsgi error --- cookbook/models.py | 19 ++++++++++++++++--- recipes/settings.py | 5 ++--- recipes/wsgi.py | 10 ---------- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/cookbook/models.py b/cookbook/models.py index 867d5fd80..68747e50d 100644 --- a/cookbook/models.py +++ b/cookbook/models.py @@ -43,8 +43,7 @@ class TreeManager(MP_NodeManager): try: return self.get(name__exact=kwargs['name'], space=kwargs['space']), False except self.model.DoesNotExist: - with scopes_disabled(): - return self.model.add_root(**kwargs), True + return self.model.add_root(**kwargs), True class TreeModel(MP_Node): @@ -353,6 +352,13 @@ class Keyword(ExportModelOperationsMixin('keyword'), TreeModel, PermissionModelM indexes = (Index(fields=['id', 'name']),) +# when starting up run fix_tree to: +# a) make sure that nodes are sorted when switching between sort modes +# b) fix problems, if any, with tree consistency +with scopes_disabled(): + Keyword.fix_tree(fix_paths=True) + + class Unit(ExportModelOperationsMixin('unit'), models.Model, PermissionModelMixin): name = models.CharField(max_length=128, validators=[MinLengthValidator(1)]) description = models.TextField(blank=True, null=True) @@ -400,6 +406,13 @@ class Food(ExportModelOperationsMixin('food'), TreeModel, PermissionModelMixin): ) +# when starting up run fix_tree to: +# a) make sure that nodes are sorted when switching between sort modes +# b) fix problems, if any, with tree consistency +with scopes_disabled(): + Food.fix_tree(fix_paths=True) + + class Ingredient(ExportModelOperationsMixin('ingredient'), models.Model, PermissionModelMixin): # a pre-delete signal on Food checks if the Ingredient is part of a Step, if it is raises a ProtectedError instead of cascading the delete food = models.ForeignKey(Food, on_delete=models.CASCADE, null=True, blank=True) @@ -793,7 +806,7 @@ class ViewLog(ExportModelOperationsMixin('view_log'), models.Model, PermissionMo class Meta(): indexes = ( Index(fields=['recipe']), - Index(fields=[ '-created_at']), + Index(fields=['-created_at']), Index(fields=['created_by']), Index(fields=['recipe', '-created_at', 'created_by']), ) diff --git a/recipes/settings.py b/recipes/settings.py index ccb23acb7..95ef48c10 100644 --- a/recipes/settings.py +++ b/recipes/settings.py @@ -73,7 +73,8 @@ ACCOUNT_SIGNUP_FORM_CLASS = 'cookbook.forms.AllAuthSignupForm' TERMS_URL = os.getenv('TERMS_URL', '') PRIVACY_URL = os.getenv('PRIVACY_URL', '') IMPRINT_URL = os.getenv('IMPRINT_URL', '') -SORT_TREE_BY_NAME = os.getenv('IMPRINT_URL', 1) +if SORT_TREE_BY_NAME:= bool(int(os.getenv('SQL_DEBUG', True))): + MIDDLEWARE += ('recipes.middleware.SqlPrintingMiddleware',) HOSTED = bool(int(os.getenv('HOSTED', False))) @@ -391,5 +392,3 @@ EMAIL_USE_SSL = bool(int(os.getenv('EMAIL_USE_SSL', False))) DEFAULT_FROM_EMAIL = os.getenv('DEFAULT_FROM_EMAIL', 'webmaster@localhost') ACCOUNT_EMAIL_SUBJECT_PREFIX = os.getenv('ACCOUNT_EMAIL_SUBJECT_PREFIX', '[Tandoor Recipes] ') # allauth sender prefix -if os.getenv('SQL_DEBUG', False): - MIDDLEWARE += ('recipes.middleware.SqlPrintingMiddleware',) diff --git a/recipes/wsgi.py b/recipes/wsgi.py index 474e7f413..64e997379 100644 --- a/recipes/wsgi.py +++ b/recipes/wsgi.py @@ -8,20 +8,10 @@ https://docs.djangoproject.com/en/2.0/howto/deployment/wsgi/ """ import os - from django.core.wsgi import get_wsgi_application -from django_scopes import scopes_disabled -from cookbook.models import Food, Keyword os.environ.setdefault("DJANGO_SETTINGS_MODULE", "recipes.settings") -# run fix tree here solves 2 problems: -# 1 if tree sorting is changed from unsorted to sorted ensures that objects are sorted -# 2 if any condition caused the tree to be in an inconsistent state this will fix most problems -with scopes_disabled(): - Food.fix_tree(fix_paths=True) - Keyword.fix_tree(fix_paths=True) - _application = get_wsgi_application() From 9bbbd72285db4961361835096b596e44ff7543e6 Mon Sep 17 00:00:00 2001 From: smilerz Date: Thu, 30 Sep 2021 16:17:08 -0500 Subject: [PATCH 6/6] catch IntegrityError caused by mis-sorted tree --- cookbook/models.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cookbook/models.py b/cookbook/models.py index 68747e50d..8d0e5a1ed 100644 --- a/cookbook/models.py +++ b/cookbook/models.py @@ -11,7 +11,7 @@ from django.contrib.postgres.indexes import GinIndex from django.contrib.postgres.search import SearchVectorField from django.core.files.uploadedfile import UploadedFile, InMemoryUploadedFile from django.core.validators import MinLengthValidator -from django.db import models +from django.db import models, IntegrityError from django.db.models import Index, ProtectedError from django.utils import timezone from django.utils.translation import gettext as _ @@ -43,7 +43,13 @@ class TreeManager(MP_NodeManager): try: return self.get(name__exact=kwargs['name'], space=kwargs['space']), False except self.model.DoesNotExist: - return self.model.add_root(**kwargs), True + with scopes_disabled(): + try: + return self.model.add_root(**kwargs), True + except IntegrityError as e: + if 'Key (path)' in e.args[0]: + self.model.fix_tree(fix_paths=True) + return self.model.add_root(**kwargs), True class TreeModel(MP_Node):