[HOLD for payment 2024-05-02] [$250] Remove MoneyRequestConfirmationList.js and copy any changes since Nov 27 into MoneyTemporaryForRefactorRequestConfirmationList.js

[tgolen]

This is a part of #29107. You can look at that issue for more context behind the cleanup process.

Problem

The app has two redundant components:

Old Component: MoneyRequestConfirmationList
New Component MoneyTemporaryForRefactorRequestConfirmationList

Solution

Now that most of the referenced components have been cleaned up and following the examples (example 1, example 2), the Old Component needs to be completely removed from the codebase

1. Look at the history of the Old Component
2. If there are any changes since Nov 27, 2023 which have not been added to the New Component, copy those changes
3. Replace all uses of the Old Component with the New Component
4. Remove all traces of Old Component

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a11e5428841cbb31
• Upwork Job ID: 1775636536127291392
• Last Price Increase: 2024-04-03
• Automatic offers:
• eh2077 | Reviewer | 0
• brunovjk | Contributor | 0

Issue Owner

Current Issue Owner: @bfitzexpensify

[melvin-bot]

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01a11e5428841cbb31

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[tgolen]

@bfitzexpensify I might have messed up on the price for this. Can you please confirm for me that the UpWork issue is $250?

[abzokhattab]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Remove MoneyRequestConfirmationList.js and copy any changes since Nov 27 into MoneyTemporaryForRefactorRequestConfirmationList.js

What is the root cause of that problem?

Remove MoneyRequestConfirmationList.js and replace it with MoneyTemporaryForRefactorRequestConfirmationList while copying any changes since Nov 27

What changes do you think we should make in order to solve the problem?

Old Component: MoneyRequestConfirmationList
New Component MoneyTemporaryForRefactorRequestConfirmationList

• Look at the history of the Old Component
• If there are any changes since Nov 27, 2023 which have not been added to the New Component, copy those changes
• Replace all uses of the Old Component with the New Component
• Remove all traces of Old Component

[brunovjk]

Proposal

Please re-state the problem that we are trying to solve in this issue.

This belongs to Wave 5 cleanup
Phase 2 is all about removing the original components and any of the duplicated efforts.

What is the root cause of that problem?

Replace MoneyRequestConfirmationList.tsx to MoneyTemporaryForRefactorRequestConfirmationList.js.

What changes do you think we should make in order to solve the problem?

MoneyRequestConfirmationList has been migrated to TypeScript, I believe we should wait for MoneyTemporaryForRefactorRequestConfirmationList to also be migrated to continue here.

Look at the history of the Old Component
If there are any changes since Nov 27, 2023 which have not been added to the New Component, copy those changes

1. From history change: 27/Nov - 04/Apr we can see that most of changes made are related with the typescript migration.

2. We must add reportActionID in the route: split bill issue fixed is this commit.

Replace all uses of the Old Component with the New Component

3. We use MoneyRequestConfirmationList at SplitBillDetailsPage. We can replace it with MoneyTemporaryForRefactorRequestConfirmationList.

• In SplitBillDetailsPage we get the splitTag
  

  App/src/pages/iou/SplitBillDetailsPage.tsx

  Line 88 in 200de35

  tag: splitTag,

  
  to pass to the MoneyRequestConfirmationList component and get the shouldShowTags
  

  App/src/components/MoneyRequestConfirmationList.tsx

  Line 260 in 2995f2c

  const shouldShowTags = isPolicyExpenseChat && (!!iouTag || OptionsListUtils.hasEnabledTags(policyTagLists));

  
  however in the new component we dont use this props
  

  App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js

  Line 284 in 200de35

  const shouldShowTags = useMemo(() => isPolicyExpenseChat && OptionsListUtils.hasEnabledTags(policyTagLists), [isPolicyExpenseChat, policyTagLists]);

Remove all traces of Old Component

4. Remove MoneyRequestConfirmationList rename MoneyTemporaryForRefactorRequestConfirmationList and in all its uses (SCREEN and ROUTE).

Be sure to update all routes and navigation

What alternative solutions did you explore? (Optional)

N/A

[bfitzexpensify]

@bfitzexpensify I might have messed up on the price for this. Can you please confirm for me that the UpWork issue is $250?

Confirmed it's $250

[eh2077]

Asked a question in the parent issue #29107

[brunovjk]

Proposal

Updated

#29107 (comment): Remove MoneyRequestConfirmationList.js and copy any changes since Nov 27 into MoneyTemporaryForRefactorRequestConfirmationList.js

MoneyRequestConfirmationList has been migrated to TypeScript, so first we need to migrate MoneyTemporaryForRefactorRequestConfirmationList to TS as well.

[eh2077]

MoneyRequestConfirmationList has been migrated to TypeScript, so first we need to migrate MoneyTemporaryForRefactorRequestConfirmationList to TS as well.

@brunovjk Thanks for the update. That's a good point.

@tgolen Should we do migrating MoneyTemporaryForRefactorRequestConfirmationList to TS in this task? Or in a separate task just like other TS migration tasks?

[tgolen]

Ah, I would probably avoid doing both the TS migration and the cleanup in the same task (to keep the PR scope smaller). Would it be better to do the TS migration first or second?

[tgolen]

Sorry everyone, I confused IOURequestStepConfrimation.js with MoneyTemporaryForRefactorRequestConfirmationList.js.

This issue doesn't need to have anything major done with IOURequestStepConfirmation.js.

This issue should be for replacing MoneyConfirmationList with MoneyTemporaryForRefactorRequestConfirmationList.

[abzokhattab]

Updated my proposal according to the last comment

[brunovjk]

Proposal

Updated

• Added clarification on passing splitTag to MoneyTemporaryForRefactorRequestConfirmationList.

[eh2077]

Ah, I would probably avoid doing both the TS migration and the cleanup in the same task (to keep the PR scope smaller). Would it be better to do the TS migration first or second?

Yeah, I agree it's better to do them one by one. I found there's an ongoing PR #37181 to migrate MoneyTemporaryForRefactorRequestConfirmationList.js. I think we can wait for that PR.

Once the TS migration PR is merged, I think we can let @brunovjk to take this task as their proposal first pointed out we should replace MoneyConfirmationList with MoneyTemporaryForRefactorRequestConfirmationList and @brunovjk also started the discussion about TS migration.

@tgolen All yours.

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @bondydaa, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[eh2077]

@brunovjk Sure. Please tag me when the PR is ready, thank you!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[eh2077]

We already have a PR #40659 to fix the DB

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.65-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Fix: Correct Footer Display in MoneyRequestConfirmationList Component #40659

If no regressions arise, payment will be issued on 2024-05-02. 🎊

For reference, here are some details about the assignees on this issue:

• @brunovjk requires payment automatic offer (Contributor)
• @eh2077 requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR:
• [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@eh2077] Determine if we should create a regression test for this bug.
• [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@bfitzexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[brunovjk]

If no regressions arise, payment will be issued on 2024-05-02. 🎊

@eh2077 Do the issue we fix here after the main PR count as a regression? ty

cc: @bfitzexpensify

[bfitzexpensify]

@brunovjk yes, the solution in this issue caused a regression, so the regression penalty applies.

Payment to @brunovjk complete.

@eh2077, please complete the BZ checklist so I can finalise payment. Thanks!

[eh2077]

BugZero Checklist

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR: This is not a bug. It's part of refactoring.
• [@eh2077] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
• [@eh2077] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• [@eh2077] Determine if we should create a regression test for this bug. N/A

[brunovjk]

@bfitzexpensify Could we get an increase in the bounty here? Mainly to cover the regression. Despite being a simple refactoring here, the component is used in several flows, create and edit as well. Excessive testing was done during and after the PR was merged. Thank you 🙏

[eh2077]

To make it easier to evaluate @brunovjk 's request above, I'd like to add on:

Though we conducted many tests in the main PR #40176, we still slipped up a bit. There're two fix PRs after the main PR

• [CP Staging] Fix: Correct Footer Display in MoneyRequestConfirmationList Component #40659

Note

The first fix PR was up before the main PR hit staging (means the DB was created later) and was merged shortly when the DB was linked, see #39559 (comment)

• Fix reintroduced dependencies from conflict resolution #41152

Note

The second fix PR was a minor issue that reported by #40176 (comment) (There's no dedicated GH issue created for it)

Edited: Let's defer to the team.

cc @bfitzexpensify @tgolen

[bfitzexpensify]

Will defer this one to you @tgolen - what do you think an appropriate bounty is for this issue?

[tgolen]

The scope of this issue was pretty large and I am OK authorizing an increase to $500.

[bfitzexpensify]

Thanks!

OK, payment of $500 finalised to @eh2077, and bonus of $375 paid to @brunovjk

Thanks for the work here, everyone!

[brunovjk]

Thank you all! It's always good to contribute to Expensify!!! 🌟