Remove MoneyRequestParticipantsSelector.js and copy any changes since Nov 27 into MoneyTemporaryForRefactorRequestParticipantsSelector

[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: MoneyRequestParticipantsSelector
New Component MoneyTemporaryForRefactorRequestParticipantsSelector

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

[melvin-bot]

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

[KrAbhas]

Proposal

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

Remove MoneyRequestParticpantsSelector.js and copy any changes since Nov 27 into MoneyTemporaryForRefactorRequestParticipantsSelector.js

What is the root cause of that problem?

Cleaning of MoneyRequestParticpantsSelector

What changes do you think we should make in order

We will be deleting the component here:

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Line 73 in a7a3e7f

function MoneyRequestParticipantsSelector({

This component appears in MoneyRequestParticipantsPage which will also be removed

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Line 24 in a7a3e7f

import MoneyRequestParticipantsSelector from './MoneyRequestParticipantsSelector';

Looking at the changes in history we can see a quiet a few commits made after Nov 27. On further analysis, some of the changes are already in MoneyTemporaryForRefactorRequestParticipantsSelector. We will go commit by commit and add the remaining necessary logic to MoneyTemporaryForRefactorRequestParticipantsSelector

[njmulsqb]

I would love to take this!

[melvin-bot]

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

[melvin-bot]

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

[MahmudjonToraqulov]

I would love to take this!

[ghost]

Dibs

[s-alves10]

I would love to take this!

[brunovjk]

Proposal

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

Remove MoneyRequestParticipantsSelector.js and copy any changes since Nov 27 into MoneyTemporaryForRefactorRequestParticipantsSelector

What is the root cause of that problem?

The Old Component needs to be completely removed from the codebase

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

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

1. We still have to wait "Remove MoneyRequestParticipantsPage.js", it seems to me that the PR is close to merging.
2. I checked the history of changes since Nov 27, 2023 of the MoneyRequestParticipantsSelector component again:
   1. I believe we most add and appropriately use const setSearchTermAndSearchInServer to it: commit.
   2. Migrate verification for participant list length:
      

      App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

      Lines 166 to 169 in 3c3c778

      	const addSingleParticipant = useCallback(
      	(option) => {
      	if (participants.length) {
      	return;

      
      I didn't find any other necessary changes, but in PR I will check again.
3. These components are used in the old components and no longer being used in new components so we only need to remove it." slack conversation

Be sure to update all routes and navigation to use the new :action param (instead of being hard-coded with "create")
Update any logic like isEditing to use the new action param from the route

• MoneyRequestParticipantsSelector is only used for the create flow. So we don’t need to use action param, is not utilized as a route. Consequently, the outlined steps are unnecessary.

What alternative solutions did you explore? (Optional)

NA

[allgandalf]

Proposal

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

Remove MoneyRequestParticipantsSelector.js and copy any changes since Nov 27 into MoneyTemporaryForRefactorRequestParticipantsSelector

What is the root cause of that problem?

Refactor/cleanup of the code

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

1. Here we need to add any changes since Nov 27, 2023 which have not been added to the New Component:
   One such edit is for the commit e5489e9650dfdeb7eebb0c109fb8bb0cfcc65d79 on Dec 7:

In MoneyRequestParticipantsSelector we check for the length of the participant variable:

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Lines 189 to 193 in 818567a

	const addSingleParticipant = (option) => {
	if (participants.length) {
	return;
	}
	onAddParticipants(

But in MoneyTemporaryForRefactorRequestParticipantsSelector, we don't have the check present:

App/src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js

Lines 178 to 179 in 0c599f0

	const addSingleParticipant = (option) => {
	onParticipantsAdded([

---

2. Delete the function MoneyRequestParticipantsSelector i.e. the MoneyRequestParticipantsSelector.js file altogether:

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Line 73 in fe9e9e3

function MoneyRequestParticipantsSelector({

---

3) The old component was used in `MoneyRequestParticipantsPage.js`, remove the reference from there too and refer to new `MoneyTemporaryForRefactorRequestParticipantsSelector` component

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

Line 24 in 0c599f0

import MoneyRequestParticipantsSelector from './MoneyRequestParticipantsSelector';

Here we need to update the props passed to the new function as well, so related changes to the variable names would be made accordingly

---

What alternative solutions did you explore? (Optional)

N/A

[kevinksullivan]

@situchan want to let me know who we're taking here?

[situchan]

checking who proposed the most complete proposal

[situchan]

I don't think anyone proposed completed proposal following Step 1-6

[Krishna2323]

Proposal

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

Remove MoneyRequestSelectorPage.js and copy any changes since Nov 27 into IOURequestStartPage.js

What is the root cause of that problem?

Component update

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

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

We will check commits after 27th Nov from here: https://github.com/Expensify/App/commits/main/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js, it has around 34 commits after 27th Nov and the new component looks updated compared to the old component, we have some differences between old & new component like navigateToRequest & navigateToSplit has been removed from the new component and new component has only onFinish prop. Will check all 34 commits and differences once again if assigned to this.

3. Component to remove:

https://github.com/Expensify/App/blob/main/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

4. Replace all uses of the Old Component with the New Component:

1. We only have one use of the old component and that is inside MoneyRequestParticipantsPage.
2. We need to update the onyx subscription inside MoneyRequestParticipantsPage because it will also be updated here [$250] Remove MoneyRequestParticipantsPage.js and copy any changes since Nov 27 into IOURequestStepParticipants.js #34616, so we need to use the action type to get the transaction, just like we do inside withFullTransactionOrNotFound HOC here:

   App/src/pages/iou/request/step/withFullTransactionOrNotFound.js

   Lines 70 to 76 in 0c5db5f

   	transaction: {
   	key: ({route}) => {
   	const transactionID = lodashGet(route, 'params.transactionID', 0);
   	const userAction = lodashGet(route, 'params.action', CONST.IOU.ACTION.CREATE);
   	return `${userAction === CONST.IOU.ACTION.CREATE ? ONYXKEYS.COLLECTION.TRANSACTION_DRAFT : ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`;
   	},
   	},

   .
3. We don't use isScanRequest anywhere so we will just remove that and isDistanceRequest is also not required because we in new component we use iouRequestType.
4. In the new component we don't have navigateToRequest & navigateToSplit props instead we have only one prop called onFinish to replace both but for that we need to use conditional statement to get the correct route, we already have done here:

   App/src/pages/iou/request/step/IOURequestStepParticipants.js

   Lines 71 to 76 in 0c5db5f

   	const goToNextStep = useCallback(() => {
   	const nextStepIOUType = numberOfParticipants.current === 1 ? iouType : CONST.IOU.TYPE.SPLIT;
   	IOU.resetMoneyRequestTag_temporaryForRefactor(transactionID);
   	IOU.resetMoneyRequestCategory_temporaryForRefactor(transactionID);
   	Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(nextStepIOUType, transactionID, selectedReportID.current || reportID));
   	}, [iouType, transactionID, reportID]);

   .

5. Current MoneyRequestParticipantsPage component which uses the old MoneyRequestParticipantsSelector.
   

   App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

   Lines 139 to 147 in 0c5db5f

   	<MoneyRequestParticipantsSelector
   	participants={iou.isSplitRequest ? iou.participants : []}
   	onAddParticipants={IOU.setMoneyRequestParticipants}
   	navigateToRequest={() => navigateToConfirmationStep(iouType)}
   	navigateToSplit={() => navigateToConfirmationStep(CONST.IOU.TYPE.SPLIT)}
   	safeAreaPaddingBottomStyle={safeAreaPaddingBottomStyle}
   	iouType={iouType}
   	isDistanceRequest={isDistanceRequest}
   	isScanRequest={isScanRequest}

5. Remove all traces of Old Component:

6. Be sure to update all routes and navigation to use the new :action param (instead of being hard-coded with "create"):

7. Update any logic like isEditing to use the new action param from the route:

We don't use MoneyRequestParticipantsSelector as a route so the above steps are not required for this component.

Result

[brunovjk]

Proposal

Updated

[DylanDylann]

Detail expectation: https://expensify.slack.com/archives/C01GTK53T8Q/p1705999852089949

cc @situchan

[situchan]

Please update proposals to meet this expectation

[brunovjk]

Please update proposals to meet this expectation

@situchan Did already

Proposal

Updated

[melvin-bot]

This issue has not been updated in over 15 days. @kevinksullivan, @situchan 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]

@kevinksullivan Because @situchan is OOO, to speed up the main issue #29107 could you help to re-assign C+ for this issue? I followed the main tracking issue for a long time, could I take over this issue as C+ to process this one

[tgolen]

Thanks for taking this one over, I've assigned it to you @DylanDylann.

[brunovjk]

Proposal

Updated

---

1. We still have to wait "Remove MoneyRequestParticipantsPage.js", it seems to me that the PR is close to merging.
2. I checked the history of changes since Nov 27, 2023 of the MoneyRequestParticipantsSelector component again:
   1. I believe we most add and appropriately use const setSearchTermAndSearchInServer to it: commit.
   2. Migrate verification for participant list length:
      

      App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

      Lines 166 to 169 in 3c3c778

      	const addSingleParticipant = useCallback(
      	(option) => {
      	if (participants.length) {
      	return;

      
      I didn't find any other necessary changes, but in PR I will check again.
3. These components are used in the old components and no longer being used in new components so we only need to remove it." slack conversation

[DylanDylann]

This is straightforward. @GandalfGwaihir's proposal looks good to me

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[dangrous]

Hi @DylanDylann - just to be sure - can you confirm that that proposal matches the guidelines for details? (I'm catching up on the conversation)

[DylanDylann]

@dangrous The guideline in the description isn't correct. We have a Slack thread to discuss it before.

3. There are 2 components (It is not a page):
   - App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js
   - App/src/components/DistanceRequest/index.js (It seems we don’t have issue to handle it like MoneyRequestParticipantsSelector)
   These components are used in the old components and no longer being used in new components so we only need to remove it

[DylanDylann]

Waiting for the response from @dangrous

[DylanDylann]

Oh wait, It seems MoneyRequestParticipantsSelector is removed somewhere so let's close this one

[DylanDylann]

@kevinksullivan This issue is outdated now, let's close this one

[dangrous]

Yep you're right, looks like that component is gone now. However we still have the weirdly named MoneyTemporaryForRefactorRequestParticipantsSelector.js - do we know if there's a plan to change that?

[brunovjk]

@dangrous Yes we have. Pasyukevich from Callstack will migrate to TS and rename: #29107 (comment)

[dangrous]

Got it, thanks! So @kevinksullivan if you agree I think we can close this, as @DylanDylann suggests

[melvin-bot]

@dangrous, @kevinksullivan, @DylanDylann Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[kevinksullivan]

Sounds good 👍