From bd50f67fba11751162a1f6aa64cf44473dc48e71 Mon Sep 17 00:00:00 2001 From: Tony Li Date: Mon, 8 Jan 2024 17:22:26 +1300 Subject: [PATCH 1/4] Add a couple of unit tests to test PageTree --- WordPress/WordPressTest/PagesListTests.swift | 84 +++++++++++++++++++- 1 file changed, 80 insertions(+), 4 deletions(-) diff --git a/WordPress/WordPressTest/PagesListTests.swift b/WordPress/WordPressTest/PagesListTests.swift index a3a9e124d6b3..fa29567fce17 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,6 +113,32 @@ class PagesListTests: CoreDataTestCase { try makeAssertions(pages: pages) } + 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 testHierachyListRepresentationRoundtrip() throws { let roundtrip: (String) throws -> Void = { string in let pages = try Array(hierarchyListRepresentation: string, context: self.mainContext) @@ -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 = HierachyList(pages: original) + let newList = HierachyList(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 HierachyList { + 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)") + } + } +} From 3eb5573f278367fae1e63c1b58c6b70e40e36017 Mon Sep 17 00:00:00 2001 From: Tony Li Date: Mon, 8 Jan 2024 17:23:10 +1300 Subject: [PATCH 2/4] Fix #22283: child pages are not moved under parent pages --- WordPress/Classes/Utility/PageTree.swift | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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 From c7054965959a23ed2f099f03d834634c107f905f Mon Sep 17 00:00:00 2001 From: Tony Li Date: Mon, 8 Jan 2024 17:41:11 +1300 Subject: [PATCH 3/4] Add a release note --- RELEASE-NOTES.txt | 1 + 1 file changed, 1 insertion(+) 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 ----- From 3839c2f583071933181e302d77e5c52cae1b28ff Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Mon, 8 Jan 2024 18:07:22 +1100 Subject: [PATCH 4/4] Fix typo in `HierarchyList` name --- WordPress/WordPressTest/PagesListTests.swift | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WordPress/WordPressTest/PagesListTests.swift b/WordPress/WordPressTest/PagesListTests.swift index fa29567fce17..2ded32394e72 100644 --- a/WordPress/WordPressTest/PagesListTests.swift +++ b/WordPress/WordPressTest/PagesListTests.swift @@ -139,7 +139,7 @@ class PagesListTests: CoreDataTestCase { try XCTAssertEqual(XCTUnwrap(sorted.firstIndex(of: parent)) + 1, XCTUnwrap(sorted.firstIndex(of: child))) } - func testHierachyListRepresentationRoundtrip() throws { + 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) @@ -209,8 +209,8 @@ class PagesListTests: CoreDataTestCase { // 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 = HierachyList(pages: original) - let newList = HierachyList(pages: new) + let originalList = HierarchyList(pages: original) + let newList = HierarchyList(pages: new) // They have the same hierachy level. XCTAssertEqual(originalList.numberOfLevels, newList.numberOfLevels) @@ -301,7 +301,7 @@ private extension Array where Element == Page { } } -private struct HierachyList { +private struct HierarchyList { let pages: [Page] var numberOfLevels: Int {