Use NetworkRequest transport types for auto-send constraints#6971
Use NetworkRequest transport types for auto-send constraints#6971seadowg merged 2 commits intogetodk:masterfrom
Conversation
| networkConstraint: Scheduler.NetworkType? | ||
| ) { | ||
| val constraintNetworkType = when (networkConstraint) { | ||
| val networkRequest = NetworkRequest.Builder().apply { |
There was a problem hiding this comment.
I think we still need to add the NetworkCapabilities.NET_CAPABILITY_INTERNET capability here right?
There was a problem hiding this comment.
Why do you think so? I think it's not needed as we specify NetworkType.
There was a problem hiding this comment.
It looks to me like (when calling from API 28 and above) NetworkType is ignored. Given that, my guess would be that we still need to specify NET_CAPABILITY_INTERNET on the NetworkRequest.
There was a problem hiding this comment.
Yes, the old network type is used only on older Android versions, but once we switch to using NetworkRequest directly (newer Android versions), specifying the transport (WIFI / CELLULAR) should already imply the type of network we want.
There was a problem hiding this comment.
specifying the transport (WIFI / CELLULAR) should already imply the type of network we want.
Wifi and cellular connections don't necessarily have internet access though. Reading the docs, it would seem to me that we'd still want NET_CAPABILITY_INTERNET or even potentially NET_CAPABILITY_VALIDATED as part of the request.
| networkConstraint: Scheduler.NetworkType? | ||
| ) { | ||
| val constraintNetworkType = when (networkConstraint) { | ||
| val networkRequest = NetworkRequest.Builder().apply { |
There was a problem hiding this comment.
It looks to me like (when calling from API 28 and above) NetworkType is ignored. Given that, my guess would be that we still need to specify NET_CAPABILITY_INTERNET on the NetworkRequest.
Closes #6937
Why is this the best possible solution? Were any other approaches considered?
The issue states that we should keep the current code for older Android versions and only use the new approach on newer ones. However, it seems we can use
setRequiredNetworkRequeston all API levels, as it performs its own version check and internally uses the appropriate implementation:How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
For users on Android 9 (API 28) and above, auto-send behavior becomes more accurate and dependable. Instead of relying on “metered vs unmetered” detection, which incorrectly triggers sending on metered Wi-Fi hotspots when “Cellular only” is selected, the app now uses explicit network transport types (Wi-Fi vs cellular).
For users on older devices (API 27 and below), behavior remains unchanged because the existing metered-based logic is retained.
Do we need any specific form for testing your changes? If so, please attach one.
No.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest