-
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
pay conditions based on reimbursement settings #36814
pay conditions based on reimbursement settings #36814
Conversation
I drafted a PR and added Tests above but I think I need accounts in different paid policies with different reimbursement options for checking with Tests. Presently I have accounts for a member, approver and non-reimburser admin in a collect policy with reimbursement choice as Similarly, I require these accounts for policies with reimbursement options as Could you help with these? |
Hi @c3024, were you able to create the testing policy in OldDot? |
It was tricky. When I created a policy on old dot the value for I think I am able to create correctly now. I'll verify all tests I can with these now. If I am not able to verify any tests I'll ask for help. Thanks. @marcochavezf |
Cool, yeah also |
We are pushing App/src/components/SettlementButton.tsx Lines 145 to 146 in 3e77445
from here
for all expense reports. Then, even for But as per this comment I think we want to show only So, do we need to change the options pushed into payment options based on the policy reimbursement choice here App/src/components/SettlementButton.tsx Line 175 in 3e77445
in the SettlementButton as well?
Secondly, as per the same comment mentioned above we should show I think the following table is what we need finally, right?
|
Oh that's correct, we'd want to update the logic in SettlementButton too, so we either show
Yes, only
Yup
I think you're referring to
Just a minor modification: for the
|
Updated the test videos for Web Chrome. Could you review the PR? 🙏 @marcochavezf Will test on other platforms and upload if the changes look fine. Thanks. |
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 work! The logic LGTM overall! Just a few changes
It seems like this PR can be merged after @marcochavezf's review, doesn't need another C+ to review it? If I'm wrong, please feel free to correct me. 😄 |
All suggested changes added. |
@c3024 conflicts, also can you upload a video or screenshot for each reimbursement choice? |
Had already updated the videos in the checklist here. After merging For Could you check if the reimburser view works well? Otherwise please add me as a remiburser for this policy temporarily. Screen.Recording.2024-02-28.at.1.17.05.PM.movDirecttest1reimbursement.movFor IndirectcollectPolicyVideo.mp4indirectReimbursement.mp4NonecollectPolicyVideo.mp4noneReimbursement.mp4 |
@c3024 Tested with the Direct configuraiton and looks good! Additionally, you can follow these instructions to add a bank account (only staging) to your policy to test with a reimburser. I noticed we're not showing the Pay button for IOUs (Request money directly from someone) and for money requests from free workspaces: I think we need to check if the policy is not a group policy, then the user is a payer if it's isCurrentUserManager |
src/libs/PolicyUtils.ts
Outdated
@@ -117,6 +117,16 @@ const isFreeGroupPolicy = (policy: OnyxEntry<Policy> | EmptyObject): boolean => | |||
|
|||
const isPolicyMember = (policyID: string, policies: OnyxCollection<Policy>): boolean => Object.values(policies ?? {}).some((policy) => policy?.id === policyID); | |||
|
|||
const isPolicyPayer = (policy: OnyxEntry<Policy> | EmptyObject, session: OnyxEntry<Session>, isApproved: boolean, isManager: boolean, isAdmin: boolean): boolean => { |
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.
Let's change the name to isPayer
since it would be used for IOUs too to avoid a regression
const isPolicyPayer = (policy: OnyxEntry<Policy> | EmptyObject, session: OnyxEntry<Session>, isApproved: boolean, isManager: boolean, isAdmin: boolean): boolean => { | |
const isPayer = (policy: OnyxEntry<Policy> | EmptyObject, session: OnyxEntry<Session>, isApproved: boolean, isManager: boolean, isAdmin: boolean): boolean => { |
src/libs/PolicyUtils.ts
Outdated
if (policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_MANUAL) { | ||
return isAdmin && (isApproved || isManager); | ||
} | ||
return false; |
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.
In order to show the Pay button for IOUs (and free workspaces for now), I think we need to check if the user is manager and if the policy is not a group policy.
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.
Thank you! Focused too much only on policies and let this blunder slide. Fixed now.
One last question, we show only Pay elsewhere
for free workspaces, right? Or do we show Pay with Expensify
as well?
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.
Oh good question, we should show both Pay elsewhere
and Pay with Expensify
for non-group policies (free workspaces and IOUs). In theory for free workspaces the admin and the manager should be the same, so we should probably show both buttons to the manager if policy is a non-group policy/workspace.
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.
Thanks. Done. 👍
IOUs and Free workspacesIOUsAndFreeWorkspaces.mp4DirectpaidPolicies.mp4 |
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.
Looking great, thanks for the changes! Just a few other minor details and I think we're good to go
src/components/SettlementButton.tsx
Outdated
const policy = ReportUtils.getPolicy(policyID); | ||
const session = useSession(); | ||
const chatReport = ReportUtils.getReport(chatReportID); | ||
const isPaidGroupPolicy = ReportUtils.isPaidGroupPolicy(iouReport as OnyxEntry<Report>) || ReportUtils.isPaidGroupPolicyExpenseChat(chatReport as OnyxEntry<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 can check only isPaidGroupPolicyExpenseChat
since isPaidGroupPolicy
is already called in isPaidGroupPolicyExpenseChat
:
const isPaidGroupPolicy = ReportUtils.isPaidGroupPolicy(iouReport as OnyxEntry<Report>) || ReportUtils.isPaidGroupPolicyExpenseChat(chatReport as OnyxEntry<Report>); | |
const isPaidGroupPolicy = ReportUtils.isPaidGroupPolicyExpenseChat(chatReport as OnyxEntry<Report>); |
src/libs/ReportUtils.ts
Outdated
session: OnyxEntry<Session>, | ||
iouReport: OnyxEntry<Report>, | ||
// eslint-disable-next-line @typescript-eslint/no-shadow | ||
isPaidGroupPolicy: boolean, |
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 we can use the function PolicyUtils.isPaidGroupPolicy(policy)
inside of isPayer
so it won't be necessary to pass isPaidGroupPolicy
as prop and diable the eslint rule
src/libs/ReportUtils.ts
Outdated
// eslint-disable-next-line @typescript-eslint/no-shadow | ||
const isPayer = currentUserAccountID === report?.managerID; | ||
|
||
return isPayer ? [managerIcon, ownerIcon] : [ownerIcon, managerIcon]; |
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.
Let's rename the variable here to also avoid the no-shadow eslint error, also the managerID is not always the payer, so the correct variable name here would be isManager
:
// eslint-disable-next-line @typescript-eslint/no-shadow | |
const isPayer = currentUserAccountID === report?.managerID; | |
return isPayer ? [managerIcon, ownerIcon] : [ownerIcon, managerIcon]; | |
const isManager = currentUserAccountID === report?.managerID; | |
return isManager ? [managerIcon, ownerIcon] : [ownerIcon, managerIcon]; |
Thanks for the suggestions. Fixed. I tested all except the reimburser's view. I could not create a policy with a bank account. Can you please verify the reimburser view? IOUandFreeWorkspace.mp4indirectAndNonePolicy.mp4collectPolicyWithReimburser.mp4 |
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.
Cool! I tested all the scenarios and looks good!
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Desktop |
✋ 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.48-0 🚀
|
Notified internally to the QA team about the changes for the Pay button. GH to update the TC in TR here |
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.48-0 🚀
|
Details
This PR modifies pay conditions for different policies based on their reimbursement settings. Refer to the discussion in the referred issue.
Fixed Issues
$ #34951
PROPOSAL:
Tests
Use the following table for reference:
Pay with Expensify
orPay elsewhere
Pay elsewhere
only(*) We only show Pay button in
Direct
reimbursement when the money request's amount is below theManual Reimbursement
threshold in OldDot:Test 1
Pre-requisite: A paid policy with reimbursement choice as
Direct
with a non-reimburser admin (an admin who is not a reimburser), a reimburser-admin, and a member.On New Dot
Pay with Expensify
andPay elsewhere
buttons appear, after approval, only for the reimburser admin and not the non-reimburser adminTest 2
Pre-requisite: A paid policy with reimbursement choice as
Indirect
with an admin and a member.On New Dot
Pay Elsewhere
button appears for the admin after approvalTest 3
Pre-requisite: A paid policy with reimbursement choice as
None
with an admin and a member.On New Dot
Offline tests
QA Steps
Same as testing 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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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
test1Android.mp4
test2and3Android.mp4
Android: mWeb Chrome
test1AndroidChrome.mp4
test2And3AndroidChrome.mp4
iOS: Native
test1iOS.mp4
test2and3iOS.mp4
iOS: mWeb Safari
test1iOSSafari.mp4
test2and3iOSSafari.mp4
MacOS: Chrome / Safari
Test 1test1reimbursement.mov
Test 2 and Test 3
test23reimbursement.mp4
MacOS: Desktop
test2and3Desktop.mp4