[No QA] Remove throw in registerNetworkResponseHandler. Clean up IOU actions file. by marcaaron · Pull Request #8087 · Expensify/App

marcaaron · 2022-03-11T02:52:25Z

Details

I noticed that for some reason we are throwing an error from deep inside the API lib. These errors should not be thrown and caught with a .catch().

Seeing as we are going to re-use some of the APIs around IOU Reports for workspace chats I'd figure that doing a some cleanup on the actions files couldn't hurt.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/201109

Tests

Create Request (CreateIOUTransaction) + Split (CreateIOUSplit) Verify that IOU transactions can be created without issue
Force different jsonCode to be returned by these API when calling the methods (edit Web-Expensify api.php code and try to send 405, 404, and 402 verify the correct errors show)

Used this diff

diff --git a/api.php b/api.php
index 6386ea1afd8..8cc96460e5d 100755
--- a/api.php
+++ b/api.php
@@ -2633,6 +2633,7 @@ try {
         $response = TransactionAPI::createIOUTransaction($authToken, $_REQUEST['debtorEmail'] ?? '', $_REQUEST['amount'] ?? 0, $_REQUEST['currency'] ?? '', $_REQUEST['comment'] ?? '');
     } elseif ($command === 'CreateIOUSplit') {
         $response = ReportAPI::createIOUSplit($authToken, $_REQUEST['reportID'] ?? 0, $_REQUEST['amount'] ?? 0, $_REQUEST['splits'] ?? '', $_REQUEST['currency'] ?? '', $_REQUEST['comment'] ?? '');
+        $response['jsonCode'] = 402;
     } elseif ($command === 'Payroll_Summary') {
         $csv = PayrollAPI::getSummary($authToken, $policyID, $_REQUEST['startDate'] ?? '', $_REQUEST['endDate'] ?? '');
         header('Content-Type: text/csv');

Verified the alerts were created

QA

No QA as it may be difficult to get these errors to throw 🤔
Carefully test creating Requests and Split Bill features for any regressions.

marcaaron · 2022-03-11T04:07:34Z

cc @mountiny if you are interested in the review here and the changes as they relate to the recent work you did

Julesssss · 2022-03-11T10:56:49Z

Is this still WIP?

mountiny

Thanks for the ping! Left just a few questions, looks great!

Oh sorry if it was not ready for a review yet, havent noticed it is a draft 🤦

mountiny · 2022-03-11T18:10:05Z

src/libs/actions/IOU.js

+    const iouReportsToUpdate = {};
+
+    _.each(reports, (reportData) => {
+        // First, the existing chat report needs updated with the details about the new IOU


Suggested change

// First, the existing chat report needs updated with the details about the new IOU

// First, the existing chat report needs to be updated with the details about the new IOU

mountiny · 2022-03-11T18:11:00Z

src/libs/actions/IOU.js

+                hasOutstandingIOU: true,
+            };
+
+            // Second, the IOU report needs updated with the new IOU details too


Suggested change

// Second, the IOU report needs updated with the new IOU details too

// Second, the IOU report needs to be updated with the new IOU details too

mountiny · 2022-03-11T18:18:54Z

src/libs/actions/IOU.js

-        .then(() => Onyx.merge(ONYXKEYS.IOU, {loading: false, creatingIOUTransaction: false}))
-        .catch(() => Onyx.merge(ONYXKEYS.IOU, {error: true}));
+        .then(() => Onyx.merge(ONYXKEYS.IOU, {loading: false, creatingIOUTransaction: false}));


How is error here being handled?

Which error do we need to handle? Where's it at?

I guess these maybe? https://github.com/Expensify/Auth/blob/d3cf26e0330c3a4cf98c6ac3abf141a53f617f33/auth/command/CreateIOUSplit.cpp#L15-L18
Should hit the standard handler https://github.com/Expensify/Web-Expensify/blob/7ecf0d26981bcf715f551c58a21744750c06fc83/lib/Auth.php#L115

And because they are 404 and we removed the default handler that rejects the promise we must now do something?

Yes, seems like maybe anywhere that we have removed a .catch() should have to look at the response jsonCode just in case. Will try to find all these places 😄

Julesssss

Nice refactor and clean up. We'll need some tests, but the changes look good to me so far

marcaaron · 2022-03-21T22:37:00Z

Actually, going to scale back these changes so they are easier to test.

I think we are going to have to fix this problem bit by bit and be sure errors are handled before removing catch(). Use of catch() is more widespread than I realized.

Gonna start with the IOU stuff only for now.

marcaaron · 2022-03-21T23:07:51Z

src/libs/API.js

-        // It's a failure, so reject the queued request
-        queuedRequest.reject(response);
-        return;
-    }


There may be consequences of removing this code for some API that are wrapped in a catch(), but it will be too much work to look at all API methods using a catch and find out whether it throws these code. I'm only going the fix the ones that are explicitly being handled.

deetergp · 2022-03-22T19:04:19Z

src/libs/actions/IOU.js

-            return Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT_IOUS, iouReportsToUpdate);
+            Onyx.mergeCollection(ONYXKEYS.COLLECTION.REPORT_IOUS, iouReportsToUpdate);
        })
-        .catch(() => Onyx.merge(ONYXKEYS.IOU, {error: true}))


Are we removing .catch()-es because the jsonCodes from the API are really the only thing we have to go on? Nothing's being thrown in the JS layer that could be caught?

Can of worms, but generally speaking there's only a couple reasons why you end up in a catch and they are:

fetch() errors

force promise to be rejected by throwing an exception because you are doing something bad in a .then() or intentionally threw a new Error()

For the fetch errors we should be handling them globally in the network layer here for persisted things like report comments

App/src/libs/Network.js

Lines 71 to 76 in 2969ed7

const retryCount = NetworkRequestQueue.incrementRetries(request);

getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message});

if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) {

// Request failed too many times removing from persisted storage

NetworkRequestQueue.removeRetryableRequest(request);

}

Any persisted fetch() requests should not ever be getting rejected they are just retried or dumped at some point.

For the ones that aren't persisted (one's being modified in this PR) we could trigger the onError() callback here where the request will get retried for a while and then eventually we do throw an error

App/src/libs/Network.js

Lines 236 to 259 in 2969ed7

processRequest(queuedRequest)

.then(response => onResponse(queuedRequest, response))

.catch((error) => {

recheckConnectivity();

// When the request did not reach its destination add it back the queue to be retried

const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry');

if (shouldRetry && error.name !== CONST.ERROR.REQUEST_CANCELLED) {

const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest);

getLogger().info('A retrieable request failed', false, {

retryCount,

command: queuedRequest.command,

error: error.message,

});

if (retryCount < CONST.NETWORK.MAX_REQUEST_RETRIES) {

networkRequestQueue.push(queuedRequest);

return;

}

getLogger().info('Request was retried too many times with no success. No more retries left');

}

onError(queuedRequest, error);

Which lands us here and rejects the promise (which would lead to an unhandled promise rejection unless we add a catch when we call the method)

App/src/libs/API.js

Lines 193 to 207 in 2969ed7

if (error.name === CONST.ERROR.REQUEST_CANCELLED) {

Log.info('[API] request canceled', false, queuedRequest);

return;

}

if (queuedRequest.command !== 'Log') {

Log.hmmm('[API] Handled error when making request', error);

} else {

console.debug('[API] There was an error in the Log API command, unable to log to server!', error);

}

// Set an error state and signify we are done loading

setSessionLoadingAndError(false, 'Cannot connect to server');

// Reject the queued request with an API offline error so that the original caller can handle it.

queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE));

So a promise could be rejected in theory. But if we are offline we should not be making the request in the first place.

Having fun yet?

Technically, we could probably argue that in the current state of the app all API methods should be using a catch() because there's always a chance that you were online -> make a request -> go offline -> get a fetch error. But barring any accidental exceptions caused by bad code the only error we should expect to see is the CONST.ERROR.API_OFFLINE error.

If you look at the catches I want to remove here. We can no longer make that assumption because we added other reasons why these promises could be rejected (which I'm undoing) or we explicitly throw errors in the .then() which isn't really necessary and makes the reason behind the .catch() ambiguous. Was it a rare "offline" error or the one we explicitly throw? Do we bother to check?

Basically, this whole business is way more complicated than it has to be and the complexity has led to a lot of non standard error handling.

deetergp

Thanks for the explanation, it definitely cleared things up. I had one NAB comment, but otherwise looks good to me.

deetergp · 2022-03-22T21:01:40Z

src/libs/actions/IOU.js

        .then((response) => {
            if (response.jsonCode !== 200) {
-                throw new Error(`${response.code} ${response.message}`);
+                Log.hmmm('Error rejecting transaction', {error: response.error});


NAB: Should we be including the error codes in these log messages?

Maybe.. I didn't think super hard about this one, but I feel like we should be able to see the request and what error was thrown along with this log line and piece together what happened. This log line felt overly cautious or possibly unnecessary but I wasn't sure.

I think in the future we could consider having a default log any time an action returns a non-200 jsonCode

I think in the future we could consider having a default log any time an action returns a non-200 jsonCode

It would certainly make debugging easier if we did.

OSBotify · 2022-03-22T21:29:35Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-03-24T21:00:52Z

🚀 Deployed to staging by @deetergp in version: 1.1.46-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-03-30T19:27:11Z

🚀 Deployed to production by @timszot in version: 1.1.46-3 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

marcaaron added 4 commits March 10, 2022 16:43

Remove throw in registerNetworkResponseHandler

352f000

Fix comment

e087e5e

Cleanup catch

c5dbe6a

standardize on return early if not jsonCode 200

f590074

marcaaron self-assigned this Mar 11, 2022

Clean up some JSDocs

15ec9c8

marcaaron changed the title ~~[No QA] Remove throw in registerNetworkResponseHandler. Clean up IOU actions file.~~ Remove throw in registerNetworkResponseHandler. Clean up IOU actions file. Mar 11, 2022

marcaaron added 2 commits March 10, 2022 17:11

fix weird indent

50d697f

Get rid of some unnecessary catch()

128b747

remove some changes to make this PR a bit more straightforward

e7b0d60

mountiny reviewed Mar 11, 2022

View reviewed changes

use early return fix comment

f563b84

Julesssss previously approved these changes Mar 14, 2022

View reviewed changes

marcaaron mentioned this pull request Mar 21, 2022

IOU Invalid Phone number error handled #8019

Merged

50 tasks

marcaaron added 2 commits March 21, 2022 12:22

fix conflicts

5fcee18

Fix conflicts

5738aae

marcaaron dismissed Julesssss’s stale review via 5738aae March 21, 2022 22:23

marcaaron added 2 commits March 21, 2022 12:34

undo Wallet change

385243d

scale back other changes

d98c5fb

Fix bad refactor

5dcdaff

marcaaron commented Mar 21, 2022

View reviewed changes

marcaaron added 3 commits March 21, 2022 13:08

Stop catching 405 in PersonalDetails

baf2505

Undo other change

86fe5c4

remove method not used

7f6c228

marcaaron changed the title ~~Remove throw in registerNetworkResponseHandler. Clean up IOU actions file.~~ [No QA] Remove throw in registerNetworkResponseHandler. Clean up IOU actions file. Mar 21, 2022

marcaaron marked this pull request as ready for review March 21, 2022 23:39

marcaaron requested a review from a team as a code owner March 21, 2022 23:39

marcaaron removed the request for review from a team March 21, 2022 23:39

melvin-bot bot requested a review from deetergp March 21, 2022 23:39

deetergp reviewed Mar 22, 2022

View reviewed changes

deetergp approved these changes Mar 22, 2022

View reviewed changes

deetergp merged commit 3278ba1 into main Mar 22, 2022

deetergp deleted the marcaaron-cleanUpIOUErrorHandling branch March 22, 2022 21:19

OSBotify mentioned this pull request Mar 24, 2022

Deploy Checklist: New Expensify 2022-03-24 #8305

Closed

69 tasks

rushatgabhane mentioned this pull request Mar 25, 2022

IOU - Infinite loading spinner appears when split bill with invalid phone number #8308

Closed

luacmartins mentioned this pull request Mar 25, 2022

Blank screen is displayed after log in with new expensifail account #8320

Closed

rushatgabhane mentioned this pull request Apr 7, 2022

[HOLD for payment 2022-04-06] [$500] Split Money - Able to proceed with split request using first 3 digits of phone number #7688

Closed

	// First, the existing chat report needs updated with the details about the new IOU
	// First, the existing chat report needs to be updated with the details about the new IOU

	// Second, the IOU report needs updated with the new IOU details too
	// Second, the IOU report needs to be updated with the new IOU details too

	const retryCount = NetworkRequestQueue.incrementRetries(request);
	getLogger().info('Persisted request failed', false, {retryCount, command: request.command, error: error.message});
	if (retryCount >= CONST.NETWORK.MAX_REQUEST_RETRIES) {
	// Request failed too many times removing from persisted storage
	NetworkRequestQueue.removeRetryableRequest(request);
	}

	processRequest(queuedRequest)
	.then(response => onResponse(queuedRequest, response))
	.catch((error) => {
	recheckConnectivity();

	// When the request did not reach its destination add it back the queue to be retried
	const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry');
	if (shouldRetry && error.name !== CONST.ERROR.REQUEST_CANCELLED) {
	const retryCount = NetworkRequestQueue.incrementRetries(queuedRequest);
	getLogger().info('A retrieable request failed', false, {
	retryCount,
	command: queuedRequest.command,
	error: error.message,
	});

	if (retryCount < CONST.NETWORK.MAX_REQUEST_RETRIES) {
	networkRequestQueue.push(queuedRequest);
	return;
	}

	getLogger().info('Request was retried too many times with no success. No more retries left');
	}

	onError(queuedRequest, error);

	if (error.name === CONST.ERROR.REQUEST_CANCELLED) {
	Log.info('[API] request canceled', false, queuedRequest);
	return;
	}
	if (queuedRequest.command !== 'Log') {
	Log.hmmm('[API] Handled error when making request', error);
	} else {
	console.debug('[API] There was an error in the Log API command, unable to log to server!', error);
	}

	// Set an error state and signify we are done loading
	setSessionLoadingAndError(false, 'Cannot connect to server');

	// Reject the queued request with an API offline error so that the original caller can handle it.
	queuedRequest.reject(new Error(CONST.ERROR.API_OFFLINE));

Conversation

marcaaron commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA

Uh oh!

marcaaron commented Mar 11, 2022

Uh oh!

Julesssss commented Mar 11, 2022

Uh oh!

mountiny left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Mar 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deetergp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Mar 22, 2022

Uh oh!

OSBotify commented Mar 24, 2022

Uh oh!

OSBotify commented Mar 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marcaaron commented Mar 11, 2022 •

edited

Loading

mountiny left a comment •

edited

Loading