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

[HOLD for payment 2024-04-15] [$250] Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepConfirmation.js #34615

Closed
Tracked by #29107
tgolen opened this issue Jan 16, 2024 · 51 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

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: MoneyRequestConfirmPage
New Component IOURequestStepConfirmation

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/~01d384157eb0d34e8a
  • Upwork Job ID: 1747632941224689664
  • Last Price Increase: 2024-01-17
  • Automatic offers:
    • DylanDylann | Contributor | 28109099
@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 @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@lancer-spaid
Copy link

lancer-spaid commented Jan 16, 2024

Proposal

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

Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepCategory.js

What is the root cause of that problem?

Cleaning of MoneyRequestConfirmPage

What changes do you think we should make in order

The component here will be deleted.



Remove this line from ModalStackNavigation.js:



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



History page here has a few changes. Most of these are with the violations feature. We will take a look at the specific logic changes and add the changes to IOURequestStepConfirmation

@njmulsqb
Copy link

I would love to take this!

@DylanDylann
Copy link
Contributor

Proposal

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

Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepConfirmation.js

What is the root cause of that problem?

Remove deprecated component

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

  1. Remove MoneyRequestConfirmPage component and all traces like

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

[SCREENS.MONEY_REQUEST.CONFIRMATION]: ROUTES.MONEY_REQUEST_CONFIRMATION.route,

App/src/ROUTES.ts

Lines 261 to 264 in a2f5bd5

MONEY_REQUEST_CONFIRMATION: {
route: ':iouType/new/confirmation/:reportID?',
getRoute: (iouType: string, reportID = '') => `${iouType}/new/confirmation/${reportID}` as const,
},

CONFIRMATION: 'Money_Request_Confirmation',

[SCREENS.MONEY_REQUEST.CONFIRMATION]: {
iouType: string;
reportID: string;
};

  1. For new component IOURequestStepConfirmation

In here

App/src/ROUTES.ts

Lines 308 to 311 in a2f5bd5

MONEY_REQUEST_STEP_CONFIRMATION: {
route: 'create/:iouType/confirmation/:transactionID/:reportID',
getRoute: (iouType: ValueOf<typeof CONST.IOU.TYPE>, transactionID: string, reportID: string) => `create/${iouType}/confirmation/${transactionID}/${reportID}` as const,
},

We need to use the new :action param (instead of being hard-coded with "create") like we did here

App/src/ROUTES.ts

Lines 362 to 366 in a2f5bd5

MONEY_REQUEST_STEP_SCAN: {
route: ':action/:iouType/scan/:transactionID/:reportID',
getRoute: (action: ValueOf<typeof CONST.IOU.ACTION>, iouType: ValueOf<typeof CONST.IOU.TYPE>, transactionID: string, reportID: string, backTo = '') =>
getUrlWithBackToParam(`${action}/${iouType}/scan/${transactionID}/${reportID}`, backTo),
},

  1. Copy changes since 27 Nov to new components based on list commit here: https://github.com/Expensify/App/commits/main/src/pages/iou/IOURequestStepConfirmation.js

What alternative solutions did you explore? (Optional)

@tgolen tgolen added the External Added to denote the issue can be worked on by a contributor label Jan 17, 2024
Copy link

melvin-bot bot commented Jan 17, 2024

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

@melvin-bot melvin-bot bot changed the title Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepConfirmation.js [$500] Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepConfirmation.js Jan 17, 2024
Copy link

melvin-bot bot commented Jan 17, 2024

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

@ghost
Copy link

ghost commented Jan 17, 2024

Dibs

@dukenv0307
Copy link
Contributor

I'd love to work on this issue.

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

melvin-bot bot commented Jan 17, 2024

Upwork job price has been updated to $250

@parasharrajat
Copy link
Member

Looks like @DylanDylann has a more complete proposal.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Jan 18, 2024

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

@NikkiWines
Copy link
Contributor

Agreed!

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 18, 2024
Copy link

melvin-bot bot commented Jan 18, 2024

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot added the Overdue label Jan 22, 2024
@adelekennedy
Copy link

adelekennedy commented Jan 22, 2024

waiting on PR

@melvin-bot melvin-bot bot removed the Overdue label Jan 22, 2024
@DylanDylann
Copy link
Contributor

I am working on PR

@parasharrajat
Copy link
Member

Oh Hold.

@parasharrajat
Copy link
Member

Still on Hold.

@parasharrajat
Copy link
Member

parasharrajat commented Mar 8, 2024

@DylanDylann any updates?

@DylanDylann
Copy link
Contributor

Waiting for this PR

Copy link

melvin-bot bot commented Mar 15, 2024

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

@parasharrajat
Copy link
Member

@DylanDylann let's go, #34613 is merged.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Mar 18, 2024
@DylanDylann
Copy link
Contributor

@adelekennedy Please remove the hold label

@parasharrajat
Copy link
Member

@adelekennedy This issue is no more on Hold. In fact, the PR for this is merged. Please fix the title.

@adelekennedy
Copy link

thank you @parasharrajat!

@adelekennedy adelekennedy changed the title [HOLD for #34613] [$250] Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepConfirmation.js [$250] Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepConfirmation.js Apr 2, 2024
@adelekennedy adelekennedy added Daily KSv2 and removed Weekly KSv2 labels Apr 2, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Apr 8, 2024
@melvin-bot melvin-bot bot changed the title [$250] Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepConfirmation.js [HOLD for payment 2024-04-15] [$250] Remove MoneyRequestConfirmPage.js and copy any changes since Nov 27 into IOURequestStepConfirmation.js Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

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

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

This comment was marked as outdated.

@parasharrajat
Copy link
Member

This is not a bug but a code refactor. Bug-zero checklist does not apply. cc: @adelekennedy

@adelekennedy
Copy link

No checklist - payment is due on 4/15

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 14, 2024
@adelekennedy
Copy link

adelekennedy commented Apr 15, 2024

Payouts due:

@parasharrajat
Copy link
Member

Payment requested as per #34615 (comment)

@JmillsExpensify
Copy link

$250 approved for @parasharrajat

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants