Merge pull request #4734 from NyanKiyoshi/unify/sorting/menu-items

Unified MenuItemMove to other reordering mutations

CHANGELOG.md

@@ -72,6 +72,7 @@ All notable, unreleased changes to this project will be documented in this file.
 - Add error codes to mutations responses - #4676 by @Kwaidan00
 - Payment gateways are now saleor plugins with dynamic configuration - #4669 by @salwator
 
+- Unified MenuItemMove to other reordering mutations. It now uses relative positions instead of absolute ones (breaking change) - #4734 by @NyanKiyoshi.
 ## 2.8.0
 
 ### Core

saleor/graphql/core/utils/reordering.py

@@ -1,4 +1,3 @@
-import warnings
 from collections import OrderedDict
 from dataclasses import dataclass
 from typing import Dict, Tuple
@@ -90,11 +89,9 @@ def process_move_operation(self, pk, move):
 
         # Skip if noting to do
         if move == 0:
-            warnings.warn(
-                f"Ignored node's reordering, did not find: {pk} "
-                f"(from {self.qs.model.__name__})"
-            )
             return
+        if move is None:
+            move = +1
 
         node_pos, target_pos, new_sort_order = self.calculate_new_sort_order(pk, move)
 

saleor/graphql/menu/mutations.py

@@ -1,10 +1,11 @@
-from collections import namedtuple
-from typing import List
+from collections import defaultdict
+from dataclasses import dataclass
+from typing import Dict, List, Optional
 
 import graphene
 from django.core.exceptions import ValidationError
-from django.db.models import Model
-from graphql_relay import from_global_id
+from django.db import transaction
+from django.db.models import Model, QuerySet
 
 from ...menu import models
 from ...menu.error_codes import MenuErrorCode
@@ -13,6 +14,8 @@
 from ...product import models as product_models
 from ..core.mutations import BaseMutation, ModelDeleteMutation, ModelMutation
 from ..core.types.common import MenuError
+from ..core.utils import from_global_id_strict_type
+from ..core.utils.reordering import perform_reordering
 from ..page.types import Page
 from ..product.types import Category, Collection
 from .enums import NavigationType
@@ -253,9 +256,12 @@ def perform_mutation(cls, _root, info, **data):
         return response
 
 
-_MenuMoveOperation = namedtuple(
-    "_MenuMoveOperation", ("menu_item", "parent", "sort_order")
-)
+@dataclass(frozen=True)
+class _MenuMoveOperation:
+    menu_item: models.MenuItem
+    parent_changed: bool
+    new_parent: Optional[models.MenuItem]
+    sort_order: int
 
 
 class MenuItemMove(BaseMutation):
@@ -274,13 +280,13 @@ class Meta:
         error_type_field = "menu_errors"
 
     @staticmethod
-    def clean_move(move):
+    def clean_move(move: MenuItemMoveInput):
         """Validate if the given move could be possibly possible."""
         if move.parent_id:
             if move.item_id == move.parent_id:
                 raise ValidationError(
                     {
-                        "parent": ValidationError(
+                        "parent_id": ValidationError(
                             "Cannot assign a node to itself.",
                             code=MenuErrorCode.CANNOT_ASSIGN_NODE,
                         )
@@ -291,11 +297,11 @@ def clean_move(move):
     def clean_operation(operation: _MenuMoveOperation):
         """Validate if the given move will be actually possible."""
 
-        if operation.parent:
-            if operation.menu_item.is_ancestor_of(operation.parent):
+        if operation.new_parent is not None:
+            if operation.menu_item.is_ancestor_of(operation.new_parent):
                 raise ValidationError(
                     {
-                        "parent": ValidationError(
+                        "parent_id": ValidationError(
                             (
                                 "Cannot assign a node as child of "
                                 "one of its descendants."
@@ -306,66 +312,87 @@ def clean_operation(operation: _MenuMoveOperation):
                 )
 
     @classmethod
-    def get_operation(cls, info, menu_id: int, move) -> _MenuMoveOperation:
-        parent_node = None
-
-        _type, menu_item_id = from_global_id(move.item_id)  # type: str, int
-        assert _type == "MenuItem", "The menu item node must be of type MenuItem."
-
-        menu_item = models.MenuItem.objects.get(pk=menu_item_id, menu_id=menu_id)
+    def get_operation(
+        cls, info, menu: models.Menu, move: MenuItemMoveInput
+    ) -> _MenuMoveOperation:
+        menu_item = cls.get_node_or_error(
+            info, move.item_id, field="item", only_type="MenuItem", qs=menu.items
+        )
+        new_parent, parent_changed = None, False
 
         if move.parent_id is not None:
-            parent_node = cls.get_node_or_error(
-                info, move.parent_id, field="parent_id", only_type=MenuItem
+            parent_pk = from_global_id_strict_type(
+                move.parent_id, only_type=MenuItem, field="parent_id"
             )
+            if int(parent_pk) != menu_item.parent_id:
+                new_parent = cls.get_node_or_error(
+                    info,
+                    move.parent_id,
+                    field="parent_id",
+                    only_type=MenuItem,
+                    qs=menu.items,
+                )
+                parent_changed = True
+        elif move.parent_id is None and menu_item.parent_id is not None:
+            parent_changed = True
 
         return _MenuMoveOperation(
-            menu_item=menu_item, parent=parent_node, sort_order=move.sort_order
+            menu_item=menu_item,
+            new_parent=new_parent,
+            parent_changed=parent_changed,
+            sort_order=move.sort_order,
         )
 
     @classmethod
     def clean_moves(
-        cls, info, menu_id: int, move_operations: List
+        cls, info, menu: models.Menu, move_operations: List[MenuItemMoveInput]
     ) -> List[_MenuMoveOperation]:
 
         operations = []
         for move in move_operations:
             cls.clean_move(move)
-            operation = cls.get_operation(info, menu_id, move)
+            operation = cls.get_operation(info, menu, move)
             cls.clean_operation(operation)
             operations.append(operation)
         return operations
 
     @staticmethod
-    def perform_operation(operation: _MenuMoveOperation):
+    def perform_change_parent_operation(operation: _MenuMoveOperation):
         menu_item = operation.menu_item  # type: models.MenuItem
 
-        # Move the parent if provided
-        if operation.parent:
-            menu_item.move_to(operation.parent)
-        # Remove the menu item's parent if was set to none (root node)
-        elif menu_item.parent_id:
-            menu_item.parent_id = None
-
-        # Move the menu item
-        if operation.sort_order is not None:
-            menu_item.sort_order = operation.sort_order
+        if not operation.parent_changed:
+            return
 
+        # Move the parent
+        menu_item.move_to(operation.new_parent)
+        menu_item.sort_order = None
         menu_item.save()
 
     @classmethod
-    def perform_mutation(cls, _root, info, menu, moves):
-        _type, menu_id = from_global_id(menu)  # type: str, int
-        assert _type == "Menu", "Expected a menu of type Menu"
+    @transaction.atomic
+    def perform_mutation(cls, _root, info, menu: str, moves):
+        qs = models.Menu.objects.prefetch_related("items")
+        menu = cls.get_node_or_error(info, menu, only_type=Menu, field="menu", qs=qs)
 
-        operations = cls.clean_moves(info, menu_id, moves)
+        operations = cls.clean_moves(info, menu, moves)
+        sort_operations: Dict[Optional[int], Dict[int, int]] = defaultdict(dict)
+        sort_querysets: Dict[Optional[int], QuerySet] = {}
 
         for operation in operations:
-            cls.perform_operation(operation)
+            cls.perform_change_parent_operation(operation)
+
+            menu_item = operation.menu_item
+            parent_pk = operation.menu_item.parent_id
+
+            sort_operations[parent_pk][menu_item.pk] = operation.sort_order
+            sort_querysets[parent_pk] = menu_item.get_ordering_queryset()
+
+        for parent_pk, operations in sort_operations.items():
+            ordering_qs = sort_querysets[parent_pk]
+            perform_reordering(ordering_qs, operations)
 
-        menu = models.Menu.objects.get(pk=menu_id)
-        update_menu(menu)
-        return cls(menu=menu)
+        menu = qs.get(pk=menu.pk)
+        return MenuItemMove(menu=menu)
 
 
 class AssignNavigation(BaseMutation):

saleor/graphql/menu/types.py

@@ -54,6 +54,15 @@ class MenuItem(CountableDjangoObjectType):
         resolver=resolve_translation,
     )
 
+    sort_order = graphene.Field(
+        graphene.Int,
+        deprecation_reason=(
+            "Will be dropped in 2.10 and is deprecated since 2.9: "
+            "use the position in list instead and relative "
+            "calculus to determine shift position."
+        ),
+    )
+
     class Meta:
         description = """Represents a single item of the related menu.
         Can store categories, collection or pages."""

saleor/graphql/schema.graphql

@@ -1840,7 +1840,7 @@ input MenuInput {
 
 type MenuItem implements Node {
   id: ID!
-  sortOrder: Int
+  sortOrder: Int @deprecated(reason: "Will be dropped in 2.10 and is deprecated since 2.9: use the position in list instead and relative calculus to determine shift position.")
   menu: Menu!
   name: String!
   parent: MenuItem

saleor/graphql/views.py

@@ -202,8 +202,13 @@ def format_error(error):
 
         if settings.DEBUG:
             lines = []
-            for line in traceback.format_exception(type(exc), exc, exc.__traceback__):
-                lines.extend(line.rstrip().splitlines())
+
+            if isinstance(exc, BaseException):
+                for line in traceback.format_exception(
+                    type(exc), exc, exc.__traceback__
+                ):
+                    lines.extend(line.rstrip().splitlines())
+
             result["extensions"] = {
                 "exception": {"code": type(exc).__name__, "stacktrace ": lines}
             }

tests/api/test_core_reordering.py

@@ -1,5 +1,3 @@
-from unittest.mock import patch
-
 import pytest
 
 from saleor.graphql.core.utils.reordering import perform_reordering
@@ -213,8 +211,7 @@ def test_reordering_null_sort_orders(dummy_attribute):
     assert actual == expected
 
 
-@patch("saleor.graphql.core.utils.reordering.warnings.warn")
-def test_reordering_nothing(mocked_warn, sorted_entries_seq, django_assert_num_queries):
+def test_reordering_nothing(sorted_entries_seq, django_assert_num_queries):
     """
     Ensures giving operations that does nothing, are skipped. Thus only one query should
     have been made: fetching the nodes.
@@ -226,9 +223,6 @@ def test_reordering_nothing(mocked_warn, sorted_entries_seq, django_assert_num_q
     with django_assert_num_queries(1) as ctx:
         perform_reordering(qs, operations)
 
-    mocked_warn.assert_called_with(
-        f"Ignored node's reordering, did not find: {pk} (from AttributeValue)"
-    )
     assert ctx[0]["sql"].startswith("SELECT "), "Should only have done a SELECT"