[$250] [Tracking] Refactor functions with many parameters to use parameter objects

[neil-marcellini]

Problem: Many functions in the App repo have a very large amount of parameters, making it very hard to work with them. If you need some data that isn’t available with existing parameters you need to add a new one. The safe way to do that is to add it as an optional param at the end of the list. In order to pass it to the function from an existing call you have to fill in all params between the last one used and this new last param. It’s time consuming to fill out the defaults, and any required params require careful considering. Re-ordering parameters is risky and requires updating all existing calls. Also, we often have cases where we have a transaction object in the component, then call a function passing various fields of the transaction split out into n parameters. Wouldn’t it be so much easier to pass the original object? Sometimes we also pass the ID of the object in Onyx only to retrieve it from an Onyx connection later.

Solution: Gradually refactor functions to take in parameter objects, where it’s easier to re-order. Strive to minimize the number of fields. Group parameters into sensible sub-objects. Thrive :smile:

Original Slack thread.

To do list

• [ ] Refactor one function as an example. Maybe requestMoney or something that's less critical but has a similarly large number of parameters.
• [ ] Update the style guide
• [ ] Create refactoring issues
  Upwork Automation - Do Not Edit

  • Upwork Job URL: https://www.upwork.com/jobs/~021846632713060454080
  • Upwork Job ID: 1846632713060454080
  • Last Price Increase: 2024-10-16
  • Automatic offers:
  • dukenv0307 | Reviewer | 104452249
  • ChavdaSachin | Contributor | 104452250
  • mkzie2 | Contributor | 104791696

Issue Owner

Current Issue Owner: @dukenv0307

[ChavdaSachin]

@neil-marcellini we could start with IOU.getTrackExpenseInformation, IOU.shareTrackedExpense, IOU.categorizeTrackedExpense after these three IOU.trackExpense. These functions mostly share common data and could be refactored with ease for a start. I would like to work on this and following if you want external contributor to work here.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[neil-marcellini]

Sure @ChavdaSachin, please go ahead and start with only IOU.categorizeTrackedExpense

[melvin-bot[bot]]

📣 @dukenv0307 🎉 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

[melvin-bot[bot]]

📣 @ChavdaSachin 🎉 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 📖

[ChavdaSachin]

Sounds good...

[ChavdaSachin]

I am OOO for 2 days on an emergency business , will start working as soon as I am back.

[melvin-bot[bot]]

@neil-marcellini, @ChavdaSachin, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this?

[ChavdaSachin]

work in progress.

[melvin-bot[bot]]

@neil-marcellini, @ChavdaSachin, @dukenv0307 Still overdue 6 days?! Let's take care of this!

[melvin-bot[bot]]

@neil-marcellini, @ChavdaSachin, @dukenv0307 10 days overdue. I'm getting more depressed than Marvin.

[neil-marcellini]

How's it coming along?

[ChavdaSachin]

hi @neil-marcellini sorry for the delay, I am actually trying to implement it on a scale - coz all tracking functions are closely tied together, So I am figuring the whole network these functions have and refactor all at once so that would be a significant improvement and then all tracking related function would probably accept somewhere around 5-7 props that's it. But it is taking time coz network is bigger.

I figured refactoring only one function - IOU.categorizeTrackedExpense would not be much of improvement so... Let me take my time and trust me you would love the results.

Probably switch it to weekly.

[melvin-bot[bot]]

@neil-marcellini, @ChavdaSachin, @dukenv0307 12 days overdue now... This issue's end is nigh!

[neil-marcellini]

Hi @ChavdaSachin. I appreciate your desire to tackle it all at once, but it's a lot easier for everyone involved if we do it in pieces. It will be faster to get the PR up for review and get it approved. I rather have many small PRs than one big one. Could you please split up your existing work into several PRs?

[melvin-bot[bot]]

This issue has not been updated in over 14 days. @neil-marcellini, @ChavdaSachin, @dukenv0307 eroding to Weekly issue.

[mkzie2]

I rather have many small PRs than one big one

@neil-marcellini I'd like to work on one of the PRs if we create other issues.

[ChavdaSachin]

hey @neil-marcellini I have my code prepared for IOU.categorizeTrackedExpense but I am unable to test it. There seems to be a BE issue related to categorizeTrackedExpanse when tax Rates feature is enabled for the workspace.

Steps:

1. Enable tax rated from more features.
2. Create a track expanse.
3. Categorize it in workspace with tax rates enabled.

BE response

``` { "jsonCode": 402, "message": "Invalid ReportID '0' for the report store", "requestID": "8decfcc139663cf4-BOM", "onyxData": [] } ```

Parameter passed

``` { "policyID": "95B63EE2BC4F1E51", "transactionID": "160923194790882510", "moneyRequestPreviewReportActionID": "8261683015292608374", "moneyRequestReportID": "2244080037308526", "moneyRequestCreatedReportActionID": "3617745669044213628", "actionableWhisperReportActionID": "1030875335587309936", "modifiedExpenseReportActionID": "4765574918611489569", "reportPreviewReportActionID": "9087019233556138435", "amount": 1100, "currency": "INR", "comment": "", "merchant": "mock", "category": "Equipment", "tag": "", "taxCode": "id_TAX_EXEMPT", "taxAmount": 0, "billable": false, "created": "2024-11-07", "receipt": {} } ```

[neil-marcellini]

Ok interesting. If that error also occurs on main you can ignore it. Can you test on a workspace without tax rates? Please raise a draft PR.

[neil-marcellini]

I rather have many small PRs than one big one

@neil-marcellini I'd like to work on one of the PRs if we create other issues.

Ok thank @mkzie2. Please pick a function unrelated to IOU.categorizeTrackedExpense and go ahead and refactor it.

[melvin-bot[bot]]

📣 @mkzie2 🎉 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 📖

[ChavdaSachin]

Draft is up, please take a look...

[mkzie2]

@dukenv0307 The PR for requestMoney function is ready.