[No QA] Allow editing fields of Request #20206

marcaaron · 2023-06-05T20:29:36Z

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/270589

Tests

Uncomment out the lines of code here:

diff --git a/src/components/MoneyRequestHeader.js b/src/components/MoneyRequestHeader.js
index ee74a89826..b84a2bf9f5 100644
--- a/src/components/MoneyRequestHeader.js
+++ b/src/components/MoneyRequestHeader.js
@@ -189,23 +189,23 @@ const MoneyRequestHeader = (props) => {
                         description={`${props.translate('iou.amount')} • ${props.translate('iou.cash')}${isSettled ? ` • ${props.translate('iou.settledExpensify')}` : ''}`}
                         titleStyle={styles.newKansasLarge}
                         // Note: These options are temporarily disabled while we figure out the required API changes
-                        // shouldShowRightIcon
-                        // iconRight={Expensicons.ArrowRight}
-                        // onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(props.report.reportID, CONST.EDIT_REQUEST_FIELD.AMOUNT))}
+                        shouldShowRightIcon
+                        iconRight={Expensicons.ArrowRight}
+                        onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(props.report.reportID, CONST.EDIT_REQUEST_FIELD.AMOUNT))}
                     />
                     <MenuItemWithTopDescription
                         description={props.translate('common.description')}
                         title={transactionDescription}
-                        // shouldShowRightIcon
-                        // iconRight={Expensicons.ArrowRight}
-                        // onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(props.report.reportID, CONST.EDIT_REQUEST_FIELD.DESCRIPTION))}
+                        shouldShowRightIcon
+                        iconRight={Expensicons.ArrowRight}
+                        onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(props.report.reportID, CONST.EDIT_REQUEST_FIELD.DESCRIPTION))}
                     />
                     <MenuItemWithTopDescription
                         description={props.translate('common.date')}
                         title={formattedTransactionDate}
-                        // shouldShowRightIcon
-                        // iconRight={Expensicons.ArrowRight}
-                        // onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(props.report.reportID, CONST.EDIT_REQUEST_FIELD.DATE))}
+                        shouldShowRightIcon
+                        iconRight={Expensicons.ArrowRight}
+                        onPress={() => Navigation.navigate(ROUTES.getEditRequestRoute(props.report.reportID, CONST.EDIT_REQUEST_FIELD.DATE))}
                     />
                 </>
             )}

Create a new workspace as admin and invite an employee account
Sign in as employee
Request money via the Workspace chat - adding all fields (amount, description)
Navigate to the workspace chat
Tap the expense to navigate to the screen with the expense details
Verify the rows have carets and are tappable
Tap description row and verify you are navigated to the Description screen
Update the description and hit "Save"
Check the JS console to confirm the correct description is logged.

Verify that no errors appear in the JS console

Offline tests

N/A or if there are any we will have to consider them when making the API changes. For now, this feature will not yet be accessible.

QA Steps

There is no QA because the code we are adding is not accessible yet.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot · 2023-06-09T16:07:58Z

@0xmiroslav @grgia One of you needs to 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]

luacmartins

UI changes are looking good. I left a few small comments.

Additionally, I'm seeing a weird behavior with the date picker.

Tap the year selector
Notice that the background is animated (this is issue #1)
Select a year
Notice that the year doesn't change when you go back to the full date picker

date.mov

src/ONYXKEYS.js

src/components/MoneyRequestHeader.js

src/components/RequestCreated.js

src/components/RequestDescription.js

src/pages/iou/IOUCurrencySelection.js

src/pages/EditRequestPage.js

src/components/MoneyRequestHeader.js

marcaaron · 2023-06-09T19:50:36Z

Hmm that bug sucks haha. Ok I think it's probably something to do with the routing because the DOB picker works fine.

marcaaron · 2023-06-09T19:59:59Z

Ah yep the "year picker" page is defined in the settings stack here:

App/src/libs/Navigation/linkingConfig.js

Lines 219 to 221 in 38ba7c8

    
           YearPicker_Root: { 
        
               path: ROUTES.SELECT_YEAR, 
        
           },

I think maybe we can just remove it from that stack and have it be pushed as it's own navigator. Maybe not the best solution, but the first one that comes to mind.

marcaaron · 2023-06-09T20:08:20Z

Hmm ok am actually pretty confused about how this works now and need to look into it some more. The code here implies we would need to check the route for each new usage of the date picker 😞

App/src/pages/YearPickerPage.js

Lines 66 to 77 in 38ba7c8

    
           updateSelectedYear(selectedYear) { 
        
               // We have to navigate using concatenation here as it is not possible to pass a function as a route param 
        
               const routes = lodashGet(this.props.navigation.getState(), 'routes', []); 
        
               const dateOfBirthRoute = _.find(routes, (route) => route.name === 'Settings_PersonalDetails_DateOfBirth'); 
        
               if (dateOfBirthRoute) { 
        
                   Navigation.setParams({year: selectedYear.toString()}, lodashGet(dateOfBirthRoute, 'key', '')); 
        
                   Navigation.goBack(); 
        
               } else { 
        
                   Navigation.goBack(`${ROUTES.SETTINGS_PERSONAL_DETAILS_DATE_OF_BIRTH}?year=${selectedYear}`); 
        
               } 
        
           }

I, for some reason, expected that something called DatePicker would "just work" 😄

marcaaron · 2023-06-09T20:48:49Z

@luacmartins The good news is that I fixed that bug...

The bad news is that I'm not feeling too great about the NewDatePicker in general. It looks like we will either need to refactor it to be more flexible and work with screens (specifically, one's with dynamic routes) or accept a temporary hack for now and follow up later to fix it.

Maybe I'm missing some obvious alternative that you will spot. The code looks a little wacky to me. Like the currency selector it's also using routes to pass parameters. And the routes are hardcoded to the component.

I attempted to start some conversation about the way we are passing around parameters in routes here earlier this week: https://expensify.slack.com/archives/C01GTK53T8Q/p1686161800267329, but it didn't get much engagement.

marcaaron · 2023-06-09T21:10:11Z

Asking here: https://expensify.slack.com/archives/C01GTK53T8Q/p1686344969813909

luacmartins · 2023-06-09T21:13:49Z

I agree that the component feels very brittle. Let's keep the discussion in that thread though

luacmartins

Code LGTM, but we have conflicts

src/pages/YearPickerPage.js

src/components/MoneyRequestHeader.js

grgia

Testing locally, I can open each caret, and the correct changes are logged. But should the admin account be able to change the amount/description as well?

luacmartins · 2023-06-14T15:26:58Z

@grgia yes, admins can edit transactions too!

luacmartins · 2023-06-14T17:34:43Z

@marcaaron can you resolve conflicts. @0xmiroslav would appreciate your review on this one.

marcaaron · 2023-06-16T18:33:27Z

Fixed. There are 3 internal engineers tagged on this issue. If @grgia is already testing maybe we can take @0xmiroslav off the review (they haven't chimed in yet and this PR has been open for a week).

grgia · 2023-06-19T16:53:48Z

Are we planning on merging this with the console logs?

marcaaron · 2023-06-19T17:22:27Z

Are we planning on merging this with the console logs?

It's unusual and not our best practice - but I would advise we merge it as is (or after review comments). The flows are commented out until we are ready to use them and we will remove that console.log soon.

The lack of reviews here is affecting my ability to GSD. So, I would prefer not to wait for the date picker refactor to happen (if that's the current blocker or cause for delay here).

luacmartins

There's a crash when accessing the description page

src/components/RequestDescription.js

marcaaron · 2023-06-19T20:49:58Z

Ok I think the crash should be fixed now. The hacky code to fix the cursor position threw me off my game.

grgia · 2023-06-20T16:14:07Z

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 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

Screen.Recording.2023-06-20.at.5.31.13.PM.mov

Mobile Web - Chrome

Mobile Web - Safari

Desktop

Screen.Recording.2023-06-20.at.5.26.49.PM.mov

iOS

Screen.Recording.2023-06-20.at.5.29.28.PM.mov

Android

src/pages/EditRequestPage.js

grgia · 2023-06-20T16:41:21Z

mobile safari / mobile chrome are crashing for me (not on main) @marcaaron

marcaaron · 2023-06-20T21:30:01Z

@grgia did you pull the branch before re-testing? I am testing on mWeb and don't see any crashes.

marcaaron · 2023-06-21T19:16:14Z

Quick update here. This PR is a lot simpler now. TL;DR I got some bad conflicts from this PR and the MoneyRequestAmountPage will need to be looked into more closely as there were other dependencies added.

Originally, I set out to refactor that component and the description stuff so that things would be more DRY, but abandoned that for now as it will take too much time to improve and this PR continues to get confusing conflicts leading to delays in general.

So what did this PR end up... doing??

Adds the route for the edit request page.
Adds the edit description page (this will need to be refactored)

The other pages will also need more thought to incorporate so I suggest we do a follow up for each one and then do a final PR to call the API.

marcaaron · 2023-06-21T19:26:37Z

Updated

luacmartins

LGTM and tests well.

marcaaron · 2023-06-21T20:55:27Z

Cool. I am skipping testing on the rest of the platforms because we have been waiting a while and parts of the UI we are adding are not even accessible. We can test the flow again in the later stages of development. I am confident that these changes are fine.

OSBotify · 2023-06-21T21:03:12Z

✋ 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-06-22T17:21:08Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.31-0 🚀

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

OSBotify · 2023-06-23T21:05:55Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.31-3 🚀

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

OSBotify · 2023-06-23T21:25:40Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.31-3 🚀

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

marcaaron self-assigned this Jun 5, 2023

marcaaron changed the title ~~[WIP & HOLD] Update UI so that menu items appear tappable~~ [WIP & HOLD] Allow editing fields of Request Jun 5, 2023

marcaaron marked this pull request as ready for review June 9, 2023 16:07

marcaaron requested a review from a team as a code owner June 9, 2023 16:07

marcaaron requested a review from luacmartins June 9, 2023 16:07

melvin-bot bot requested review from 0xmiroslav and grgia and removed request for a team June 9, 2023 16:07

marcaaron changed the title ~~[WIP & HOLD] Allow editing fields of Request~~ [No QA] Allow editing fields of Request Jun 9, 2023

luacmartins requested changes Jun 9, 2023

View reviewed changes

marcaaron requested a review from luacmartins June 9, 2023 19:45

luacmartins reviewed Jun 12, 2023

View reviewed changes

src/pages/YearPickerPage.js Outdated Show resolved Hide resolved

Julesssss self-requested a review June 13, 2023 16:47

Julesssss reviewed Jun 13, 2023

View reviewed changes

src/components/MoneyRequestHeader.js Show resolved Hide resolved

luacmartins mentioned this pull request Jun 13, 2023

Make NewDatePicker reusable #20698

Closed

mountiny mentioned this pull request Jun 14, 2023

[$1000] [HOLD for payment 2023-07-20] [Manual Requests] Make the Datepicker component more reusable #20630

Closed

grgia reviewed Jun 14, 2023

View reviewed changes

marcaaron requested a review from luacmartins June 16, 2023 18:31

luacmartins requested changes Jun 19, 2023

View reviewed changes

src/components/RequestDescription.js Outdated Show resolved Hide resolved

grgia reviewed Jun 20, 2023

View reviewed changes

src/pages/EditRequestPage.js Outdated Show resolved Hide resolved

grgia removed the request for review from 0xmiroslav June 20, 2023 16:21

luacmartins previously approved these changes Jun 21, 2023

View reviewed changes

marcaaron added 3 commits June 21, 2023 09:04

Remove changes related to Amount page

07b6401

Add pages

1f8a90d

Merge branch 'main' into marcaaron-editManualRequests

50f15c8

marcaaron dismissed luacmartins’s stale review via 50f15c8 June 21, 2023 19:09

marcaaron force-pushed the marcaaron-editManualRequests branch from 65ae4e9 to 50f15c8 Compare June 21, 2023 19:09

fix style

1e01612

luacmartins reviewed Jun 21, 2023

View reviewed changes

luacmartins approved these changes Jun 21, 2023

View reviewed changes

marcaaron merged commit be4ee9a into main Jun 21, 2023

marcaaron deleted the marcaaron-editManualRequests branch June 21, 2023 20:55

OSBotify mentioned this pull request Jun 22, 2023

Deploy Checklist: New Expensify 2023-06-22 #21330

Closed

39 tasks

dhairyasenjaliya mentioned this pull request Jul 5, 2023

[Awaiting Payment August 2nd] [$1000] input is not auto-focused on debit card details page #21587

Closed

6 tasks

[No QA] Allow editing fields of Request #20206

[No QA] Allow editing fields of Request #20206

Uh oh!

Conversation

marcaaron commented Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Jun 9, 2023

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcaaron commented Jun 9, 2023

Uh oh!

marcaaron commented Jun 9, 2023

Uh oh!

marcaaron commented Jun 9, 2023

Uh oh!

marcaaron commented Jun 9, 2023

Uh oh!

marcaaron commented Jun 9, 2023

Uh oh!

luacmartins commented Jun 9, 2023

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

grgia left a comment

Choose a reason for hiding this comment

Uh oh!

luacmartins commented Jun 14, 2023

Uh oh!

luacmartins commented Jun 14, 2023

Uh oh!

marcaaron commented Jun 16, 2023

Uh oh!

grgia commented Jun 19, 2023

Uh oh!

marcaaron commented Jun 19, 2023

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcaaron commented Jun 19, 2023

Uh oh!

grgia commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

Uh oh!

grgia commented Jun 20, 2023

Uh oh!

marcaaron commented Jun 20, 2023

Uh oh!

marcaaron commented Jun 21, 2023

Uh oh!

marcaaron commented Jun 21, 2023

Uh oh!

luacmartins left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Jun 5, 2023 •

edited

Loading

grgia commented Jun 20, 2023 •

edited

Loading