[HOLD for payment 2024-12-30] [$250] Undo defaults of -1 and "-1" for account/report/policy IDs

[iwiznia]

Context https://expensify.slack.com/archives/C01GTK53T8Q/p1728329641957179

Problem

Here we decided to use -1 or "-1" as defaults for when there was no report/account/policy IDs (not sure if there are others), but in most places we don't check if the IDs are -1, instead we do !id which for -1 would return true. That means we are applying logic to the -1 ids when we are really intending to skip the logic for those altogether.

Solution

Undo that and instead use 0 for numbers and "" for strings

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021844825805028000985
• Upwork Job ID: 1844825805028000985
• Last Price Increase: 2024-10-11
• Automatic offers:
• paultsimura | Reviewer | 104595527

Issue Owner

Current Issue Owner: @paultsimura

[ChavdaSachin]

Proposal

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

Update default values for report/account/policy IDs from -1 to 0 and "-1" to empty string "".

What is the root cause of that problem?

• NA

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

Fix all the occurrences where default report/account/policy IDs value is -1 or "-1" to 0 or empty string.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[fabioh8010]

@iwiznia @neil-marcellini Based on our discussions on Slack, I did some experiments considering that we would want to remove default values for both strings and numbers, allowing them to be undefined where we would then handle the changes in the codebase.

So first I did the following replaces in order in the codebase:

1. Replace "ID ?? ''" with "ID"
2. Replace "id ?? ''" with "id"
3. Replace "ID ?? '-1'" with "ID"
4. Replace "id ?? '-1'" with "id"
5. Replace "ID ?? -1" with "ID"
6. Replace "id ?? -1" with "id"
7. Replace "ID ?? '0'" with "ID"
8. Replace "id ?? '0'" with "id" // nothing
9. Replace "ID ?? 0" with "ID"
10. Replace "id ?? 0" with "id" // nothing
11. Replace "ID || ''" with "ID"
12. Replace "id || ''" with "id" // nothing
13. Replace "ID || '-1'" with "ID"
14. Replace "id || '-1'" with "id" // nothing
15. Replace "ID || -1" with "ID"
16. Replace "id || -1" with "id" // nothing
17. Replace "ID || '0'" with "ID" // nothing
18. Replace "id || '0'" with "id" // nothing
19. Replace "id || 0" with "id" // nothing
20. Replace " : '-1'" with " : undefined"
21. Replace " : -1" with " : undefined"
22. Replace " : '0'" with " : undefined"
23. Replace " ?? '-1'" with ""
24. Replace " ?? -1" with ""

All these changes combined with Prettier format resulted in 377 changed files with 1127 additions and 1189 deletions, as we can see in this WIP PR.

After running npm run typecheck to check for TS errors, it was found found 1212 errors in 290 files.

And after running npm run lint to check for lint errors, it was found 348 errors in 44 files.

It's worth noting that these changes and errors are only due to replacing the defaultings, and these numbers can be very different when we start fixing the codebase.

The most common TS errors are:

Argument of type 'X | undefined' is not assignable to parameter of type 'Y'.

Let's take this example on ReportScreen.tsx

src/pages/home/ReportScreen.tsx:261:90 - error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
  Type 'undefined' is not assignable to type 'string'.

261     const transactionThreadReportID = ReportActionsUtils.getOneTransactionThreadReportID(reportID, reportActions ?? [], isOffline);
                                                                                             ~~~~~~~~

ReportActionsUtils.getOneTransactionThreadReportID() requires reportID to be string. In this case we can modify reportID to be string | undefined and we probably won't need to do anything else here.

diff --git a/src/libs/ReportActionsUtils.ts b/src/libs/ReportActionsUtils.ts
index 8fe49eaa80e..9f9a146bda7 100644
--- a/src/libs/ReportActionsUtils.ts
+++ b/src/libs/ReportActionsUtils.ts
@@ -1021,7 +1021,7 @@ const iouRequestTypes = new Set<ValueOf<typeof CONST.IOU.REPORT_ACTION_TYPE>>([
  * Gets the reportID for the transaction thread associated with a report by iterating over the reportActions and identifying the IOU report actions.
  * Returns a reportID if there is exactly one transaction thread for the report, and null otherwise.
  */
-function getOneTransactionThreadReportID(reportID: string, reportActions: OnyxEntry<ReportActions> | ReportAction[], isOffline: boolean | undefined = undefined): string | undefined {
+function getOneTransactionThreadReportID(reportID: string | undefined, reportActions: OnyxEntry<ReportActions> | ReportAction[], isOffline: boolean | undefined = undefined): string | undefined {
     // If the report is not an IOU, Expense report, or Invoice, it shouldn't be treated as one-transaction report.
     const report = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`];
     if (report?.type !== CONST.REPORT.TYPE.IOU && report?.type !== CONST.REPORT.TYPE.EXPENSE && report?.type !== CONST.REPORT.TYPE.INVOICE) {

Now let's take this example on ReportActionsView.tsx

src/pages/home/report/ReportActionsView.tsx:272:40 - error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
  Type 'undefined' is not assignable to type 'string'.

272                 Report.getNewerActions(newestActionCurrentReport?.reportID, newestActionCurrentReport?.reportActionID);
                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

We need to change Report.getNewerActions() arguments to allow undefined. By doing that we could add a condition that return early if one of the parameters are falsy, protecting the code and making TS happy.

diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts
index f58f29d2640..4c8ab5be2cb 100644
--- a/src/libs/actions/Report.ts
+++ b/src/libs/actions/Report.ts
@@ -1163,7 +1163,11 @@ function getOlderActions(reportID: string, reportActionID: string) {
  * Gets the newer actions that have not been read yet.
  * Normally happens when you are not located at the bottom of the list and scroll down on a chat.
  */
-function getNewerActions(reportID: string, reportActionID: string) {
+function getNewerActions(reportID: string | undefined, reportActionID: string | undefined) {
+    if (!reportID || !reportActionID) {
+        return;
+    }
+
     const optimisticData: OnyxUpdate[] = [
         {
             onyxMethod: Onyx.METHOD.MERGE,

The cases and difficult can greatly vary in the codebase, but I think the best move would be to handle these undefined values in the action / lib files like I demonstrated above, so every place that uses these functions can benefit from it.

'X' is possibly 'undefined'.

Let's take this example on ReconciliationAccountSettingsPage.tsx

src/pages/workspace/accounting/reconciliation/ReconciliationAccountSettingsPage.tsx:39:65 - error TS18048: 'paymentBankAccountID' is possibly 'undefined'.

39     const selectedBankAccount = useMemo(() => bankAccountList?.[paymentBankAccountID.toString()], [paymentBankAccountID, bankAccountList]);
                                                                   ~~~~~~~~~~~~~~~~~~~~

This error is inside a component, so we can't just make conditions with early returns here. Also we can't do paymentBankAccountID?.toString() because we'll get another TS error: Type 'undefined' cannot be used as an index type.

diff --git a/src/pages/workspace/accounting/reconciliation/ReconciliationAccountSettingsPage.tsx b/src/pages/workspace/accounting/reconciliation/ReconciliationAccountSettingsPage.tsx
index ee65d11e147..e4bf0138dfe 100644
--- a/src/pages/workspace/accounting/reconciliation/ReconciliationAccountSettingsPage.tsx
+++ b/src/pages/workspace/accounting/reconciliation/ReconciliationAccountSettingsPage.tsx
@@ -36,7 +36,7 @@ function ReconciliationAccountSettingsPage({route}: ReconciliationAccountSetting
     const [cardSettings] = useOnyx(`${ONYXKEYS.COLLECTION.PRIVATE_EXPENSIFY_CARD_SETTINGS}${workspaceAccountID}`);
     const paymentBankAccountID = cardSettings?.paymentBankAccountID;
 
-    const selectedBankAccount = useMemo(() => bankAccountList?.[paymentBankAccountID.toString()], [paymentBankAccountID, bankAccountList]);
+    const selectedBankAccount = useMemo(() => bankAccountList?.[String(paymentBankAccountID)], [paymentBankAccountID, bankAccountList]);
     const bankAccountNumber = useMemo(() => selectedBankAccount?.accountData?.accountNumber ?? '', [selectedBankAccount]);
     const settlementAccountEnding = getLastFourDigits(bankAccountNumber);

A possible solution would be using String(paymentBankAccountID) to try to convert the value to string. If the value is undefined the result string will be "undefined", which will be used to find a record inside bankAccountList and, same as -1, would find nothing.

Type 'undefined' cannot be used as an index type.

Let's take this example on Report.ts

src/libs/actions/Report.ts:901:63 - error TS2538: Type 'undefined' cannot be used as an index type.

901             const isOptimisticAccount = !allPersonalDetails?.[accountID];
                                                                  ~~~~~~~~~

Here the best solution is put a condition to make the function return early if accountID is falsy.

diff --git a/src/libs/actions/Report.ts b/src/libs/actions/Report.ts
index f58f29d2640..b32d5b6abdf 100644
--- a/src/libs/actions/Report.ts
+++ b/src/libs/actions/Report.ts
@@ -898,6 +898,10 @@ function openReport(
         const participantAccountIDs = PersonalDetailsUtils.getAccountIDsByLogins(participantLoginList);
         participantLoginList.forEach((login, index) => {
             const accountID = participantAccountIDs.at(index);
+            if (!accountID) {
+                return;
+            }
+
             const isOptimisticAccount = !allPersonalDetails?.[accountID];
 
             if (!isOptimisticAccount) {

We could use String() to convert accountID to string or "undefined", but this will create more TS errors as optimisticPersonalDetails[accountID] object expects a number in the accountID property.

---

These are only some examples of errors with proposed solutions, but we will definitely have more complex scenarios that need to be evaluated individually.

As you can see this is a big refactor in the application, so the cost and impact of this must be taken into consideration. Also we should decide if this is really the definitive move we want to do.

I imagine that these would be the tasks:

1. Make all these replacements in the codebase and fix them properly in one PR (maybe we could split into more?).
2. Update the guidelines to reflect the new standards (don't default any ID values unless absolutely necessary), and provide some examples and common use cases.
3. Maybe add a new item to the checklist for the reviewer to check this and ensure we keep the codebase standardised.
4. Additionally, we could implement an improvement in Onyx to "ignore" any updates and retrievals of collection entries that has the key undefined or -1 (we can make this configurable in Onyx API), something that could be done in parallel or before/after this.

[melvin-bot]

@neil-marcellini Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

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

[melvin-bot]

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

[paultsimura]

I guess this GH won't result in a large PR, right? Maybe we'll create a GH workflow though?

[fabioh8010]

@paultsimura I mentioned this idea on Friday, see -> https://expensify.slack.com/archives/C01GTK53T8Q/p1728662249916699?thread_ts=1728329641.957179&cid=C01GTK53T8Q

[melvin-bot]

@paultsimura, @neil-marcellini Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[paultsimura]

@iwiznia @neil-marcellini are we anywhere close to a consensus on the new approach?

[melvin-bot]

@paultsimura, @neil-marcellini Eep! 4 days overdue now. Issues have feelings too...

[fabioh8010]

Waiting for feedback here.

[neil-marcellini]

I reviewed all messages since I last look and replied in that Slack thread.

[fabioh8010]

As discussed in the Slack thread here's the refined P/S statement. cc @iwiznia @neil-marcellini @aldo-expensify @paultsimura

Problem

Throughout the app we default strings to '-1' and numbers to -1. Doing so breaks conditions checking if an ID is set, such as if (!policyID) {, because the defaults are truthy when they should be falsy. That can directly cause bugs, and it’s also in conflict with our backend norms, where we have many similar checks.

Other problems:

1. If a string ID defaults to '' and is passed to withOnyx / useOnyx, we could accidentally subscribe to the entire Onyx collection which will cause crashes/bugs since we expect a single record of the collection.
2. If a string ID defaults to '-1' or some other non-empty string or even 'undefined', it’s possible that there is mistakenly a record in Onyx with that ID, which can cause very tricky bugs.
3. If the type of an ID is not possibly undefined (e.g. always a string), then a naive developer might assume it’s always a valid value, vs something falsy.

Solution

1. Update guidelines to require developers to default all number IDs to 0 and forbid defaulting string IDs unless absolutely necessary. Include examples about how to deal with common scenarios when removing the default values.
2. Create an ESLint rule to check for the following exact matches through the changed files in the PR and fail if one is found. With that the PR author will have to change/remove the default values in the files and make the necessary adjustments in other files if needed (something similar like we have for withOnyx deprecation). If the PR is critical, already too big or we have false positives, exceptions can be made to the rule.
   1. ?? '-1'
   2. ?? -1
   3. ID ?? ''
   4. id ?? ''
   5. ID ?? '0'
   6. id ?? '0'
   7. || '-1'
   8. || -1
   9. ID || ''
   10. id || ''
   11. ID || '0'
   12. id || '0'
   13. : '-1'
   14. : -1
   15. : '0'
3. Implement an improvement in Onyx to allow us to specify a list of record IDs that we want to be "nullable". So any updates/access to a collection with this record ID will produce no effect or make Onyx return undefined, thus eliminating risks of passing something like policy_undefined and actually getting an unexpected record (it’s possible to happen).

[neil-marcellini]

@fabioh8010 we got a thumbs up in Slack from two other internal engineers, so I'm going to assign you to get started. There's only a very small possibility that others will come in an disagree in the next few days.

[iwiznia]

Doing them in different PRs sounds good, but I don't think we need separate issues?

[VickyStash]

@iwiznia No problem, could you please assign me to this issue?
Fabio is OOO till Jan 7th, I can start these files updates one by one!

[adhorodyski]

I'll jump in and help with the src/libs/actions/IOU.ts file :)

[pac-guerreiro]

I'll jump in and help with the src/libs/actions/Report.ts file :)

[VickyStash]

There are several files which different contributors complained on additionally:

'src/libs/OptionsListUtils.ts'
'src/libs/ReportActionsUtils.ts'
'src/libs/TransactionUtils/index.ts'
'src/pages/home/ReportScreen.tsx'

Some of them overlapping with other utils requiring a lot of changes, some causing jest tests failing after updates. So it will be better to update these files separately as well.

I've opened the PR to add the to exceptions list, let me know what you think!

[VickyStash]

Updates:

• Created a small PR to add more files, requested by devs to the exceptions list (see comment)
• Working on theReportUtils + ReportActionsUtils files updates to align default IDs. I've opened Draft PR, but I still need to resolve some TS issues and do some polish and testing.

[VickyStash]

Updates:

• Prepared ReportUtils file updates PR for the review (tested, resolved conflicts, added recordings)

[pac-guerreiro]

Today I applied most of the suggestions I got in my PR but left some questions

[VickyStash]

Updates:

• WIP on updating of IOU.ts file together with Adam, cause the file is pretty big and complex.

[pac-guerreiro]

Today I resolved the remaining eslint issues 😄

I left some replies to @VickyStash but I wasn't able to confirm what the API expects in some situations. I guess it should be face to pass undefined for those ids.

I'll be away until January 2nd but someone should take care of my work 😄 Happy holidays! 🎉

[VickyStash]

I have not made much progress today as I needed to focus on another issue.
Tomorrow, I should be fully focused on this one!

[melvin-bot]

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

[melvin-bot]

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

• [No QA] Bump eslint-config-expensify to 2.0.75 #54117
• [No QA] Follow-up: Disable default id eslint rule for the large files #54236

If no regressions arise, payment will be issued on 2024-12-30. 🎊

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

• @paultsimura requires payment automatic offer (Reviewer)
• @fabioh8010 does not require payment (Contractor)
• @VickyStash does not require payment (Contractor)

[VickyStash]

Updates:

• Applied feedback in ReportUtils updates PR
• Fixed all of the lint errors in IOU file and opened Draft PR:
  TODOs:
  • Re-review the code making sure all of the updates reasonable
  • Test related functionality
  • Prepare PR for the review

Note: I'm going to be OOO Dec 25-29 🎄