[HOLD for payment 2025-02-07] [HOLD for payment 2025-02-04] [HOLD for payment 2025-01-28] [HOLD for payment 2025-01-24] [HOLD for payment 2025-01-22] [HOLD for payment 2025-01-15] [HOLD for payment 2025-01-02] [HOLD for payment 2024-12-30] [$250] Undo defaults of -1 and "-1" for account/report/policy IDs #50360

iwiznia · 2024-10-07T21:23:41Z

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: @laurenreidexpensify

The text was updated successfully, but these errors were encountered:

ChavdaSachin · 2024-10-07T21:39:33Z

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 · 2024-10-10T18:52:17Z

@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:

Replace "ID ?? ''" with "ID"
Replace "id ?? ''" with "id"
Replace "ID ?? '-1'" with "ID"
Replace "id ?? '-1'" with "id"
Replace "ID ?? -1" with "ID"
Replace "id ?? -1" with "id"
Replace "ID ?? '0'" with "ID"
Replace "id ?? '0'" with "id" // nothing
Replace "ID ?? 0" with "ID"
Replace "id ?? 0" with "id" // nothing
Replace "ID || ''" with "ID"
Replace "id || ''" with "id" // nothing
Replace "ID || '-1'" with "ID"
Replace "id || '-1'" with "id" // nothing
Replace "ID || -1" with "ID"
Replace "id || -1" with "id" // nothing
Replace "ID || '0'" with "ID" // nothing
Replace "id || '0'" with "id" // nothing
Replace "id || 0" with "id" // nothing
Replace " : '-1'" with " : undefined"
Replace " : -1" with " : undefined"
Replace " : '0'" with " : undefined"
Replace " ?? '-1'" with ""
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:

Make all these replacements in the codebase and fix them properly in one PR (maybe we could split into more?).
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.
Maybe add a new item to the checklist for the reviewer to check this and ensure we keep the codebase standardised.
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 · 2024-10-11T18:21:14Z

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

melvin-bot · 2024-10-11T19:42:03Z

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

melvin-bot · 2024-10-11T19:42:08Z

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

paultsimura · 2024-10-12T08:56:43Z

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

fabioh8010 · 2024-10-14T14:50:43Z

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

melvin-bot · 2024-10-15T18:16:09Z

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

paultsimura · 2024-10-16T17:30:28Z

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

melvin-bot · 2024-10-21T18:25:29Z

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

fabioh8010 · 2024-10-22T10:28:18Z

Waiting for feedback here.

neil-marcellini · 2024-10-22T19:04:18Z

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

fabioh8010 · 2024-10-23T16:17:50Z

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.

Solution

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.
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'
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 · 2024-10-25T19:07:47Z

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

kubabutkiewicz · 2025-01-27T15:59:50Z

Updates on PureReportActionItem:

I make changes requested by @neil-marcellini, waiting for final review

fabioh8010 · 2025-01-27T22:43:24Z

Updates:

All remaining PRs were merged!
Based on this discussion, we created a new issue to add a case to the ESLint rule. PR was merged and will proceed with E/App one.

melvin-bot · 2025-01-28T01:52:10Z

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

melvin-bot · 2025-01-28T01:52:16Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.89-8 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 2025-02-04. 🎊

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 · 2025-01-28T12:23:48Z

Updates:

The PR #55535 which cleans up wrong default number IDs CONST.DEFAULT_NUMBER_ID usages inside strings was opened for the review.

paultsimura · 2025-01-28T12:24:37Z

@neil-marcellini could you please create a sub-issue for #55535?

VickyStash · 2025-01-28T13:01:03Z

I guess this one is related: #55814
Should I post my PR under it?

fabioh8010 · 2025-01-29T23:44:35Z

Updates:

ESLint version upgrade is being handled in Vicky's PR.
My Onyx bump PR was merged and then reverted because some unit tests started to fail in GH actions, I will open another PR tomorrow and we'll see how it goes this time.

melvin-bot · 2025-01-31T06:51:23Z

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

melvin-bot · 2025-01-31T06:51:28Z

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

Bump Onyx to 2.0.91 / Enable skippable collection member IDs in Onyx #54163

If no regressions arise, payment will be issued on 2025-02-07. 🎊

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)

laurenreidexpensify · 2025-02-05T12:28:45Z

laurenreidexpensify · 2025-02-05T12:29:02Z

@paultsimura please fill out the above ^^ thanks

paultsimura · 2025-02-05T12:33:25Z

Thanks @laurenreidexpensify, I will fill out today.
However, I would like to request a bounty increase for this issue as I've reviewed multiple pull requests here, most of which were quite large:

WDYT about it?

paultsimura · 2025-02-06T15:31:44Z

marcochavezf · 2025-02-06T17:00:30Z

Hi @laurenreidexpensify, can we also handle the payment for C+ for @allgandalf here for the related PR? Or should we create a new one?

iwiznia added the Task label Oct 7, 2024

iwiznia assigned neil-marcellini Oct 7, 2024

iwiznia added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 7, 2024

melvin-bot bot added the Daily KSv2 label Oct 7, 2024

melvin-bot bot added the Overdue label Oct 10, 2024

neil-marcellini added the External Added to denote the issue can be worked on by a contributor label Oct 11, 2024

melvin-bot bot changed the title ~~Undo defaults of -1 and "-1" for account/report/policy IDs~~ [$250] Undo defaults of -1 and "-1" for account/report/policy IDs Oct 11, 2024

neil-marcellini removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 11, 2024

melvin-bot bot assigned paultsimura Oct 11, 2024

melvin-bot bot removed the Overdue label Oct 11, 2024

melvin-bot bot added the Overdue label Oct 14, 2024

melvin-bot bot removed the Overdue label Oct 16, 2024

melvin-bot bot added the Overdue label Oct 21, 2024

melvin-bot bot removed the Overdue label Oct 22, 2024

melvin-bot bot added the Overdue label Oct 24, 2024

melvin-bot bot removed the Overdue label Oct 25, 2024

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jan 28, 2025

melvin-bot bot removed the Reviewing Has a PR in review label Jan 28, 2025

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Jan 28, 2025

melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jan 31, 2025

melvin-bot bot removed the Reviewing Has a PR in review label Jan 31, 2025

fabioh8010 mentioned this issue Feb 3, 2025

Bump Onyx to 2.0.92 #56166

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iwiznia commented Oct 7, 2024 •

edited by melvin-bot bot

Loading

ChavdaSachin commented Oct 7, 2024

fabioh8010 commented Oct 10, 2024

melvin-bot bot commented Oct 11, 2024

melvin-bot bot commented Oct 11, 2024

melvin-bot bot commented Oct 11, 2024

paultsimura commented Oct 12, 2024 •

edited

Loading

fabioh8010 commented Oct 14, 2024

melvin-bot bot commented Oct 15, 2024

paultsimura commented Oct 16, 2024

melvin-bot bot commented Oct 21, 2024

fabioh8010 commented Oct 22, 2024

neil-marcellini commented Oct 22, 2024

fabioh8010 commented Oct 23, 2024 •

edited

Loading

neil-marcellini commented Oct 25, 2024

kubabutkiewicz commented Jan 27, 2025

fabioh8010 commented Jan 27, 2025

melvin-bot bot commented Jan 28, 2025

melvin-bot bot commented Jan 28, 2025

VickyStash commented Jan 28, 2025

paultsimura commented Jan 28, 2025

VickyStash commented Jan 28, 2025

fabioh8010 commented Jan 29, 2025

melvin-bot bot commented Jan 31, 2025

melvin-bot bot commented Jan 31, 2025

laurenreidexpensify commented Feb 5, 2025

Regression Test Proposal

Precondition:

Test:

laurenreidexpensify commented Feb 5, 2025

paultsimura commented Feb 5, 2025

paultsimura commented Feb 6, 2025

marcochavezf commented Feb 6, 2025

Comments

iwiznia commented Oct 7, 2024 • edited by melvin-bot bot Loading

Problem

Solution

ChavdaSachin commented Oct 7, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

fabioh8010 commented Oct 10, 2024

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

'X' is possibly 'undefined'.

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

melvin-bot bot commented Oct 11, 2024

melvin-bot bot commented Oct 11, 2024

melvin-bot bot commented Oct 11, 2024

paultsimura commented Oct 12, 2024 • edited Loading

fabioh8010 commented Oct 14, 2024

melvin-bot bot commented Oct 15, 2024

paultsimura commented Oct 16, 2024

melvin-bot bot commented Oct 21, 2024

fabioh8010 commented Oct 22, 2024

neil-marcellini commented Oct 22, 2024

fabioh8010 commented Oct 23, 2024 • edited Loading

Problem

Solution

neil-marcellini commented Oct 25, 2024

kubabutkiewicz commented Jan 27, 2025

fabioh8010 commented Jan 27, 2025

melvin-bot bot commented Jan 28, 2025

melvin-bot bot commented Jan 28, 2025

VickyStash commented Jan 28, 2025

paultsimura commented Jan 28, 2025

VickyStash commented Jan 28, 2025

fabioh8010 commented Jan 29, 2025

melvin-bot bot commented Jan 31, 2025

melvin-bot bot commented Jan 31, 2025

laurenreidexpensify commented Feb 5, 2025

BugZero Checklist:

Regression Test Proposal

Precondition:

Test:

laurenreidexpensify commented Feb 5, 2025

paultsimura commented Feb 5, 2025

paultsimura commented Feb 6, 2025

BugZero Checklist:

Regression Test Proposal

marcochavezf commented Feb 6, 2025

iwiznia commented Oct 7, 2024 •

edited by melvin-bot bot

Loading

paultsimura commented Oct 12, 2024 •

edited

Loading

fabioh8010 commented Oct 23, 2024 •

edited

Loading