Fix/74127 Invoicify related bugs #79950
Conversation
melvin-bot
bot
requested review from
JmillsExpensify and
rojiphil
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return; | ||
| } | ||
|
|
||
| if (isSubscriptionTypeOfInvoicing(privateSubscription?.type) && item?.policyID) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (https://github.com/Expensify/App/blob/main/.claude/agents/code-inline-reviewer.md#consistency-3-eliminate-code-duplication)
The logic for checking invoicing subscription type and validating other control workspaces is duplicated between WorkspacesListPage.tsx and WorkspaceOverviewPage.tsx. This same logic appears in both files with nearly identical implementation.
Extract this logic into a reusable utility function to eliminate the duplication and improve maintainability:
// In a shared utility file (e.g., src/libs/PolicyUtils.ts)
function shouldBlockWorkspaceDeletion(
privateSubscriptionType: string | undefined,
policies: OnyxCollection<Policy>,
currentUserAccountID: number,
currentPolicyID: string
): boolean {
if (\!isSubscriptionTypeOfInvoicing(privateSubscriptionType)) {
return false;
}
const ownerPolicies = ownerPoliciesSelector(policies, currentUserAccountID);
const ownerPoliciesWithoutPending = (ownerPolicies ?? []).filter(
(policy) =>
policy.pendingAction \!== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD &&
policy.pendingAction \!== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
);
return \!hasOtherControlWorkspaces(ownerPoliciesWithoutPending, currentPolicyID);
}
// Usage in WorkspacesListPage.tsx
if (shouldBlockWorkspaceDeletion(privateSubscription?.type, policies, currentUserPersonalDetails?.accountID, item.policyID)) {
Navigation.navigate(ROUTES.SETTINGS_SUBSCRIPTION_DOWNGRADE_BLOCKED.getRoute(Navigation.getActiveRoute()));
return;
}There was a problem hiding this comment.
I thought about this while working on PR, but I'm not sure it makes much sense, since we get ownerPolicies in two different places: in one, we get them through Onyx using a selector, and in the other, we just use filtering.
I think it would be possible to create some kind of function here, but I'm not sure if it would make much sense, because with this approach, we would be filtering the array for all users: see this comment: #79950 (comment)
Let me know what do you think @rojiphil
There was a problem hiding this comment.
This makes sense to me though as a utility function will help the code remain DRY. I think we can pass the filtered owner policies to the utility function. If available, we can use it straightaway. And if not available, we can filter it in the utility function itself and use it. Would this work?
| }, [isFocused, isPendingDelete, prevIsPendingDelete, policyLastErrorMessage]); | ||
|
|
||
| const onDeleteWorkspace = useCallback(() => { | ||
| if (isSubscriptionTypeOfInvoicing(privateSubscription?.type) && policy?.id) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (https://github.com/Expensify/App/blob/main/.claude/agents/code-inline-reviewer.md#consistency-3-eliminate-code-duplication)
The logic for checking invoicing subscription type and validating other control workspaces is duplicated between WorkspaceOverviewPage.tsx and WorkspacesListPage.tsx. This same logic appears in both files with nearly identical implementation.
Extract this logic into a reusable utility function to eliminate the duplication and improve maintainability. See the comment on WorkspacesListPage.tsx for the suggested implementation.
| const [session] = useOnyx(ONYXKEYS.SESSION, {canBeMissing: true}); | ||
| const personalDetails = usePersonalDetails(); | ||
| const [isCannotLeaveWorkspaceModalOpen, setIsCannotLeaveWorkspaceModalOpen] = useState(false); | ||
| const privateSubscription = usePrivateSubscription(); |
There was a problem hiding this comment.
❌ PERF-6 (https://github.com/Expensify/App/blob/main/.claude/agents/code-inline-reviewer.md#perf-6-derive-state-from-props)
The selector function is being created with useCallback and then passed to useOnyx, but this is unnecessary complexity. The selector can be defined directly inline or moved outside the component since it only depends on currentUserPersonalDetails?.accountID.
If currentUserPersonalDetails?.accountID changes, the selector will naturally re-run with useOnyx. There's no need for useCallback here as it doesn't prevent re-renders or improve performance in this case.
Suggested fix:
// Option 1: Define selector inline
const [ownerPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {
canBeMissing: true,
selector: (policies: OnyxCollection<Policy>) =>
ownerPoliciesSelector(policies, currentUserPersonalDetails?.accountID)
});
// Option 2: If accountID is stable, move outside useCallback entirely
const accountID = currentUserPersonalDetails?.accountID;
const [ownerPolicies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {
canBeMissing: true,
selector: (policies: OnyxCollection<Policy>) => ownerPoliciesSelector(policies, accountID)
});There was a problem hiding this comment.
I used the same approach as here:
App/src/pages/workspace/downgrade/DowngradeConfirmation.tsx
Lines 24 to 30 in 042f966
Let me know if anything needs to be changed @rojiphil.
There was a problem hiding this comment.
As we do not see any compelling reason to keep useCallback, let us add the selector inline and avoid unnecessary complexity.
There was a problem hiding this comment.
It seems that eslint prohibits inline selectors, so I'll leave it as it is for now.
| } | ||
|
|
||
| if (isSubscriptionTypeOfInvoicing(privateSubscription?.type) && item?.policyID) { | ||
| const ownerPolicies = (() => { |
There was a problem hiding this comment.
❌ PERF-6 (https://github.com/Expensify/App/blob/main/.claude/agents/code-inline-reviewer.md#perf-6-derive-state-from-props)
The ownerPolicies value is computed inside an event handler using an IIFE (Immediately Invoked Function Expression). This logic should be moved outside the event handler and computed earlier in the component.
Reasoning: The ownerPolicies computation depends only on policies and currentUserPersonalDetails?.accountID, which are available in the component scope. Computing this in the event handler adds unnecessary complexity and makes the code harder to read.
Suggested fix:
// At the component level (before the useMemo that creates workspacesList)
const ownerPolicies = useMemo(() => {
if (\!policies || \!currentUserPersonalDetails?.accountID) {
return undefined;
}
return ownerPoliciesSelector(policies, currentUserPersonalDetails?.accountID);
}, [policies, currentUserPersonalDetails?.accountID]);
// Then in the event handler, simply use:
if (isSubscriptionTypeOfInvoicing(privateSubscription?.type) && item?.policyID) {
const ownerPoliciesWithoutCreateOrDeletePendingAction = (ownerPolicies ?? []).filter(
(policy) => policy.pendingAction \!== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD &&
policy.pendingAction \!== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
);
const hasOtherControlWorkspaces = hasOtherControlWorkspacesPolicyUtils(
ownerPoliciesWithoutCreateOrDeletePendingAction,
item.policyID
);
if (\!hasOtherControlWorkspaces) {
Navigation.navigate(ROUTES.SETTINGS_SUBSCRIPTION_DOWNGRADE_BLOCKED.getRoute(Navigation.getActiveRoute()));
return;
}
}This makes the derived state clearer and avoids recomputing it on every event handler execution.
There was a problem hiding this comment.
In my opinion, this seems inappropriate, since I deliberately put the logic inside the function. The reason for this behavior is that the vast majority of users are not Invoicify users, so if I move this logic outside the function, it will be executed every time for all users, even if they don't need this change in 99% of cases. Therefore, this is done for optimization purposes, so as not to perform operations on potentially large data when it is not necessary. With the current implementation, this will only be performed when the subscription type is Invoicify.
There was a problem hiding this comment.
Looks fine to me but let's add a comment as to why we are deliberately putting the logic inside the function.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80726e5f11
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| currentUserAccountIDParam: currentUserPersonalDetails.accountID, | ||
| currentUserEmailParam: currentUserPersonalDetails.email ?? '', | ||
| shouldCreateControlPolicy: isSubscriptionTypeOfInvoicing(privateSubscription?.type), |
There was a problem hiding this comment.
usePrivateSubscription returns undefined while Onyx is still loading, and isSubscriptionTypeOfInvoicing(undefined) is false. That means an Invoicify user who opens this page before NVP_PRIVATE_SUBSCRIPTION is ready (e.g., fresh login or cache miss/offline) will still create a TEAM/Collect workspace, recreating the optimistic-type bug you’re trying to fix. Consider blocking submission until the subscription is loaded or handling the “loading/unknown” state explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think this applies in this case, because this function is only executed when the user clicks the button, not when the page loads, so this data will already be available at the moment of clicking.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Product review not required.
|
Oh! Unfortunately, during testing, I just managed to delete the last control workspace in OD. I just had to assign the 79950-invoicify-001.mp4 |
|
@heyjennahay Also, the BE has reset my subscription status to |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@Eskalifer1 Thanks for the PR.
I have left a few comments. Also, we have conflicts.
Please have a look. Thanks.
| const [session] = useOnyx(ONYXKEYS.SESSION, {canBeMissing: true}); | ||
| const personalDetails = usePersonalDetails(); | ||
| const [isCannotLeaveWorkspaceModalOpen, setIsCannotLeaveWorkspaceModalOpen] = useState(false); | ||
| const privateSubscription = usePrivateSubscription(); |
There was a problem hiding this comment.
As we do not see any compelling reason to keep useCallback, let us add the selector inline and avoid unnecessary complexity.
| } | ||
|
|
||
| if (isSubscriptionTypeOfInvoicing(privateSubscription?.type) && item?.policyID) { | ||
| const ownerPolicies = (() => { |
There was a problem hiding this comment.
Looks fine to me but let's add a comment as to why we are deliberately putting the logic inside the function.
| return; | ||
| } | ||
|
|
||
| if (isSubscriptionTypeOfInvoicing(privateSubscription?.type) && item?.policyID) { |
There was a problem hiding this comment.
This makes sense to me though as a utility function will help the code remain DRY. I think we can pass the filtered owner policies to the utility function. If available, we can use it straightaway. And if not available, we can filter it in the utility function itself and use it. Would this work?
| activePolicyID, | ||
| currentUserAccountIDParam: currentUserPersonalDetails.accountID, | ||
| currentUserEmailParam: currentUserPersonalDetails.email ?? '', | ||
| shouldCreateControlPolicy: isSubscriptionTypeOfInvoicing(privateSubscription?.type), |
There was a problem hiding this comment.
Don't we need to set shouldCreateControlPolicy here too?
Line 590 in 87e6d66
There was a problem hiding this comment.
I'm not even entirely sure how to test this functionality, as it seems that this logic is rarely used, and I think that Invoicy users will almost never use it, especially in offline mode, and in the online backend will return the correct data.
In addition, it seems that this useEffect is called once when the page is mounted after the transition, at a time when privateSettings will still be undefined. I'm afraid that if we add any dependencies to this useEffect, we will cause many more bugs than we will gain benefits, because the chance of such a scenario occurring is very small and only applies to offline mode, which, in my opinion, does not justify the risks.
What do you think about this?
There was a problem hiding this comment.
Yeah. Digged in a little deeper to figure out the usage and it’s here.
I too don’t see this being applicable in our invoicify scenario here. Let us leave this as it is.
|
Tests well too. Will complete the checklist once review comments are resolved. 79950-test-001.mp4 |
|
Hi! I will update PR today later! |
Eskalifer1
changed the title
|
Changes looks good now.. Will complete the review today. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp79950-android-hyrbid-001.mp4Android: mWeb Chrome79950-mweb-chrome-001.mp4iOS: HybridApp79950-ios-hybrid-001.mp4iOS: mWeb Safari79950-mweb-safari-001.mp4MacOS: Chrome / Safari79950-web-chrome-001.mp4 |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@Eskalifer1 Thanks for the updates to PR.
@justinpersaud Changes LGTM. Works well too.
Over to you. Thanks.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/justinpersaud in version: 9.3.11-0 🚀
|
|
@Eskalifer1 how to be an Invoicify user, or is this something to be tested internally? |
|
Hi @nlemma this should be tested internally |
7 participants
Explanation of Change
While working on the issue #74127, three more bugs related to Invoicify users were found:
See this comment for last 2 bugs: #74127 (comment)
Fixed Issues
$#74127
PROPOSAL:
Tests
Precondition for every bug: Be Invoicify user
Bug 1: Last workspace deletion
Precondition: Have one control workspace where you are the owner
Bug 2: Optimistic workspace creation
Bug 3: Workspac downgrade
Precondition: Have at least 1 control workspace where you are owner
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
74127-follow-up-android-native.mp4
Android: mWeb Chrome
74127-follow-up-android-web.mp4
iOS: Native
74127-follow-up-ios-native.mp4
iOS: mWeb Safari
74127-follow-up-ios-web.mp4
MacOS: Chrome / Safari
74127-follow-up-web.mp4