Post List: Fix excessive media requests #20939

zwarm · 2024-06-04T23:18:03Z

Fixes #20905

This PR fixes the excessive calls to fetch media when loading the post list by introducing and managing ongoing fetch requests in PostListFeaturedImageTracker. These changes aim to prevent redundant fetch requests.

Changes include

In PostListFeaturedImageTracker Introduced ongoingRequests, to keep track of ongoing image fetch requests and prevent redundant requests.
Updated the getFeaturedImageUrl() to check and update the ongoingRequests set appropriately.
Updated PostListEventListener.onMediaChanged() to invoke invalidateFeaturedMedia on both success and error. This ensures that the featuredImageMap and ongoingRequests are properly updated whether the media fetch was successful or not.

To Test:

Steps to reproduce the behavior
Preparation:

Create multiple posts (20 or more) and attach a featured image. This can be done quickly by using the WPCOM console and the endpoint rest/v1.1/sites/${siteId}/posts/new.

Existing: Extra requests to the media endpoint for the same featuredImageId

Using "trunk" branch
On the device/emulator, clear storage of the app, or delete and reinstall the app.
Open the app and navigate to the site (the same site that you created the 20+ posts with featured images)
Connect the device/emulator to the Charles proxy (or see below for alternative)
Open the app and go to the Post List screen.
Observe all extra requests to the media endpoint in the Charles proxy.

Fix: Extra requests to the media endpoint are eliminated

Using the app from this PR
On the device/emulator, clear storage of the app, or delete and reinstall the app.
Open the app and navigate to the site (the same site that you created the 20+ posts with featured images)
Connect the device/emulator to the Charles proxy (or see below for alternative)
Open the app and go to the Post list screen.
✅ Verify all the extra requests for the same media id have been eliminated.

Alternative to using Charles

Checkout the branch you will test (trunk for pre-existing test or this branch for the fix test
Checkout FluxC (trunk)
Use local FluxC to build WP
Update GSONRequest.parseNetworkRequest to log info

@Override
    protected Response<T> parseNetworkResponse(NetworkResponse response) {
        logGsonRequest();
        try {
            String json = new String(response.data, HttpHeaderParser.parseCharset(response.headers));
            T res;
            if (mClass == null) {
                res = mGson.fromJson(json, mType);
            } else {
                res = mGson.fromJson(json, mClass);
            }
            return Response.success(res, createCacheEntry(response));
        } catch (UnsupportedEncodingException | JsonSyntaxException e) {
            logRequestPath();
            return Response.error(new ParseError(e));
        }
    }

    private void logGsonRequest() {
        String path = getPath();
        String url = getUrl();

        if (path != null && (path.contains("media"))) {
            if (url != null) {
                Log.i("GsonRequest", "***=> Request URL is: " + url);
            }
        }
    }

Regression Notes

Potential unintended areas of impact
Multiple media requests continue to be excessive
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual and unit testing
What automated tests I added (or what prevented me from doing so)
Existing tests

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.

Testing Checklist (strike-out the not-applying and unnecessary ones): N/A

dangermattic · 2024-06-04T23:20:37Z

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

Generated by 🚫 Danger

sonarcloud · 2024-06-04T23:22:29Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

wpmobilebot · 2024-06-04T23:37:08Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20939-0637eec
	Commit	`0637eec`
	Direct Download	`jetpack-prototype-build-pr20939-0637eec.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-06-04T23:39:02Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20939-0637eec
	Commit	`0637eec`
	Direct Download	`wordpress-prototype-build-pr20939-0637eec.apk`

Note: Google Login is not supported on these builds.

codecov · 2024-06-04T23:49:08Z

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 40.95%. Comparing base (0e05482) to head (0637eec).
Report is 7 commits behind head on trunk.

Files	Patch %	Lines
...ordpress/android/ui/posts/PostListEventListener.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #20939      +/-   ##
==========================================
+ Coverage   40.93%   40.95%   +0.01%     
==========================================
  Files        1518     1518              
  Lines       69550    69558       +8     
  Branches    11473    11474       +1     
==========================================
+ Hits        28473    28488      +15     
+ Misses      38491    38486       -5     
+ Partials     2586     2584       -2

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

fluiddot · 2024-06-06T14:06:29Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt

+        featuredImageIds.forEach {
+            featuredImageMap.remove(it)
+            ongoingRequests.remove(it)
+        }


Will this be called when the featured image request is completed? I mention this because I haven't found any other place where the ongoing request is removed.

Hey @fluiddot
The places where this funcation gets called is PostListEventListener → featuredMediaChanged.
The featuredMediaChanged is called when the media is changed or uploaded. Correct me if I am wrong. @zwarm

Thanks for the question, which @AjeshRPai answered before me down below #20939 (comment)

The places where this funcation gets called is PostListEventListener → featuredMediaChanged.
The featuredMediaChanged is called when the media is changed or uploaded. Correct me if I am wrong.

This is correct, previously only success calls were removed from the featureImageMap and in the updated version, both success and errors are removed. We need to make sure we don't have requests hanging around when they shouldn't be.

fluiddot

I followed the testing instructions and confirmed that only one request is made per featured image. Thanks @zwarm for addressing this issue 🙇 !

I've checked the code and looks good to me, however, I'd defer the approval to an Android developer.

AjeshRPai

@zwarm

The changes looks good to me. Feel free to merge 👍🏼

AjeshRPai · 2024-06-07T08:55:55Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt

+        featuredImageIds.forEach {
+            featuredImageMap.remove(it)
+            ongoingRequests.remove(it)
+        }


Hey @fluiddot
The places where this funcation gets called is PostListEventListener → featuredMediaChanged.
The featuredMediaChanged is called when the media is changed or uploaded. Correct me if I am wrong. @zwarm

zwarm added 4 commits June 4, 2024 17:17

Add an ongoingRequest hashset to track fetch requests

1a062ae

Invoke featureMediaChanged on error and success

dc601e9

Mark the mapSet and hashMap as internal and set visible for testing

bc7d8c5

Add unit tests for PostListFeaturedImageTracker

0637eec

zwarm added [Type] Bug Post List labels Jun 4, 2024

zwarm requested review from fluiddot and AjeshRPai June 4, 2024 23:18

zwarm self-assigned this Jun 4, 2024

zwarm requested a review from pantstamp June 6, 2024 11:50

fluiddot reviewed Jun 6, 2024

View reviewed changes

fluiddot approved these changes Jun 6, 2024

View reviewed changes

AjeshRPai approved these changes Jun 7, 2024

View reviewed changes

zwarm merged commit e56dcc8 into trunk Jun 7, 2024
21 of 23 checks passed

zwarm deleted the issue/20905-fix-excessive-media-requests branch June 7, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post List: Fix excessive media requests #20939

Post List: Fix excessive media requests #20939

zwarm commented Jun 4, 2024 •

edited

Loading

dangermattic commented Jun 4, 2024

sonarcloud bot commented Jun 4, 2024

wpmobilebot commented Jun 4, 2024

wpmobilebot commented Jun 4, 2024

codecov bot commented Jun 4, 2024

fluiddot Jun 6, 2024

AjeshRPai Jun 7, 2024

zwarm Jun 7, 2024

zwarm Jun 7, 2024

fluiddot left a comment

AjeshRPai left a comment

AjeshRPai Jun 7, 2024

Post List: Fix excessive media requests #20939

Post List: Fix excessive media requests #20939

Conversation

zwarm commented Jun 4, 2024 • edited Loading

To Test:

Regression Notes

PR Submission Checklist:

Testing Checklist (strike-out the not-applying and unnecessary ones): N/A

dangermattic commented Jun 4, 2024

sonarcloud bot commented Jun 4, 2024

Quality Gate passed

wpmobilebot commented Jun 4, 2024

wpmobilebot commented Jun 4, 2024

codecov bot commented Jun 4, 2024

Codecov Report

fluiddot Jun 6, 2024

Choose a reason for hiding this comment

AjeshRPai Jun 7, 2024

Choose a reason for hiding this comment

zwarm Jun 7, 2024

Choose a reason for hiding this comment

zwarm Jun 7, 2024

Choose a reason for hiding this comment

fluiddot left a comment

Choose a reason for hiding this comment

AjeshRPai left a comment

Choose a reason for hiding this comment

AjeshRPai Jun 7, 2024

Choose a reason for hiding this comment

zwarm commented Jun 4, 2024 •

edited

Loading