[Media Library] Update parameter key for number of items to fetch to be `per_page` #14444

hafizrahman · 2024-11-18T10:06:04Z

Description

When logging in with application password, the Media Library only fetches 10 images regardless. From investigating and comparing with Android, the API request in iOS uses number parameter.

According to the API doc, number parameter does not exist and instead per_page is needed: https://developer.wordpress.org/rest-api/reference/media/#arguments

Android also uses per_page and thus do not see this issue:

https://github.com/wordpress-mobile/WordPress-FluxC-Android/blob/cc17141a0e910d5b808ba6bbfb4b7d393d3517a0/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/media/BaseWPV2MediaRestClient.kt#L231

This PR updates the parameter to the correct one.

The above should also apply the same for Jetpack CP site.

For full Jetpack site, the parameter number should still be used, and this PR differentiates between the two usage.

Reference:

Steps to reproduce

a. No Jetpack case (Application Password)

Have a .org site with no Jetpack,
Upload more than 25 images in wp-admin Media > Library,
In the app, login to site with application password,
Go to Products > Add Product > Add Product Image > WordPress Media Library
Ensure all the images are loaded, not just 10.

b. Jetpack CP site case

Have a .org site with no Jetpack but with a plugin with Jetpack CP (I use "Blaze Ads"),
Setup the Jetpack CP plugin and connect it to your WordPress.com account,
Upload more than 25 images in wp-admin Media > Library,
In the app, login to site with WordPress.com account,
Go to Products > Add Product > Add Product Image > WordPress Media Library
Ensure all the images are loaded, not just 10.

c. WordPress.com / full Jetpack site case

Have a .org site with Jetpack plugin,
Setup the Jetpack plugin and connect it to your WordPress.com account,
Upload more than 25 images in wp-admin Media > Library,
In the app, login to site with WordPress.com account,
Go to Products > Add Product > Add Product Image > WordPress Media Library
Ensure all the images are loaded, not just 10.

New: "media_type" fix test

Have a .org site with no Jetpack,
Upload some images and at least one video to media library,
In the app, login to site with application password,
Go to Products > Add Product > Add Product Image > WordPress Media Library
Ensure all the images are loaded, but the video is not shown

Testing information

I tested this on three type of sites above, on iPhone simulator with iOS 18.

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

wpmobilebot · 2024-11-18T10:19:20Z

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

	App Name	WooCommerce iOS
	Build Number	pr14444-43c0679
	Version	21.1
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`43c0679`
	App Center Build	WooCommerce - Prototype Builds #11666

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

selanthiraiyan · 2024-11-18T12:18:58Z

Networking/Networking/Remote/MediaRemote.swift

    private enum ParameterKey {
        static let pageNumber: String = "page"
-        static let pageSize: String   = "number"
+        static let pageSize: String   = "per_page"


Thanks for working on this, @hafizrahman! I will wait for @hichamboushaba to review this.

I am jumping in as this parameter key change will affect both loadMediaLibrary and loadMediaLibraryFromWordPressSite methods in this MediaRemote file.

The number parameter key is from the following documentation. https://developer.wordpress.com/docs/api/1.2/get/sites/%24site/media/

Please make sure to test the following login cases and update the PR testing instructions accordingly.

Jetpack

JCP

No Jetpack. (Application password)

I am afraid that one of the above cases could be broken/affected by this change.

Nice to have: It would be nice to add a unit test to ensure that the parameter key is correct.

Oh good catch, I tested in WordPress.com console and indeed it wants number. Let me refactor this.

Updated in 7215568

Good for another review.

hichamboushaba · 2024-11-19T08:11:40Z

@hafizrahman just sharing this bug that I identified in Android last week, it's similar to this one, we use the mime_type parameter for all implementations, while it should be media_type for loadMediaLibraryFromWordPressSite, do you think it makes sense to fix it on the same PR?

hafizrahman · 2024-11-20T05:20:26Z

@hichamboushaba thanks for the ping! I'll have it added here.

Issue originally identified in Android wordpress-mobile/WordPress-FluxC-Android#3109 (comment)

hafizrahman · 2024-11-20T05:55:31Z

Added media_type fix on 02bd74d and updated test instruction at the top with this scenario.

selanthiraiyan

Thanks for the updates, Hafiz!

b. Jetpack CP site case

Unfortunately, the above case doesn't work. I used Blaze Ads plugin to connect my WPCOM account with the JN site.

While trying to load images from the media library, I end up seeing the following error alert.

To debug this,

I looked into proxyman, and I see that I do receive a success response from the store for the following request.

https://public-api.wordpress.com/wp/v2/sites/1234/media?_fields=id%2Cdate_gmt%2Cslug%2Cmime_type%2Csource_url%2Calt_text%2Cmedia_details%2Ctitle&media_type=image&page=1&per_page=25

I looked into Xcode logs, and I see the following decoding error.

<> Mapping Error: typeMismatch(Swift.Dictionary<Swift.String, Any>, Swift.DecodingError.Context(codingPath: [_CodingKey(stringValue: "Index 6", intValue: 6), CodingKeys(stringValue: "media_details", intValue: nil), CodingKeys(stringValue: "sizes", intValue: nil)], debugDescription: "Expected to decode Dictionary<String, Any> but found an array instead.", underlyingError: nil))

hichamboushaba · 2024-11-21T17:03:10Z

I looked into Xcode logs, and I see the following decoding error.

<> Mapping Error: typeMismatch(Swift.Dictionary<Swift.String, Any>, Swift.DecodingError.Context(codingPath: [_CodingKey(stringValue: "Index 6", intValue: 6), CodingKeys(stringValue: "media_details", intValue: nil), CodingKeys(stringValue: "sizes", intValue: nil)], debugDescription: "Expected to decode Dictionary<String, Any> but found an array instead.", underlyingError: nil))

This looks like the media_details.sizes property is returning a dictionary instead of the expected object, I had this happen in my tests when I worked on this in Android, but it was only for non-image types, which was happening because we weren't passing the correct argument media_type and we were using the mime_type one.
(This issue shouldn't be because of the PR's changes, and should happen also on trunk I think)

I tried to replicate this, but I couldn't on a test site that I have right now. It's the EOD for me, but I'll try to take a look at this if needed tomorrow, if it's an issue then it would impact Android as well.

selanthiraiyan · 2024-11-22T03:49:39Z

Thanks for taking a look, @hichamboushaba!

I tried to replicate this, but I couldn't on a test site that I have right now.

I have sent invites to my JN store to both you and @hafizrahman. (To your A8C emails)

wpmobilebot · 2024-11-22T05:13:54Z

Version 21.2 has now entered code-freeze, so the milestone of this PR has been updated to 21.3.

hichamboushaba · 2024-11-22T13:36:17Z

Using your site @selanthiraiyan, I can see that an image item has the sizes property empty, and when using the WPCom endpoint, this item is returned as an array instead of an empty object (I think this is something that happens a lot with WordPress, in FluxC we have an old class just to handle similar cases).

So, my assumption that sizes won't be empty for images is wrong, we need to fix this in both platforms then, we should:

Either gracefully handle the switch to empty array (this is what I'll do in Android).
Remove the property from the Networking model, this is an easier solution if we don't use the sizes property in the app.

hichamboushaba · 2024-11-22T13:40:53Z

An update on why this is happening on your site @selanthiraiyan, one of the images in your site is pretty small (4x2 pixels...), when an image is small, WordPress skips generating any other alternative sizes (as they are generally smaller than the original one), so this leads to the empty object.

selanthiraiyan

👋 @hafizrahman I am approving this PR as Hicham's explanation makes sense to me.

Could you please clone this woocommerce/woocommerce-android#12989 for iOS and merge this PR? Thanks!

hafizrahman · 2024-11-26T09:39:48Z

Could you please clone this woocommerce/woocommerce-android#12989 for iOS and merge this PR? Thanks!

Sure: #14516

selanthiraiyan · 2024-11-26T09:57:34Z

@hafizrahman The release note for this PR is mentioned under version 21.2. The PR is targeting 21.3, though.

hafizrahman · 2024-11-26T12:11:41Z

@selanthiraiyan good catch, thanks, fixing in #14521

Update parameter key for number of items to fetch to be per_page

71aff86

hafizrahman added type: bug A confirmed bug. feature: REST API Authenticating requests using application password and using REST API instead of Jetpack tunnel. labels Nov 18, 2024

hafizrahman added this to the 21.2 milestone Nov 18, 2024

Update release notes.

15196e0

hafizrahman requested review from hichamboushaba and selanthiraiyan November 18, 2024 10:09

hafizrahman marked this pull request as ready for review November 18, 2024 10:10

selanthiraiyan reviewed Nov 18, 2024

View reviewed changes

Differentiate parameter between dotcom API and dotorg API.

7215568

hafizrahman requested a review from selanthiraiyan November 19, 2024 06:58

hafizrahman added 3 commits November 20, 2024 12:20

Merge branch 'trunk' into issue/14432-media-library-missing-images

ec31e45

Update .org API to use media_type instead of mime_type

02bd74d

Issue originally identified in Android wordpress-mobile/WordPress-FluxC-Android#3109 (comment)

Update comments

43c0679

selanthiraiyan self-assigned this Nov 21, 2024

selanthiraiyan reviewed Nov 21, 2024

View reviewed changes

wpmobilebot modified the milestones: 21.2, 21.3 Nov 22, 2024

hichamboushaba mentioned this pull request Nov 22, 2024

Media library fails to load if the site has a small media woocommerce/woocommerce-android#12989

Closed

selanthiraiyan approved these changes Nov 26, 2024

View reviewed changes

hafizrahman mentioned this pull request Nov 26, 2024

Media library fails to load if the site has a small media #14516

Closed

hafizrahman merged commit 459a09d into trunk Nov 26, 2024
17 of 18 checks passed

hafizrahman deleted the issue/14432-media-library-missing-images branch November 26, 2024 09:40

hafizrahman mentioned this pull request Nov 26, 2024

Fix release note for PR 14444 to 21.3 #14521

Merged

[Media Library] Update parameter key for number of items to fetch to be per_page #14444

[Media Library] Update parameter key for number of items to fetch to be per_page #14444

Uh oh!

Conversation

hafizrahman commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Steps to reproduce

Testing information

Reviewer (or Author, in the case of optional code reviews):

Uh oh!

wpmobilebot commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

selanthiraiyan Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

hafizrahman Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

hafizrahman Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

hichamboushaba commented Nov 19, 2024

Uh oh!

hafizrahman commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hafizrahman commented Nov 20, 2024

Uh oh!

selanthiraiyan left a comment

Choose a reason for hiding this comment

Uh oh!

hichamboushaba commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

selanthiraiyan commented Nov 22, 2024

Uh oh!

wpmobilebot commented Nov 22, 2024

Uh oh!

hichamboushaba commented Nov 22, 2024

Uh oh!

hichamboushaba commented Nov 22, 2024

Uh oh!

selanthiraiyan left a comment

Choose a reason for hiding this comment

Uh oh!

hafizrahman commented Nov 26, 2024

Uh oh!

Uh oh!

selanthiraiyan commented Nov 26, 2024

Uh oh!

hafizrahman commented Nov 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Media Library] Update parameter key for number of items to fetch to be `per_page` #14444

[Media Library] Update parameter key for number of items to fetch to be `per_page` #14444

hafizrahman commented Nov 18, 2024 •

edited

Loading

wpmobilebot commented Nov 18, 2024 •

edited

Loading

hafizrahman commented Nov 20, 2024 •

edited

Loading

hichamboushaba commented Nov 21, 2024 •

edited

Loading