Merge pull request #4707 from FinnStutzenstein/fixTreeSorting

fix tree sorting

client/src/app/core/repositories/agenda/item-repository.service.ts

@@ -1,6 +1,6 @@
 import { Injectable } from '@angular/core';
 
-import { tap, map } from 'rxjs/operators';
+import { map } from 'rxjs/operators';
 import { Observable } from 'rxjs';
 import { TranslateService } from '@ngx-translate/core';
 
@@ -12,7 +12,7 @@ import { DataStoreService } from '../../core-services/data-store.service';
 import { HttpService } from 'app/core/core-services/http.service';
 import { Item } from 'app/shared/models/agenda/item';
 import { ViewItem } from 'app/site/agenda/models/view-item';
-import { TreeService, TreeIdNode } from 'app/core/ui-services/tree.service';
+import { TreeIdNode } from 'app/core/ui-services/tree.service';
 import { BaseAgendaViewModel } from 'app/site/base/base-agenda-view-model';
 import { ViewModelStoreService } from 'app/core/core-services/view-model-store.service';
 import { BaseViewModel } from 'app/site/base/base-view-model';
@@ -48,8 +48,7 @@ export class ItemRepositoryService extends BaseRepository<ViewItem, Item> {
         viewModelStoreService: ViewModelStoreService,
         translate: TranslateService,
         private httpService: HttpService,
-        private config: ConfigService,
+        private config: ConfigService
-        private treeService: TreeService
     ) {
         super(DS, dataSend, mapperService, viewModelStoreService, translate, Item, [
             Topic,
@@ -152,29 +151,6 @@ export class ItemRepositoryService extends BaseRepository<ViewItem, Item> {
         await this.httpService.post('/rest/agenda/item/sort/', data);
     }
 
-    /**
-     * Add custom hook into the observables. The ViewItems get a virtual agendaListWeight (a sequential number)
-     * for the agenda topic order, and a virtual level for the hierarchy in the agenda list tree. Both values can be used
-     * for sorting and ordering instead of dealing with the sort parent id and weight.
-     *
-     * @override
-     */
-    public getViewModelListObservable(): Observable<ViewItem[]> {
-        return super.getViewModelListObservable().pipe(
-            tap(items => {
-                const iterator = this.treeService.traverseItems(items, 'weight', 'parent_id');
-                let m: IteratorResult<ViewItem>;
-                let virtualWeightCounter = 0;
-                while (!(m = iterator.next()).done) {
-                    m.value.agendaListWeight = virtualWeightCounter++;
-                    m.value.agendaListLevel = m.value.parent_id
-                        ? this.getViewModel(m.value.parent_id).agendaListLevel + 1
-                        : 0;
-                }
-            })
-        );
-    }
-
     /**
      * Calculates the estimated end time based on the configured start and the
      * sum of durations of all agenda items

client/src/app/core/repositories/motions/motion-repository.service.ts

@@ -3,13 +3,12 @@ import { DomSanitizer, SafeHtml } from '@angular/platform-browser';
 
 import { TranslateService } from '@ngx-translate/core';
 import { Observable } from 'rxjs';
-import { tap, map } from 'rxjs/operators';
+import { map } from 'rxjs/operators';
 
 import { Category } from 'app/shared/models/motions/category';
 import { ChangeRecoMode, ViewMotion } from 'app/site/motions/models/view-motion';
 import { CollectionStringMapperService } from '../../core-services/collection-string-mapper.service';
 import { ConfigService } from 'app/core/ui-services/config.service';
-
 import { DataSendService } from '../../core-services/data-send.service';
 import { DataStoreService } from '../../core-services/data-store.service';
 import { DiffLinesInParagraph, DiffService, LineRange, ModificationType } from '../../ui-services/diff.service';
@@ -21,7 +20,7 @@ import { Motion } from 'app/shared/models/motions/motion';
 import { MotionBlock } from 'app/shared/models/motions/motion-block';
 import { MotionChangeRecommendation } from 'app/shared/models/motions/motion-change-reco';
 import { MotionPoll } from 'app/shared/models/motions/motion-poll';
-import { TreeService, TreeIdNode } from 'app/core/ui-services/tree.service';
+import { TreeIdNode } from 'app/core/ui-services/tree.service';
 import { User } from 'app/shared/models/users/user';
 import { ViewMotionChangeRecommendation } from 'app/site/motions/models/view-change-recommendation';
 import { ViewMotionAmendedParagraph } from 'app/site/motions/models/view-motion-amended-paragraph';
@@ -44,7 +43,7 @@ import { ViewPersonalNote } from 'app/site/users/models/view-personal-note';
 import { OperatorService } from 'app/core/core-services/operator.service';
 import { CollectionIds } from 'app/core/core-services/data-store-update-manager.service';
 
-type SortProperty = 'callListWeight' | 'identifier';
+type SortProperty = 'weight' | 'identifier';
 
 /**
  * Describes the single paragraphs from the base motion.
@@ -122,7 +121,6 @@ export class MotionRepositoryService extends BaseAgendaContentObjectRepository<V
         private readonly sanitizer: DomSanitizer,
         private readonly lineNumbering: LinenumberingService,
         private readonly diff: DiffService,
-        private treeService: TreeService,
         private operator: OperatorService
     ) {
         super(DS, dataSend, mapperService, viewModelStoreService, translate, Motion, [
@@ -306,25 +304,6 @@ export class MotionRepositoryService extends BaseAgendaContentObjectRepository<V
         }
     }
 
-    /**
-     * Add custom hook into the observables. The motions get a virtual weight (a sequential number) for the
-     * call list order. One can just sort for this number instead of dealing with the sort parent id and weight.
-     *
-     * @override
-     */
-    public getViewModelListObservable(): Observable<ViewMotion[]> {
-        return super.getViewModelListObservable().pipe(
-            tap(motions => {
-                const iterator = this.treeService.traverseItems(motions, 'weight', 'sort_parent_id');
-                let m: IteratorResult<ViewMotion>;
-                let virtualWeightCounter = 0;
-                while (!(m = iterator.next()).done) {
-                    m.value.callListWeight = virtualWeightCounter++;
-                }
-            })
-        );
-    }
-
     /**
      * Set the state of a motion
      *
@@ -962,9 +941,9 @@ export class MotionRepositoryService extends BaseAgendaContentObjectRepository<V
                 if (a[this.sortProperty] === b[this.sortProperty]) {
                     return this.languageCollator.compare(a.title, b.title);
                 } else {
-                    if (this.sortProperty === 'callListWeight') {
+                    if (this.sortProperty === 'weight') {
                         // handling numerical values
-                        return a.callListWeight - b.callListWeight;
+                        return a.weight - b.weight;
                     } else {
                         return this.languageCollator.compare(a[this.sortProperty], b[this.sortProperty]);
                     }

client/src/app/core/ui-services/tree.service.ts

@@ -179,25 +179,6 @@ export class TreeService {
         return getChildren(parentItems);
     }
 
-    /**
-     * Traverses the given tree in pre order.
-     *
-     * @param tree The tree to traverse
-     * @returns An iterator for all items in the right order.
-     */
-    public *traverseTree<T>(tree: OSTreeNode<T>[]): Iterator<T> {
-        const nodesToVisit = tree.reverse();
-        while (nodesToVisit.length > 0) {
-            const node = nodesToVisit.pop();
-            if (node.children) {
-                node.children.reverse().forEach(n => {
-                    nodesToVisit.push(n);
-                });
-            }
-            yield node.item;
-        }
-    }
-
     /**
      * Removes `item` from the tree.
      *
@@ -217,25 +198,6 @@ export class TreeService {
         });
     }
 
-    /**
-     * Traverses items in pre-order givem (implicit) by the weight and parentId.
-     *
-     * Just builds the tree with `makeTree` and get the iterator from `traverseTree`.
-     *
-     * @param items All items to traverse
-     * @param weightKey The key giving access to the weight property
-     * @param parentIdKey The key giving access to the parentId property
-     * @returns An iterator for all items in the right order.
-     */
-    public traverseItems<T extends Identifiable & Displayable>(
-        items: T[],
-        weightKey: keyof T,
-        parentIdKey: keyof T
-    ): Iterator<T> {
-        const tree = this.makeTree(items, weightKey, parentIdKey);
-        return this.traverseTree(tree);
-    }
-
     /**
      * Reduce a list of items to nodes independent from each other in a given
      * branch of a tree

client/src/app/shared/models/agenda/item.ts

@@ -40,6 +40,7 @@ export class Item extends BaseModel<Item> {
     public content_object: ContentObject;
     public weight: number;
     public parent_id: number;
+    public level: number;
 
     public constructor(input?: any) {
         super(Item.COLLECTIONSTRING, input);

client/src/app/site/agenda/components/agenda-list/agenda-list.component.html

@@ -43,7 +43,7 @@
         <ng-container matColumnDef="title">
             <mat-header-cell *matHeaderCellDef mat-sort-header>Topic</mat-header-cell>
             <mat-cell *matCellDef="let item">
-                <div [ngStyle]="{ 'margin-left': item.agendaListLevel * 25 + 'px' }">
+                <div [ngStyle]="{ 'margin-left': item.level * 25 + 'px' }">
                     <span *ngIf="item.closed"> <mat-icon class="done-check">check</mat-icon> </span>
                     <span class="table-view-list-title">{{ item.getListTitle() }}</span>
                 </div>

client/src/app/site/agenda/components/agenda-list/agenda-list.component.ts

@@ -127,7 +127,7 @@ export class AgendaListComponent extends ListViewBaseComponent<ViewItem, Item> i
 
     protected onFilter(): void {
         this.filterService.filter().subscribe(newAgendaItems => {
-            newAgendaItems.sort((a, b) => a.agendaListWeight - b.agendaListWeight);
+            newAgendaItems.sort((a, b) => a.weight - b.weight);
             this.dataSource.data = newAgendaItems;
             this.checkSelection();
         });

client/src/app/site/agenda/components/agenda-sort/agenda-sort.component.ts

@@ -70,7 +70,7 @@ export class AgendaSortComponent extends BaseViewComponent implements CanCompone
     public seenNodes: [number, number] = [0, 0];
 
     /**
-     * All agendaItems sorted by their virtual weight {@link ViewItem.agendaListWeight}
+     * All agendaItems sorted by their weight {@link ViewItem.weight}
      */
     public itemsObservable: Observable<ViewItem[]>;
 

client/src/app/site/agenda/models/view-item.ts

@@ -10,19 +10,6 @@ export class ViewItem extends BaseViewModel {
     private _item: Item;
     private _contentObject: BaseAgendaViewModel;
 
-    /**
-     * virtual weight defined by the order in the agenda tree, representing a shortcut to sorting by
-     * weight, parent_id and the parents' weight(s)
-     * TODO will be accurate if the viewMotion is observed via {@link getSortedViewModelListObservable}, else, it will be undefined
-     */
-    public agendaListWeight: number;
-
-    /**
-     * The amount of parents in the agenda list tree.
-     * TODO will be accurate if the viewMotion is observed via {@link getSortedViewModelListObservable}, else, it will be undefined
-     */
-    public agendaListLevel: number;
-
     public get item(): Item {
         return this._item;
     }
@@ -66,6 +53,10 @@ export class ViewItem extends BaseViewModel {
         return this.item.comment;
     }
 
+    public get level(): number {
+        return this.item.level;
+    }
+
     /**
      * Gets the string representation of the item type
      * @returns The visibility for this item, as defined in {@link itemVisibilityChoices}

client/src/app/site/motions/models/view-motion.ts

@@ -65,12 +65,6 @@ export class ViewMotion extends BaseAgendaViewModel implements Searchable {
     protected _changeRecommendations: ViewMotionChangeRecommendation[];
     public personalNote?: PersonalNoteContent;
 
-    /**
-     * Is set by the repository; this is the order of the flat call list given by
-     * the properties weight and sort_parent_id
-     */
-    public callListWeight: number;
-
     public get motion(): Motion {
         return this._motion;
     }

client/src/app/site/motions/modules/call-list/call-list.component.ts

@@ -68,7 +68,7 @@ export class CallListComponent extends BaseViewComponent implements CanComponent
         this.motionsObservable = this.motionRepo.getViewModelListObservable();
         this.motionsObservable.subscribe(motions => {
             // Sort motions and make a copy, so it will stay sorted.
-            this.motions = motions.map(x => x).sort((a, b) => a.callListWeight - b.callListWeight);
+            this.motions = motions.map(x => x).sort((a, b) => a.weight - b.weight);
         });
     }
 

client/src/app/site/motions/services/motion-csv-export.service.ts

@@ -123,7 +123,7 @@ export class MotionCsvExportService {
     /**
      * Exports the call list.
      *
-     * @param motions All motions in the CSV. They should be ordered by callListWeight correctly.
+     * @param motions All motions in the CSV. They should be ordered by weight correctly.
      */
     public exportCallList(motions: ViewMotion[]): void {
         this.csvExport.export(

client/src/app/site/motions/services/motion-pdf.service.ts

@@ -581,7 +581,7 @@ export class MotionPdfService {
      * @returns definitions ready to be opened or exported via {@link PdfDocumentService}
      */
     public callListToDoc(motions: ViewMotion[]): object {
-        motions.sort((a, b) => a.callListWeight - b.callListWeight);
+        motions.sort((a, b) => a.weight - b.weight);
         const title = {
             text: this.translate.instant('Call list'),
             style: 'title'

client/src/app/site/motions/services/motion-sort-list.service.ts

@@ -12,10 +12,10 @@ import { _ } from 'app/core/translate/translation-marker';
 })
 export class MotionSortListService extends BaseSortListService<ViewMotion> {
     public sortOptions: OsSortingDefinition<ViewMotion> = {
-        sortProperty: 'callListWeight',
+        sortProperty: 'weight',
         sortAscending: true,
         options: [
-            { property: 'callListWeight', label: 'Call list' },
+            { property: 'weight', label: 'Call list' },
             { property: 'identifier' },
             { property: 'title' },
             { property: 'submitters' },

openslides/agenda/models.py

@@ -318,6 +318,20 @@ class Item(RESTModelMixin, models.Model):
             self.parent is not None and self.parent.is_hidden()
         )
 
+    @property
+    def level(self):
+        """
+        Returns the level in agenda (=tree of all items). Level 0 means this
+        item is a root item in the agenda. Level 1 indicates that the parent is
+        a root item, level 2 that the parent's parent is a root item and so on.
+
+        Attention! This executes one query for each ancestor of the item.
+        """
+        if self.parent is None:
+            return 0
+        else:
+            return self.parent.level + 1
+
     def get_next_speaker(self):
         """
         Returns the speaker object of the speaker who is next.

openslides/agenda/serializers.py

@@ -61,4 +61,5 @@ class ItemSerializer(ModelSerializer):
             "content_object",
             "weight",
             "parent",
+            "level",
         )

openslides/agenda/views.py

@@ -68,6 +68,8 @@ class ItemViewSet(
     def update(self, *args, **kwargs):
         """
         Customized view endpoint to update all children if the item type has changed.
+        We do not check the level (affected by changing the parent) in fact that this
+        change is currentl only done via the sort view.
         """
         old_type = self.get_object().type
 

openslides/utils/views.py

@@ -46,8 +46,8 @@ class TreeSortMixin:
         self, request: Any, model: models.Model, weight_key: str, parent_id_key: str
     ) -> None:
         """
-        Sorts the all model objects represented in a tree of ids. The request data should be a list (the root)
+        Sorts the all model objects represented in a tree of ids. The request data should
-        of all main agenda items. Each node is a dict with an id and optional children:
+        be a list (the root) of all main models. Each node is a dict with an id and optional children:
         {
             id: <the id>
             children: [
@@ -55,56 +55,62 @@ class TreeSortMixin:
             ]
         }
         Every id has to be given.
+
+        This function traverses this tree in preorder to assign the weight. So even if a client
+        does not have every model, the remaining models are sorted correctly.
         """
         if not isinstance(request.data, list):
             raise ValidationError("The data must be a list.")
 
         # get all item ids to verify, that the user send all ids.
-        all_item_ids = set(model.objects.all().values_list("pk", flat=True))
+        all_model_ids = set(model.objects.all().values_list("pk", flat=True))
+
+        ids_found: Set[int] = set()  # Set to save all found ids.
+
+        fake_root: Dict[str, Any] = {"id": None, "children": []}
+        fake_root["children"].extend(
+            request.data
+        )  # this will prevent mutating the request data.
 
         # The stack where all nodes to check are saved. Invariant: Each node
         # must be a dict with an id, a parent id (may be None for the root
         # layer) and a weight.
-        nodes_to_check = []
+        nodes_to_check = [fake_root]
-        ids_found: Set[int] = set()  # Set to save all found ids.
-        # Insert all root nodes.
-        for index, node in enumerate(request.data):
-            if not isinstance(node, dict) or not isinstance(node.get("id"), int):
-                raise ValidationError("node must be a dict with an id as integer")
-            node[parent_id_key] = None
-            node[weight_key] = index
-            nodes_to_check.append(node)
-
         # Traverse and check, if every id is given, valid and there are no duplicate ids.
+        weight = 1
         while len(nodes_to_check) > 0:
             node = nodes_to_check.pop()
             id = node["id"]
 
-            if id in ids_found:
+            if id is not None:  # exclude the fake_root
-                raise ValidationError(f"Duplicate id: {id}")
+                node[weight_key] = weight
-            if id not in all_item_ids:
+                weight += 1
-                raise ValidationError(f"Id does not exist: {id}")
+                if id in ids_found:
+                    raise ValidationError(f"Duplicate id: {id}")
+                if id not in all_model_ids:
+                    raise ValidationError(f"Id does not exist: {id}")
+                ids_found.add(id)
 
-            ids_found.add(id)
             # Add children, if exist.
             if isinstance(node.get("children"), list):
-                for index, child in enumerate(node["children"]):
+                node["children"].reverse()
+                for child in node["children"]:
                     # ensure invariant for nodes_to_check
-                    if not isinstance(node, dict) or not isinstance(
+                    if not isinstance(child, dict) or not isinstance(
-                        node.get("id"), int
+                        child.get("id"), int
                     ):
                         raise ValidationError(
-                            "node must be a dict with an id as integer"
+                            "child must be a dict with an id as integer"
                         )
                     child[parent_id_key] = id
-                    child[weight_key] = index
                     nodes_to_check.append(child)
 
-        if len(all_item_ids) != len(ids_found):
+        if len(all_model_ids) != len(ids_found):
             raise ValidationError(
-                f"Did not recieved {len(all_item_ids)} ids, got {len(ids_found)}."
+                f"Did not recieved {len(all_model_ids)} ids, got {len(ids_found)}."
             )
 
+        # Do the actual update:
         nodes_to_update = []
         nodes_to_update.extend(
             request.data