Fix/34609: Remove old description page

[DylanDylann]

Details

Remove MoneyRequestDescriptionPage and EditRequestDescriptionPage and only use IOURequestStepDescription for all description pages

Fixed Issues

$ https://github.com/Expensify/App/issues/34609 PROPOSAL: https://github.com/Expensify/App/issues/34609#issuecomment-1894910002

Tests

1. Create a new request
2. While creating a new request click on the description field
3. Verify that the route display /create/request/
4. After creating request click on description field to edit
5. Verify that the route display /edit/request/
6. Create a split bill
7. While creating a new split bill click on the description field
8. Verify that the route display /create/split/
9. After creating a new split bill click on description field to edit
10. Verify that the route display /edit/split/
11. Verify that everything works well as before

• [x] Verify that no errors appear in the JS console

Offline tests

Same above

QA Steps

Same above

• [x] Verify that no errors appear in the JS console

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for the expected offline behavior in the Offline steps section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] 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)
  • [x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] 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)
  • [x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] 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.
  • [x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • [x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] 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.
  • [x] 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.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] 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
• [x] I verified that if a function's arguments changed that all usages have also been updated correctly
• [x] If any new file was added I verified that:
  • [x] 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
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• [x] 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.
• [x] 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)
• [x] 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.
• [x] 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.
• [x] If the PR modifies the form input styles:
  • [x] I verified that all the inputs inside a form are aligned with each other.
  • [x] I added Design label so the design team can review the changes.
• [x] 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.
• [x] 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.

Screenshots/Videos

Android: Native

https://github.com/Expensify/App/assets/141406735/b17df686-622e-46ae-8f52-a7d3f5ee3e38

https://github.com/Expensify/App/assets/141406735/c32521b9-0a7e-4b0c-8e91-549f7656d1fb

Android: mWeb Chrome

https://github.com/Expensify/App/assets/141406735/69a4dcd2-92e6-4e84-81b5-280fac408c56

iOS: Native

https://github.com/Expensify/App/assets/141406735/fd13ee23-39b2-49a0-8a1a-bbeaf5eb15cf

iOS: mWeb Safari

https://github.com/Expensify/App/assets/141406735/38373ece-f758-4324-8622-3dc6e892a3e7

MacOS: Chrome / Safari

https://github.com/Expensify/App/assets/141406735/62f74f40-2486-45da-b348-e95028419548

MacOS: Desktop

https://github.com/Expensify/App/assets/141406735/452105df-8964-42e1-bbc2-d7bd8726313e

[melvin-bot[bot]]

@s77rt 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]

[s77rt]

Trying to edit a split bill request crashes the app, please make sure we are replacing all the old routes

https://github.com/Expensify/App/assets/16493223/743b804c-b759-4ec9-9bdf-13156b8ab832

[DylanDylann]

@s77rt I can't reproduce your bug

https://github.com/Expensify/App/assets/141406735/03b0edac-0b34-44ef-bcbb-b606e7e4b88c

Could you help to point out the step to create this request?

[DylanDylann]

@s77rt Thanks for your commit suggestion. I updated

[DylanDylann]

@s77rt From your comment, I think we should revert this change in here. I agree with you that

it will be much confusing to see isDraft false yet still update the drafts collections

But we have 3 cases when user enter description and click save button:

1. Create money request/Create split bill: We will save description to transactionsDraft and only send API if user click request money
2. Edit money request: Send API immediately to update new description
3. Edit split bill: We will save description to splitTransactionsDraft and only send API if user click save in the detail split bill page

[DylanDylann]

Note that in case 3: we only call setDraftSplitTransaction if isDraft is true and iouType === CONST.IOU.TYPE.SPLIT. Draft in isDraft and draft in setDraftSplitTransaction are different. Draft in isDraft is to check if it is creating or editing flow. draft in setDraftSplitTransaction is a draft description. We can consider to rename one of them to be more clear. Please correct me if I miss anything

[s77rt]

Edit money request: Send API immediately to update new description

We should update the transaction too same as we do with receipt

[s77rt]

https://github.com/Expensify/App/pull/35137#issuecomment-1915289028

Creating/Editing a split request => isDraft = true (and a comment explaining the reason) Creating/Editing a regular request => isDraft = action === CREATE

[DylanDylann]

@s77rt

Creating/Editing a split request => isDraft = true (and a comment explaining the reason)

We still need to use action param to check if it is Creating/editing a split bill request

Creating a split bill request => save to transactionDraft Editing a split bill request => save to splitTransactionDraft

[s77rt]

Creating a split bill request => save to transactionDraft

Is that the current behavior on main? Don't we save to splitTransactionDraft?

[DylanDylann]

Is that the current behavior on main? Don't we save to splitTransactionDraft?

@s77rt Yes, it is current behavior on main

[s77rt]

Can you please link to that? As far as I can see we use completeSplitBill in SplitBillDetailsPage and the draft is coming from SPLIT_TRANSACTION_DRAFT

[s77rt]

Or I think we are using that draft in IOURequestStepConfirmation which uses withFullTransactionOrNotFound

[DylanDylann]

@s77rt This PR is reverted

[s77rt]

Does that effect our progress here?

[DylanDylann]

@s77rt

Can you please link to that? As far as I can see we use completeSplitBill in SplitBillDetailsPage and the draft is coming from SPLIT_TRANSACTION_DRAFT

You mean the link for this case: Creating a split bill request => save to transactionDraft

We are using IOURequestStepConfirmation for both creating request money and creating split bill and the information is saved to transactionsDraft_1 Because this logic https://github.com/Expensify/App/blob/b4fe0c8256d9bfbf8715bfc290c9a16959d226f1/src/pages/iou/request/step/IOURequestStepDescription.js#L73

https://github.com/Expensify/App/assets/141406735/88bf8f42-7ec1-4e58-9e6e-6e653fd6c617

[s77rt]

@DylanDylann Can you please check https://github.com/Expensify/App/pull/35137#discussion_r1471112690

[DylanDylann]

@s77rt Sorry, I forgot to ping you again. I updated

[s77rt]

I think you didn't check the previous comment ^ Can you please check

[DylanDylann]

Added comment

[s77rt]

Looks we have conflicts. Please resolve

[s77rt]

Reviewer Checklist

• [X] I have verified the author checklist is complete (all boxes are checked off).
• [X] I verified the correct issue is linked in the ### Fixed Issues section above
• [X] I verified testing steps are clear and they cover the changes made in this PR
  • [X] I verified the steps for local testing are in the Tests section
  • [X] I verified the steps for Staging and/or Production testing are in the QA steps section
  • [X] 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)
  • [X] 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)
• [X] I checked that screenshots or videos are included for tests on all platforms
• [X] I included screenshots or videos for tests on all platforms
• [x] I verified tests pass on all platforms & I tested again on:
  • [X] Android: Native
  • [X] Android: mWeb Chrome
  • [X] iOS: Native
  • [X] iOS: mWeb Safari
  • [X] MacOS: Chrome / Safari
  • [X] MacOS: Desktop
• [X] 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
• [x] I verified proper code patterns were followed (see Reviewing the code)
  • [X] 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).
  • [X] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • [X] I verified that comments were added to code that is not self explanatory
  • [X] 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.
  • [X] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  • [X] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [X] 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.
  • [X] 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.
  • [X] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [X] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [X] I verified that this PR follows the guidelines as stated in the Review Guidelines
• [X] 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)
• [X] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [X] 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
• [X] If a new component is created I verified that:
  • [X] A similar component doesn't exist in the codebase
  • [X] All props are defined accurately and each prop has a /** comment above it */
  • [X] The file is named correctly
  • [X] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [X] The only data being stored in the state is data necessary for rendering and nothing else
  • [X] 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)
  • [X] 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)
  • [X] All JSX used for rendering exists in the render method
  • [X] 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
• [X] If any new file was added I verified that:
  • [X] 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
• [X] If a new CSS style is added I verified that:
  • [X] A similar style doesn't already exist
  • [X] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
• [X] 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.
• [X] 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)
• [X] 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.
• [X] 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.
• [X] If the PR modifies the form input styles:
  • [X] I verified that all the inputs inside a form are aligned with each other.
  • [X] I added Design label so the design team can review the changes.
• [X] 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.
• [X] 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.
• [x] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

https://github.com/Expensify/App/assets/16493223/c2264486-23e8-4aff-be1d-f90755858b4f

Android: mWeb Chrome

https://github.com/Expensify/App/assets/16493223/aa492de7-27a2-44d9-8aa5-19c3b775b467

iOS: Native

https://github.com/Expensify/App/assets/16493223/ff2c9a01-0a25-4e58-8470-0f1c5831a51f

iOS: mWeb Safari

https://github.com/Expensify/App/assets/16493223/a5938407-d1dc-4ae6-a488-1376147fc437

MacOS: Chrome / Safari

https://github.com/Expensify/App/assets/16493223/c7d8fdad-fa22-4984-92a9-db1917039a55

MacOS: Desktop

https://github.com/Expensify/App/assets/16493223/91ff37e6-14e3-49c4-a107-f32ee017f3f4

[s77rt]

Edit split description does not work as expected, the description is changed in the menu item view but when trying to edit once again the current value is displayed

https://github.com/Expensify/App/assets/16493223/36642b89-9ee9-4261-8ebd-25bf6f2b0274

[DylanDylann]

@s77rt Nice catch. I updated the code to fix this bug

https://github.com/Expensify/App/assets/141406735/d63d9c3b-5861-4dd5-87cf-d63cc7290d7d

[DylanDylann]

@s77rt Updated. Please help to check again

[s77rt]

@deetergp The perf failing test is unrelated to this PR (happens on other PRs too)

[DylanDylann]

@s77rt Do you know how to re-run Reassure Performance Tests ?

[DylanDylann]

I have seen this problem many times while implementing PR. I ofter merge the main and push a new commit to re-run Reassure Performance Tests

[s77rt]

Any new commits should trigger the test but it seems unstable, even after merging main it could still fail

[DylanDylann]

@s77rt What do you think about putting splitDraftTransaction into WithFullTransactionOrNotFound? In general, It will help us dry code a lot