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

[Pay 4/29][$250] Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.js #34610

Closed
Tracked by #29107
tgolen opened this issue Jan 16, 2024 · 45 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

Held on #34613

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: NewDistanceRequestPage
New Component IOURequestStepDistance

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/~01f10eeacf3fc8d218
  • Upwork Job ID: 1747632624829898752
  • Last Price Increase: 2024-01-17
  • Automatic offers:
    • cubuspl42 | Reviewer | 28115391
    • DylanDylann | Contributor | 28115392
@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 @johncschuster (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@KrAbhas
Copy link
Contributor

KrAbhas commented Jan 16, 2024

Proposal

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

Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.js

What is the root cause of that problem?

Refactoring
 of navigation among screens for NewDistanceRequestPage

What changes do you think we should make in order

We will be deleting the component here:

function NewDistanceRequestPage({iou, report, route}) {

We will remove the screen from ModalStackNavigators.tsx

@njmulsqb
Copy link

I would love to take this!

@DylanDylann
Copy link
Contributor

DylanDylann commented Jan 17, 2024

Proposal

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

Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.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. Currently, we only use NewDistanceRequestPage in the MoneyRequestSelectorPage but MoneyRequestSelectorPage also be removed in this issue. So that we can remove NewDistanceRequestPage component and all traces like

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

[SCREENS.MONEY_REQUEST.DISTANCE]: ROUTES.MONEY_REQUEST_DISTANCE.route,

App/src/ROUTES.ts

Lines 293 to 296 in a2f5bd5

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

DISTANCE: 'Money_Request_Distance',

[SCREENS.MONEY_REQUEST.DISTANCE]: {
iouType: ValueOf<typeof CONST.IOU.TYPE>;
reportID: string;
};

  1. For new component IOURequestStepDistance.

In here

App/src/ROUTES.ts

Lines 347 to 351 in a2f5bd5

MONEY_REQUEST_STEP_DISTANCE: {
route: 'create/:iouType/distance/:transactionID/:reportID',
getRoute: (iouType: ValueOf<typeof CONST.IOU.TYPE>, transactionID: string, reportID: string, backTo = '') =>
getUrlWithBackToParam(`create/${iouType}/distance/${transactionID}/${reportID}`, backTo),
},

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/NewDistanceRequestPage.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
@melvin-bot melvin-bot bot changed the title Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.js [$500] Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.js Jan 17, 2024
Copy link

melvin-bot bot commented Jan 17, 2024

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

Copy link

melvin-bot bot commented Jan 17, 2024

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

@MahmudjonToraqulov
Copy link
Contributor

I would love to take this!

1 similar comment
@s-alves10
Copy link
Contributor

I would love to take this!

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

melvin-bot bot commented Jan 17, 2024

Upwork job price has been updated to $250

@cubuspl42
Copy link
Contributor

I approve the proposal by @DylanDylann

  • As this is not TypeScript migration, we should still do proposals as per the guidelines
  • The proposals shows some research related to the refactoring

C+ reviewed 🎀 👀 🎀

Copy link

melvin-bot bot commented Jan 18, 2024

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

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

melvin-bot bot commented Jan 22, 2024

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

Offer link
Upwork job

Copy link

melvin-bot bot commented Jan 22, 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 📖

@cristipaval
Copy link
Contributor

Thanks @cubuspl42!

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

Anyway, we indeed need to re-open this.

We're working on two of the regressions:

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Apr 2, 2024
@cristipaval cristipaval reopened this Apr 2, 2024
@cristipaval
Copy link
Contributor

I think it was automatically closed because it was linked in that PR

@cristipaval cristipaval added Daily KSv2 and removed Weekly KSv2 labels Apr 2, 2024
@DylanDylann
Copy link
Contributor

@cristipaval Please also help to remove the hold label on the title. Thanks

@cristipaval cristipaval changed the title [HOLD][$250] Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.js [$250] Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.js Apr 2, 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 Apr 24, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 24, 2024
Copy link

melvin-bot bot commented Apr 24, 2024

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

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

@eVoloshchak reassigning, please take over as C+. If you don't have bandwidth, unassign yourself. Thanks
@DylanDylann can you please post an update on where we're at and what @eVoloshchak needs to help with?

@DylanDylann
Copy link
Contributor

DylanDylann commented Apr 24, 2024

@mallenexpensify Everything is done here, waiting for payment

@mallenexpensify mallenexpensify added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review labels Apr 25, 2024
@mallenexpensify mallenexpensify changed the title [$250] Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.js [Pay 4/29][$250] Remove NewDistanceRequestPage.js and copy any changes since Nov 27 into IOURequestStepDistance.js Apr 25, 2024
@mallenexpensify
Copy link
Contributor

Thanks @DylanDylann , removed Reviewing, added Awaiting Payment and updated the title to reflect payment date.

@mallenexpensify
Copy link
Contributor

Contributor: @DylanDylann paid $250 via Upwork
Contributor+: @cubuspl42 paid $250 via Upwork

Doesn't look like this needs a regression test created, comment if anyone disagrees.

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