-
Notifications
You must be signed in to change notification settings - Fork 3k
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 report approvals #24639
Implement report approvals #24639
Conversation
14fc52f
to
00fb139
Compare
4716a6b
to
5de9792
Compare
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.edu
andprincipal@school.edu
submitsTo
ofteacher@school.edu
toprincipal@school.edu
, meaning the principal will be the manager of the teacher. Do not set aforwardsTo
.principal@school.edu
approved ${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
$0
because we are not fetching the transactions from Auth and setting them in Onyx. The Split report action will also show$0
when you log out then back in.Offline tests
QA Steps
Must be QA'ed internally, same steps as above.
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
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)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
Mobile Web - Chrome
Screen.Recording.2023-08-28.at.14.33.18.mov
Mobile Web - Safari
Desktop
iOS
Android