-
Notifications
You must be signed in to change notification settings - Fork 135
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
Hack week: Update target SDK from 31 to 33 #8538
Conversation
You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code: |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## trunk #8538 +/- ##
============================================
- Coverage 43.00% 42.90% -0.10%
+ Complexity 3908 3871 -37
============================================
Files 818 803 -15
Lines 42881 42545 -336
Branches 5635 5609 -26
============================================
- Hits 18441 18255 -186
+ Misses 22818 22677 -141
+ Partials 1622 1613 -9
... and 40 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/util/WooPermissionUtils.kt # WooCommerce/src/main/res/layout/activity_main.xml
# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/AppPrefs.kt
Generated by 🚫 dangerJS |
Found 1 violations: The PR caused the following dependency changes:expand
-+--- org.wordpress:mediapicker:0.1.1
-| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.7.20 (*)
-| +--- org.wordpress.mediapicker:domain:0.1.1
-| | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.7.20 (*)
-| | +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
-| | +--- androidx.core:core-ktx:1.7.0 -> 1.8.0 (*)
-| | \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
-| +--- androidx.databinding:viewbinding:7.2.1 (*)
-| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
-| +--- com.google.android.material:material:1.5.0 -> 1.6.1 (*)
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
-| +--- androidx.navigation:navigation-fragment-ktx:2.3.5 -> 2.5.3 (*)
-| +--- androidx.core:core-ktx:1.7.0 -> 1.8.0 (*)
-| +--- androidx.appcompat:appcompat:1.4.1 -> 1.5.0 (*)
-| +--- androidx.constraintlayout:constraintlayout:2.1.1 -> 2.1.4 (*)
-| +--- androidx.swiperefreshlayout:swiperefreshlayout:1.1.0 (*)
-| +--- androidx.lifecycle:lifecycle-viewmodel-ktx:2.4.0 -> 2.5.1 (*)
-| +--- androidx.datastore:datastore-preferences:1.0.0 (*)
-| +--- com.github.bumptech.glide:glide:4.12.0 -> 4.13.2 (*)
-| +--- com.google.dagger:hilt-android:2.42 (*)
-| +--- com.google.dagger:hilt-android-compiler:2.42
-| | +--- com.google.dagger:dagger:2.42 -> 2.44 (*)
-| | +--- com.google.dagger:dagger-compiler:2.42
-| | | +--- com.google.dagger:dagger:2.42 -> 2.44 (*)
-| | | +--- com.google.dagger:dagger-producers:2.42
-| | | | +--- com.google.dagger:dagger:2.42 -> 2.44 (*)
-| | | | +--- com.google.guava:failureaccess:1.0.1
-| | | | +--- com.google.guava:guava:31.0.1-jre -> 31.1-android (*)
-| | | | +--- javax.inject:javax.inject:1
-| | | | \--- org.checkerframework:checker-compat-qual:2.5.5
-| | | +--- com.google.dagger:dagger-spi:2.42
-| | | | +--- com.google.dagger:dagger:2.42 -> 2.44 (*)
-| | | | +--- com.google.dagger:dagger-producers:2.42 (*)
-| | | | +--- com.google.code.findbugs:jsr305:3.0.2
-| | | | +--- com.google.devtools.ksp:symbol-processing-api:1.5.30-1.0.0
-| | | | | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.5.30 -> 1.7.20 (*)
-| | | | +--- com.google.guava:failureaccess:1.0.1
-| | | | +--- com.google.guava:guava:31.0.1-jre -> 31.1-android (*)
-| | | | +--- com.squareup:javapoet:1.13.0
-| | | | +--- javax.inject:javax.inject:1
-| | | | +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.7.20 (*)
-| | | | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.5.32 -> 1.7.20 (*)
-| | | +--- com.google.code.findbugs:jsr305:3.0.2
-| | | +--- com.google.googlejavaformat:google-java-format:1.5
-| | | | +--- com.google.guava:guava:22.0 -> 31.1-android (*)
-| | | | \--- com.google.errorprone:javac-shaded:9-dev-r4023-3
-| | | +--- com.google.guava:failureaccess:1.0.1
-| | | +--- com.google.guava:guava:31.0.1-jre -> 31.1-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.6.10 -> 1.7.20 (*)
-| | | +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.5.32 -> 1.7.20 (*)
-| | | \--- org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.4.2
-| | | \--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.7.20 (*)
-| | +--- com.google.dagger:dagger-spi:2.42 (*)
-| | +--- com.google.code.findbugs:jsr305:3.0.2
-| | +--- com.google.guava:failureaccess:1.0.1
-| | +--- com.google.guava:guava:31.0.1-jre -> 31.1-android (*)
-| | +--- com.squareup:javapoet:1.13.0
-| | +--- javax.annotation:jsr250-api:1.0
-| | +--- javax.inject:javax.inject:1
-| | +--- net.ltgt.gradle.incap:incap:0.2
-| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.7.20 (*)
-| | \--- org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.4.2 (*)
-| \--- com.github.chrisbanes:PhotoView:2.3.0 (*)
++--- org.wordpress:mediapicker:0.1.2
+| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.7.20 (*)
+| +--- org.wordpress.mediapicker:domain:0.1.2
+| | +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.7.20 (*)
+| | +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
+| | +--- androidx.core:core-ktx:1.7.0 -> 1.8.0 (*)
+| | \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
+| +--- androidx.databinding:viewbinding:7.2.1 (*)
+| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
+| +--- com.google.android.material:material:1.5.0 -> 1.6.1 (*)
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
+| +--- androidx.navigation:navigation-fragment-ktx:2.3.5 -> 2.5.3 (*)
+| +--- androidx.core:core-ktx:1.7.0 -> 1.8.0 (*)
+| +--- androidx.appcompat:appcompat:1.4.1 -> 1.5.0 (*)
+| +--- androidx.constraintlayout:constraintlayout:2.1.1 -> 2.1.4 (*)
+| +--- androidx.swiperefreshlayout:swiperefreshlayout:1.1.0 (*)
+| +--- androidx.lifecycle:lifecycle-viewmodel-ktx:2.4.0 -> 2.5.1 (*)
+| +--- androidx.datastore:datastore-preferences:1.0.0 (*)
+| +--- com.github.bumptech.glide:glide:4.12.0 -> 4.13.2 (*)
+| +--- com.google.dagger:hilt-android:2.42 (*)
+| +--- com.google.dagger:hilt-android-compiler:2.42
+| | +--- com.google.dagger:dagger:2.42 -> 2.44 (*)
+| | +--- com.google.dagger:dagger-compiler:2.42
+| | | +--- com.google.dagger:dagger:2.42 -> 2.44 (*)
+| | | +--- com.google.dagger:dagger-producers:2.42
+| | | | +--- com.google.dagger:dagger:2.42 -> 2.44 (*)
+| | | | +--- com.google.guava:failureaccess:1.0.1
+| | | | +--- com.google.guava:guava:31.0.1-jre -> 31.1-android (*)
+| | | | +--- javax.inject:javax.inject:1
+| | | | \--- org.checkerframework:checker-compat-qual:2.5.5
+| | | +--- com.google.dagger:dagger-spi:2.42
+| | | | +--- com.google.dagger:dagger:2.42 -> 2.44 (*)
+| | | | +--- com.google.dagger:dagger-producers:2.42 (*)
+| | | | +--- com.google.code.findbugs:jsr305:3.0.2
+| | | | +--- com.google.devtools.ksp:symbol-processing-api:1.5.30-1.0.0
+| | | | | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.5.30 -> 1.7.20 (*)
+| | | | +--- com.google.guava:failureaccess:1.0.1
+| | | | +--- com.google.guava:guava:31.0.1-jre -> 31.1-android (*)
+| | | | +--- com.squareup:javapoet:1.13.0
+| | | | +--- javax.inject:javax.inject:1
+| | | | +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.7.20 (*)
+| | | | \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.5.32 -> 1.7.20 (*)
+| | | +--- com.google.code.findbugs:jsr305:3.0.2
+| | | +--- com.google.googlejavaformat:google-java-format:1.5
+| | | | +--- com.google.guava:guava:22.0 -> 31.1-android (*)
+| | | | \--- com.google.errorprone:javac-shaded:9-dev-r4023-3
+| | | +--- com.google.guava:failureaccess:1.0.1
+| | | +--- com.google.guava:guava:31.0.1-jre -> 31.1-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.6.10 -> 1.7.20 (*)
+| | | +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.5.32 -> 1.7.20 (*)
+| | | \--- org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.4.2
+| | | \--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.7.20 (*)
+| | +--- com.google.dagger:dagger-spi:2.42 (*)
+| | +--- com.google.code.findbugs:jsr305:3.0.2
+| | +--- com.google.guava:failureaccess:1.0.1
+| | +--- com.google.guava:guava:31.0.1-jre -> 31.1-android (*)
+| | +--- com.squareup:javapoet:1.13.0
+| | +--- javax.annotation:jsr250-api:1.0
+| | +--- javax.inject:javax.inject:1
+| | +--- net.ltgt.gradle.incap:incap:0.2
+| | +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 -> 1.7.20 (*)
+| | \--- org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.4.2 (*)
+| \--- com.github.chrisbanes:PhotoView:2.3.0 (*)
-+--- org.wordpress.mediapicker:source-device:0.1.1
-| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.7.20 (*)
-| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
-| +--- org.wordpress.mediapicker:domain:0.1.1 (*)
-| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
-| +--- com.google.dagger:hilt-android:2.42 (*)
-| \--- com.google.dagger:hilt-android-compiler:2.42 (*)
++--- org.wordpress.mediapicker:source-device:0.1.2
+| +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.7.20 (*)
+| +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
+| +--- org.wordpress.mediapicker:domain:0.1.2 (*)
+| +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
+| +--- com.google.dagger:hilt-android:2.42 (*)
+| \--- com.google.dagger:hilt-android-compiler:2.42 (*)
-\--- org.wordpress.mediapicker:source-wordpress:0.1.1
- +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.7.20 (*)
- +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
- +--- org.wordpress.mediapicker:domain:0.1.1 (*)
- +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
- +--- com.google.dagger:hilt-android:2.42 (*)
- \--- com.google.dagger:hilt-android-compiler:2.42 (*)
+\--- org.wordpress.mediapicker:source-wordpress:0.1.2
+ +--- org.jetbrains.kotlin:kotlin-parcelize-runtime:1.6.10 -> 1.7.20 (*)
+ +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 -> 1.7.20 (*)
+ +--- org.wordpress.mediapicker:domain:0.1.2 (*)
+ +--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 -> 1.6.4 (*)
+ +--- com.google.dagger:hilt-android:2.42 (*)
+ \--- com.google.dagger:hilt-android-compiler:2.42 (*)
Please review and act accordingly
|
@kidinov @JorgeMucientes @ParaskP7 Because you like this PR so much, you'll be happy to know it's ready for a review 😝 |
private val shouldShowNotificationsPermissionBar: Boolean | ||
get() { | ||
return VERSION.SDK_INT >= VERSION_CODES.TIRAMISU && | ||
mainView?.hasNotificationsPermission == false && | ||
!AppPrefs.getWasNotificationsPermissionBarDismissed() && | ||
selectedSite.get().connectionType == Jetpack | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor np, I'd say we tend to add new UI logic to MainActivityViewModel
for the sake of getting rid of the few presenters left bit by bit. Furthermore, adding it to the view model should be more straightforward as you won't have to add any interface functions and its implementations, just expose the NotificationsPermissionBar
visibility as live data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I wasn't even aware we had MainActivityViewModel
since there was still a presenter 😅. I'll move it, thanks!
import com.woocommerce.android.util.WooAnimUtils | ||
import com.woocommerce.android.util.WooPermissionUtils | ||
|
||
class NotificationsPermissionBarView @JvmOverloads constructor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why didn't you go the Compose way here? Was it less straightforward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -40,10 +41,10 @@ | |||
|
|||
<androidx.compose.ui.platform.ComposeView | |||
android:id="@+id/store_onboarding_view" | |||
style="@style/Woo.Card" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, why did you decide to add this style? Haven't noticed any impact on the UI after this change. Also, this style change will impact (in case it has an visible impact) the onboarding full screen mode which is not a card UI.
I'd say that if we need to fix any UI glitch on the onboarding UI, I'd handle it in the compose code. Let me know wdyt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was some UI inconsistency with the shadow or margins but I don't see it anymore.. I'll just get rid of it.
|
||
<uses-permission android:name="android.permission.BLUETOOTH_SCAN" /> | ||
<uses-permission android:name="android.permission.BLUETOOTH_CONNECT" /> | ||
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" /> | ||
<uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to targeting API 33? Why are both needed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:text="@string/dismiss" | ||
android:layout_margin="0dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Np. Needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree looks better. Let's keep it as it is 👍🏼
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:text="@string/allow" | ||
android:layout_margin="0dp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Np. Needed?
@@ -2251,6 +2253,8 @@ | |||
<string name="permissions_denied_message">It looks like you turned off permissions required for this feature.<br/><br/>To change this, edit your permissions and make sure <strong>%s</strong> is enabled.</string> | |||
<string name="permission_camera">Camera</string> | |||
<string name="button_edit_permissions">Edit permissions</string> | |||
<string name="notifications_permission_title">Notifications</string> | |||
<string name="notifications_permission_description">We need your permission to send you push notifications for new orders, reviews, etc. delivered to your device.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the string value correct? Feels a bit weird after the etc
part. Should it be a comma after etc
? Although I'm not an expert in English 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied it from jetpack_benefits_modal_push_notifications_subtitle
(Get push notifications for new orders, reviews, etc. delivered to your device.
) and modified it, so I didn't really think about it. But it is ok :).
Thanks for handling this task @0nko! 🏅. I did a first round and didn't find anything blocking. But I left a couple of questions and a few nitpicks I think are worth considering looking into before merging. Also, I've seen there are some changes related to media picker library. Is there anything I should test related to that? |
# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/main/MainActivity.kt
Thanks for the review, @JorgeMucientes. I've addressed your comments, ready for another round. |
Btw, the Media Picker has been already tested separately, so you don't need to spend too much time on it. Smoke testing is fine. |
Thanks for applying the suggestions @0nko. The code looks good and works as expected. |
This PR updates the target SDK from 31 to 33 and makes the necessary changes:
Updated Gradle version to 7.3.0, which addressed the warning caused by the target SDK updateMore changes are required in the Media library, which are addressed in a separate PR.
To test:
You'll need a device running Android version 13 (API level 33) to test the new bahavior.
Notifications
is listed in theAllowed
sectionNotifications
is listed in theNot Allowed
section