Allow Editing Distance Requests #26141

jasperhuangg · 2023-08-28T23:13:55Z

~~Need https://github.com/Expensify/Web-Expensify/pull/38594 and #25707~~

Details

Fixed Issues

$ #22715

Tests/QA

Comment out this block so you can receive Pusher events.

Create a distance request following the steps here.
Navigate to the workspace chat that you created the distance request in. Click on the distance request preview to open the distance request.
Select the "Distance" menu item. Input new waypoints.
Click "Save". Verify that a loading spinner appears on the Save button and the modal closes after the transaction is saved.
After a bit (this could take a few seconds) verify the distance request amount and the title in the "Distance" menu item are updated to reflect your new waypoints.
Close the EditRequestDistancePage and edit all of the other fields. Verify that they still work as expected.

Offline tests

Go offline.
Edit any distance request fields.
Save the changes
Verify the modal closes and the distance field is greyed out in the pending state
Go online
Verify that after a few moments, the field is no longer pending and the changes were synced with the server

Note: There is a separate issue for implementing the RBR for server errors during this process.

PR Author Checklist

Screenshots/Videos

Web

web_1.mov

Mobile Web - Chrome

Mobile Web - Safari

mweb_safari_1.mp4

Desktop

desktop_1.mov

iOS

ios_1.mp4

Android

…ify/App into jasper-editDistanceRequests

…le edits

…nceRequests

fedirjh · 2023-09-26T10:25:27Z

We have some weird bugs

Open waypoint editor
Select a duplicate waypoint -> an error should appears
Close the modal
Open amount modal && Update the amount
Check the server response , it return the same error as step 2 💥
Verify the amount is not reset to its old value (since the request failed) 💥
Open the waypoints editor and remove the duplicate waypoint && press save
Verify the amount is not update 💥
Open amount modal again and update the amount
Check the server response , verify it return the same error related the waypoints even after you fixed it inside in previous steps 💥
Navigate again to the waypoint editor
Verify you see the sale error even when the duplicate point is removed 💥

CleanShot.2023-09-26.at.11.13.01.mp4

tgolen · 2023-09-27T00:50:06Z

@fedirjh Thank you for reporting that. I am finding it extremely difficult to work around the API limitations here, so I don't think this is going to work very well until the API is refactored in https://github.com/Expensify/Expensify/issues/312241. I am going to chat with @neil-marcellini and @arosiclair about it this morning to see if we can come up with a better API strategy to deal with this problem.

For now, I think it's OK to merge this with this known limitation (so we can unblock others that are waiting on a lot of the changes in this PR).

tgolen · 2023-09-27T04:29:06Z

@neil-marcellini this is ready for another review from you. Also, let me know what you think about the API issues above. The problem is that the UpdateDistanceRequest API command is used when updating amount (or any field other than distance). This requires each of those requests to have a valid set of waypoints. Effectively, if you end up with an error on the waypoints, all the other fields won't update until the errors have been cleared on the waypoints. Also, I tried omitting the waypoints completely when editing the other fields and the API returns an error that 2 valid waypoints are required.

fedirjh · 2023-09-27T16:25:36Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

CleanShot.2023-09-27.at.16.17.46.mp4

CleanShot.2023-09-27.at.16.19.08.mp4

Mobile Web - Chrome

CleanShot.2023-09-27.at.16.49.55.mp4

Mobile Web - Safari

CleanShot.2023-09-27.at.16.22.36.mp4

Desktop

CleanShot.2023-09-27.at.17.03.47.mp4

iOS

CleanShot.2023-09-27.at.16.36.51.mp4

Android

CleanShot.2023-09-27.at.17.07.58.mp4

fedirjh · 2023-09-27T16:32:43Z

Deep linking seems to be broken

CleanShot.2023-09-27.at.17.27.21.mp4

On the desktop, the save button is not stacked at the bottom; there is an extra margin at the bottom compared to other pages.

tgolen · 2023-09-28T01:52:01Z

Thank you!

I will handle the first bug as part of #26538 and is not a blocker for this PR.

For the second bug, I would prefer to merge this PR as-is, and then can you please report that bug through normal channels so that (you) or a contributor can pick it up to fix it?

Comments were addressed and I am trying to get this merged

OSBotify · 2023-09-28T02:21:35Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-09-28T19:17:10Z

🚀 Deployed to staging by https://github.com/tgolen in version: 1.3.75-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-10-02T17:21:05Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.75-12 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2023-10-02T17:49:40Z

🚀 Deployed to staging by https://github.com/tgolen in version: 1.3.76-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-10-03T15:50:37Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.76-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

s77rt · 2023-10-06T16:29:49Z

src/components/ReportActionItem/MoneyRequestView.js

+                        subtitle={hasErrors && isEmptyMerchant ? translate('common.error.enterMerchant') : ''}
+                        subtitleTextStyle={styles.textLabelError}


This caused a regression #28498. The error was displayed via the errors prop and not as a subtitle. This made the error appears next to the field instead of below.

OK, thanks! I imagine that got lost in the shuffle of conflicts I kept having to resolve.

DylanDylann · 2023-10-26T03:30:40Z

src/libs/actions/Transaction.ts

    Onyx.merge(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`, {
+        pendingFields: {
+            comment: isEditingWaypoint ? CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE : CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
+        },


@jasperhuangg Hi, I have a bit curious about this change. Why do you add pendingFields here? Normally, we only add pendingFields in optimisticData (before sending API) and reset pending field in successData and FailureData

Coming from #32120.
It looks like this pendingFields is not needed, we have removed it to prevent premature updates of data in the distance message preview.

rayane-d · 2025-01-22T17:19:59Z

src/pages/EditRequestDistancePage.js

+        // On mount, create the backup transaction.
+        TransactionEdit.createBackupTransaction(transaction);


Coming from issue #52413, The effect is also executed when refreshing the page on web, which causes the changes to be saved upon refresh.

Proposal: #52413 (comment)
PR: #54475

jasperhuangg added 11 commits August 25, 2023 15:10

Merge branch 'hayata-map-on-confirmation-screen' of github.com:Expens…

b75f0c6

…ify/App into jasper-editDistanceRequests

add distance request consts

43a7296

add DISTANCE_REQUEST ONYXKEY

28c293e

add routes for editdistancerequest and editrequestwaypoint

322504a

add distance translations

c5c72a5

add editDistanceRequest actions

85ac77d

add new pages for editing distance requests

4cac094

simplify DistanceRequest to just take a transactionID, modify to hand…

ec14060

…le edits

setup navigation

94e0137

updates

1bfff19

setup nav for moneyrequesteditwaypointpage

c55dcab

jasperhuangg self-assigned this Aug 28, 2023

jasperhuangg added 18 commits August 28, 2023 16:21

fix merge conficts

3dced83

remove unused

aac0217

remove unused

b868243

remove unused

2f63e7b

remove unused

f6637d5

remove unused

5944e01

remove unused

89dbc2d

remove unused

aa0ef77

remove unused

8ad2b54

remove unused

f594b64

remove unused

0585fbd

remove unused

b3eb1c5

remove unused

3263467

remove unused

211cb93

remove unused

9da21a7

Merge branch 'main' of github.com:Expensify/App into jasper-editDista…

fda8d5e

…nceRequests

rename variable

9d416c8

fix params

2782890

Delete the backup transaction in success data

0869c9d

Merge branch 'main' into jasper-editDistanceRequests

ad21933

Merge branch 'main' into jasper-editDistanceRequests

64b5073

tgolen approved these changes Sep 28, 2023

View reviewed changes

tgolen merged commit e1ba6aa into main Sep 28, 2023

tgolen deleted the jasper-editDistanceRequests branch September 28, 2023 02:14

OSBotify mentioned this pull request Sep 28, 2023

Deploy Checklist: New Expensify 2023-09-28 #28412

Closed

lanitochka17 mentioned this pull request Sep 29, 2023

[$500] Distance IOU - Recent appears in the Category list when there is no recent category #28517

Closed

6 tasks

BhuvaneshPatil mentioned this pull request Oct 2, 2023

[HOLD 28358][$500] Edited currency Changes back to the previous currency #28605

Closed

6 tasks

OSBotify mentioned this pull request Oct 2, 2023

Deploy Checklist: New Expensify 2023-10-02 #28635

Closed

melvin-bot bot mentioned this pull request Oct 2, 2023

[HOLD for payment 2023-10-09] 🟡 Editing distance requests in App #22715

Closed

s77rt reviewed Oct 6, 2023

View reviewed changes

s77rt mentioned this pull request Oct 6, 2023

[HOLD for payment 2023-10-13] Request money - Merchant scan error is not below the merchant field #28498

Closed

6 tasks

fedirjh mentioned this pull request Oct 11, 2023

[HOLD for payment 2023-10-10] [HOLD for payment 2023-10-09] [$500] Android - Scan IOU - Error shows up when tapping on Capture button to take a photo #28526

Closed

6 tasks

DylanDylann mentioned this pull request Oct 13, 2023

[$500] Distance - Amount is not clickable on Distance final confirmation screen #29477

Closed

6 tasks

DylanDylann reviewed Oct 26, 2023

View reviewed changes

DylanDylann mentioned this pull request Oct 28, 2023

[$500] Distance - Empty description row is grayed out in offline mode #30378

Closed

6 tasks

rayane-d reviewed Jan 22, 2025

View reviewed changes

		subtitle={hasErrors && isEmptyMerchant ? translate('common.error.enterMerchant') : ''}
		subtitleTextStyle={styles.textLabelError}

		// On mount, create the backup transaction.
		TransactionEdit.createBackupTransaction(transaction);

Allow Editing Distance Requests #26141

Allow Editing Distance Requests #26141

Uh oh!

Conversation

jasperhuangg commented Aug 28, 2023 • edited by tgolen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests/QA

Offline tests

PR Author Checklist

Screenshots/Videos

Uh oh!

fedirjh commented Sep 26, 2023

Uh oh!

tgolen commented Sep 27, 2023

Uh oh!

tgolen commented Sep 27, 2023

Uh oh!

fedirjh commented Sep 27, 2023 • edited by tgolen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

fedirjh commented Sep 27, 2023

Uh oh!

tgolen commented Sep 28, 2023

Uh oh!

OSBotify commented Sep 28, 2023

Uh oh!

OSBotify commented Sep 28, 2023

Uh oh!

OSBotify commented Oct 2, 2023

Uh oh!

OSBotify commented Oct 2, 2023

Uh oh!

OSBotify commented Oct 3, 2023

Uh oh!

s77rt Oct 6, 2023

Choose a reason for hiding this comment

Uh oh!

tgolen Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

DylanDylann Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntdiary Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

rayane-d Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

jasperhuangg commented Aug 28, 2023 •

edited by tgolen

Loading

fedirjh commented Sep 27, 2023 •

edited by tgolen

Loading

DylanDylann Oct 26, 2023 •

edited

Loading