-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add Approve button for collect policies in the Settlement button #28976
Conversation
Deploying with
|
Latest commit: |
516f0cc
|
Status: | ✅ Deploy successful! |
Preview URL: | https://60b54954.helpdot.pages.dev |
Branch Preview URL: | https://marco-approvebuttoncollectpo.helpdot.pages.dev |
Co-authored-by: Marc Glasser <[email protected]>
Co-authored-by: Marc Glasser <[email protected]>
Thanks for the review! Updated and ready for review again |
That was suggested here, but after applying the change the linter complained about it. Since this is just comment formatting, I will leave it as is. |
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.
Approving again :)
@cristipaval Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@marcochavezf Are we waiting on something? |
Just waiting for @marcaaron or @cristipaval for final review |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/marcochavezf in version: 1.4.12-0 🚀
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 1.4.12-0 🚀
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 1.4.12-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.12-2 🚀
|
@@ -147,7 +147,9 @@ function MoneyRequestView({report, parentReport, parentReportActions, policyCate | |||
if (!isDistanceRequest) { | |||
amountDescription += ` • ${translate('iou.cash')}`; | |||
} | |||
if (isCancelled) { | |||
if (ReportUtils.isReportApproved(report)) { |
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.
Coming from #35666
As @marcochavezf pointed out in this comment, we were getting the 'Approved' status from the wrong report (transaction report) instead of the money request
It should be ReportUtils.isReportApproved(moneyRequestReport)
On hold https://github.com/Expensify/Web-Expensify/pull/39328Details
This PR utilizes the
NextStep
logic from OldDot to show the "approve" or "pay" button based on the user role (either approver, reimburser, or both) and the reimbursement setup in the collect policy on NewDot (more details here). The approve button was moved to theSettlementButton
to show it as another option in the dropdown selector.This PR can only be tested by someone internal who can set up a control policy with an admin account to approve and reimburse the money requests from NewDot.
Fixed Issues
$ #29145
PROPOSAL:
Tests
Pre-requisites (only in OldDot)
Scenario 1: Approver as admin
Setup
Submit reports to:
inSubmit and Approve
.Direct
, and select the new collect policy admin as reimburser.Manual reimbursement threshold
to 0 (to test the Approval and then the Pay functionality).Tests (NewDot)
Approve
option appears in the settlement button.Cash - Approved
appears in the report and request preview, the system messageapproved $x.xx
appears in the expense report and the report header contains theapproved
text.paid
, and also the system message is shown in the expense report.Screen.Recording.2023-11-05.at.5.06.58.p.m.mov
Scenario 2: Approver as non-admin
Setup
Submit reports to:
inSubmit and Approve
.Tests (NewDot)
Approve
option appears in the settlement button.Cash - Approved
appears in the report and request preview, the system messageapproved $x.xx
appears in the expense report and the report header contains theapproved
text.paid
, and also the system message is shown in the expense report.Screen.Recording.2023-11-05.at.5.10.40.p.m.mov
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
Screen.Recording.2023-10-31.at.6.56.33.p.m.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Screen.Recording.2023-12-06.at.9.04.18.p.m.mov
Android