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

Crash: Media URL null returned by the API #10554

Merged
merged 2 commits into from
Jan 16, 2024
Merged

Conversation

0nko
Copy link
Contributor

@0nko 0nko commented Jan 16, 2024

Fixes #10264. The WCAndroid client works with the wrong assumption that the MediaModel.url cannot be null. This PR handles that case.

To test:
There's nothing to test since it's difficult to reproduce the scenario.

@0nko 0nko added the type: crash The worst kind of bug. label Jan 16, 2024
@0nko 0nko added this to the 17.2 milestone Jan 16, 2024
@0nko 0nko requested a review from JorgeMucientes January 16, 2024 14:22
@JorgeMucientes JorgeMucientes self-assigned this Jan 16, 2024
@@ -145,7 +146,7 @@ class MediaFilesRepository @Inject constructor(

event.completed -> {
val media = event.media
val channelResult = if (media != null && media.url.isNotBlank()) {
val channelResult = if (media != null && media.url.isNotNullOrEmpty() && media.url.isNotBlank()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor np, so feel free to merge without applying. But maybe with the check media.url.isNotNullOrEmpty() should suffice? I can't think of a scenario where the URL value would be white spaces only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I just wanted to keep the original logic intact. I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code.

@0nko 0nko enabled auto-merge January 16, 2024 14:42
@JorgeMucientes
Copy link
Contributor

Thanks for handling this @0nko. The changes look good. I wonder if we should add some tracking for the error case where the URL == null. I mention this because the fix will only hide the real issue where the media URL coming from a successful upload is unexpectedly null. In any case no need to handle this now. PR is good to go.

@wpmobilebot
Copy link
Collaborator

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

App Name WooCommerce Android
FlavorJalapeno
Build TypeDebug
Commit272ad60
Direct Downloadwoocommerce-prototype-build-pr10554-272ad60.apk

@0nko 0nko merged commit 2c20f8a into trunk Jan 16, 2024
14 checks passed
@0nko 0nko deleted the issue/10264-media-url-null branch January 16, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: crash The worst kind of bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EventBusException: Invoking subscriber failed
3 participants