From 10c329da8d53bf9d118b1921cd182276d53a3a0f Mon Sep 17 00:00:00 2001 From: FinnStutzenstein Date: Mon, 13 May 2019 15:50:27 +0200 Subject: [PATCH] fix tree sorting Assigns the weight in the preorder traversal of the tree. Now one without every object (e.g. missing motions/items) still have the correct sorting. Intorduces the level attribute of items giving the amount of parents in the agenda. This allows to reduce complexits in the client. --- .../agenda/item-repository.service.ts | 30 +--------- .../motions/motion-repository.service.ts | 31 ++-------- .../src/app/core/ui-services/tree.service.ts | 38 ------------- client/src/app/shared/models/agenda/item.ts | 1 + .../agenda-list/agenda-list.component.html | 2 +- .../agenda-list/agenda-list.component.ts | 2 +- .../agenda-sort/agenda-sort.component.ts | 2 +- .../src/app/site/agenda/models/view-item.ts | 17 ++---- .../app/site/motions/models/view-motion.ts | 6 -- .../modules/call-list/call-list.component.ts | 2 +- .../services/motion-csv-export.service.ts | 2 +- .../motions/services/motion-pdf.service.ts | 2 +- .../services/motion-sort-list.service.ts | 4 +- openslides/agenda/models.py | 14 +++++ openslides/agenda/serializers.py | 1 + openslides/agenda/views.py | 2 + openslides/utils/views.py | 56 ++++++++++--------- 17 files changed, 69 insertions(+), 143 deletions(-) diff --git a/client/src/app/core/repositories/agenda/item-repository.service.ts b/client/src/app/core/repositories/agenda/item-repository.service.ts index c50a8e311..019086110 100644 --- a/client/src/app/core/repositories/agenda/item-repository.service.ts +++ b/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 { viewModelStoreService: ViewModelStoreService, translate: TranslateService, private httpService: HttpService, - private config: ConfigService, - private treeService: TreeService + private config: ConfigService ) { super(DS, dataSend, mapperService, viewModelStoreService, translate, Item, [ Topic, @@ -152,29 +151,6 @@ export class ItemRepositoryService extends BaseRepository { 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 { - return super.getViewModelListObservable().pipe( - tap(items => { - const iterator = this.treeService.traverseItems(items, 'weight', 'parent_id'); - let m: IteratorResult; - 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 diff --git a/client/src/app/core/repositories/motions/motion-repository.service.ts b/client/src/app/core/repositories/motions/motion-repository.service.ts index 4f0059960..c8ec52dc5 100644 --- a/client/src/app/core/repositories/motions/motion-repository.service.ts +++ b/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 { - return super.getViewModelListObservable().pipe( - tap(motions => { - const iterator = this.treeService.traverseItems(motions, 'weight', 'sort_parent_id'); - let m: IteratorResult; - let virtualWeightCounter = 0; - while (!(m = iterator.next()).done) { - m.value.callListWeight = virtualWeightCounter++; - } - }) - ); - } - /** * Set the state of a motion * @@ -934,9 +913,9 @@ export class MotionRepositoryService extends BaseAgendaContentObjectRepository(tree: OSTreeNode[]): Iterator { - 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( - items: T[], - weightKey: keyof T, - parentIdKey: keyof T - ): Iterator { - 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 diff --git a/client/src/app/shared/models/agenda/item.ts b/client/src/app/shared/models/agenda/item.ts index 5f1f8a8e4..660ea37fc 100644 --- a/client/src/app/shared/models/agenda/item.ts +++ b/client/src/app/shared/models/agenda/item.ts @@ -40,6 +40,7 @@ export class Item extends BaseModel { public content_object: ContentObject; public weight: number; public parent_id: number; + public level: number; public constructor(input?: any) { super(Item.COLLECTIONSTRING, input); diff --git a/client/src/app/site/agenda/components/agenda-list/agenda-list.component.html b/client/src/app/site/agenda/components/agenda-list/agenda-list.component.html index 162df2237..f91861a40 100644 --- a/client/src/app/site/agenda/components/agenda-list/agenda-list.component.html +++ b/client/src/app/site/agenda/components/agenda-list/agenda-list.component.html @@ -43,7 +43,7 @@ Topic -
+
check {{ item.getListTitle() }}
diff --git a/client/src/app/site/agenda/components/agenda-list/agenda-list.component.ts b/client/src/app/site/agenda/components/agenda-list/agenda-list.component.ts index b42c5c3b9..947c33eeb 100644 --- a/client/src/app/site/agenda/components/agenda-list/agenda-list.component.ts +++ b/client/src/app/site/agenda/components/agenda-list/agenda-list.component.ts @@ -127,7 +127,7 @@ export class AgendaListComponent extends ListViewBaseComponent 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(); }); diff --git a/client/src/app/site/agenda/components/agenda-sort/agenda-sort.component.ts b/client/src/app/site/agenda/components/agenda-sort/agenda-sort.component.ts index 8e53d657b..0a76246b2 100644 --- a/client/src/app/site/agenda/components/agenda-sort/agenda-sort.component.ts +++ b/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; diff --git a/client/src/app/site/agenda/models/view-item.ts b/client/src/app/site/agenda/models/view-item.ts index dc35acf14..1e512ee1e 100644 --- a/client/src/app/site/agenda/models/view-item.ts +++ b/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} diff --git a/client/src/app/site/motions/models/view-motion.ts b/client/src/app/site/motions/models/view-motion.ts index 4cbe0d779..d3536b3ce 100644 --- a/client/src/app/site/motions/models/view-motion.ts +++ b/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; } diff --git a/client/src/app/site/motions/modules/call-list/call-list.component.ts b/client/src/app/site/motions/modules/call-list/call-list.component.ts index 70d71909f..1690fbc21 100644 --- a/client/src/app/site/motions/modules/call-list/call-list.component.ts +++ b/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); }); } diff --git a/client/src/app/site/motions/services/motion-csv-export.service.ts b/client/src/app/site/motions/services/motion-csv-export.service.ts index a13dd2dac..d5600ba33 100644 --- a/client/src/app/site/motions/services/motion-csv-export.service.ts +++ b/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( diff --git a/client/src/app/site/motions/services/motion-pdf.service.ts b/client/src/app/site/motions/services/motion-pdf.service.ts index 422b978f2..68be9bde7 100644 --- a/client/src/app/site/motions/services/motion-pdf.service.ts +++ b/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' diff --git a/client/src/app/site/motions/services/motion-sort-list.service.ts b/client/src/app/site/motions/services/motion-sort-list.service.ts index c65c6f80d..49cd06e69 100644 --- a/client/src/app/site/motions/services/motion-sort-list.service.ts +++ b/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 { public sortOptions: OsSortingDefinition = { - sortProperty: 'callListWeight', + sortProperty: 'weight', sortAscending: true, options: [ - { property: 'callListWeight', label: 'Call list' }, + { property: 'weight', label: 'Call list' }, { property: 'identifier' }, { property: 'title' }, { property: 'submitters' }, diff --git a/openslides/agenda/models.py b/openslides/agenda/models.py index 1e04febe6..2e8e1012d 100644 --- a/openslides/agenda/models.py +++ b/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. diff --git a/openslides/agenda/serializers.py b/openslides/agenda/serializers.py index 3f12ae52e..fa26479bb 100644 --- a/openslides/agenda/serializers.py +++ b/openslides/agenda/serializers.py @@ -61,4 +61,5 @@ class ItemSerializer(ModelSerializer): "content_object", "weight", "parent", + "level", ) diff --git a/openslides/agenda/views.py b/openslides/agenda/views.py index c1a3940ff..b169c96ad 100644 --- a/openslides/agenda/views.py +++ b/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 diff --git a/openslides/utils/views.py b/openslides/utils/views.py index c18002f44..80f7d8ea8 100644 --- a/openslides/utils/views.py +++ b/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) - of all main agenda items. Each node is a dict with an id and optional children: + Sorts the all model objects represented in a tree of ids. The request data should + be a list (the root) of all main models. Each node is a dict with an id and optional children: { 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 = [] - 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) - + nodes_to_check = [fake_root] # 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: - raise ValidationError(f"Duplicate id: {id}") - if id not in all_item_ids: - raise ValidationError(f"Id does not exist: {id}") + if id is not None: # exclude the fake_root + node[weight_key] = weight + weight += 1 + 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( - node.get("id"), int + if not isinstance(child, dict) or not isinstance( + 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