Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
4.03k stars 3.03k forks source link

[$250] [not here] Company cards - After enabling company card, tapping new card directs to hmm not here page #56372

Open lanitochka17 opened 1 week ago

lanitochka17 commented 1 week ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 9.0.94-1 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Go to workspace settings - more features - enable company card
  3. Tap members
  4. Tap admin - new card

Expected Result:

After enabling company card, tapping new card must not direct to hmm not here page

Actual Result:

After enabling company card, tapping new card directs to hmm not here page

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/174b1f45-dee5-4394-ae93-1c694627ca8b

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021889092664985570923
  • Upwork Job ID: 1889092664985570923
  • Last Price Increase: 2025-02-10
Issue OwnerCurrent Issue Owner: @madmax330
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @mallenexpensify (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.

Shahidullah-Muffakir commented 1 week ago

Proposal

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

After enabling company card, tapping new card directs to hmm not here page.

What is the root cause of that problem?

  1. The New Card button is shown based on these conditions:
    shouldShowCardsSection
    https://github.com/Expensify/App/blob/386ea3e4abed12484ed7a2d9a2fe574d967a3891/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx#L336-L340
    The issue is that !!policy?.areExpensifyCardsEnabled is false.

  2. The IssueNewCardPage is only displayed if !!policy?.areExpensifyCardsEnabled is true:
    https://github.com/Expensify/App/blob/1e4ff8afb942c5df8a61ae74422ba8bf7c524932/src/pages/workspace/expensifyCard/issueNew/IssueNewCardPage.tsx#L75

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

  3. Add a condition to show the New Card button only if !!policy?.areExpensifyCardsEnabled is true, like:

{!!policy?.areExpensifyCardsEnabled && (
    <MenuItem
        title={translate('workspace.expensifyCard.newCard')}
        icon={Expensicons.Plus}
        onPress={handleIssueNewCard}
    />
)}

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

We can write test for the WorkspaceMemberDetailsPage if the !!policy?.areExpensifyCardsEnabled is false, then the New Card button should not be displayed

What alternative solutions did you explore? (Optional)

Alternative 1: If the !!policy?.areExpensifyCardsEnabled is false, we can disable the new card button, instead of hiding it.

Alternative 2: If it is allowed for a user to add a new card if the !!policy?.areCompanyCardsEnabled is true, then we can add this condition here : https://github.com/Expensify/App/blob/1e4ff8afb942c5df8a61ae74422ba8bf7c524932/src/pages/workspace/expensifyCard/issueNew/IssueNewCardPage.tsx#L75
like:

            featureName={CONST.POLICY.MORE_FEATURES.ARE_EXPENSIFY_CARDS_ENABLED ||CONST.POLICY.MORE_FEATURES.ARE_COMPANY_CARDS_ENABLED }
Krishna2323 commented 1 week ago

Proposal

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

Company cards - After enabling company card, tapping new card directs to hmm not here page

What is the root cause of that problem?

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

{(policy.areExpensifyCardsEnabled || hasMultipleFeeds) && (
    <MenuItem
        title={translate('workspace.expensifyCard.newCard')}
        icon={Expensicons.Plus}
        onPress={handleIssueNewCard}
    />
)}

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 6 days ago

@mallenexpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 6 days ago

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

melvin-bot[bot] commented 6 days ago

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

mallenexpensify commented 6 days ago

@DylanDylann , I'm hoping you can reproduce. I just tried but ran into an issue (when inviting a new users, I'm not seeing the invite in the other account so I can't accept then, from my main account, make that user an Admin).

Assuming you're able to reproduce, please review the proposals above. Thx

thelullabyy commented 4 days ago

Proposal

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

Company cards - After enabling company card, tapping new card directs to hmm not here page

What is the root cause of that problem?

The SectionCard include "New card" button is displayed when shouldShowCardsSection is true. One of the conditions is check areExpensifyCardsEnabled || areCompanyCardsEnabled so shouldShowCardsSection remain true even areExpensifyCardsEnabled is false. https://github.com/Expensify/App/blob/6000b2a26c3b9c165474c41cbbd660a0b9d94d66/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx#L212 Upon clicking the button, it navigates to either WORKSPACE_MEMBER_NEW_CARD or WORKSPACE_EXPENSIFY_CARD_ISSUE_NEW https://github.com/Expensify/App/blob/6000b2a26c3b9c165474c41cbbd660a0b9d94d66/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx#L336-L340 https://github.com/Expensify/App/blob/6000b2a26c3b9c165474c41cbbd660a0b9d94d66/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx#L168-L183 But the correct conditions to add new card is need to connect bank and enable feature. Thus with the WORKSPACE_MEMBER_NEW_CARD the condition is hasMultipleFeeds, with the WORKSPACE_EXPENSIFY_CARD_ISSUE_NEW the condition is paymentBankAccountID has value. https://github.com/Expensify/App/blob/6000b2a26c3b9c165474c41cbbd660a0b9d94d66/src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx#L83 https://github.com/Expensify/App/blob/5ddef34fc3c17fa420884efa3a24f574b57a9505/src/pages/workspace/expensifyCard/WorkspaceExpensifyCardPage.tsx#L27-L38

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

Replace areExpensifyCardsEnabled || areCompanyCardsEnabled with !!paymentBankAccountID || hasMultipleFeeds

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

N/A

What alternative solutions did you explore? (Optional)

DylanDylann commented 4 days ago

@Krishna2323 Why don't you move your update to shouldShowCardsSection condition? The condition to show the new card button and the condition to show the card list should be the same.

@Krishna2323 @Shahidullah-Muffakir with your solution, we still issue new cards without connecting bank accounts on Expensify Card

https://github.com/user-attachments/assets/b098480f-3497-41e3-b6d6-2295fd884376

DylanDylann commented 4 days ago

@thelullabyy Your proposal looks fine to me. Just an improvement, we can consider removing (hasWorkspaceCardsAssigned || !!cardFeeds) condition. Let's discuss it further in the PR phase

🎀 👀 🎀 C+ Reviewed

melvin-bot[bot] commented 4 days ago

Triggered auto assignment to @madmax330, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

mallenexpensify commented 2 days ago

@madmax330 👀 on the proposal above plz