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

hafizrahman commented 4 days ago

Closes: #14432

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.

[x] 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 commented 4 days ago

📲 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	43c06797e93fc51f8e1b0ac071dd4d141f6943f6
	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.

hichamboushaba commented 3 days ago

@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 commented 2 days ago

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

hafizrahman commented 2 days ago

Added media_type fix on https://github.com/woocommerce/woocommerce-ios/pull/14444/commits/02bd74d9967b94e677b427694b36851bb6f8011a and updated test instruction at the top with this scenario.

hichamboushaba commented 18 hours ago

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 commented 7 hours ago

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 commented 5 hours ago

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

woocommerce / woocommerce-ios