[HOLD for payment 2024-05-08] [Simplified Collect][CLEAN UP][LOW] Standardize on Workspace wrapper

[luacmartins]

Problem

Coming from here, we have a few wrappers we use in Workspace pages, i.e. WorkspacePageWithSections, AdminPolicyAccessOrNotFoundWrapper and PaidPolicyAccessOrNotFoundWrapper. We should standardize on using a single one for all our pages.

Those wrappers also break the not-found views.

Going to e.g. https://staging.new.expensify.com/workspace/dssaddssdaads/categories` we see duplicated not found (that's what is the scope of the other issue) and after pressing the back button we see another ones...

Why is this important

DRYs code and makes it more maintainable

Solution

Investigate how to combine them and update all usages.

cc @kosmydel

Issue Owner

Current Issue Owner: @JmillsExpensify

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

@JmillsExpensify, @luacmartins Eep! 4 days overdue now. Issues have feelings too...

[JmillsExpensify]

Added a weekly here since this is low and in the polish bucket.

[luacmartins]

Polish item

[kosmydel]

A new wrapper FeatureEnabledAccessOrNotFoundWrapper.tsx emerged in this PR.

I've consulted it internally, and someone from SWM will take care of this issue.

[BrtqKr]

Hey, I'm an engineer from SWM, I'll be taking over this issue 👋

[luacmartins]

Nice! Excited for this one! @BrtqKr would you mind sharing the general approach you're thinking of taking before we dive deep into the code?

[BrtqKr]

@luacmartins the code is literally identical in most of those wrappers, so the idea is to:

1. Merge AdminPolicyAccessOrNotFoundWrapper and PaidPolicyAccessOrNotFoundWrapper into a single AccessOrNotFoundWrapper. AccessOrNotFoundWrapper will receive accessVariants array prop, which will be mapped into a proper function for determining whether the not found page should be displayed, something like this to be more exact:

const POLICY_ACCESS_VARIANTS = {
    PAID: (policy: OnyxEntry<OnyxTypes.Policy>) => !PolicyUtils.isPaidGroupPolicy(policy) || !policy?.isPolicyExpenseChatEnabled,
    ADMIN: (policy: OnyxEntry<OnyxTypes.Policy>) => !PolicyUtils.isPolicyAdmin(policy),
} as const satisfies Record<string, (policy: OnyxTypes.Policy) => boolean>;

2. Merge the the existing FeatureEnabledAccessOrNotFoundWrapper with the previously mentioned wrappers, based on the optional featureName param - we just check whether the feature is enabled and add it to the other conditions. And the other difference is a fallback page being displayed - for now, that would be NotFoundPage for the standard conditions and FullPageNotFoundView if this is based on a feature flag, but we could make it more configurable in the future.

[luacmartins]

Sounds good. Could we add FeatureEnabledAccessOrNotFoundWrapper as a predicate to the POLICY_ACCESS_VARIANTS as well? Maybe each entry could be an object with a specific fallback page too so it can handle future changes without updating the wrapper itself, something like:

const POLICY_ACCESS_VARIANTS = {
    PAID: {
         predicate: (policy: OnyxEntry<OnyxTypes.Policy>) => !PolicyUtils.isPaidGroupPolicy(policy) || !policy?.isPolicyExpenseChatEnabled,
         fallback: NotFoundPage
    },
    ...
}

[hayata-suenaga]

hello hello 👋

There are some PRs that add another set of new screens that should be limited to admin users.

Depending on when the unified wrapper that is suggested here is ready, we might use that one or create temporary wrappers. I'd appreciate it if you could share the rough ETA for it if you could, @BrtqKr 🙇

[BrtqKr]

@hayata-suenaga, we'll be sending a PR with those changes this evening at the latest

[hayata-suenaga]

@BrtqKr

I forgot to update here, but we're reassessing our approach to protecting routes (i.e., restricting screen access to certain users).

We started developing a pattern where a wrapper screen is created for each protected screen to check if the user has the necessary permissions to access it or not. Based on the user's access level, either the requested screen is displayed or the "not-found" page is displayed.

I understand that this GH ticket is to standardize this pattern and create a unified wrapper component.

However, after internal discussions, our engineers have decided to adjust our strategy. Instead of implementing access logic at the component level, we now prefer to do so in the navigation layer. React Navigation allows for the conditional rendering of screens within a navigator. We will use this feature to implement protected routes. This approach is recommended in the React Navigation documentation.

cc: @roryabraham

[hayata-suenaga]

We plan to test that the conditional rendering approach does indeed work with our current codebase by using this pattern in the newly added accounting-related screens. If the approach is confirmed to be effective, we will replace the currently implemented wrappers with conditional rendering inside navigators.

[BrtqKr]

Sounds good. Could we add FeatureEnabledAccessOrNotFoundWrapper as a predicate to the POLICY_ACCESS_VARIANTS as well? Maybe each entry could be an object with a specific fallback page too so it can handle future changes without updating the wrapper itself, something like:

const POLICY_ACCESS_VARIANTS = {
    PAID: {
         predicate: (policy: OnyxEntry<OnyxTypes.Policy>) => !PolicyUtils.isPaidGroupPolicy(policy) || !policy?.isPolicyExpenseChatEnabled,
         fallback: NotFoundPage
    },
    ...
}

In general, this was unnecessarily increasing complexity, so I ended up treating feature flags as a separate case. Overall, the main problem is the fact that those conditions can happen simultaneously, so we would need to somehow determine which one of them is more important. There might be different cases with a different priority, so I'd just make the feature flag the more important variant since it was able to cover all existing cases and there have been a lot of them.

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

• [Simplified Collect][CLEAN UP][LOW] Standardize on Workspace wrapper #40598

If no regressions arise, payment will be issued on 2024-05-08. 🎊

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

• @BrtqKr does not require payment (Contractor)

[melvin-bot]

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

• [@luacmartins] The PR that introduced the bug has been identified. Link to the PR:
• [@luacmartins] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@luacmartins] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@BrtqKr] Determine if we should create a regression test for this bug.
• [@BrtqKr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[melvin-bot]

@JmillsExpensify, @luacmartins, @BrtqKr Huh... This is 4 days overdue. Who can take care of this?

[BrtqKr]

It was merged some time ago #40598, not sure why Melvin is still mentioning us. Would you mind closing this issue, or is there anything left to do?

[luacmartins]

It seems like we need to payout C+ @ahmedGaber93 for the PR review.

[melvin-bot]

@JmillsExpensify, @luacmartins, @ahmedGaber93, @BrtqKr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[JmillsExpensify]

Payment summary: @ahmedGaber93 $500 for pr review and testing.

[JmillsExpensify]

Offer sent via Upwork

[JmillsExpensify]

Offer paid out, so closing out this issue.