amplify-android icon indicating copy to clipboard operation
amplify-android copied to clipboard

fix(datastore): Update network connection availability checking status logic in ReachabilityMonitor of Datastore

Open aladine opened this issue 1 year ago • 6 comments

  • [x] PR title and description conform to Pull Request guidelines.

Issue #, if available: https://github.com/aws-amplify/amplify-android/issues/2738

Description of changes: Enhance the capability to detect network connection availabilities in syncengine of aws datastore. This includes:

  • Instead of checking activeNetwork, connectivity provider could retrieve getNetworkCapabilities and check the network capability is valid.
  • Network callback should have an extra onCapabilitiesChanged(). Since amplify-android supports API 23 and above, onCapabilitiesChanged should be called immediately after onAvailable callback and contains accurate information to determine network connection type.
  • A more fine grained detail to support different Android SDK version for network checking.

How did you test these changes? (Please add a line here how the changes were tested) Add unit test to verify the new change

Documentation update required?

  • [x] No
  • [ ] Yes (Please include a PR link for the documentation update)

General Checklist

  • [x] Added Unit Tests
  • [ ] Added Integration Tests
  • [ ] Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

aladine avatar Jun 19 '24 14:06 aladine

Hi @aladine, thank you for your contribution submission. We will prioritize reviewing this PR and reply back!

tylerjroach avatar Jun 19 '24 14:06 tylerjroach

Any update in this pull request?

aladine avatar Jul 29 '24 15:07 aladine

Hi @aladine, sorry for the delay. I will try and provide some feedback within next few days and suggest any updates if needed.

tylerjroach avatar Jul 30 '24 15:07 tylerjroach

No problem, I am happy to answer all the questions or concerns for this pull request.

aladine avatar Aug 02 '24 00:08 aladine

Overall changes look good. Can you take a look at an alternate implementation here: https://github.com/aws-amplify/amplify-android/tree/tjroach/improve-network-detection-datastore

I attempted to take in Matt's suggestions and simplify it a bit. I believe the code functions identically to yours. The tests would just need updated to use MockK

Thanks Tyler, I move the codes from NetworkUtil to extensions folder as your branch. Update test location and mock method also.

aladine avatar Aug 14 '24 02:08 aladine

Hi all, a kind ping for the progress of this pull request. I can simplify my logic if it introduces churns to the current implementation.

aladine avatar Nov 14 '24 01:11 aladine