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

[Reader IA] Add "Followed P2s" item to dropdown menu #20057

Merged

Conversation

RenanLukas
Copy link
Contributor

@RenanLukas RenanLukas commented Jan 26, 2024

Fixes #19898


To Test:

  • Install JP and sign in with an account that has followed P2s and is not associated with A8C
  • Open the Reader: you should see the Followed P2s item in the dropdown menu
image
  • Tap the Followed P2s item: you should see the post list and blogs filter. You can compare it with web to check the expected behavior.
Screenshot 2024-01-26 at 6 05 19 PM
  • Verify that the following events are tracked:
🔵 Tracked: reader_dropdown_menu_item_tapped, Properties: {"id":"p2"}
🔵 Tracked: reader_p2_shown
  • Verify that filter blog also works as expected.

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)

    • Manual testing and updated unit tests
  3. What automated tests I added (or what prevented me from doing so)

    --


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.

UI Changes Testing Checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

@RenanLukas RenanLukas marked this pull request as ready for review January 26, 2024 21:08
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 26, 2024

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20057-dfde3f8
Commitdfde3f8
Direct Downloadwordpress-prototype-build-pr20057-dfde3f8.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 26, 2024

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20057-dfde3f8
Commitdfde3f8
Direct Downloadjetpack-prototype-build-pr20057-dfde3f8.apk
Note: Google Login is not supported on these builds.

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e01610c) 40.47% compared to head (dfde3f8) 40.47%.
Report is 147 commits behind head on feature/reader-ia.

Files Patch % Lines
...rdpress/android/ui/reader/tracker/ReaderTracker.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           feature/reader-ia   #20057   +/-   ##
==================================================
  Coverage              40.47%   40.47%           
==================================================
  Files                   1447     1447           
  Lines                  66779    66787    +8     
  Branches               11028    11030    +2     
==================================================
+ Hits                   27026    27035    +9     
+ Misses                 37269    37268    -1     
  Partials                2484     2484           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@thomashorta thomashorta left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, the code looks good to me!

I found an issue in this new "Followed P2s" though that might be worth investigating now since the same thing doesn't happen in the "Subscriptions" feed, which is very similar to this one.

The issue is an infinite loading progress indicator whenever I select a site in the Blog filter, and that indicator continues to show even after I clear the filter. I tested on a Samsung S21+ with Android 13. Here's a video:

reader-ia-followed-p2s-infinite-loading-blurred.mov

@daniloercoli
Copy link
Contributor

I've also noticed the issue where the loading indicator remains on the screen, particularly with P2s that have fewer posts. However, when I switch to a P2 with more posts, the loading indicator disappears appropriately once the network call is completed.

@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@RenanLukas
Copy link
Contributor Author

Thanks for the review, @thomashorta and @daniloercoli.

I've pushed a fix for the issue where the progress would not disappear.

Comment on lines 2326 to 2328
if (!isUpdating) {
mRecyclerView.setRefreshing(false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ I don't think that addresses the root cause and has the potential of introducing a issues/crash, since it's now using the mRecyclerView when the Fragment might not be added to the Activity.

I did a quick investigation and it looks like the problem is that we have only 1 mIsUpdating field that is being used by 2 different progress indicators (showLoadingProgress and mRecyclerView.setRefreshing) but only one of them uses it, depending on the updateAction conditional statement below.

What is happening, in practice, is that for P2s with small number of posts we are doing several setIsUpdating calls with both REQUEST_OLDER and REQUEST_NEWER so there's a point where the NEWER sets the Recycler progress bar to visible, but then an OLDER call comes with isUpdating=false, which doesn't hide the Recycler progress bar so when the actual call with NEWER and isUpdating=false comes we don't do anything because of the logic on line 2325, since mIsUpdating is now false.

Here are some logs I added before line 2325 showing what happened for me when I set the filter on a P2 with just 1 post:

isUpdating=true,  mIsUpdating=false, updateAction=REQUEST_NEWER, isAdded = true        <--- caused mIsUpdating to be true and the Recycler progress to be visible

isUpdating=true,  mIsUpdating=true,  updateAction=REQUEST_NEWER, isAdded = true

isUpdating=true,  mIsUpdating=true,  updateAction=REQUEST_OLDER, isAdded = true        <--- was ignored, since mIsUpdating == isUpdating, so the bottom progress was never shown

isUpdating=true,  mIsUpdating=true,  updateAction=REQUEST_OLDER, isAdded = true

isUpdating=false, mIsUpdating=true,  updateAction=REQUEST_OLDER, isAdded = true        <--- caused mIsUpdating to be false, but since is REQUEST_OLDER, it tried hiding the bottom progress (but it was not visible) and didn't touch the Recycler progress (stayed visible)

isUpdating=false, mIsUpdating=false, updateAction=REQUEST_NEWER, isAdded = true        <--- was ignored, since mIsUpdating == isUpdating, so the Recycler progress was never hidden

isUpdating=true,  mIsUpdating=false, updateAction=REQUEST_OLDER, isAdded = true        <--- caused mIsUpdating to be true and the bottom progress to be visible

isUpdating=true,  mIsUpdating=true,  updateAction=REQUEST_OLDER, isAdded = true

isUpdating=false, mIsUpdating=true,  updateAction=REQUEST_OLDER, isAdded = true        <--- caused mIsUpdating to be false and hid the bottom progress

I see you're AFK today so I will try to fix it by avoiding using a single field for both progress indicators.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just pushed some commits in this PR to fix this issue and hopefully make the progress indicator states more reliable as well. @daniloercoli can you also confirm it works for you and review the code?

Copy link
Contributor

@daniloercoli daniloercoli Jan 31, 2024

Choose a reason for hiding this comment

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

The test appears successful and everything seems to be functioning correctly. H̶o̶w̶e̶v̶e̶r̶,̶ ̶I̶ ̶r̶e̶c̶o̶m̶m̶e̶n̶d̶ ̶a̶d̶d̶i̶n̶g̶ ̶a̶ ̶s̶y̶n̶c̶h̶r̶o̶n̶i̶z̶e̶ ̶b̶l̶o̶c̶k̶ ̶w̶h̶e̶n̶ ̶w̶e̶ ̶u̶s̶e̶ ̶t̶o̶ ̶t̶h̶e̶ ̶n̶e̶w̶l̶y̶ ̶i̶n̶t̶r̶o̶d̶u̶c̶e̶d̶ ̶o̶b̶j̶e̶c̶t̶ ̶̶m̶C̶u̶r̶r̶e̶n̶t̶U̶p̶d̶a̶t̶e̶A̶c̶t̶i̶o̶n̶s̶̶ ̶t̶o̶ ̶p̶r̶e̶v̶e̶n̶t̶ ̶a̶n̶y̶ ̶p̶o̶t̶e̶n̶t̶i̶a̶l̶ ̶r̶a̶c̶e̶ ̶c̶o̶n̶d̶i̶t̶i̶o̶n̶s̶ ̶t̶h̶a̶t̶ ̶c̶o̶u̶l̶d̶ ̶o̶c̶c̶u̶r̶ ̶w̶h̶e̶n̶ ̶u̶p̶d̶a̶t̶i̶n̶g̶/̶r̶e̶a̶d̶i̶n̶g̶ ̶i̶t̶ ̶o̶n̶ ̶t̶w̶o̶ ̶d̶i̶f̶f̶e̶r̶e̶n̶t̶ ̶t̶h̶r̶e̶a̶d̶s̶.̶ ̶W̶i̶l̶l̶ ̶a̶d̶d̶ ̶a̶ ̶m̶o̶r̶e̶ ̶d̶e̶t̶a̶i̶l̶e̶d̶ ̶c̶o̶m̶m̶e̶n̶t̶ ̶t̶o̶ ̶t̶h̶e̶ ̶c̶o̶m̶m̶i̶t̶.̶

@RenanLukas
Copy link
Contributor Author

Thanks @thomashorta for fixing the issue in my code and @daniloercoli for reviewing again

Since now this PR also contains code from @thomashorta, I think it makes sense for @daniloercoli to approve it in case it's working as expected. WDYT?

@daniloercoli daniloercoli self-requested a review January 31, 2024 19:59
Copy link
Contributor

@daniloercoli daniloercoli 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 LGTM!

@thomashorta thomashorta merged commit eeec540 into feature/reader-ia Feb 1, 2024
21 checks passed
@thomashorta thomashorta deleted the issue/19898-followed-p2s-in-dropdown-menu branch February 1, 2024 13:14
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.

5 participants