Refactor the money request creation flow #28618
Conversation
|
We missed to handle one case #32865 |
| [reportID, transactionID], | ||
| ); | ||
|
|
||
| const goToNextStep = useCallback(() => { |
| return isPolicyExpenseChat ? OptionsListUtils.getPolicyExpenseReportOption(participant) : OptionsListUtils.getParticipantsOption(participant, personalDetails); | ||
| }) | ||
| .filter((participant) => !!participant.login || !!participant.text) |
| lat: null, | ||
| lng: null, | ||
| address: waypointValue, | ||
| name: values.name, |
There was a problem hiding this comment.
Coming from #33505
We didn't provide a fallback value, as we did in the "old flow".
| @@ -49,7 +49,7 @@ function EditRequestAmountPage({defaultAmount, defaultCurrency, onNavigateToCurr | |||
| > | |||
| <HeaderWithBackButton title={translate('iou.amount')} /> | |||
| <MoneyRequestAmountForm | |||
| isEditing | |||
There was a problem hiding this comment.
This removal caused #33011
| getRoute: (iouType: ValueOf<typeof CONST.IOU.TYPE>, transactionID: string, reportID: string) => `create/${iouType}/confirmation/${transactionID}/${reportID}/` as const, | ||
| }, | ||
| MONEY_REQUEST_STEP_AMOUNT: { | ||
| route: 'create/:iouType/amount/:transactionID/:reportID/', |
There was a problem hiding this comment.
Coming from #33205 (comment)
The routes added here have a trailing /. It lead to inconsistenecy because our routes don't have trailing slashes.
There was also a bug that broke "go back" because our code didn't handle trailing slashes.
| return ErrorUtils.getLatestErrorField(transaction, 'route'); | ||
| } | ||
|
|
||
| if (_.size(validatedWaypoints) < 2) { |
There was a problem hiding this comment.
Coming from #29895 (comment).
We missed a case when consecutive duplicate waypoints are present for more than 2 waypoints.
| {shouldShowDate && ( | ||
| <MenuItemWithTopDescription | ||
| shouldShowRightIcon={!isReadOnly} | ||
| title={iouCreated || format(new Date(), CONST.DATE.FNS_FORMAT_STRING)} |
There was a problem hiding this comment.
Coming from #33751 , this led to a regression where the order of menu items is incorrect. The correct order is :
- Distance
- Date
|
|
||
| // If we have a receipt let's start the split bill by creating only the action, the transaction, and the group DM if needed | ||
| if (iouType === CONST.IOU.TYPE.SPLIT && receiptFile) { | ||
| const existingSplitChatReportID = CONST.REGEX.NUMBER.test(reportID) ? reportID : ''; |
There was a problem hiding this comment.
We are passing the randomly generated reportID as existingSplitChatReportID cause a regression more details here #34060 (comment)
|
|
||
| const {unit, rate, currency} = mileageRate; | ||
| const distance = lodashGet(transaction, 'routes.route0.distance', 0); | ||
| const shouldCalculateDistanceAmount = isDistanceRequest && iouAmount === 0; |
There was a problem hiding this comment.
Coming from #34226, the amount remains unchanged when a user modifies the endpoint using the distance option on the confirmation page.shouldCalculateDistanceAmount prevents recalculation of the correct amount.
| <StepScreenWrapper | ||
| headerTitle={policyTagListName} | ||
| onBackButtonPress={navigateBack} | ||
| shouldShowWrapper |
There was a problem hiding this comment.
This pages was not restricted, An employee can access in a paid IOU the tag selection menu via a URL request we should show a not found page for this case
| useEffect(() => { | ||
| if (!trackRef.current) { | ||
| return; | ||
| } | ||
|
|
||
| trackRef.current.applyConstraints({ | ||
| advanced: [{torch: torchOn}], | ||
| }); | ||
| }, [torchOn]); |
There was a problem hiding this comment.
On native the torch is enabled only while taking a photo. This code made mWeb turn the torch on indefinitely causing inconsistency.
| return; | ||
| } | ||
|
|
||
| camera.current |
There was a problem hiding this comment.
Coming from this issue #36520, It's an edge case that we haven't covered yet. This is an async function, so we need to return it in order to work with useSingleExecution to avoid user tap multiple times
| <View style={styles.flex1}> | ||
| <DraggableList | ||
| data={waypointsList} | ||
| keyExtractor={(item) => item} |
There was a problem hiding this comment.
This might have caused this #40105
Details
This is a large refactoring for the money request creation flow. It will be done in two phases in an attempt to reduce conflicts with existing components. This PR builds out all the new routes and components while trying to leave as much of the original flows untouched. In a second PR, I will come back and remove all the old components and ensure no changes were missed during development (see this GH for phase 2).
I tried to leave as much of the original functionality untouched, so also be aware that as you review this, it might look like I am writing new code, when in actuality, the code was just copied from the original component. Again, this phase 2 GH lists the old components and their counter parts if you really want to compare the differences.
The main things that changed in this PR are:
transactionIDandreportIDin the URL. This ensures that each component is connected to the correct data.Fixed Issues
$ #26538
Tests
Offline tests
Same as the above
QA Steps
Same as the 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 */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
I struggled to create distance requests. It seems the map kept causing Chrome to crash in my Android emulator.
Mobile Web - Safari
Desktop
iOS
Android