Skip to content

Fix #22283: child pages are not moved under parent pages - #22338

Closed
crazytonyli wants to merge 5 commits into
trunkfrom
fix-issue-22283-page-list-hierachy
Closed

Fix #22283: child pages are not moved under parent pages#22338
crazytonyli wants to merge 5 commits into
trunkfrom
fix-issue-22283-page-list-hierachy

Conversation

@crazytonyli

Copy link
Copy Markdown
Contributor

Issue

See #22283 for the issue. The issue is pretty easy to reproduce and it's my mistake that I didn't double check the app after a refactor. For context, the issue happens when a child page presents before a parent page in a pages array, which should be pretty common.

Details

When building a page tree, the elements in a pages array are processed one by one. When a child page is processed before its parent page, it is treated as an "orphan node" util it's parent page is later added to the tree. When adding the parent page to the tree, we should look up the existing orphan nodes and moving child pages under the parent page node. This logic is implemented in the relocateOrphanNodes(to:) function. However, during a refactor, I placed this function call in an incorrect place, which led to the bug in the app.

Regression Notes

  1. Potential unintended areas of impact
    None.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Verified the issue is fixed using a test site.

  3. What automated tests I added (or what prevented me from doing so)
    A couple of unit tests that can reproduce the issue.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist: N/A

@crazytonyli crazytonyli added this to the 24.0 milestone Jan 8, 2024
@crazytonyli
crazytonyli requested review from kean and mokagio January 8, 2024 04:39
@crazytonyli crazytonyli self-assigned this Jan 8, 2024
@peril-wordpress-mobile

Copy link
Copy Markdown
Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
@wpmobilebot

wpmobilebot commented Jan 8, 2024

Copy link
Copy Markdown
Contributor
WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22338-3839c2f
Version23.9
Bundle IDorg.wordpress.alpha
Commit3839c2f
App Center BuildWPiOS - One-Offs #8355
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot

wpmobilebot commented Jan 8, 2024

Copy link
Copy Markdown
Contributor
Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22338-3839c2f
Version23.9
Bundle IDcom.jetpack.alpha
Commit3839c2f
App Center Buildjetpack-installable-builds #7378
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@mokagio

mokagio commented Jan 8, 2024

Copy link
Copy Markdown
Contributor

Hey @crazytonyli 👋 You might notice a merge trunk commit here from me. That is to bring this branch up to date with the latest CI changes on trunk and ensure everything works on the new Xcode 15.1 CI stack.

Don't hesitate to ping me if there's any trouble. I hope to get to review this before the end of the day as it would be ideal to include it in 24.0

@wpmobilebot

Copy link
Copy Markdown
Contributor
1 Warning
⚠️ This PR is assigned to the milestone 24.0. This milestone is due in less than 4 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@mokagio mokagio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crazytonyli I run this branch locally but didn't get the pages sorted hierarchically in the UI 🤔

iOS Web
image image

Semi-related, not all pages appear in the "Set parent" menu in the app, while they do on the web.

RPReplay_Final1704697339.MP4

@crazytonyli

Copy link
Copy Markdown
Contributor Author

@mokagio 🤦‍♂️ My brain obviously wasn't working on my first day back.

@mokagio

mokagio commented Jan 9, 2024

Copy link
Copy Markdown
Contributor

@mokagio 🤦‍♂️ My brain obviously wasn't working on my first day back.

LOL. I know the feeling 😄 🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants