Skip to content
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

Add a switch on dev and staging to force offline mode #12135

Merged
merged 29 commits into from
Dec 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
075f04f
Add a dev only switch to force offline mode
neil-marcellini Oct 25, 2022
acabf93
Force offline if needed when the app mounts
neil-marcellini Oct 25, 2022
5101892
Ignore network state change when forcing offline
neil-marcellini Oct 25, 2022
587955a
Default to false for shouldForceOffline connection
neil-marcellini Oct 31, 2022
294101e
Fix network prop types
neil-marcellini Oct 31, 2022
2e8dea9
Merge branch 'main' into neil-switch-offline
neil-marcellini Oct 31, 2022
3b79301
Fix shouldForceOffline again
neil-marcellini Oct 31, 2022
036b718
Ignore shouldForceOffline while signed out
neil-marcellini Oct 31, 2022
d4f7f2d
Merge branch 'main' into neil-switch-offline
neil-marcellini Nov 10, 2022
70c5008
Merge branch 'main' into neil-switch-offline
neil-marcellini Nov 22, 2022
14bbfd8
Clean up log line and a comment
neil-marcellini Nov 22, 2022
b6db0a7
Network store is also offline when forcing it
neil-marcellini Nov 22, 2022
a16d31a
Fail to fetch if forcing offline
neil-marcellini Nov 22, 2022
6ba03a9
Convert shouldForceOffline to boolean if undefined
neil-marcellini Nov 22, 2022
db4ce83
Force offline mode in the Expensify constructor
neil-marcellini Nov 23, 2022
a96133b
Boolean constructor for clarity and consistency
neil-marcellini Nov 23, 2022
6fada1a
Remove redundant default
neil-marcellini Nov 23, 2022
1bbe8ae
Evaluate shouldForceOffline first
neil-marcellini Nov 23, 2022
3608ff4
Clean up Onyx network connections
neil-marcellini Nov 23, 2022
16802f0
Set real isOffline value after force offline
neil-marcellini Nov 23, 2022
2d1c45f
Fetch net info when no longer forcing offline
neil-marcellini Nov 23, 2022
6d86036
Always provide boolean isOn to switches
neil-marcellini Nov 25, 2022
41fa6c1
Ignore all Pusher events when forcing offline
neil-marcellini Nov 28, 2022
e4d0e3f
Ignore Push events when offline with devtools
neil-marcellini Nov 28, 2022
09007c1
trigger app reconnect after forcing offline
neil-marcellini Nov 28, 2022
3104a7e
Revert "Ignore Push events when offline with devtools"
neil-marcellini Nov 28, 2022
5d44b69
Ignore changes irrelevant to shouldForceOffline
neil-marcellini Nov 28, 2022
a467667
Remove duplicate setup based on shouldForceOffline
neil-marcellini Nov 28, 2022
7960bdc
Merge branch 'main' into neil-switch-offline
neil-marcellini Nov 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/components/TestToolMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,18 @@ const TestToolMenu = props => (
/>
</TestToolRow>

{/* When toggled the app will be forced offline. */}
<TestToolRow title="Force offline">
<Switch
isOn={Boolean(props.network.shouldForceOffline)}
onToggle={() => Network.setShouldForceOffline(!props.network.shouldForceOffline)}
/>
</TestToolRow>
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine if we want to start with this - but I personally think this would be more useful if you could quickly switch to offline from anywhere. Maybe with a secret gesture or something. Having to navigate to the /settings/preferences screen and back to whatever you were doing before is not ideal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the toggle is a good first step for now. I like the gesture idea, as long as its difficult to accidentally trigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking maybe 4 or 5 quick taps in a row?

Copy link
Contributor

@Julesssss Julesssss Nov 30, 2022

Choose a reason for hiding this comment

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

Maybe in an ideal world, we could launch a PopOver containing the debug tools settings page.

As for how to launch this, 4-5 taps still seems too easy to accidentally trigger IMO. By any chance do we have a list of currently unused recognized gestures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Launching a Popover with the debug tools is a good idea, that way it's easy to observe the state of the settings.

As for how to launch this, 4-5 taps still seems too easy to accidentally trigger IMO. By any chance do we have a list of currently unused recognized gestures?

To be clear we would only add this gesture on staging or dev, or maybe just on dev. I don't think there would be a huge consequence to triggering it if it just opens a popover. I really have no idea about gestures, I'll create an issue to add this and we can investigate there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


{/* When toggled all network requests will fail. */}
<TestToolRow title="Simulate failing network requests">
<Switch
isOn={props.network.shouldFailAllRequests || false}
isOn={Boolean(props.network.shouldFailAllRequests)}
onToggle={() => Network.setShouldFailAllRequests(!props.network.shouldFailAllRequests)}
/>
</TestToolRow>
Expand Down
3 changes: 3 additions & 0 deletions src/components/networkPropTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ export default PropTypes.shape({
/** Is the network currently offline or not */
isOffline: PropTypes.bool,

/** Should the network be forced offline */
shouldForceOffline: PropTypes.bool,

/** Whether we should fail all network requests */
shouldFailAllRequests: PropTypes.bool,
});
11 changes: 9 additions & 2 deletions src/libs/HttpUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,16 @@ Onyx.connect({
});

let shouldFailAllRequests = false;
let shouldForceOffline = false;
Onyx.connect({
key: ONYXKEYS.NETWORK,
callback: val => shouldFailAllRequests = (val && _.isBoolean(val.shouldFailAllRequests)) ? val.shouldFailAllRequests : false,
callback: (network) => {
if (!network) {
return;
}
shouldFailAllRequests = Boolean(network.shouldFailAllRequests);
shouldForceOffline = Boolean(network.shouldForceOffline);
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
},
});

// We use the AbortController API to terminate pending request in `cancelPendingRequests`
Expand All @@ -40,7 +47,7 @@ function processHTTPRequest(url, method = 'get', body = null, canCancel = true)
})
.then((response) => {
// Test mode where all requests will succeed in the server, but fail to return a response
if (shouldFailAllRequests) {
if (shouldFailAllRequests || shouldForceOffline) {
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
throw new HttpsError({
message: CONST.ERROR.FAILED_TO_FETCH,
});
Expand Down
2 changes: 1 addition & 1 deletion src/libs/Network/NetworkStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ Onyx.connect({
triggerReconnectCallback();
}

offline = network.isOffline;
offline = Boolean(network.shouldForceOffline) || network.isOffline;
neil-marcellini marked this conversation as resolved.
Show resolved Hide resolved
},
});

Expand Down
29 changes: 29 additions & 0 deletions src/libs/NetworkConnection.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import _ from 'underscore';
import Onyx from 'react-native-onyx';
import NetInfo from '@react-native-community/netinfo';
import AppStateMonitor from './AppStateMonitor';
import Log from './Log';
import * as NetworkActions from './actions/Network';
import CONFIG from '../CONFIG';
import CONST from '../CONST';
import ONYXKEYS from '../ONYXKEYS';

let isOffline = false;
let hasPendingNetworkCheck = false;
Expand Down Expand Up @@ -39,6 +41,29 @@ function setOfflineStatus(isCurrentlyOffline) {
isOffline = isCurrentlyOffline;
}

// Update the offline status in response to changes in shouldForceOffline
let shouldForceOffline = false;
Onyx.connect({
key: ONYXKEYS.NETWORK,
callback: (network) => {
if (!network) {
return;
}
const currentShouldForceOffline = Boolean(network.shouldForceOffline);
if (currentShouldForceOffline === shouldForceOffline) {
return;
}
shouldForceOffline = currentShouldForceOffline;
if (shouldForceOffline) {
setOfflineStatus(true);
} else {
// If we are no longer forcing offline fetch the NetInfo to set isOffline appropriately
NetInfo.fetch()
.then(state => setOfflineStatus(state.isInternetReachable === false));
}
},
});

/**
* Set up the event listener for NetInfo to tell whether the user has
* internet connectivity or not. This is more reliable than the Pusher
Expand All @@ -65,6 +90,10 @@ function subscribeToNetInfo() {
// whether a user has internet connectivity or not.
NetInfo.addEventListener((state) => {
Log.info('[NetworkConnection] NetInfo state change', false, state);
if (shouldForceOffline) {
Log.info('[NetworkConnection] Not setting offline status because shouldForceOffline = true');
return;
}
aldo-expensify marked this conversation as resolved.
Show resolved Hide resolved
setOfflineStatus(state.isInternetReachable === false);
});
}
Expand Down
18 changes: 18 additions & 0 deletions src/libs/Pusher/pusher.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,21 @@
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import ONYXKEYS from '../../ONYXKEYS';
import Pusher from './library';
import TYPE from './EventType';
import Log from '../Log';

let shouldForceOffline = false;
Onyx.connect({
key: ONYXKEYS.NETWORK,
callback: (network) => {
if (!network) {
return;
}
shouldForceOffline = Boolean(network.shouldForceOffline);
},
});

let socket;
const socketEventCallbacks = [];
let customAuthorizer;
Expand Down Expand Up @@ -112,6 +125,11 @@ function bindEventToChannel(channel, eventName, eventCallback = () => {}) {

const chunkedDataEvents = {};
const callback = (eventData) => {
if (shouldForceOffline) {
Log.info('[Pusher] Ignoring a Push event because shouldForceOffline = true');
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

coming from #28063, also check shouldForceOffline for sendEvent. : )


let data;
try {
data = _.isObject(eventData) ? eventData : JSON.parse(eventData);
Expand Down
9 changes: 9 additions & 0 deletions src/libs/actions/Network.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ function setIsOffline(isOffline) {
Onyx.merge(ONYXKEYS.NETWORK, {isOffline});
}

/**
*
* @param {Boolean} shouldForceOffline
*/
function setShouldForceOffline(shouldForceOffline) {
Onyx.merge(ONYXKEYS.NETWORK, {shouldForceOffline});
}

/**
* Test tool that will fail all network requests when enabled
* @param {Boolean} shouldFailAllRequests
Expand All @@ -18,5 +26,6 @@ function setShouldFailAllRequests(shouldFailAllRequests) {

export {
setIsOffline,
setShouldForceOffline,
setShouldFailAllRequests,
};
15 changes: 13 additions & 2 deletions src/libs/actions/SignInRedirect.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,16 @@ Onyx.connect({
});

let currentIsOffline;
let currentShouldForceOffline;
Onyx.connect({
key: ONYXKEYS.NETWORK,
callback: val => currentIsOffline = val.isOffline,
callback: (network) => {
if (!network) {
return;
}
currentIsOffline = network.isOffline;
currentShouldForceOffline = Boolean(network.shouldForceOffline);
},
});

/**
Expand All @@ -31,6 +38,7 @@ function clearStorageAndRedirect(errorMessage) {
const activeClients = currentActiveClients;
const preferredLocale = currentPreferredLocale;
const isOffline = currentIsOffline;
const shouldForceOffline = currentShouldForceOffline;

// Clearing storage discards the authToken. This causes a redirect to the SignIn screen
Onyx.clear()
Expand All @@ -41,7 +49,10 @@ function clearStorageAndRedirect(errorMessage) {
if (activeClients && activeClients.length > 0) {
Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, activeClients);
}
if (isOffline) {

// After signing out, set ourselves as offline if we were offline before logging out and we are not forcing it.
// If we are forcing offline, ignore it while signed out, otherwise it would require a refresh because there's no way to toggle the switch to go back online while signed out.
if (isOffline && !shouldForceOffline) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This also seems like a good opportunity to just use the "real" value of the network and just let the sign in screen be not testable. If we were logged in before and had shouldForceOffline set to true then we'd be stuck in offline mode and couldn't log in to switch it off?

If we want to be able to use "forced offline mode" on the sign in screen then we can do a more global switch that can be used from anywhere. The way we've done it here we can only set it on or off if we are logged in and looking at the /settings/preferences route. I think there's a chance QA will find this confusing and lock themselves out of the app 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exact purpose of this change is to prevent the user from being unable to sign in if they log out when shouldForceOffline is true.

Onyx.set(ONYXKEYS.NETWORK, {isOffline});
}

Expand Down
Loading