[HOLD for payment 2024-04-25] [$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.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: IOUCurrencySelection
New Component IOURequestStepCurrency

Solution

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
5. Be sure to update all routes and navigation to use the new :action param (instead of being hard-coded with "create")
6. Update any logic like isEditing to use the new action param from the route

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0107fbe87259b2ef2c
• Upwork Job ID: 1747632778655997952
• Last Price Increase: 2024-01-24
• Automatic offers:
• akinwale | Reviewer | 28119949
• ZhenjaHorbach | Contributor | 28119950

Issue Owner

Current Issue Owner: @sakluger

[melvin-bot]

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

[ZhenjaHorbach]

Proposal

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

Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js

What is the root cause of that problem?

This is not a bug. This is a part of #29107

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

To make this ticket we need

1. Check history IOUCurrencySelection

https://github.com/Expensify/App/commits/main/src/pages/iou/IOUCurrencySelection.js?since=2023-11-25&until=2024-01-15

Here we have a few changes
But judging by the new component IOURequestStepCurrency
These changes are not entirely relevant because we don't have useEffect unlike Old component

App/src/pages/iou/IOUCurrencySelection.js

Lines 79 to 97 in d4c28a4

	useEffect(() => {
	// Do not dismiss the modal, when it is not the edit flow.
	if (!threadReportID) {
	return;
	}
	const report = ReportUtils.getReport(threadReportID);
	const parentReportAction = ReportActionsUtils.getReportAction(report.parentReportID, report.parentReportActionID);
	// Do not dismiss the modal, when a current user can edit this currency of this money request.
	if (ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.CURRENCY)) {
	return;
	}
	// Dismiss the modal when a current user cannot edit a money request.
	Navigation.isNavigationReady().then(() => {
	Navigation.dismissModal();
	});
	}, [threadReportID]);

BUT in Old component, we added useEffect in October which is not used in IOURequestStepCurrency
and if I understand correctly
We need to add this useEffect in New component to use other routes ( For example SCREENS.EDIT_REQUEST.CURRENCY)

App/src/libs/Navigation/AppNavigator/ModalStackNavigators.tsx

Line 268 in d4c28a4

[SCREENS.EDIT_REQUEST.CURRENCY]: () => require('../../../pages/iou/IOUCurrencySelection').default as React.ComponentType,

2. Make the necessary changes to the new component

Add useEffect from old component to new (We need this component to use other routes ( For example SCREENS.EDIT_REQUEST.CURRENCY) )

App/src/pages/iou/IOUCurrencySelection.js

Lines 79 to 97 in d4c28a4

	useEffect(() => {
	// Do not dismiss the modal, when it is not the edit flow.
	if (!threadReportID) {
	return;
	}
	const report = ReportUtils.getReport(threadReportID);
	const parentReportAction = ReportActionsUtils.getReportAction(report.parentReportID, report.parentReportActionID);
	// Do not dismiss the modal, when a current user can edit this currency of this money request.
	if (ReportUtils.canEditFieldOfMoneyRequest(parentReportAction, CONST.EDIT_REQUEST_FIELD.CURRENCY)) {
	return;
	}
	// Dismiss the modal when a current user cannot edit a money request.
	Navigation.isNavigationReady().then(() => {
	Navigation.dismissModal();
	});
	}, [threadReportID]);

Why SCREENS.EDIT_REQUEST.CURRENCY?

UseEffect expects to receive threadReportID from params from navigation

App/src/pages/iou/IOUCurrencySelection.js

Lines 81 to 83 in d4c28a4

	if (!threadReportID) {
	return;
	}

And **SCREENS.EDIT_REQUEST.CURRENCY** this is the only place where we use this parameter

App/src/ROUTES.ts

Lines 171 to 174 in 7c8336b

	EDIT_CURRENCY_REQUEST: {
	route: 'r/:threadReportID/edit/currency',
	getRoute: (threadReportID: string, currency: string, backTo: string) => `r/${threadReportID}/edit/currency?currency=${currency}&backTo=${backTo}` as const,
	},

3. Update routes

Old component are used in several places. Therefore, we need to make sure that everything is updated (And check that the new component works correctly) and does not cause any regression

App/src/libs/Navigation/AppNavigator/ModalStackNavigators.tsx

Line 97 in a7a3e7f

[SCREENS.MONEY_REQUEST.CURRENCY]: () => require('../../../pages/iou/IOUCurrencySelection').default as React.ComponentType,

App/src/libs/Navigation/AppNavigator/ModalStackNavigators.tsx

Line 114 in a7a3e7f

[SCREENS.SPLIT_DETAILS.EDIT_CURRENCY]: () => require('../../../pages/iou/IOUCurrencySelection').default as React.ComponentType,

App/src/libs/Navigation/AppNavigator/ModalStackNavigators.tsx

Line 268 in a7a3e7f

[SCREENS.EDIT_REQUEST.CURRENCY]: () => require('../../../pages/iou/IOUCurrencySelection').default as React.ComponentType,

What alternative solutions did you explore? (Optional)

NA

[njmulsqb]

I would love to take this!

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0107fbe87259b2ef2c

[melvin-bot]

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

[ghost]

I want to work on it

[melvin-bot]

Upwork job price has been updated to $250

[sakluger]

@mountiny are we treating this like a TS Migration issue? I.e. do we require proposals or just a volunteer?

[tgolen]

We should get a full proposal to ensure that all the paths are covered properly. The initial instructions give a brief overview, but the code should really be inspected to clearly understand all the necessary changes.

…

On Fri, Jan 19, 2024 at 1:41 PM Sasha Kluger ***@***.***> wrote: @mountiny <https://github.com/mountiny> are we treating this like a TS Migration issue? I.e. do we require proposals or just a volunteer? — Reply to this email directly, view it on GitHub <#34607 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB4DFTAX3HVHSY7UAATYPLK73AVCNFSM6AAAAABB5OFKMOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBRGA4DIMJUGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[sakluger]

Thanks @tgolen. @akinwale looks like we got a proposal here, could you please give it a look to see if it works well?

[akinwale]

@ZhenjaHorbach Could you please provide more details about the files and the routes that need to be updated in your proposal? Thanks.

[ZhenjaHorbach]

@ZhenjaHorbach Could you please provide more details about the files and the routes that need to be updated in your proposal? Thanks.

Done )

[dangrous]

how are we looking on this one @ZhenjaHorbach and @akinwale?

[ZhenjaHorbach]

In fact, the PR is ready, but I suggested waiting so as not to cause any regression )
Since we have places that need to be deleted in other PR

#35161 (comment)

[dangrous]

responded in the PR! let me know if that makes sense

[dangrous]

Just updating the title to reflect that we're holding on #35833

[melvin-bot]

This issue has not been updated in over 15 days. @akinwale, @dangrous, @sakluger, @ZhenjaHorbach eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[DylanDylann]

@ZhenjaHorbach @akinwale No longer hold, let's process the next step

[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.62-17 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js  #39562

If no regressions arise, payment will be issued on 2024-04-25. 🎊

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

• @akinwale requires payment automatic offer (Reviewer)
• @ZhenjaHorbach requires payment automatic offer (Contributor)

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

• [@akinwale] The PR that introduced the bug has been identified. Link to the PR:
• [@akinwale] 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:
• [@akinwale] 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:
• [@akinwale] Determine if we should create a regression test for this bug.
• [@akinwale] 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.
• [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mountiny]

$250 to @akinwale and to @ZhenjaHorbach

[sakluger]

All paid up! I don't think any new regression steps are needed since this is part of a larger project.