Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reader related posts loading the wrong post after rotation #15352

Conversation

dnalves
Copy link
Contributor

@dnalves dnalves commented Sep 19, 2021

Fixes #14195

Issue description

There's an issue that when using the reader to see Related Posts, after going more than 2 levels deep in related posts and rotating the device, the post loaded will be the first one displayed.

Work done

I changed where the Fragment restores its state from onActivityCreated to onViewCreated.
onActivityCreated is called after onViewCreated, so the state was being restored after the view was ready and the post was already loaded with the wrong id.

To test

  1. Open a wp.com post on Reader tab.
  2. Scroll to bottom of the post details page.
  3. Notice that related posts are displayed.
  4. Click a related post.
  5. Notice that selected related post details are shown on "Related Post" screen with a cross button.
  6. Open another related post from the previous "Related Post" screen.
  7. Notice that selected related post details are shown on "Related Post" screen with a cross button.
  8. Rotate the device.
  9. Notice that first related post is shown instead of the last one.

Video

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)
    None

  3. What automated tests I added (or what prevented me from doing so)
    None. No automated tests are present for the area (just adding them should be a separate PR)

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.

… onActivityCreated

This fixes wordpress-mobile#14195
`onActivityCreated` is called after `onViewCreated`, so the state was being restored
after the view was ready and the post was already loaded with the wrong id.
RELEASE-NOTES.txt Outdated Show resolved Hide resolved
@AliSoftware
Copy link
Contributor

Thanks @dnalves for submitting this PR! 🙇

I am doing the Code Freeze for WPAndroid 18.4 today. I'm thus moving this PR's milestone to 18.5

@jd-alexander I see you reviewed the PR previously, I'll let you decide if this needs to go in 18.4 – and in that case, we will want to update the PR to target the upcoming release/18.4 branch to get it included in 18.4, revert the milestone to 18.4, and do a new beta after the one I'll do post-code-freeze – or if you're ok moving this to 18.5 so it has more time to get reviewed and tested.

@AliSoftware AliSoftware modified the milestones: 18.4, 18.5 Oct 4, 2021
Copy link
Contributor

@jd-alexander jd-alexander left a comment

Choose a reason for hiding this comment

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

@dnalves this PR was bumped to be shipped in 18.5. Could you update the release notes entry to this section? Thank you.

Otherwise from that, I tested the PR and the changes worked as expected. Good fix! I will approve and merge once the changes are made. 🚢

@dnalves
Copy link
Contributor Author

dnalves commented Oct 5, 2021

@dnalves this PR was bumped to be shipped in 18.5. Could you update the release notes entry to this section? Thank you.

Otherwise from that, I tested the PR and the changes worked as expected. Good fix! I will approve and merge once the changes are made. 🚢

Done 👍
Thank you! :D

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 6, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 6, 2021

You can test the changes on this Pull Request by downloading the APKs:

Copy link
Contributor

@jd-alexander jd-alexander left a comment

Choose a reason for hiding this comment

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

Tested and working as expected. Thanks for this fix @dnalves LGTM 🚢

@jd-alexander jd-alexander merged commit 9cc8248 into wordpress-mobile:develop Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reader: Wrong related post shown on rotation
3 participants