Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$250] Remove MoneyRequestParticipantsPage.js and copy any changes since Nov 27 into IOURequestStepParticipants.js #34616

Closed
Tracked by #29107
tgolen opened this issue Jan 16, 2024 · 43 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review

Comments

@tgolen
Copy link
Contributor

tgolen commented Jan 16, 2024

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: MoneyRequestParticipantsPage
New Component IOURequestStepParticipants

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/~015fc3b79f31f2289a
  • Upwork Job ID: 1747632960650227712
  • Last Price Increase: 2024-01-24
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 28131654
    • Krishna2323 | Contributor | 28131655
@tgolen tgolen added Engineering Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors Bug Something is broken. Auto assigns a BugZero manager. labels Jan 16, 2024
Copy link

melvin-bot bot commented Jan 16, 2024

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 16, 2024

Proposal

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

Remove MoneyRequestParticipantsPage.js and copy any changes since Nov 27 into IOURequestStepParticipants.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?

I checked all the commits after 27th Nov and the new component is updated according to the old component, we just need to remove the file and route created for it. I checked the commits thoroughly but will check again in the PR.

Component to remove:
https://github.com/Expensify/App/blob/main/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsPage.js

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

We only use MoneyRequestParticipantsPage as a screen and we already replaced the route with IOURequestStepParticipants, so we don't have any instances to replace.

We also need to remove all traces from src/libs/Navigation/types.ts & src/libs/Navigation/linkingConfig.ts:

[SCREENS.MONEY_REQUEST.PARTICIPANTS]: ROUTES.MONEY_REQUEST_PARTICIPANTS.route,

[SCREENS.MONEY_REQUEST.PARTICIPANTS]: {

We will update to :action param here:

route: 'create/:iouType/participants/:transactionID/:reportID',

We are using ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute from several places so we need to add action accordingly:

Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute(iouType, transactionID, reportID));

Navigation.goBack(ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute(iouType, transactionID, reportID));

Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute(iouType, transactionID, reportID));

Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute(iouType, transactionID, reportID));

Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute(iouType, transactionID, reportID));

Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute(iouType, transactionID, reportID));

_Navigation to old component that needs to be updated to MONEY_REQUEST_STEP_SCAN _

Navigation.navigate(ROUTES.MONEY_REQUEST_PARTICIPANTS.getRoute(iouType));

fallback = ROUTES.MONEY_REQUEST_PARTICIPANTS.getRoute(iouType);

Note: The below action param step can be omitted because we only use IOURequestStepParticipants for IOU creation flow.

Result

@njmulsqb
Copy link

I would love to take this!

@tgolen tgolen added the External Added to denote the issue can be worked on by a contributor label Jan 17, 2024
@melvin-bot melvin-bot bot changed the title Remove MoneyRequestParticipantsPage.js and copy any changes since Nov 27 into IOURequestStepParticipants.js [$500] Remove MoneyRequestParticipantsPage.js and copy any changes since Nov 27 into IOURequestStepParticipants.js Jan 17, 2024
Copy link

melvin-bot bot commented Jan 17, 2024

Job added to Upwork: https://www.upwork.com/jobs/~015fc3b79f31f2289a

Copy link

melvin-bot bot commented Jan 17, 2024

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

@MahmudjonToraqulov
Copy link
Contributor

I would love to take this!

@ghost
Copy link

ghost commented Jan 17, 2024

Dibs

@ishpaul777

This comment was marked as off-topic.

@mountiny mountiny changed the title [$500] Remove MoneyRequestParticipantsPage.js and copy any changes since Nov 27 into IOURequestStepParticipants.js [$250] Remove MoneyRequestParticipantsPage.js and copy any changes since Nov 27 into IOURequestStepParticipants.js Jan 17, 2024
Copy link

melvin-bot bot commented Jan 17, 2024

Upwork job price has been updated to $250

@melvin-bot melvin-bot bot added the Overdue label Jan 19, 2024
@Krishna2323
Copy link
Contributor

Proposal Updated

Added a not about replacing the instances of the old component.

@isabelastisser
Copy link
Contributor

@0xmiroslav, can you please review the proposal above? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2024
@mallenexpensify mallenexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

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

@mallenexpensify
Copy link
Contributor

@abdulrahuman5196 reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks

@mountiny
Copy link
Contributor

mountiny commented Feb 5, 2024

@Krishna2323 I have seen you post proposals on other issues, can you please give ETA for this PR or I will reassign to someone else tomorrow. Thank you

@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2024
@Krishna2323
Copy link
Contributor

@mountiny, sorry for the delay, I believe we should wait for #35455 to be merged because similar PRs caused regressions, and I don't want this one to cause a regression. Therefore, I suggest we update the IOU flow sequence accordingly.

Will try again to have a look if we could get this done without waiting for that one.

@Krishna2323
Copy link
Contributor

Krishna2323 commented Feb 5, 2024

@abdulrahuman5196, we need to wait until this gets merged & these #34614 #34610 are merged because we will be using IOURequestStepParticipants for edit flows also so we need to pass the correct action from IOURequestStepAmount & IOURequestStepDistance.

cc: @mountiny

edit: Will raise a PR by EOD.

@Krishna2323
Copy link
Contributor

@abdulrahuman5196, PR is up but as I said we need to wait for #35455 to get merged, because the send money flow still uses old components/routes.

send_flow_participants.mp4

@abdulrahuman5196
Copy link
Contributor

Got it. I agree we could wait for the PR, to avoid cross conflicts given this is LOW priority.

@Krishna2323 since you are owner of the PR as well, kindly do update us once the PR is ready for review.

@Krishna2323
Copy link
Contributor

Sure!

Copy link

melvin-bot bot commented Mar 4, 2024

This issue has not been updated in over 15 days. @isabelastisser, @mountiny, @abdulrahuman5196, @Krishna2323 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!

@isabelastisser
Copy link
Contributor

@Krishna2323 @abdulrahuman5196 what's the next step here?

@Krishna2323
Copy link
Contributor

#35455 is still in progress. I'm the author of that PR also, and we are very close to merging it.

@Krishna2323
Copy link
Contributor

#35455 is merged, will be raising PR for this very soon.

@DylanDylann
Copy link
Contributor

@Krishna2323 Could you speed-up this issue to unblock other issues

@Krishna2323
Copy link
Contributor

Sorry for delay, started working on the PR, will be ready for review today.

@Krishna2323
Copy link
Contributor

@abdulrahuman5196, the PR is ready for review. I haven't added recordings yet because I believe we should incorporate all required/suggested changes first. What do you think? I recently encountered a regression because I updated the code after the recordings were uploaded. The updated code had an issue that could have been easily identified during recordings.

@isabelastisser
Copy link
Contributor

Hey @abdulrahuman5196, can you please provide an update? Thanks!

@mountiny
Copy link
Contributor

Asked in slack too https://expensify.slack.com/archives/C02NK2DQWUX/p1711752832574839

@Krishna2323
Copy link
Contributor

@isabelastisser, PR was deployed to production on 24th April, I think this is ready for payments process.

@isabelastisser
Copy link
Contributor

@Krishna2323 and @abdulrahuman5196, please accept the offers in Upwork and I will process the payments. Thanks!

@Krishna2323
Copy link
Contributor

@isabelastisser, accepted, thanks!

@abdulrahuman5196
Copy link
Contributor

@isabelastisser Accepted the offer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests