Issue/12126 fix image upload issues #12127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hichamboushaba
added
type: bug
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
hichamboushaba
commented
Jul 24, 2024
Comment on lines
11
to
14
| <!-- <uses-permission--> | ||
| <!-- android:name="android.permission.WRITE_EXTERNAL_STORAGE"--> | ||
| <!-- android:maxSdkVersion="28" />--> | ||
| <!-- <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE" />--> |
There was a problem hiding this comment.
I removed these permissions temporarily, I'll make sure to revert this commit before merging.
My goal is to make testing the PR changes easier for Paolo without having to build the app locally, because since the alpha builds from PRs use debug variant, they would have the needed permissions, and then the issue won't be reproducible on them.
|
@pmusolino can you please test the changes of this PR using the build from above and confirm if it fixes the issues you encountered on your device. |
pmusolino
reviewed
Jul 24, 2024
JorgeMucientes
approved these changes
Jul 24, 2024
There was a problem hiding this comment.
Excellent job at finding the culprit of the issue @hichamboushaba . This was a tricky one.
As talked in private I was not able to reproduce the bug due to the emulators with Androdi 8 and 9 not showing the photo picker. But since Paolo tested that the bug is fixed for him I think that should be enough. Also checked there are no regressions with the changes
This is needed to fix a bug where media upload fails on devices running on versions lower than Android 10
hichamboushaba
force-pushed
the
issue/12126-fix-image-upload-issues
branch
from
c8d0f45 to
d5c3154
Compare
hichamboushaba
removed
the
status: do not merge
|
Found 1 violations: The PR caused the following dependency changes:expand
-+--- org.wordpress:mediapicker:0.3.0
-| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.8.21 -> 1.9.22 (*)
-| +--- com.google.android.material:material:1.6.1 -> 1.9.0 (*)
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.6.4 -> 1.8.1 (*)
-| +--- androidx.navigation:navigation-fragment-ktx:2.5.3 -> 2.7.7 (*)
-| +--- androidx.core:core-ktx:1.12.0 (*)
-| +--- androidx.appcompat:appcompat:1.4.2 -> 1.6.1 (*)
-| +--- androidx.constraintlayout:constraintlayout:2.1.4 (*)
-| +--- androidx.swiperefreshlayout:swiperefreshlayout:1.1.0 (*)
-| +--- androidx.lifecycle:lifecycle-viewmodel-ktx:2.6.2 (*)
-| +--- androidx.datastore:datastore-preferences:1.0.0 (*)
-| +--- com.github.bumptech.glide:glide:4.13.2 -> 4.16.0 (*)
-| +--- com.google.dagger:hilt-android:2.45 -> 2.50 (*)
-| +--- com.google.dagger:hilt-android-compiler:2.45
-| | +--- com.google.dagger:dagger:2.45 -> 2.50 (*)
-| | +--- com.google.dagger:dagger-compiler:2.45
-| | | +--- com.google.dagger:dagger:2.45 -> 2.50 (*)
-| | | +--- com.google.dagger:dagger-producers:2.45
-| | | | +--- com.google.dagger:dagger:2.45 -> 2.50 (*)
-| | | | +--- com.google.guava:failureaccess:1.0.1 -> 1.0.2
-| | | | +--- com.google.guava:guava:31.0.1-jre -> 33.1.0-android (*)
-| | | | +--- javax.inject:javax.inject:1
-| | | | \--- org.checkerframework:checker-compat-qual:2.5.5
-| | | +--- com.google.dagger:dagger-spi:2.45
-| | | | +--- com.google.dagger:dagger:2.45 -> 2.50 (*)
-| | | | +--- com.google.dagger:dagger-producers:2.45 (*)
-| | | | +--- com.google.code.findbugs:jsr305:3.0.2
-| | | | +--- com.google.devtools.ksp:symbol-processing-api:1.7.0-1.0.6
-| | | | | \--- org.jetbrains.kotlin:kotlin-stdlib:1.7.0 -> 1.9.22 (*)
-| | | | +--- com.google.guava:failureaccess:1.0.1 -> 1.0.2
-| | | | +--- com.google.guava:guava:31.0.1-jre -> 33.1.0-android (*)
-| | | | +--- com.squareup:javapoet:1.13.0
-| | | | +--- com.squareup:kotlinpoet:1.11.0
-| | | | | +--- org.jetbrains.kotlin:kotlin-reflect:1.6.10
-| | | | | | \--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.9.22 (*)
-| | | | | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.9.10 (*)
-| | | | +--- javax.inject:javax.inject:1
-| | | | +--- org.jetbrains.kotlin:kotlin-stdlib:1.7.0 -> 1.9.22 (*)
-| | | | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.7.0 -> 1.9.10 (*)
-| | | +--- com.google.code.findbugs:jsr305:3.0.2
-| | | +--- com.google.devtools.ksp:symbol-processing-api:1.7.0-1.0.6 (*)
-| | | +--- com.google.googlejavaformat:google-java-format:1.5
-| | | | +--- com.google.guava:guava:22.0 -> 33.1.0-android (*)
-| | | | \--- com.google.errorprone:javac-shaded:9-dev-r4023-3
-| | | +--- com.google.guava:failureaccess:1.0.1 -> 1.0.2
-| | | +--- com.google.guava:guava:31.0.1-jre -> 33.1.0-android (*)
-| | | +--- com.squareup:javapoet:1.13.0
-| | | +--- javax.inject:javax.inject:1
-| | | +--- net.ltgt.gradle.incap:incap:0.2
-| | | +--- org.checkerframework:checker-compat-qual:2.5.5
-| | | +--- org.jetbrains.kotlin:kotlin-stdlib:1.7.0 -> 1.9.22 (*)
-| | | \--- org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.5.0
-| | | \--- org.jetbrains.kotlin:kotlin-stdlib:1.7.0 -> 1.9.22 (*)
-| | +--- com.google.dagger:dagger-spi:2.45 (*)
-| | +--- com.google.code.findbugs:jsr305:3.0.2
-| | +--- com.google.guava:failureaccess:1.0.1 -> 1.0.2
-| | +--- com.google.guava:guava:31.0.1-jre -> 33.1.0-android (*)
-| | +--- com.squareup:javapoet:1.13.0
-| | +--- javax.annotation:javax.annotation-api:1.3.2
-| | +--- javax.inject:javax.inject:1
-| | +--- net.ltgt.gradle.incap:incap:0.2
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.7.0 -> 1.9.22 (*)
-| | \--- org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.5.0 (*)
-| +--- com.github.chrisbanes:PhotoView:2.3.0 (*)
-| +--- org.wordpress.mediapicker:domain:0.3.0
-| | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.8.21 -> 1.9.22 (*)
-| | +--- androidx.core:core-ktx:1.12.0 (*)
-| | +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.6.4 -> 1.8.1 (*)
-| | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 -> 1.9.10 (*)
-| +--- androidx.databinding:viewbinding:8.1.0 (*)
-| \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 -> 1.9.10 (*)
++--- org.wordpress:mediapicker:0.3.1 FAILED
-+--- org.wordpress.mediapicker:source-camera:0.3.0
-| +--- org.wordpress.mediapicker:domain:0.3.0 (*)
-| +--- androidx.core:core-ktx:1.12.0 (*)
-| \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 -> 1.9.10 (*)
++--- org.wordpress.mediapicker:source-camera:0.3.1 FAILED
-\--- org.wordpress.mediapicker:source-wordpress:0.3.0
- +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.8.21 -> 1.9.22 (*)
- +--- org.wordpress.mediapicker:domain:0.3.0 (*)
- +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.6.4 -> 1.8.1 (*)
- +--- com.google.dagger:hilt-android:2.45 -> 2.50 (*)
- +--- com.google.dagger:hilt-android-compiler:2.45 (*)
- \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 -> 1.9.10 (*)
+\--- org.wordpress.mediapicker:source-wordpress:0.3.1 FAILED
Please review and act accordingly
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #12126
Description
This PR fixes two issues regarding media upload:
Fixes an issue where the lack fo the permission to read external storage broke media upload from local files on devices with Android 9 and lower.
The issue happens because we don't
fetchmedia when the Uri is in the media storecontent:://media, then later when we try to read it directly in FluxC, it causes an issue.This wasn't happening on devices running on Android 10 and above, as we still had to copy the file to internal storage later on, so accidentally the issue wasn't caught when we removed the permissions.
The fix I applied here is to remove the check, and proceed to
fetchthe external media in all cases: meaning the external file will be copied to app's internal cache before the upload, this will align the behavior for all platforms, and it was already happening for newer Android versions.Fixes an issue with Camera flow, the issue was also caused by the lack of the external storage permissions, mainly the
WRITE_EXTERNAL_STORAGEone, and I believe this was caused by a misinterpretation of the documentation, the library requires the permissionWRITE_EXTERNAL_STORAGEfor devices running on API lower than 29, where the documentation states (in a non-clear way TBH) that this permission is needed only for devices with API lower than 19.This fix comes from the MediaPicker library PR.
Steps to reproduce
Preparation
trunkor therelease/19.6branchLocal files
Camera
Testing information
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.