[HOLD for payment 2025-01-02] Remove isArray checks for onboarding

[carlosmiceli]

We have a few isArray checks for displaying or not the onboarding flow. For example:

    // Onboarding is an array or an empty object for old accounts and accounts created from OldDot
    if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {
        return true;
    }

We should clean this up since onboarding has changed and should never be an array. Please include videos of all the flows where we are making this change and showing that it's still working properly afterwards.

Issue Owner

Current Issue Owner: @CortneyOfstad

[melvin-bot]

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

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-12-03 16:11:19 UTC.

Proposal

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

Remove isArray checks for onboarding

What is the root cause of that problem?

We're checking isArray here

App/src/libs/onboardingSelectors.ts

Line 14 in 2c78c93

if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {

App/src/libs/actions/Welcome/index.ts

Line 46 in 2c78c93

if (Array.isArray(onboarding) || isEmptyObject(onboarding) || onboarding?.hasCompletedGuidedSetupFlow === undefined) {

App/src/libs/onboardingSelectors.ts

Line 51 in 2c78c93

if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {

App/src/libs/ReportUtils.ts

Line 8293 in 2c78c93

if (onboarding && !Array.isArray(onboarding) && !isEmptyObject(onboarding) && onboarding.chatReportID) {

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

Remove isArray check in these places.

App/src/libs/onboardingSelectors.ts

Line 14 in 2c78c93

if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {

App/src/libs/actions/Welcome/index.ts

Line 46 in 2c78c93

if (Array.isArray(onboarding) || isEmptyObject(onboarding) || onboarding?.hasCompletedGuidedSetupFlow === undefined) {

App/src/libs/onboardingSelectors.ts

Line 51 in 2c78c93

if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {

App/src/libs/ReportUtils.ts

Line 8293 in 2c78c93

if (onboarding && !Array.isArray(onboarding) && !isEmptyObject(onboarding) && onboarding.chatReportID) {

We should also update the OnboardingData type to remove the array type

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

NA

What alternative solutions did you explore? (Optional)

[saifelance]

Problem:

isArray checks for the onboarding flow are outdated. Onboarding should no longer be an array, but the code still checks for it.

Root Cause:

The onboarding data structure has changed, but the code still checks for an array, causing redundancy and potential issues.

Proposed Changes:

Remove Array.isArray checks for onboarding.
Update conditions to reflect the new structure:

if (isEmptyObject(onboarding)) {
    return true;
}

Ensure all references to onboarding match its new structure.

Test Scenarios:

Verify onboarding flow works for old and new accounts.
Ensure no UI issues or broken functionality.
Check onboarding for OldDot accounts.

Alternative Solutions:

Removing these checks is the best solution, no alternatives needed.

[carlosmiceli]

FYI, when I just remove the check, I see this warning: Property 'hasCompletedGuidedSetupFlow' does not exist on type 'Onboarding | []'. Property 'hasCompletedGuidedSetupFlow' does not exist on type '[]'.

@getusha Something to consider when looking at proposals.

[nkdengineer]

@carlosmiceli We also need to update OnboardingData type to remove the array type.

[saifelance]

Update the type of onboarding

let onboarding: Onboarding | [];

to

let onboarding: Onboarding; // Assuming Onboarding is the correct type now

Type assertion (if needed): If you can't change the type definition directly

if (isEmptyObject(onboarding)) {
    return true;
}

const onboardingData = onboarding as Onboarding; // Type assertion
if (onboardingData.hasCompletedGuidedSetupFlow) {
    // Use hasCompletedGuidedSetupFlow safely here
}

Refactor conditional logic:

After removing the Array.isArray check, ensure your checks and conditions align with the new data structure.

[twilight2294]

Edited by proposal-police: This proposal was edited at 2024-12-03 17:50:10 UTC.

Proposal

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

Remove isArray checks for onboarding

What is the root cause of that problem?

We are no longer sending arrays as Onboarding value, so we are removing them

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

We need to do three things, 1 is to remove the usage of isArray check and then 2 is to remove the unit test associated with it as well and 3 is to clear the array type from multiple places
Remove the usage of Array.isArray(onboarding) from the following paces:

App/src/libs/onboardingSelectors.ts

Line 51 in b255293

if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {

App/src/libs/onboardingSelectors.ts

Line 14 in b255293

if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {

App/src/libs/actions/Welcome/index.ts

Line 46 in b255293

if (Array.isArray(onboarding) || isEmptyObject(onboarding) || onboarding?.hasCompletedGuidedSetupFlow === undefined) {

App/src/libs/ReportUtils.ts

Line 8295 in b255293

if (onboarding && !Array.isArray(onboarding) && !isEmptyObject(onboarding) && onboarding.chatReportID) {

Also we need to remove the | [] type assumption from below:

App/src/libs/actions/Welcome/index.ts

Line 15 in b255293

type OnboardingData = Onboarding | [] | undefined;

Also need to clear the [] array type from below (Note that it is important to also remove the array assumption from below and not only OnboardingData:

App/src/ONYXKEYS.ts

Lines 875 to 876 in b255293

	// NVP_ONBOARDING is an array for old users.
	[ONYXKEYS.NVP_ONBOARDING]: Onboarding | [];

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

Instead of covering a extra test, we should remove an existing unit test which covers the array case:

App/tests/unit/OnboardingSelectorsTest.ts

Lines 11 to 14 in b255293

	it('Should return true if onboarding NVP is an array', () => {
	const onboarding = [] as OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>;
	expect(hasCompletedGuidedSetupFlowSelector(onboarding)).toBe(true);
	});

(This is also important)

What alternative solutions did you explore? (Optional)

N/A

[nkdengineer]

Just a note here: it's easy to remove the existing unit test while implementing the PR because the typecheck will fail after we remove array type from OnboardingData.

[twilight2294]

I would also like to note that proposals are meant to cover these cases that is why we have the new section in proposal template, it takes time to research and write a complete and correct proposal

[melvin-bot]

@carlosmiceli, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[carlosmiceli]

Removing @getusha since there's been no response. Asking in Slack for a new C+.

[dominictb]

Reviewing.

[dominictb]

Thanks all for your proposals.

There are 2 minor improvements suggested by @twilight2294 that has not been covered by @nkdengineer. But I agree with @nkdengineer reasoning here #53476 (comment) that these are straight forward and can be easily discovered during implementation.

Let's go with @nkdengineer proposal #53476 (comment)
🎀👀🎀

[melvin-bot]

Current assignee @carlosmiceli is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[twilight2294]

@dominictb can you please re-evaluate the proposals, The new proposal format requires things like tests required and the places we need updates, the other contributors proposal also missed the updates for ONYXKEYS.ts , which is important.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

This new part of the proposal is also important otherwise what sense does it make to add it when no one follows the format ?

I think writing a clear and complete solution takes time, that is why we added tests too. in the rush of posting proposals first, these things are ignored, it really sets a wrong precedence that we should be quick that correct!. also other contributor realised that we need to remove those | [] assumptions after @carlosmiceli mentioned them here, so i request you to please re-evaluate the proposal here as I think i have a more correct and complete solution

[carlosmiceli]

This is not urgent, so we should prioritize the cleanest and most robust solution. Are you sure that's the one you selected @dominictb ?

[nkdengineer]

We should also update the OnboardingData type to remove the array type

1. I already mentioned removing the array type of onboarding data. As I mentioned here, other places can be done easily in the PR phase. For example, the type in ONYXKEYS.ts will conflict with OnboardingData after we remove the array type and we can do it easily in the PR.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

The purpose of the test is to add the test to prevent a bug can happening in the future. In this case, it's not a bug, it's just a
clean-up and the test with the wrong value can be done at the point 1 I mentioned above.

[dominictb]

Appreciate the input, everyone! To wrap things up, @nkdengineer's proposal tackled the key changes we needed, while @twighlight highlighted two additional cleanups that are pretty straightforward—those would have been flagged anyway as lint or type errors while pushing the code.

Just a heads-up, my choice is based on the internal guidelines here. That said, I'll leave the final call to the internal engineer @carlosmiceli. Thanks again for the collaboration!

[carlosmiceli]

To confirm @dominictb, are you saying to assign this to @nkdengineer and the added value to @twilight2294 ?

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[carlosmiceli]

Fyi, @CortneyOfstad This should be split between @nkdengineer (main proposal) and @twilight2294 (improvement on top of the proposal).

[nkdengineer]

This should be split between @nkdengineer (main proposal) and @twilight2294 (improvement on top of the proposal).

@carlosmiceli Thanks, I agree. I can take the PR phrase.

[CortneyOfstad]

@carlosmiceli — I'm a bit confused by the comment here. Are you referencing splitting the payment?

[CortneyOfstad]

Also, just a heads up — I will be OOO starting this afternoon (December 20th) and will be returning January 6th. A handful of folks on the BZ team will be online for a few days in between the 25th and the 1st, but we'll be operating with a skeleton crew. If there is any action needed from a BZ perspective, please post this issue in #expensify-open-source and someone on the team will jump in. Otherwise, I will review when I return on January 6th. Thanks and Happy Holidays!

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

• Remove isArray checks for onboarding #54353

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

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

• @nkdengineer requires payment (Needs manual offer from BZ)
• @dominictb requires payment (Needs manual offer from BZ)
• @twilight2294 requires payment (Needs manual offer from BZ)

[melvin-bot]

@dominictb @CortneyOfstad @dominictb The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]