Add Guides call request flow to workspace card configuration screen #4637

alex-mechler · 2021-08-13T00:20:30Z

cc @jasperhuangg

Details

This PR does two things

Generalizes the ability to queue an inbox call, so that we can continue to use the same modal for different taskIDs
Adds the call button to the Company Cards page of the workspace modal

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/173720#issuecomment-897795837

Tests

Regression Tests

Navigate to your concierge DM
Click the call button
Verify it opens the request a call modal
Fill out the form and submit it
Verify you have a new Expensiworks/InboxCallUser job with the taskID ExpensifyCashConciergeDM in your jobs table
Navigate to a non-concierge DM
Click the call button
Verify you are given the choice between Zoom and Google Meet

Feature Tests

From the FAB, click the New Workspace button
[mobile only] Click the Company Cards button
Verify you see the call button
[Web & Desktop only] Hover the call button, verify you see the tooltip
Click the call button
Verify it opens the inbox call dialog
Fill out the form and submit it
Verify you have a new Expensiworks/InboxCallUser job with the taskID WorkspaceCompanyCards in your jobs table

QA Steps

From the FAB, click the New Workspace button
[mobile only] Click the Company Cards button
Verify you see the call button
[Web & Desktop only] Hover the call button, verify you see the tooltip
Click the call button
Verify it opens the inbox call dialog

Repeat for each platform

Tested On

Screenshots

Web

Also tested the translation on web

Mobile Web

Desktop

iOS

Android

alex-mechler · 2021-08-13T15:34:39Z

Putting on hold for this https://github.com/Expensify/Expensify/issues/173720#issuecomment-898323941

src/components/HeaderWithCloseButton.js

jasperhuangg · 2021-08-16T22:54:06Z

src/components/HeaderWithCloseButton.js

    /** Whether we should show a download button */
    shouldShowDownloadButton: PropTypes.bool,

+    /** Whether weshould show a inbox call button */


Suggested change

/** Whether weshould show a inbox call button */

/** Whether we should show a inbox call button */

src/pages/workspace/WorkspaceCardPage.js

jasperhuangg

Lookin good! Some minor changes/concerns

alex-mechler · 2021-08-18T16:33:19Z

Off hold @chiragsalian @jasperhuangg

chiragsalian · 2021-08-18T16:43:35Z

src/components/HeaderWithCloseButton.js


+                {
+                    props.shouldShowInboxCallButton && <InboxCallButton taskID={props.inboxCallTaskID} />
+


remove the unnecessary line break

src/components/VideoChatButtonAndMenu.js

chiragsalian · 2021-08-18T16:49:16Z

src/components/VideoChatButtonAndMenu.js

    ...withLocalizePropTypes,
    ...windowDimensionsPropTypes,
-    isConcierge: PropTypes.bool,
+    openInboxCall: PropTypes.bool,


openInboxCall feels like an odd name for a boolean. I feel like this should start with is or should, maybe something like shouldOpenInboxCall or something. Either way, looking at the logic and now understanding it i felt like isConciergeChat is more obvious of a variable to understand whats going on i.e., if not concierge chat, open video menu, else if concierge chat open inbox call to schedule a setup call.

If you'd like to keep inboxCall then maybe add some comments explaining what happens if this variable is true or false.

Ah yes, before all the design changes, the new button also used this flow, so the variable name was changed. Since the design changes mean they are two different flows, I will change this back! Good catch

src/pages/RequestCallPage.js

chiragsalian · 2021-08-18T17:07:11Z

src/pages/home/HeaderView.js

                                <IOUBadge iouReportID={props.report.iouReportID} />
                            )}
-                            <VideoChatButtonAndMenu isConcierge={isConcierge} />
+                            <VideoChatButtonAndMenu openInboxCall={isConcierge} taskID="ExpensifyCashConciergeDM" />


Thoughts on making the 2nd param consistent and more verbose i.e., instead of taskID calling it inboxCallTaskID.
Two reasons why I suggest this,

taskID for VideoChatButtonAndMenu doesn't mean much imo so someone needs to look at the code to know what it is.

Renaming to inboxCallTaskID would keep it consistent to the variable here.

alex-mechler · 2021-08-18T18:16:01Z

Updated!

chiragsalian

Small question. Rest LGTM

chiragsalian · 2021-08-18T19:04:41Z

src/components/VideoChatButtonAndMenu.js

-                            if (this.props.openInboxCall) {
-                                Navigation.navigate(ROUTES.getRequestCallRoute(this.props.taskID));
+                            if (this.props.isConcierge) {
+                                Navigation.navigate(ROUTES.getRequestCallRoute('NewExpensifyConciergeDM'));


umm whats NewExpensifyConciergeDM? i don't see this on web-e master.
Don't you mean ExpensifyCashConciergeDM? Or is there a PR i'm unaware of that's updating this text?

The taskID was updated in this PR #4408

I don't get it. But where is the new taskID defined in web-e. Like shouldn't web-E be aware of this taskID?
In web-E i see this which uses ExpensifyCashConciergeDM. So if we pass NewExpensifyConciergeDM then its treated as false here which feels incorrect. Thoughts? Am i missing something?

No I don't think you are missing something, I think that was missed in the other updates PR. I'll spin up a separate PR for that.

PR: https://github.com/Expensify/Web-Expensify/pull/31749
Follow up issue to remove the old task ID from web: https://github.com/Expensify/Expensify/issues/174159

So does this PR now go on HOLD till the web-E code is on production or not really?

I think that since this is a n6 issue, we should avoid HOLDing this. We can CP the other PR to help with backwards compatability

chiragsalian

LGTM

jasperhuangg · 2021-08-18T23:16:26Z

Lookin good! Feel free to merge if the Web-E stuff with taskID is all good.

alex-mechler · 2021-08-18T23:30:55Z

The task ID stuff has been merged and cp'd! Merging this! Thanks for the reviews yall!

OSBotify · 2021-08-18T23:31:46Z

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

MitchExpensify · 2021-08-26T19:17:03Z

I presume an endless spinner is because this hasn't hit staging yet? (Although I was able to initiate the flow on staging)

alex-mechler · 2021-08-26T21:21:42Z

This PR is on staging. Can you try the flow again, but have your JS console open so that I can see the error (if there is any)

MitchExpensify · 2021-08-26T22:59:33Z

It worked! @alex-mechler

isagoico · 2021-08-27T00:44:44Z

#4866 this issue was found while executing this PR

OSBotify · 2021-08-30T17:39:03Z

🚀 Deployed to production by @roryabraham in version: 1.0.88-2 🚀

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

Alexander added 4 commits August 12, 2021 12:14

Add inbox call button to HeaderWithCloseButton

6d2ab5c

Genericize requesting inbox calls

a458099

functional calls from the workspace page

1a60440

Style

3f8c5cb

alex-mechler self-assigned this Aug 13, 2021

alex-mechler requested a review from a team as a code owner August 13, 2021 00:20

MelvinBot requested review from chiragsalian and removed request for a team August 13, 2021 00:20

alex-mechler changed the title ~~Add Guides call request flow to workspace card configuration screen~~ [HOLD] Add Guides call request flow to workspace card configuration screen Aug 13, 2021

Alexander added 3 commits August 13, 2021 11:23

Add inbox call button component

9dd5edb

Translate

b9b2a2d

Convert to functional component, style

df251c7

jasperhuangg reviewed Aug 16, 2021

View reviewed changes

src/components/HeaderWithCloseButton.js Show resolved Hide resolved

jasperhuangg reviewed Aug 16, 2021

View reviewed changes

src/pages/workspace/WorkspaceCardPage.js Show resolved Hide resolved

jasperhuangg reviewed Aug 16, 2021

View reviewed changes

Alexander added 5 commits August 17, 2021 12:39

Merge branch 'main' into amechler-inbox-call-workspace

3ca524a

Fix styling

c2e0b6e

Fix comments

853c14e

Style

71d3f50

Add tooltip

d87afd3

alex-mechler changed the title ~~[HOLD] Add Guides call request flow to workspace card configuration screen~~ Add Guides call request flow to workspace card configuration screen Aug 18, 2021

chiragsalian requested changes Aug 18, 2021

View reviewed changes

Alexander added 2 commits August 18, 2021 11:12

PR feedback

9449998

Merge branch 'main' into amechler-inbox-call-workspace

18bb7bb

chiragsalian reviewed Aug 18, 2021

View reviewed changes

chiragsalian approved these changes Aug 18, 2021

View reviewed changes

jasperhuangg approved these changes Aug 18, 2021

View reviewed changes

alex-mechler merged commit 0d263e5 into main Aug 18, 2021

alex-mechler deleted the amechler-inbox-call-workspace branch August 18, 2021 23:30

This was referenced Aug 20, 2021

Deploy Checklist: New Expensify 2021-08-17 #4701

Closed

Deploy Checklist: New Expensify 2021-08-25 #4828

Closed

isagoico mentioned this pull request Aug 27, 2021

Card Screen - p character on 'Help' button is cropped #4866

Closed

	/** Whether weshould show a inbox call button */
	/** Whether we should show a inbox call button */


		{
		props.shouldShowInboxCallButton && <InboxCallButton taskID={props.inboxCallTaskID} />

Add Guides call request flow to workspace card configuration screen #4637

Add Guides call request flow to workspace card configuration screen #4637

Uh oh!

Conversation

alex-mechler commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Regression Tests

Feature Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

alex-mechler commented Aug 13, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jasperhuangg left a comment

Choose a reason for hiding this comment

Uh oh!

alex-mechler commented Aug 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-mechler commented Aug 18, 2021

Uh oh!

chiragsalian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-mechler Aug 18, 2021 • 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!

alex-mechler Aug 18, 2021 • 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!

chiragsalian left a comment

Choose a reason for hiding this comment

Uh oh!

jasperhuangg commented Aug 18, 2021

Uh oh!

alex-mechler commented Aug 18, 2021

Uh oh!

OSBotify commented Aug 18, 2021

Uh oh!

MitchExpensify commented Aug 26, 2021

Uh oh!

alex-mechler commented Aug 26, 2021

Uh oh!

MitchExpensify commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alex-mechler commented Aug 13, 2021 •

edited

Loading

alex-mechler Aug 18, 2021 •

edited

Loading

alex-mechler Aug 18, 2021 •

edited

Loading

MitchExpensify commented Aug 26, 2021 •

edited

Loading