Skip to content
This repository has been archived by the owner on Sep 5, 2023. It is now read-only.

Update isOffline(errorCode:) to catch additional NSURLError #710

Merged

Conversation

jncosideout
Copy link
Contributor

@jncosideout jncosideout commented Aug 11, 2020

PR Description

The app was not saving scanned barcodes when offline/no internet connectivity. I added an error code to be checked for verifying offline status.

I tested the offline scan by setting my device to airplane mode and scanning a food product.
This throws to the onError callback (line 442) of dataManager.getProduct(byBarcode:) inside ScannerViewController.getProduct(barcode: ...)

The next line checks isOffline(errorCode:) and if true, we call handleGetProductSuccess(barcode: ...)
According to the comment in the source code, this is when we:
//Assume product does not exist and store locally for later upload

For me, the error code passed is -1004 "Could not connect to the server." According to my research, that matches NSURLErrorCannotConnectToHost

My only concern is that there are other NSURLErrors or other errors that would be thrown when offline or when connectivity is bad. isOffline(errorCode:) should anticipate all possible such error codes.

Type of Changes

Checklist

  • If you have multiple commits please combine them into one commit by squashing them.
  • Code is well documented
  • Included unit tests for new functionality
  • All user-visible strings are made translatable
  • Code passes Travis builds in your branch

Copy link
Collaborator

@philippeauriach philippeauriach left a comment

Choose a reason for hiding this comment

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

As we are touching those, we might want to add more cases to "no internet connection" test ?
The full list of errors is listed here : https://developer.apple.com/documentation/cfnetwork/cfnetworkerrors

I think we should go for those (open to discussion) :
kCFURLErrorCannotFindHost
kCFURLErrorCannotConnectToHost
kCFURLErrorNetworkConnectionLost
kCFURLErrorDNSLookupFailed
kCFURLErrorInternationalRoamingOff
kCFURLErrorCallIsActive
kCFURLErrorDataNotAllowed

@jncosideout
Copy link
Contributor Author

jncosideout commented Aug 12, 2020

As we are touching those, we might want to add more cases to "no internet connection" test ?

That is exactly what I'm thinking. I will include them into isOffline(errorCode:) and re-submit my PR.

I checked the CFNetworkErrors you listed, each one corresponds to an NSURLError and they have the same raw values.
Therefore, I shouldn't need to re-cast the error into a CFNetworkError before passing them to isOffline(errorCode:)

But how do I test those scenarios to throw the particular errors? Most of them seem out of my control so I don't know how to replicate them, e.g. "CannotFindHost", "InternationalRoamingOff", "CannotConnectToHost"
Also, my testing devices do not have cellular plans. So they're WiFi only.

[Update] I downloaded the [Apple Developer Tools ](Network Link Conditioner) Network Link Conditioner to simulate bad connections.

@jncosideout jncosideout force-pushed the pr/issue#267-offline-history branch from 27ea606 to ab7a50c Compare August 14, 2020 23:27
@jncosideout
Copy link
Contributor Author

I wasn't able to trigger all the newly included network errors while testing, but I can confirm that func isOffline(:errorCode) will recognize when the device has no connection to the internet, because the error that is thrown when the device in airplane mode (CannotConnectToHost -1004) has been included.

@teolemon teolemon requested a review from a team August 19, 2020 15:01
@teolemon teolemon merged commit d281d0a into openfoodfacts:develop Aug 19, 2020
@teolemon
Copy link
Member

Thank you @jncosideout and @philippeauriach for the review 👍

@jncosideout jncosideout deleted the pr/issue#267-offline-history branch August 19, 2020 19:24
@aleene aleene added this to the Version 3.4 milestone Sep 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants