Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/pages/workspace/upgrade/UpgradeIntro.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,9 @@ function UpgradeIntro({feature, onUpgrade, buttonDisabled, loading, isCategorizi
* However, the policyID check is only necessary when the user is not coming from the "Categorize" option.
* The "isCategorizing" flag is set to true when the user accesses the "Categorize" option in the Self-DM whisper.
* In such scenarios, a separate Categories upgrade UI is displayed.
* Auto-pay approved reports requires Control but not advanced approvals, so it also uses the generic Control upgrade view.
*/
if (!feature || (!isCategorizing && !isDistanceRateUpgrade && !isReporting && !policyID)) {
if (!feature || feature.id === CONST.UPGRADE_FEATURE_INTRO_MAPPING.autoPayApprovedReports.id || (!isCategorizing && !isDistanceRateUpgrade && !isReporting && !policyID)) {
return (
<GenericFeaturesView
onUpgrade={onUpgrade}
Expand All @@ -134,7 +135,7 @@ function UpgradeIntro({feature, onUpgrade, buttonDisabled, loading, isCategorizi

const getOnlyAvailableOnPlanHTML = () => {
const planParams = {formattedPrice, hasTeam2025Pricing};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-2 (docs)

This modified condition compares feature.id against the hardcoded strings 'preventSelfApproval' and 'autoApproveCompliantReports', but these feature IDs are already defined as named constants in CONST.UPGRADE_FEATURE_INTRO_MAPPING (src/CONST/index.ts). This is inconsistent with the same PR, which correctly references CONST.UPGRADE_FEATURE_INTRO_MAPPING.autoPayApprovedReports.id on line 110. Using the raw strings risks silent breakage if the IDs ever change and reduces readability.

Reference the constants instead:

if (
    feature.id === CONST.UPGRADE_FEATURE_INTRO_MAPPING.preventSelfApproval.id ||
    feature.id === CONST.UPGRADE_FEATURE_INTRO_MAPPING.autoApproveCompliantReports.id
) {
    return translate('workspace.upgrade.approvals.onlyAvailableOnPlan', planParams);
}

Reviewed at: eb5e870 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MelvinBot wdyt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The PR already uses the CONST id for autoPayApprovedReports, so the other two ids should match. I switched the check to CONST.UPGRADE_FEATURE_INTRO_MAPPING.preventSelfApproval.id and CONST.UPGRADE_FEATURE_INTRO_MAPPING.autoApproveCompliantReports.id in 75b60a6. Nothing else changed.

Typecheck, lint, and tests/ui/WorkspaceUpgradeTest.tsx (16/16) pass.

if (feature.id === 'preventSelfApproval' || feature.id === 'autoApproveCompliantReports' || feature.id === 'autoPayApprovedReports') {
if (feature.id === CONST.UPGRADE_FEATURE_INTRO_MAPPING.preventSelfApproval.id || feature.id === CONST.UPGRADE_FEATURE_INTRO_MAPPING.autoApproveCompliantReports.id) {
return translate('workspace.upgrade.approvals.onlyAvailableOnPlan', planParams);
}
if (feature.id === CONST.UPGRADE_FEATURE_INTRO_MAPPING.rules.id) {
Expand Down
44 changes: 44 additions & 0 deletions tests/ui/WorkspaceUpgradeTest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,50 @@ describe('WorkspaceUpgrade', () => {
await waitForBatchedUpdatesWithAct();
});

it('should render the generic Control upgrade view instead of Advanced Approvals for auto-pay approved reports', async () => {
const policy: Policy = LHNTestUtils.getFakePolicy();

// Given that a policy is initialized in Onyx
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, policy);
});

// When the upgrade page is opened from the Auto-pay approved reports toggle, which requires Control but not advanced approvals
const {unmount} = renderPage(SCREENS.WORKSPACE.UPGRADE, {policyID: policy.id, featureName: CONST.UPGRADE_FEATURE_INTRO_MAPPING.autoPayApprovedReports.alias});
await waitForBatchedUpdatesWithAct();

// Then the generic Control title is shown
expect(await screen.findByText(TestHelper.translateLocal('workspace.upgrade.commonFeatures.title'))).toBeTruthy();

// And the Advanced Approvals copy is not shown
expect(screen.queryByText(TestHelper.translateLocal('workspace.upgrade.approvals.title'))).toBeNull();

unmount();
await waitForBatchedUpdatesWithAct();
});

it.each([CONST.UPGRADE_FEATURE_INTRO_MAPPING.preventSelfApproval.alias, CONST.UPGRADE_FEATURE_INTRO_MAPPING.autoApproveCompliantReports.alias])(
'should keep the Advanced Approvals upgrade view for %s',
async (featureName) => {
const policy: Policy = LHNTestUtils.getFakePolicy();

// Given that a policy is initialized in Onyx
await act(async () => {
await Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`, policy);
});

// When the upgrade page is opened from an approval toggle, which is an advanced approvals feature
const {unmount} = renderPage(SCREENS.WORKSPACE.UPGRADE, {policyID: policy.id, featureName});
await waitForBatchedUpdatesWithAct();

// Then the Advanced Approvals title is shown
expect(await screen.findByText(TestHelper.translateLocal('workspace.upgrade.approvals.title'))).toBeTruthy();

unmount();
await waitForBatchedUpdatesWithAct();
},
);

it.each([ROUTES.WORKSPACE_COMPANY_CARDS.getRoute('1'), ROUTES.WORKSPACE_COMPANY_CARDS_SELECT_FEED.getRoute('1')])(
'should resume the add-card flow nested under %s after acknowledging the company cards upgrade',
async (backTo) => {
Expand Down
Loading