-
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
Feat: update the report when a payment is refunded #29344
Feat: update the report when a payment is refunded #29344
Conversation
@robertKozik 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] |
@getusha How can I test this PR? |
@robertKozik If you can do a code review, I can handle testing on all platforms since we don't have the backend pieces for this just yet. |
Sure! |
type CanceledRequestParams = {amount: string; submitterDisplayName: string}; | ||
|
||
type SettledAfterAddedBankAccountParams = {submitterDisplayName: string; amount: 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.
These two types are the same, can we merged them?
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.
Line 103 in ba75ce9
type RequestAmountParams = {amount: number}; |
Line 109 in ba75ce9
type AmountEachParams = {amount: number}; |
Line 107 in ba75ce9
type SplitAmountParams = {amount: number}; |
there are a lot of duplicate types, don't you think its better to keep it for its clear naming? if we want to add another param it'll be easier since we have types for each function, if not what should we name the type after combining it as one.
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.
Ok, makes sense, lets keep it as it is
Reviewer Checklist
Screenshots/VideosMobile Web - ChromeScreen.Recording.2023-11-28.at.4.12.18.PM.movAndroidScreen.Recording.2023-11-28.at.4.26.14.PM.movScreen.Recording.2023-11-28.at.4.27.18.PM.mov |
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.
Code looks good
@getusha can you merge main here? 🙏🏼 |
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.
@getusha I added a REIMBURSEMENTDEQUEUED action to an IOU report with this information:
The report action is shown without the amount and payee's email, is there something I'm missing?
I added a mock report action in my local dev database 😅 Do you know of a way to manually add Onyx data? |
@MariaHCD Let me try setting it using Onyx.merge, can you send me the full mock data you used? |
Mock data (you will have to change the accountIDs, reportIDs, etc.){
"9223114381001494361": {
"person": [
{
"type": "TEXT",
"style": "strong",
"text": "Platinum User"
}
],
"actorAccountID": 9412,
"message": [
{
"type": "TEXT",
"style": "strong",
"text": "You"
},
{
"type": "TEXT",
"style": "normal",
"text": " cancelled the reimbursement"
}
],
"originalMessage": {
"IOUReportID": 684660233193177,
"lastModified": "2023-10-17 14:14:52",
"reason": "CANCEL_REASON_PAYMENT_EXPIRED"
},
"avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_19.png",
"created": "2023-10-17 14:14:52",
"timestamp": 1697552092,
"reportActionTimestamp": 1697552092000,
"automatic": false,
"actionName": "REIMBURSEMENTDEQUEUED",
"shouldShow": true,
"reportActionID": "9223114381001494361",
"previousReportActionID": "5326942128745065457",
"lastModified": "2023-10-17 14:14:52",
"whisperedToAccountIDs": []
},
} |
@MariaHCD changed some things, can you test it again? 🙇 |
Apologies for the delay here, @getusha! I've retested and it looks better: What the payee/recipient sees: What the payer sees: |
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.
Lint errors 😬 |
@robertKozik could you give this another review? We've made some updates since your last review. |
One thing I noticed, @getusha, the IOU in the LHN doesn't show Payee:Screen.Recording.2023-11-28.at.3.41.19.PM.movPayer (Title in the LHN shows correctly but the last message is the payee's email, is that expected?):Screen.Recording.2023-11-28.at.3.47.14.PM.mov |
Sure I'll schedule it for today @MariaHCD. Are we still talking about only code review, or maybe backend is deployed and I should test cases as well? |
Just the code review for now, @robertKozik. Thanks! I'm updating the reviewer checklist with screens/videos of my tests |
@MariaHCD my understanding is that the |
Fixed this one. |
Ah, I see. That makes sense. Then, like you suggested before, adding a new prop from the backend (like |
@getusha I've added a new prop |
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.
Tests well and backend changes are out for review:
Final review, please, @robertKozik!
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.
Code-wise looks good to me as well
We did not find an internal engineer to review this PR, trying to assign a random engineer to #29319 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
🚀 Deployed to staging by https://github.com/MariaHCD in version: 1.4.7-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.7-4 🚀
|
Details
Fixed Issues
$ #29319
PROPOSAL: N/A
Tests
Same as QA
Offline tests
Same as QA
QA Steps
Internal QA, can be tested once this backend PR is deployed.
Canceled the $2.00 payment, because [email protected] did not enable their Expensify Wallet within 30 days
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
Screen.Recording.2023-11-28.at.2.47.38.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2023-11-28.at.1.20.59.PM.mov
MacOS: Desktop