diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 18667bca6217..a7f725a3b929 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -17,6 +17,7 @@ * [*] Fix an issue with BlogDashboardPersonalizationService being used on the background thread [#22335] * [***] Block Editor: Avoid keyboard dismiss when interacting with text blocks [https://github.com/WordPress/gutenberg/pull/57070] * [**] Block Editor: Auto-scroll upon block insertion [https://github.com/WordPress/gutenberg/pull/57273] +* [**] Fix an issue in Pages List where the pages are not displayed in a hierarchical order [#22338] 23.9 ----- diff --git a/WordPress/Classes/Utility/PageTree.swift b/WordPress/Classes/Utility/PageTree.swift index ea0a58fa91b4..58239335b200 100644 --- a/WordPress/Classes/Utility/PageTree.swift +++ b/WordPress/Classes/Utility/PageTree.swift @@ -110,7 +110,6 @@ final class PageTree { /// This function assumes none of array elements already exists in the current page tree. func add(_ newPages: [Page]) { let newNodes = newPages.map { TreeNode(page: $0) } - relocateOrphans(to: newNodes) // First try to constrcuture a smaller subtree from the given pages, then move the new subtree to the existing // page tree (`self`). @@ -151,6 +150,8 @@ final class PageTree { } private func add(_ newNodes: [TreeNode]) { + relocateOrphans(to: newNodes) + newNodes.forEach { newNode in let parentID = newNode.pageData.parentID ?? 0 @@ -177,13 +178,17 @@ final class PageTree { /// Move all the nodes in the given argument to the current page tree. private func merge(subtree: PageTree) { - var parentIDs = subtree.nodes.reduce(into: Set()) { $0.insert($1.pageData.parentID ?? 0) } + let subtreeNodes = subtree.nodes + + relocateOrphans(to: subtreeNodes) + + var parentIDs = subtreeNodes.reduce(into: Set()) { $0.insert($1.pageData.parentID ?? 0) } // No need to look for root level parentIDs.remove(0) // Look up parent nodes upfront, to avoid repeated iteration for each node in `subtree`. let parentNodes = findNodes(postIDs: parentIDs) - subtree.nodes.forEach { newNode in + subtreeNodes.forEach { newNode in let parentID = newNode.pageData.parentID ?? 0 // If the new node is at the root level, then simply add it as a child diff --git a/WordPress/WordPressTest/PagesListTests.swift b/WordPress/WordPressTest/PagesListTests.swift index a3a9e124d6b3..2ded32394e72 100644 --- a/WordPress/WordPressTest/PagesListTests.swift +++ b/WordPress/WordPressTest/PagesListTests.swift @@ -22,6 +22,11 @@ class PagesListTests: CoreDataTestCase { try makeAssertions(pages: pages) } + func testOneNestedListInReversedOrder() throws { + let pages = parentPage(childrenCount: 17, additionalLevels: 7).reversed() + try makeAssertions(pages: Array(pages)) + } + func testManyNestedLists() throws { let groups = [ parentPage(childrenCount: 5), @@ -108,7 +113,33 @@ class PagesListTests: CoreDataTestCase { try makeAssertions(pages: pages) } - func testHierachyListRepresentationRoundtrip() throws { + func testDistantChildAndParentPages() throws { + let child = PageBuilder(mainContext).build() + child.postID = NSNumber(value: randomID.next()) + child.parentID = NSNumber(value: randomID.next()) + + let parent = PageBuilder(mainContext).build() + parent.postID = child.parentID + parent.parentID = 0 + + let manyPages = parentPage(childrenCount: 17, additionalLevels: 7) + + // Test 1: place the child page at the begining and the parent page at the end. + var sorted = try PageTree.hierarchyList(of: [child] + manyPages + [parent]) + XCTAssertEqual(parent.hierarchyIndex, 0) + XCTAssertEqual(child.hierarchyIndex, 1) + // The child page should follow the parent page in the sorted list + try XCTAssertEqual(XCTUnwrap(sorted.firstIndex(of: parent)) + 1, XCTUnwrap(sorted.firstIndex(of: child))) + + // Test 2: place the child page at the end and the parent page at the begining. + sorted = try PageTree.hierarchyList(of: [parent] + manyPages + [child]) + XCTAssertEqual(parent.hierarchyIndex, 0) + XCTAssertEqual(child.hierarchyIndex, 1) + // The child page should follow the parent page in the sorted list + try XCTAssertEqual(XCTUnwrap(sorted.firstIndex(of: parent)) + 1, XCTUnwrap(sorted.firstIndex(of: child))) + } + + func testHierarchyListRepresentationRoundtrip() throws { let roundtrip: (String) throws -> Void = { string in let pages = try Array(hierarchyListRepresentation: string, context: self.mainContext) try XCTAssertEqual(PageTree.hierarchyList(of: pages).hierarchyListRepresentation(), string) @@ -175,10 +206,27 @@ class PagesListTests: CoreDataTestCase { _ = pages.sorted { ($0.postID?.int64Value ?? 0) < ($1.postID?.int64Value ?? 0) } NSLog("Array.sort took \(String(format: "%.3f", (CFAbsoluteTimeGetCurrent() - start) * 1000)) millisecond to process \(pages.count) pages") - let originalIDs = original.map { $0.postID! } - let newIDs = new.map { $0.postID! } - let diff = originalIDs.difference(from: newIDs).inferringMoves() - XCTAssertTrue(diff.count == 0, "Unexpected diff: \(diff)", file: file, line: line) + // Compare the two implementions to make sure their results are similar. The pages don'n't need to be in the exact same order, + // but each hierachy level should contain the same child pages in it. + + let originalList = HierarchyList(pages: original) + let newList = HierarchyList(pages: new) + + // They have the same hierachy level. + XCTAssertEqual(originalList.numberOfLevels, newList.numberOfLevels) + + // For each hierachy level, the same child pages are present in both results, without the need of being in the same order. + for level in 1...(originalList.numberOfLevels) { + let pagesAtLevelOriginal = originalList.pages(atLevel: level) + let pagesAtLevelNew = newList.pages(atLevel: level) + XCTAssertEqual(Set(pagesAtLevelOriginal.keys), Set(pagesAtLevelNew.keys), "The parent page ids in each level should be the same") + + for parentPageID in pagesAtLevelOriginal.keys { + let childrenPageIDsOriginal = try XCTUnwrap(pagesAtLevelOriginal[parentPageID]).map { $0.postID } + let childrenPageIDsNew = try XCTUnwrap(pagesAtLevelNew[parentPageID]).map { $0.postID } + XCTAssertEqual(Set(childrenPageIDsOriginal), Set(childrenPageIDsNew), "The children page ids in each level should be the same") + } + } } } @@ -252,3 +300,31 @@ private extension Array where Element == Page { self = pages } } + +private struct HierarchyList { + let pages: [Page] + + var numberOfLevels: Int { + pages.map { $0.hierarchyIndex }.max()! + 1 + } + + func pages(atLevel level: Int) -> [NSNumber: [Page]] { + var result = [NSNumber: [Page]]() + for page in pages { + guard page.hierarchyIndex + 1 == level else { + continue + } + + let parentID = page.parentID ?? 0 + result[parentID, default: []].append(page) + } + return result + } + + func print() { + for page in pages { + Swift.print(String(repeating: " ", count: page.hierarchyIndex * 2), terminator: "|- ") + Swift.print("post id: \(page.postID!), parent id: \(page.parentID ?? 0)") + } + } +}