Return promise in payIOUReport / Fix AdditionalDetailsStep

[marcaaron]

cc @nickmurray47 @aldo-expensify

Details

• Return promise in payIOUReport and remove shouldRedirectToChatReport param
• Also fixes the AdditionalDetailsStep which I noticed was broken by this PR when testing

Fixed Issues

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

Tests

Switch to https://github.com/Expensify/Web-Expensify/pull/32181

1. Add personal bank account and pass KYC checks by following -> https://stackoverflow.com/c/expensify/questions/10607
2. Attempt to send money to someone else
3. Verify you are navigated back to the current report on success

QA Steps

None yet as these flows are under beta

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

[aldo-expensify]

Not sure if this could cause a bug. You are returning the original promise, not the one that gets created after chaining then-catch-finally, which you later use to finally(() => Navigation.navigate(ROUTES.REPORT)).

In the original code, the Navigation.navigate(ROUTES.REPORT); happens inside the finally of the new promise object that gets created after chaining then and catch. The current code may make your new finally run before the finally that was chained after the then and catch. To see this, run these in the console:

Case 1 (similar to PR code):

payIOUPromise = new Promise((resolve, reject) => {
  setTimeout(() => {
    resolve('foo');
  }, 300);
});

payIOUPromise
  .then(response => {
    console.log('Then:', response);
  })
  .catch(error => {
    console.log('ERROR:', error);
  })
  .finally(() => {
    console.log('Finally 1');
  });

payIOUPromise.finally(() => {
  console.log('Finally 2')
});

Output (reversed "Finally X"):

Then: foo
Finally 2
Finally 1

Case 2

payIOUPromise = new Promise((resolve, reject) => {
  setTimeout(() => {
    resolve('foo');
  }, 300);
});

payIOUPromise = payIOUPromise // < -- The difference
  .then(response => {
    console.log('Then:', response);
    return response; // Useful in this case if someone wants to do a .then outside
  })
  .catch(error => {
    console.log('ERROR:', error);
  })
  .finally(() => {
    console.log('Finally 1');
  });

payIOUPromise.finally(() => {
  console.log('Finally 2')
});

Output:

Then: foo
Finally 1
Finally 2

[marcaaron]

Just to be sure I'm following the logic here... the claim is that there are two different promises: one being returned from the method and another one that handles the .then() and .catch()? The observation of the difference in order seems correct, but not sure about the explanation. How is the returned promise different from the one passed to asyncOpenURL()?

For sure open to thoughts on how this change might cause a bug if you have ideas or alternatives. I couldn't think of anything. The only thing we do in the first finally() is set loading to false and navigating back to the report has no dependency on each other.

[Jag96]

The only scenario I think this would cause a bug is if you wanted to make some other type of API call inside the inner catch and make sure that call completes before the outer finally, but agreed with this specific use case the difference in order doesn't seem to affect the desired behavior. That being said, my expectation here would be that the inner promise handling happens before the outer handling, so I'd prefer to have it updated so there isn't any confusion down the line.

[marcaaron]

How is the returned promise different from the one passed to asyncOpenURL()?

Just answering my own question here, but after looking at this for a while I learned something new - which is that a promise will only be handled if you chain off of the result of handling it and not the first promise set up. So code like this is fine:

promise = Promise.reject();

promise.catch(() => {
    console.log('caught');
});

But if you do this...

promise = Promise.reject()

promise.catch(() => {
    console.log('caught');
});

promise.then(() => {
    console.log('I will never run');
});

you'll get an unhandled promise rejection notice 🤯

[aldo-expensify]

you'll get an unhandled promise rejection notice 🤯

🤯

Playing with your example, this other thing was unexpected to me, I thought that reversing the order of chained catch and then didn't matter:

but reversing the callbacks:

Promise.reject()
  .catch(() => {
    console.log('caught');
  })
  .then(() => {
    console.log('I will never run');
  })

prints

caught
I will never run

[marcaaron]

🧙

[aldo-expensify]

Added console logs in the finally callbacks and they are indeed getting reversed, but this doesn't seem to be causing any problem here. I would still suggest fixing that because it may cause an unwanted behaviour under some future use.

This tested well on web.

IMO, the approach of returning the promise rather than passing the boolean as a prop is much flexible and better 👍 .

Thanks for fixing AdditionalDetailsStep!

[marcaaron]

Updated

[botify]

@Jag96 looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

[OSBotify]

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

[Jag96]

Hmmm E2E was showing as passed, not sure why emergency label showed up

[marcaaron]

https://github.com/Expensify/Expensify/issues/189984

[OSBotify]

🚀 Deployed to staging by @Jag96 in version: 1.1.25-2 🚀

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

[OSBotify]

🚀 Deployed to production by @Jag96 in version: 1.1.26-1 🚀

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