-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Implement Send Invoice flow from Global Create #40015
Implement Send Invoice flow from Global Create #40015
Conversation
# Conflicts: # src/pages/iou/request/IOURequestStartPage.js # src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js # src/pages/iou/request/step/withWritableReportOrNotFound.tsx
# Conflicts: # src/components/MoneyTemporaryForRefactorRequestConfirmationList.tsx # src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js # src/pages/iou/request/step/withFullTransactionOrNotFound.tsx # src/pages/iou/request/step/withWritableReportOrNotFound.tsx
# Conflicts: # src/libs/API/parameters/index.ts # src/libs/API/types.ts # src/libs/Navigation/types.ts # src/libs/ReportUtils.ts # src/pages/iou/request/IOURequestStartPage.tsx # src/pages/iou/request/step/withFullTransactionOrNotFound.tsx
# Conflicts: # src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left comments.
src/components/MoneyTemporaryForRefactorRequestConfirmationList.tsx
Outdated
Show resolved
Hide resolved
# Conflicts: # src/components/MoneyTemporaryForRefactorRequestConfirmationList.tsx # src/libs/PolicyUtils.ts # src/libs/actions/IOU.ts # src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js # src/pages/iou/request/step/IOURequestStepConfirmation.tsx
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Similar comment here as the other PR, I think we might not be using the correct room avatars here? cc @puneetlath I think the idea was to do this for Workspace to Workspace: And to do this for Workspace to Individual: Does that sound right Puneet & @davidcardoza ? |
Yes, that's right. Just like workspace chats we'd have the big sender avatar of the sender's workspace with the small payer user avatar. |
@cristipaval Can we please create a follow-up issue to update the invoice room avatars display?
|
I created the issue here: #41261 Let me know if I need to do anything else. |
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 1.4.67-7 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to staging by https://github.com/cristipaval in version: 1.4.68-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 1.4.67-7 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
There's a bug noted mostly coming from this PR #41287. If anyone can please take a look |
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.68-3 🚀
|
type SendInvoiceParams = { | ||
senderWorkspaceID: string; | ||
accountID: number; | ||
receiverEmail?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice little pro tip: either receiverEmail
or receiverInvoiceRoomID
is required. So I see why you made them both optional, but it later led to a problem here. So what you can do is use RequireAtLeastOne from type-fest.
We missed considering the enabled features of the collect workspace to show in the send invoice confirmation screen which raised an issue here. |
} | ||
const reportPreviewAction = ReportUtils.buildOptimisticReportPreview(chatReport, optimisticInvoiceReport, trimmedComment, optimisticTransaction); | ||
|
||
// STEP 6: Build Onyx Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We forgot to properly link the reportAction and parent report, which led to bug mentioned #43582
@@ -0,0 +1,19 @@ | |||
type SendInvoiceParams = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, This PR missed createdIOUReportActionID
, createdReportActionIDForThread
, reportActionID
Send Invoice params and caused these issues: #43797, #43571, #43577, #44992. More info in this proposal: #43797 (comment)
*/ | ||
function getPrimaryPolicy(activePolicyID?: string): Policy | undefined { | ||
const activeAdminWorkspaces = PolicyUtils.getActiveAdminWorkspaces(allPolicies); | ||
const primaryPolicy: Policy | null | undefined = allPolicies?.[activePolicyID ?? '']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we did not exclude personal policies while fetching primary policy, this resulted in #46705
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this PR caused this issue: #47922 because it missed updating the logic for invoking openDraftWorkspaceRequest
to retrieve the policy details (e.g., categories) when changing the invoice sender policy. More info in this proposal: #47922 (comment)
if (isNewChatReport) { | ||
inviteReportAction = ReportUtils.buildOptimisticInviteReportAction(receiver?.displayName ?? '', receiver.accountID ?? -1); | ||
} | ||
const reportPreviewAction = ReportUtils.buildOptimisticReportPreview(chatReport, optimisticInvoiceReport, trimmedComment, optimisticTransaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We returned an incorrect actorAccountID
for the invoice report here. It caused the report preview sender appears and disappears when sending invoices consecutively. More details here #51128 (comment)
Details
Implementation of invoice creation flow from Global Create
Fixed Issues
$ #40012
PROPOSAL: N/A
Tests
Note (!): Use staging server by turning it on in
Settings
->Troubleshoot
->Use Staging Server
Note (!): the backend has a know issue related to the invoice currency. If you want your test results to not be affected by that issue, set the sender workspace and the invoice currencies to be USD.
Flow 1
Send Invoice
appears in Global Create3.1 Verify “Distance” and “Scan" do not appear in the big number pad (BNP) screen.
3.2 Verify the Currency can be modified
6.1 The sender Workspace is displayed as expected.
6.2 The receiver is displayed as expected.
Send from
workspace, you should be navigated to the Send from the screen.Flow 2
Flow 3
Send Invoice
option in Global CreateOffline tests
Same as in the Tests section
QA Steps
Same as in the Tests section
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android1.mp4
Android: mWeb Chrome
android_web1.mp4
iOS: Native
ios1.mp4
iOS: mWeb Safari
ios_web1.mp4
MacOS: Chrome / Safari
web1.mp4
MacOS: Desktop
desktop1.mp4