-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Remove MoneyRequestConfirmationList
and replace to MoneyTemporaryForRefactorRequestConfirmationList
#40176
Conversation
MoneyTemporaryForRefactorRequestConfirmationList
MoneyRequestConfirmationList
and replace to MoneyTemporaryForRefactorRequestConfirmationList
@eh2077 our PR is ready for review. |
|
||
useEffect(() => { | ||
if (!shouldCalculateDistanceAmount) { | ||
return; | ||
} | ||
|
||
const amount = DistanceRequestUtils.getDistanceRequestAmount(distance, unit, rate ?? 0); | ||
IOU.setMoneyRequestAmount(amount); | ||
}, [shouldCalculateDistanceAmount, distance, rate, unit]); | ||
IOU.setMoneyRequestAmount_temporaryForRefactor(transaction?.transactionID ?? '', amount, 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.
IOU.setMoneyRequestAmount
can be replaced by IOU.setMoneyRequestAmount_temporaryForRefactor
right? If so, then I think we should remove IOU.setMoneyRequestAmount
and rename the temporary method.
I'm not sure if IOU.setMoneyRequestAmount_temporaryForRefactor
can fit split distance request.
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 believe so, however, you were right about SplitBill
, I changed it to be dynamic when going to CREATE
OR EDIT
. Can you take a look, please? Especially this commit
@brunovjk I think we should test each IOU flow separately to ensure that these critical flows are not broken. We have four IOU types, [
Would you like to update test steps? |
Co-authored-by: Eric Han <[email protected]>
@brunovjk Thanks for starting this discussion on Slack! The fix PR #40385 should be merged soon. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemobile-chrome.moviOS: Native0-ios.mp4iOS: mWeb Safari0-mobile-safari.mp4MacOS: Chrome / Safari0-send-requst-track-split-money.mp4MacOS: Desktop0-desktop.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.
LGTM and tested well!
I can review this today, but there are a few conflicts currently. Can you please fix them before I review it? Thanks! |
Resolved the conflicts, let me test again. |
OK, the changes look fine to me. Let me know once you've run through a few tests and then I'll merge it. |
Good to go @tgolen :D |
Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.64-6 🚀
|
Thanks, @Krishna2323, for catching that. My apologies for the reintroduced issue. I'm available to submit another PR if the team agrees. Do we only need to remove
and add back "eslint-disable-next-line"? |
@brunovjk, I assume transactionID remains unchanged when transaction details are altered, so we can leave it as is. We only need to remove transaction, but please verify this while recording the videos to be certain. Thanks :) |
@Krishna2323 Sorry, I can't reproduce #38744 on production and staging anymore. I try both category and tag. @brunovjk Can you reproduce that? |
Details
This is a part of #29107 and is all about removing the original components and any of the duplicated efforts.
Old Component:
MoneyRequestConfirmationList
.New Component
MoneyTemporaryForRefactorRequestConfirmationList
.Fixed Issues
$ #39559
PROPOSAL: #39559 (comment)
Tests
This is a refactoring PR, everything should work the same as before.
The
MoneyRequestConfirmationList
component is utilized across various IOU types (send, split, request, track-expense) to confirm requests (manual, scan, distance). Therefore, comprehensive testing is required across all flows.On the
Confirmation Page
, thoroughly test each field:Next/Request/Send/Track, ...
to proceed.Next/Request/Send/Track, ...
without changing the value.Back button
and confirm that the value remains unchanged and all other fields are correct.Confirmation Page
press theBack button
, pressNext
from the previous page, and confirm that theConfirmation Page
retains its state.After completing the field testing, press the
Request
button and confirm redirection to the corresponding chat (report).For Split flows, click on the newly created report preview and verify redirection to the Confirmation Page. Ensure all fields are correct and expected. Additionally, verify the ability to edit editable fields.
Below are steps to reach the Confirmation Page in each flow:
Send Money (Global Fab):
Request Money - Manual (Global Fab):
Request Money - Scan (Global Fab):
Request Money - Distance (Global Fab):
Track Expense - Manual (Global Fab):
Track Expense - Scan (Global Fab):
Track Expense - Distance (Global Fab):
Split Bill - Manual (Workspace Default Chat):
Split Bill - Scan (Workspace Default Chat):
Send Money (1:1 Chat):
Request Money - Manual (1:1 Chat):
Request Money - Scan (1:1 Chat):
Split Bill - Manual (1:1 Chat):
Split Bill - Scan (1:1 Chat):
Track Expense - Manual (Workspace Chat):
Track Expense - Scan (Workspace Chat):
Track Expense - Distance (Workspace Chat):
Request Money - Manual (Workspace Chat):
Request Money - Scan (Workspace Chat):
Request Money - Distance (Workspace Chat):
Split Bill - Manual (Workspace Chat):
Split Bill - Scan (Workspace Chat):
Split Bill - Distance (Workspace Chat):
Verify that no errors appear in the JS console
Offline tests
Same as
Tests
, mirroring online ones with optimistic values.QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.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 and/or tagged@Expensify/design
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
iOS: Native
39559.-.iOS_.Native.mp4
iOS: mWeb Safari
39559.-.iOS_.mWeb.Safari.mp4
MacOS: Chrome / Safari
39559.-.MacOs_.Safari.mp4
MacOS: Desktop
39559.-.MacOS_.Desktop.mp4
Android: Native
Android: mWeb Chrome