[HOLD for payment 2024-06-28] [HOLD for payment 2024-06-24] [$250] [TS Migration] Define a concise way to access or default to an inexistent record

[mountiny]

Follow up issue for TS migration project. Coming from this spreadsheet

Define a concise way to access or default to an inexistent record (0, -1, etc)

Any more details for this one cc @blazejkustra @fabioh8010

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0127e590dfa2accb42
• Upwork Job ID: 1780548153949810688
• Last Price Increase: 2024-04-24
• Automatic offers:
• jjcoffee | Reviewer | 0
• godofoutcasts94 | Contributor | 0

Issue Owner

Current Issue Owner: @johncschuster

[fabioh8010]

Problem

Currently we have been defaulting nullable entity IDs to either 0 or -1 when it doesn’t exist as we can see here and here (and many other files throughout the project). The lack of conciseness when defaulting such IDs leads to confusion between developers and increased chance of bugs.

Solution

Standartise the default values of all Onyx entity IDs. Personally I think we should default all nullable entity IDs to -1 as it is a negative ID and theoretically we would never have a negative one in the DB (@mountiny makes sense to you?):

1. For each Onyx type inside src/types/onyx/, look for properties that ends with ID?: ..., e.g. managerID?: number;, parentReportID?: string;, iouReportID?: string; inside Report.ts
2. For each property, look for its usage throughout the codebase.
3. If that property is being defaulted to either 0 or -1, change it to -1.
4. Change TS_STYLE.md to include this advice of using -1 to default these kind of ID properties.
5. Test everything extensively.

[mountiny]

@fabioh8010 this makes sense to me, but I think we need to bring this up to slack for broader agreement, can you post the proposal to open source channel before proceeding?

[fabioh8010]

@mountiny Sure, I can post! But could you make this issue External? It was marked as External initially.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @johncschuster (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[melvin-bot]

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

[ghost]

Proposal

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

Define a concise way to access or default to an inexistent record

What is the root cause of that problem?

Coming from TS Migration Issue, As discussed in the comment here - #39125 (comment), we need to look for properties that ends with ID?: ..., e.g. managerID?: number;, parentReportID?: string;, iouReportID?: string; inside Report.ts

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

We need to standartise the default values of all Onyx entity IDs. we need to change the property to -1 in the codebase. We need to add tests for it as well. Also, as mentioned here, we need to update TS_STYLE.md to include this advice of using -1 to default these kind of ID properties.

Just an example situtaion :

Here in the Report.ts:

App/src/types/onyx/Report.ts

Line 135 in 944751f

managerID?: number;

we have managerID?: number;. So if I search for managerID in the codebase I can find out that it is used here
ReportPreview.tsx :

App/src/components/ReportActionItem/ReportPreview.tsx

Line 121 in f05d306

const managerID = iouReport?.managerID ?? 0;

we are using const managerID = iouReport?.managerID ?? 0; and defining it as 0, so we need to update it to -1

What alternative solutions did you explore? (Optional)

N/A

[fabioh8010]

@fabioh8010 this makes sense to me, but I think we need to bring this up to slack for broader agreement, can you post the proposal to open source channel before proceeding?

Posted here.

[fabioh8010]

@fabioh8010 this makes sense to me, but I think we need to bring this up to slack for broader agreement, can you post the proposal to open source channel before proceeding?

Posted here.

⚠️ Solution was approved, but let's consider this additional change:

Change TS_STYLE.md to include this advice of using -1 to default these kind of ID properties.

[jjcoffee]

@godofoutcasts94 Thanks for your proposal! It's a bit copy & pastey as it stands, so could I ask you to provide an example of a change you would make so that I can make sure you understand the issue. Thanks!

[ghost]

Hey @jjcoffee sorry for late reply. Updated proposal

[fabioh8010]

@kubabutkiewicz We can also ask for a full regression test when opening the PR, would be good to have it.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[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 1.4.84-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [TS Migration] Define a concise way to access or default to an inexistent record #42634

If no regressions arise, payment will be issued on 2024-06-24. 🎊

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

• @kubabutkiewicz does not require payment (Contractor)
• @jjcoffee requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@jjcoffee] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[melvin-bot]

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

• [TS Migration] Define a concise way to access or default to an inexistent record #42634

If no regressions arise, payment will be issued on 2024-06-28. 🎊

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

• @kubabutkiewicz does not require payment (Contractor)
• @jjcoffee requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@jjcoffee] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@johncschuster] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[johncschuster]

Discussing in Slack to make sure I'm understanding the correct payment date

[johncschuster]

@jjcoffee do we need a regression test for this? If so, can you propose the steps?

[johncschuster]

Payment Summary:

Contributor: @kubabutkiewicz - does not require payment
Contributor+: @jjcoffee - $250 - paid via Upwork - PAID

[jjcoffee]

do we need a regression test for this? If so, can you propose the steps?

@johncschuster No I don't think so, since this is an app-wide change.

[johncschuster]

Awesome. Thank you!