Fix/34609: Remove old description page #35137

DylanDylann · 2024-01-25T08:31:48Z

Details

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

Fixed Issues

$ #34609
PROPOSAL: #34609 (comment)

Tests

Create a new request
While creating a new request click on the description field
Verify that the route display /create/request/
After creating request click on description field to edit
Verify that the route display /edit/request/
Create a split bill
While creating a new split bill click on the description field
Verify that the route display /create/split/
After creating a new split bill click on description field to edit
Verify that the route display /edit/split/
Verify that everything works well as before

Verify that no errors appear in the JS console

Offline tests

Same above

QA Steps

Same above

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

a1.mp4

a2.mp4

Android: mWeb Chrome

c.mp4

iOS: Native

i.mp4

iOS: mWeb Safari

s.mp4

MacOS: Chrome / Safari

w.mp4

MacOS: Desktop

d.mp4

melvin-bot · 2024-01-25T10:36:01Z

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

src/pages/iou/request/step/IOURequestStepDescription.js

s77rt · 2024-01-26T18:09:41Z

src/pages/iou/request/step/IOURequestStepDescription.js

+        const newComment = value.moneyRequestComment;
+        // Only update comment if it has changed
+        if (newComment.trim() !== lodashGet(transaction, 'comment.comment', '')) {
+            if (isDraft) {
+                IOU.setMoneyRequestDescription_temporaryForRefactor(transactionID, newComment, isDraft);
+            } else {
+                if (iouType === CONST.IOU.TYPE.REQUEST) {
+                    IOU.updateMoneyRequestDescription(transaction.transactionID, reportID, newComment.trim());
+                }
+                if (iouType === CONST.IOU.TYPE.SPLIT) {
+                    IOU.setDraftSplitTransaction(transaction.transactionID, {
+                        comment: newComment.trim(),
+                    });
+                }
+            }
+        }


I expected to see all this logic handled in setMoneyRequestDescription_temporaryForRefactor

s77rt · 2024-01-26T18:17:37Z

src/components/MoneyRequestConfirmationList.js

-                    if (props.isEditingSplitBill) {
-                        Navigation.navigate(ROUTES.EDIT_SPLIT_BILL.getRoute(props.reportID, props.reportActionID, CONST.EDIT_REQUEST_FIELD.DESCRIPTION));
+                    if (!props.isEditingSplitBill) {
                        return;
                    }
-                    Navigation.navigate(ROUTES.MONEY_REQUEST_DESCRIPTION.getRoute(props.iouType, props.reportID));
+                    Navigation.navigate(
+                        ROUTES.MONEY_REQUEST_STEP_DESCRIPTION.getRoute(
+                            CONST.IOU.ACTION.EDIT,
+                            CONST.IOU.TYPE.SPLIT,
+                            transaction.transactionID,
+                            props.reportID,
+                            Navigation.getActiveRouteWithoutParams(),
+                        ),
+                    );


Why we are assuming that this is going to be used for edits only? (similar question for MoneyTemporaryForRefactorRequestConfirmationList.js)

@s77rt if props.isEditingSplitBill is false, we will disable this field by this logic

App/src/components/MoneyRequestConfirmationList.js

Line 754 in 9dbc389

interactive={!props.isReadOnly}

Because of isReadOnly={!isEditingSplitBill}

The isReadOnly is a different prop and we can't assume it will always be related to isEditingSplitBill.

Why we are assuming that this is going to be used for edits only?

@s77rt In that time, there was only editing split bill that used this component and this component MoneyRequestConfirmationList also be removed in this refactor phrase. But you're right, we can't assume that this is going to be used for edits only. So I updated in this commit to remove condition

if (!props.isEditingSplitBill) { return; }

and using iouType as param of navigate function

@DylanDylann Is there any need to change the logic there? Can't we just keep using the same logic (i.e. keep isEditingSplitBill conditions) and navigate to the correct pages?

@s77rt We need to change logic to use a new route to IOURequestStepDescription for both case. The above code is outdated. This is code after updating

s77rt · 2024-01-26T18:21:24Z

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

Screen.Recording.2024-01-26.at.7.20.13.PM.mov

DylanDylann · 2024-01-27T03:58:59Z

@s77rt I can't reproduce your bug

so.mp4

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

src/libs/actions/IOU.js

src/pages/EditRequestPage.js

src/libs/actions/IOU.js

src/pages/EditSplitBillPage.js

src/pages/EditRequestPage.js

src/libs/actions/IOU.js

DylanDylann · 2024-01-29T05:16:04Z

@s77rt Thanks for your commit suggestion. I updated

src/libs/actions/IOU.js

DylanDylann · 2024-01-29T18:03:35Z

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

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

DylanDylann · 2024-01-29T18:07:07Z

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 · 2024-01-29T18:23:48Z

Edit money request: Send API immediately to update new description

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

s77rt · 2024-01-29T18:30:11Z

#35137 (comment)

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

DylanDylann · 2024-01-30T04:36:32Z

@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 · 2024-01-30T07:45:03Z

Creating a split bill request => save to transactionDraft

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

DylanDylann · 2024-02-06T14:52:38Z

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

DylanDylann · 2024-02-06T14:53:44Z

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 · 2024-02-06T15:02:25Z

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

DylanDylann · 2024-02-06T16:23:52Z

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

s77rt · 2024-02-06T17:02:12Z

It may not help us well as we will still need to check if splitDraftTransaction is empty and if it is then we would need to use transaction instead. So we will still subscribe to two onyx values afterall.

DylanDylann · 2024-02-06T17:24:58Z

@s77rt I mean

putting splitDraftTransaction into WithFullTransactionOrNotFound

to avoid the use withOnyx in IOURequestStepDescription and many other similar pages

s77rt · 2024-02-06T17:32:49Z

But we will be returning two transactions from withFullTransactionOrNotFound: transaction and splitDraftTransaction. Is that what you meant?

DylanDylann · 2024-02-06T17:36:26Z

Yes. I checked all the places that used withFullTransactionOrNotFound. And see that almost places also need to import splitDraftTransaction to handle similar case as we did here

deetergp · 2024-02-06T18:22:43Z

You can generally restart the either the failing tests or all the tests by clicking "Details" next to the failing test and selecting "Re-Run Jobs" at the top right. With that said, I restarted the failing job twice and it continued to fail. Can you please try pulling main into your branch and try again?

s77rt · 2024-02-06T22:08:40Z

@DylanDylann I think it's fine to keep it that way for now. If anything a follow would be better than handling it here

DylanDylann · 2024-02-07T15:22:28Z

@deetergp All jobs are successful. Please help to check again

deetergp · 2024-02-07T16:20:14Z

@DylanDylann I just came here to say "It looks like the tests were fixed here" 😂

deetergp

Changes look good and test well 👍

OSBotify · 2024-02-07T16:38:13Z

✋ 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 · 2024-02-08T18:43:43Z

🚀 Deployed to staging by https://github.com/deetergp in version: 1.4.39-0 🚀

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

lanitochka17 · 2024-02-08T20:07:37Z

This PR is failing because of issue #36175

The issue is reproducible in: IOS Android mWeb

Bug6372489_1707422129564.Screen_Recording_20240209_032141_New_Expensify.1.mp4

This reverts commit f88d526, reversing changes made to 880e187.

marcaaron · 2024-02-09T22:22:03Z

src/components/ReportActionItem/MoneyRequestView.tsx

+                                    CONST.IOU.TYPE.REQUEST,
+                                    transaction?.transactionID ?? '',
+                                    report.reportID,
+                                    Navigation.getActiveRouteWithoutParams(),


I think this needs to be removed maybe?

OSBotify · 2024-02-12T14:01:22Z

🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.39-8 🚀

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

DylanDylann added 3 commits January 25, 2024 15:29

fix Remove old description page

199a826

move change from old description page to new one

c983c5f

update logic to save description in each case

d93929e

DylanDylann marked this pull request as ready for review January 25, 2024 10:35

DylanDylann requested a review from a team as a code owner January 25, 2024 10:35

melvin-bot bot requested review from s77rt and removed request for a team January 25, 2024 10:35

fix lint

e690103

DylanDylann commented Jan 25, 2024

View reviewed changes

src/pages/iou/request/step/IOURequestStepDescription.js Show resolved Hide resolved

fix remove redandunt changr

f1de3f2

s77rt suggested changes Jan 26, 2024

View reviewed changes

move logic to IOU.js

61632f5

s77rt suggested changes Jan 27, 2024

View reviewed changes

src/libs/actions/IOU.js Outdated Show resolved Hide resolved

src/pages/EditRequestPage.js Show resolved Hide resolved

DylanDylann added 2 commits January 28, 2024 15:07

merge main

df5b209

remove comment

da8f760

s77rt reviewed Jan 28, 2024

View reviewed changes

remove comment

ccc3f5a

s77rt suggested changes Jan 29, 2024

View reviewed changes

src/libs/actions/IOU.js Outdated Show resolved Hide resolved

update logic split flow

481119f

merge main

9d0e118

DylanDylann mentioned this pull request Feb 6, 2024

remove MoneyRequestCategoryPage and refactor route #35458

Merged

50 tasks

DylanDylann added 2 commits February 7, 2024 01:51

merge main

2a8a2ff

merge main

79c4485

DylanDylann added 2 commits February 7, 2024 13:15

merge main

6c41870

merge main

4924c67

deetergp approved these changes Feb 7, 2024

View reviewed changes

deetergp merged commit f88d526 into Expensify:main Feb 7, 2024
15 checks passed

github-actions bot mentioned this pull request Feb 8, 2024

Deploy Checklist: New Expensify 2024-02-08 #36168

Closed

77 tasks

lanitochka17 mentioned this pull request Feb 8, 2024

[HOLD for payment 2024-02-20] [HOLD for payment 2024-02-19] Request money - App reopens request details page after saving description #36175

Closed

6 tasks

arosiclair added a commit that referenced this pull request Feb 9, 2024

Revert "Merge pull request #35137 from DylanDylann/fix/34609"

522733f

This reverts commit f88d526, reversing changes made to 880e187.

arosiclair mentioned this pull request Feb 9, 2024

Revert "Merge pull request #35137 from DylanDylann/fix/34609" #36249

Closed

50 tasks

marcaaron reviewed Feb 9, 2024

View reviewed changes

melvin-bot bot mentioned this pull request Feb 12, 2024

[HOLD for payment 2024-02-19] [$250] Remove MoneyRequestDescriptionPage.js and copy any changes since Nov 27 into IOURequestStepDescription.js #34609

Closed

allgandalf mentioned this pull request Feb 13, 2024

[HOLD App 35455][$500] Send money - Not here page opens when opening Description in Send money flow #36342

Closed

6 tasks

Fix/34609: Remove old description page #35137

Fix/34609: Remove old description page #35137

Conversation

DylanDylann commented Jan 25, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DylanDylann Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s77rt commented Jan 26, 2024

DylanDylann commented Jan 27, 2024

DylanDylann commented Jan 29, 2024

DylanDylann commented Jan 29, 2024

DylanDylann commented Jan 29, 2024 • edited Loading

s77rt commented Jan 29, 2024

s77rt commented Jan 29, 2024

DylanDylann commented Jan 30, 2024

s77rt commented Jan 30, 2024

DylanDylann commented Feb 6, 2024

DylanDylann commented Feb 6, 2024

s77rt commented Feb 6, 2024

DylanDylann commented Feb 6, 2024

s77rt commented Feb 6, 2024

DylanDylann commented Feb 6, 2024 • edited Loading

s77rt commented Feb 6, 2024

DylanDylann commented Feb 6, 2024

deetergp commented Feb 6, 2024

s77rt commented Feb 6, 2024

DylanDylann commented Feb 7, 2024

deetergp commented Feb 7, 2024

deetergp left a comment

Choose a reason for hiding this comment

OSBotify commented Feb 7, 2024

OSBotify commented Feb 8, 2024

lanitochka17 commented Feb 8, 2024

Choose a reason for hiding this comment

OSBotify commented Feb 12, 2024

DylanDylann commented Jan 25, 2024 •

edited

Loading

DylanDylann Jan 27, 2024 •

edited

Loading

DylanDylann commented Jan 29, 2024 •

edited

Loading

DylanDylann commented Feb 6, 2024 •

edited

Loading