Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser #9573

yuwenmemon · 2022-06-27T20:51:06Z

@marcaaron, @neil-marcellini please review

HOLD ON https://github.com/Expensify/Web-Expensify/pull/34119

Details

Keeps the functionality pretty much the same, however, we now show a confirmation screen upon successfully requesting a call as opposed to a Growl.

Fixed Issues

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

Tests/QA

Create a workspace if you do not already have one
Click on your profile photo in the top left (Settings) -> Click on your workspace -> Click on the ❔ symbol in the top right -> "Request a setup call"
Fill out the form but provide an invalid phone number, like 1243
Make sure you see the following error show up on the form

Use a real phone number, make sure that you get a confirmation screen showing you have successfully request a call:

Confirm you get a text message confirming you have requested a call:

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Screenshots

Web

Kapture.2022-06-27.at.16.32.40.mp4

Mobile Web

Kapture.2022-06-27.at.16.49.03.mp4

Desktop

Kapture.2022-06-27.at.16.35.53.mp4

iOS

Kapture.2022-06-27.at.17.28.08.mp4

Android

…l Page

… update with success or fail data

neil-marcellini

This looks really good and it works well. I have a question about how we render the confirmation page, and we also need to have the blocking form implemented.

src/pages/RequestCallPage.js

neil-marcellini · 2022-06-29T21:14:45Z

src/pages/RequestCallPage.js

+                                    <Button
+                                        success
+                                        pressOnEnter
+                                        onPress={this.onSubmit}
+                                        style={[styles.w100]}
+                                        text={this.props.translate('requestCallPage.callMe')}
+                                        isLoading={this.props.requestCallForm.loading}
+                                        isDisabled={isBlockedFromConcierge}
+                                    />


To implement offline UX pattern C we need to disable this button, prevent the form from submitting, and show the user that they are offline.

Ah yes. Using FormAlertWithSubmitButton to fix this. Thanks!

neil-marcellini

Awesome, good to go from me. Let's see what @marcaaron has to say.

marcaaron · 2022-07-01T23:43:28Z

src/libs/Middleware/SaveResponseInOnyx.js

-                    }
+            const data = [];
+
+            // Make sure we have a response (i.e. this isn't a promise being passed down to us by a failed retry request and responseData is undefined)


Suggested change

// Make sure we have a response (i.e. this isn't a promise being passed down to us by a failed retry request and responseData is undefined)

// Make sure we have response data (i.e. response isn't a promise being passed down to us by a failed retry request and responseData is not undefined)

Not sure if I get this comment. response and not responseData is the part that would be a promise passed down to us by a failed retry request?

marcaaron · 2022-07-01T23:46:33Z

src/libs/Middleware/SaveResponseInOnyx.js

-                    if (request.successData) {
-                        data.push(...request.successData);
-                    }
+            const data = [];


const onyxUpdates = [];

marcaaron · 2022-07-01T23:49:44Z

src/libs/Middleware/SaveResponseInOnyx.js

+            // Make sure we have a response (i.e. this isn't a promise being passed down to us by a failed retry request and responseData is undefined)
+            if (responseData) {
+                // Handle the request's success/failure data (client-side data)
+                if (responseData.jsonCode === 200 && request.successData) {


It looks like if we have request.failureData, but no request.successData and the responseData.jsonCode === 200 then the failureData will be pushed.

marcaaron · 2022-07-01T23:51:14Z

src/libs/Middleware/SaveResponseInOnyx.js

+            const data = [];
+
+            // Make sure we have a response (i.e. this isn't a promise being passed down to us by a failed retry request and responseData is undefined)
+            if (responseData) {


NAB, I think this would be a little cleaner if we just returned early with the responseData if it's not defined.

src/libs/actions/Inbox.js

marcaaron · 2022-07-01T23:54:02Z

src/libs/actions/Inbox.js

-                Navigation.goBack();
-                return;
-            }
+    // Set loading spinner as we wait for the request to complete


NAB, these details feel like something the UI should worry about. Not sure if the comments here help since the context is somewhere else.

src/libs/actions/Inbox.js

marcaaron · 2022-07-01T23:56:49Z

src/libs/actions/Inbox.js

+            onyxMethod: 'merge',
+            key: ONYXKEYS.ACCOUNT,
+            value: {
+                success: true,


What is success: true for? Is this ACCOUNT data supposed to be here?

Yeah this will show the confirmation that the call was requested here:
https://github.com/Expensify/App/pull/9573/files#diff-2950751ea6e5a3b24fe47d391faa69d373aa4ee9d27dfdda253d7857beb1b9faR300-R303

This is similar to how the PasswordPage does it (and where I copied from):

App/src/pages/settings/PasswordPage.js

Lines 156 to 161 in 53431af

/>

{!_.isEmpty(this.props.account.success)

? (

<PasswordConfirmationScreen />

) : (

<>

marcaaron · 2022-07-02T00:03:24Z

src/pages/RequestCallPage.js

+    /** Holds information about the users account that is requesting the call */
+    account: PropTypes.shape({
+        /** Success state to show confirmation of requesting a call */
+        success: PropTypes.boolean,


Would it be better to use requestCallForm to store this? account is pretty generic.

account.success doesn't really scream "Success state to show confirmation of requesting a call"

Maybe we can err on the side of being specific here and make it requestCallForm.didRequestCallSucceed or something?

Ah okay, sorry, this is carrying over from your comment above.

Yeah that works as an alternative for me 👍

…obal account prop

yuwenmemon · 2022-07-05T23:05:27Z

Updated!

marcaaron

LGTM thanks for the changes!

neil-marcellini

Good to go.

OSBotify · 2022-07-07T16:40:52Z

✋ 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-07-08T01:33:21Z

🚀 Deployed to staging by @yuwenmemon in version: 1.1.81-0 🚀

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

OSBotify · 2022-07-13T01:19:36Z

🚀 Deployed to production by @roryabraham in version: 1.1.82-5 🚀

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

aldo-expensify · 2022-08-25T10:45:41Z

src/libs/Middleware/SaveResponseInOnyx.js

+            if (!responseData) {
+                return;
+            }
+
+            // Handle the request's success/failure data (client-side data)
+            if (responseData.jsonCode === 200 && request.successData) {
+                onyxUpdates.push(...request.successData);
+            } else if (responseData.jsonCode !== 200 && request.failureData) {
+                onyxUpdates.push(...request.failureData);
+            }


cc @yuwenmemon

I'm not sure if this case matters, but if a requests hits the back end with a command that is not implemented, the backend returns a 404. I would have expected that the failureData to be pushed into Onyx, but it isn't because responseData is undefined so I ended up without feedback in the UI. Could this happen in a real case? like if we try to send a request about a workspace or a chat that doesn't exist anymore?

I see what you mean because the response for that case does not come via jsonCode: https://github.com/Expensify/Web-Expensify/blob/30edfabd582f9ac70ec59cef3172a5bf25657efa/api.php#L2761-L2766

Maybe we can add a parameter that indicates if this call to the Middleware is coming from a failed retry request or not? Maybe checking the retryCount? Or check the HTTP response code somehow? The original issue was that if a request was a failed retry (i.e. could not connect to the Server), we'd have no responseData and thus get an undefined error.

However I don't really think this case matters too much right? Why would we want to support error messages for non-existent API commands? We shouldn't be calling non-existent API commands in the production App ever, right? Any "feedback in the UI" would solely be serving developers and that's it, no?

However I don't really think this case matters too much right? Why would we want to support error messages for non-existent API commands? We shouldn't be calling non-existent API commands in the production App ever, right? Any "feedback in the UI" would solely be serving developers and that's it, no?

I completely agree that this doesn't matter if it only happens when a command doesn't exist, but I had my doubt if this could happen with other crashes like and exception being thrown. I'll test and let you know.

Ok, retested throwing an exception in the web API, we get a jsonCode = 666, responseData is not undefined, and the failureData is applied correctly. I think this is the case that matters and works fine.. so lets do nothing :)

Add use new requestCall Onyx-optimized API command

fe6c2d4

yuwenmemon requested a review from a team as a code owner June 27, 2022 20:51

yuwenmemon self-assigned this Jun 27, 2022

melvin-bot bot requested review from neil-marcellini and removed request for a team June 27, 2022 20:51

yuwenmemon force-pushed the yuwen-requestCall branch 2 times, most recently from 3bdfb77 to 1e08c0b Compare June 27, 2022 21:12

yuwenmemon requested a review from marcaaron June 27, 2022 21:19

Make sure that we reset the errorMessage when opening the Request Cal…

b9df3b1

…l Page

yuwenmemon force-pushed the yuwen-requestCall branch from 1e08c0b to b9df3b1 Compare June 27, 2022 21:35

yuwenmemon added 2 commits June 27, 2022 15:43

Update copy

7c4639c

Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall

6e5b8ee

yuwenmemon changed the title ~~[HOLD Copy] Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser~~ [HOLD WEB#34119] Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser Jun 27, 2022

yuwenmemon added 8 commits June 27, 2022 20:24

Don't neccesistate the presence of onyxData in a response in order to…

76629f3

… update with success or fail data

Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall

68ba343

Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall

b07fec9

Fix tests

1b496ac

Spanish

d6fa52d

More Spanish

6339767

More spanish

880007d

errorMessage -> error

26b0111

neil-marcellini requested changes Jun 29, 2022

View reviewed changes

yuwenmemon added 2 commits June 30, 2022 13:34

Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall

dc47777

Use FormAlertWithSubmitButton to enforce offline mode

f3f6aab

yuwenmemon force-pushed the yuwen-requestCall branch from 4349eac to f3f6aab Compare June 30, 2022 21:18

neil-marcellini previously approved these changes Jul 1, 2022

View reviewed changes

marcaaron requested changes Jul 2, 2022

View reviewed changes

yuwenmemon added 2 commits July 5, 2022 14:56

Merge branch 'main' of github.com:Expensify/App into yuwen-requestCall

c2643fe

PR Comments - use property in requestCallForm Onyx key rather than gl…

60b381c

…obal account prop

yuwenmemon dismissed neil-marcellini’s stale review via 60b381c July 5, 2022 23:03

marcaaron approved these changes Jul 5, 2022

View reviewed changes

neil-marcellini approved these changes Jul 6, 2022

View reviewed changes

yuwenmemon changed the title ~~[HOLD WEB#34119] Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser~~ Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser Jul 7, 2022

yuwenmemon merged commit 3c22789 into main Jul 7, 2022

yuwenmemon deleted the yuwen-requestCall branch July 7, 2022 16:33

roryabraham mentioned this pull request Jul 8, 2022

Deploy Checklist: New Expensify 2022-07-07 #9781

Closed

kbecciv mentioned this pull request Jul 16, 2022

[$250] iOS/Android - Workspace - Keypad overlaps "Call me" button #9807

Closed

aldo-expensify reviewed Aug 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser #9573

Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser #9573

yuwenmemon commented Jun 27, 2022 •

edited

Loading

neil-marcellini left a comment

neil-marcellini Jun 29, 2022

yuwenmemon Jun 30, 2022

neil-marcellini left a comment •

edited

Loading

marcaaron Jul 1, 2022

marcaaron Jul 1, 2022

marcaaron Jul 1, 2022

marcaaron Jul 1, 2022

marcaaron Jul 1, 2022

marcaaron Jul 1, 2022

marcaaron Jul 1, 2022

yuwenmemon Jul 5, 2022

marcaaron Jul 2, 2022

yuwenmemon Jul 5, 2022

yuwenmemon commented Jul 5, 2022

marcaaron left a comment

neil-marcellini left a comment

OSBotify commented Jul 7, 2022

OSBotify commented Jul 8, 2022

OSBotify commented Jul 13, 2022

aldo-expensify Aug 25, 2022 •

edited

Loading

yuwenmemon Aug 26, 2022

aldo-expensify Aug 26, 2022

aldo-expensify Aug 26, 2022

	// Make sure we have a response (i.e. this isn't a promise being passed down to us by a failed retry request and responseData is undefined)
	// Make sure we have response data (i.e. response isn't a promise being passed down to us by a failed retry request and responseData is not undefined)

	/>
	{!_.isEmpty(this.props.account.success)
	? (
	<PasswordConfirmationScreen />
	) : (
	<>

Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser #9573

Start using new new RequestCall Onyx-optimized API command instead of Inbox_CallUser #9573

Conversation

yuwenmemon commented Jun 27, 2022 • edited Loading

Details

Fixed Issues

Tests/QA

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web

Desktop

iOS

Android

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-marcellini left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenmemon commented Jul 5, 2022

marcaaron left a comment

Choose a reason for hiding this comment

neil-marcellini left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 7, 2022

OSBotify commented Jul 8, 2022

OSBotify commented Jul 13, 2022

aldo-expensify Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuwenmemon commented Jun 27, 2022 •

edited

Loading

neil-marcellini left a comment •

edited

Loading

aldo-expensify Aug 25, 2022 •

edited

Loading