Implement report approvals #24639
Implement report approvals #24639
Conversation
youssef-lr
force-pushed
the
youssef_approve_money_request
branch
from
14fc52f
to
00fb139
Compare
youssef-lr
force-pushed
the
youssef_approve_money_request
branch
from
4716a6b
to
5de9792
Compare
youssef-lr
changed the title
@luacmartins Hmm I'm not able to reproduce this, can you show me please how the configuration looks like in oldDot => people? |
Hmm yeah I noticed this recently, I could swear it was working fine before. I'll create an issue for this to be fixed in Web-E. Edit: created issue https://github.com/Expensify/Expensify/issues/311998 |
There was a problem hiding this comment.
Worked 1:1 with @youssef-lr to solve remaining blockers. All good now!
|
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
tests passed |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@youssef-lr seems causing a console warning regression where policy is passed undefined and has required prop for the child component MoneyRequestHeader. 
https://expensify.slack.com/archives/C049HHMV9SM/p1693292429308959 |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.59-0 🚀
|
| return false; | ||
| } | ||
|
|
||
| return report.stateNum === CONST.REPORT.STATE_NUM.SUBMITTED && report.statusNum === CONST.REPORT.STATUS.REIMBURSED; |
There was a problem hiding this comment.
This is causing the deploy blocker here: #26322
If I change this line back to
return report.stateNum > CONST.REPORT.STATE_NUM.PROCESSING;
It works again.
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.60-0 🚀
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.60-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.60-3 🚀
|
| // If the report is already settled, there's no action required from any user. | ||
| if (isSettled(report.reportID)) { | ||
| return false; | ||
| } | ||
|
|
||
| // Report is pending approval and the current user is the manager | ||
| if (isReportManager(report) && !isReportApproved(report)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Current user is an admin and the report has been approved but not settled yet | ||
| return policy.role === CONST.POLICY.ROLE.ADMIN && isReportApproved(report); |
There was a problem hiding this comment.
I think this may be missing the case where the employee is missing a bank account and the report gets the RNVP isWaitingOnBankAccount . For CORPORATE policies, I think we should also return true if the report has isWaitingOnBankAccount and the current user is the submitter, right?
There was a problem hiding this comment.
And we should return false if report.isWaitingOnBankAccount is truthy and the current user is NOT the submitter because the we are expecting an action on the submitter. What actions could the admin or manager take?
| } | ||
| // STEP 2: Get existing IOU/Expense report and update its total OR build a new optimistic one | ||
| // For Control policy expense chats, if the report is already approved, create a new expense report | ||
| let oneOnOneIOUReport = lodashGet(allReports, `${ONYXKEYS.COLLECTION.REPORT}${oneOnOneChatReport.iouReportID}`, undefined); |
There was a problem hiding this comment.
Coming from #28330:
As this line was called without !isNewOneOnOneIOUReport check, it was possible to get all reports with oneOnOneChatReport.iouReportID being empty string.
| let amount = CurrencyUtils.convertToDisplayString(total, currency); | ||
| const amount = | ||
| type === CONST.IOU.REPORT_ACTION_TYPE.PAY | ||
| ? CurrencyUtils.convertToDisplayString(getMoneyRequestTotal(getReport(iouReportID)), currency) |
There was a problem hiding this comment.
When we create all optimistic reports, report actions etc the report is not readily available in Onyx and even though this function gets total as a param the total is shown as zero for the first time the optimistic actions were created. This caused #43907
| */ | ||
| function getPolicyName(report, returnEmptyIfNotFound = false, policy = undefined) { | ||
| const noPolicyFound = returnEmptyIfNotFound ? '' : Localize.translateLocal('workspace.common.unavailable'); | ||
| if (_.isEmpty(report)) { |
There was a problem hiding this comment.
We should had checked if allPolicies are empty here, this caused #44480
Details
cc @mountiny @luacmartins
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/303752
Tests
teacher@school.eduandprincipal@school.edusubmitsToofteacher@school.edutoprincipal@school.edu, meaning the principal will be the manager of the teacher. Do not set aforwardsTo.principal@school.eduapproved ${amount}action has been created.Cash • Approved.principal@school.edu approved ${amount}.Note: there is currently a bug being worked on where the expense report shows initially
$0because we are not fetching the transactions from Auth and setting them in Onyx. The Split report action will also show$0when you log out then back in.Offline tests
QA Steps
Must be QA'ed internally, same steps as above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Screen.Recording.2023-08-28.at.14.33.18.mov
Mobile Web - Safari
Desktop
iOS
Android