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-25] [$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js #34607

Closed
Tracked by #29107
tgolen opened this issue Jan 16, 2024 · 31 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: 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 OwnerCurrent Issue Owner: @sakluger
@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 @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Jan 16, 2024

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

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)

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

  1. 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) )

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

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,
},

  1. 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

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

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

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

What alternative solutions did you explore? (Optional)

NA

@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 IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js [$500] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js Jan 17, 2024
Copy link

melvin-bot bot commented Jan 17, 2024

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

Copy link

melvin-bot bot commented Jan 17, 2024

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

@ghost
Copy link

ghost commented Jan 17, 2024

I want to work on it

@ishpaul777

This comment was marked as off-topic.

@mountiny mountiny changed the title [$500] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js [$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.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
@sakluger
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Jan 19, 2024
@tgolen
Copy link
Contributor Author

tgolen commented Jan 19, 2024 via email

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

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

@melvin-bot melvin-bot bot removed the Overdue label Jan 23, 2024
@mountiny mountiny reopened this Jan 23, 2024
@akinwale
Copy link
Contributor

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

@ZhenjaHorbach
Copy link
Contributor

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

Done )

@dangrous
Copy link
Contributor

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

@ZhenjaHorbach
Copy link
Contributor

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
Copy link
Contributor

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

@dangrous dangrous changed the title [$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js [HOLD #35833][$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js Feb 13, 2024
@dangrous
Copy link
Contributor

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

Copy link

melvin-bot bot commented Mar 7, 2024

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!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Mar 7, 2024
@DylanDylann
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Monthly KSv2 labels Apr 3, 2024
@mountiny mountiny self-assigned this Apr 10, 2024
@dangrous dangrous changed the title [HOLD #35833][$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js [$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js Apr 10, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 18, 2024
@melvin-bot melvin-bot bot changed the title [$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js [HOLD for payment 2024-04-25] [$250] Remove IOUCurrencySelection.js and copy any changes since Nov 27 into IOURequestStepCurrency.js Apr 18, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 18, 2024
Copy link

melvin-bot bot commented Apr 18, 2024

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

Copy link

melvin-bot bot commented Apr 18, 2024

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:

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

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

Copy link

melvin-bot bot commented Apr 18, 2024

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
Copy link
Contributor

$250 to @akinwale and to @ZhenjaHorbach

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 24, 2024
@sakluger
Copy link
Contributor

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

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