[AWAITING BZ REVIEW][Due for payment 2025-02-13] [$250] Reports - Card filter page is empty when physical card is not activated

[vincdargento]

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.89-2
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): applausetester+100106kh@applause.expensifail.com
Issue reported by: Applause Internal Team
Device used: Mac 15.0 / Chrome
App Component: Search

Action Performed:

Precondition:

• Workspace has enabled Expensify Card feature and set up a bank account.
• Workspace has not assigned any card.

1. Go to staging.new.expensify.com
2. Go to workspace settings > Expensify Card.
3. Click Issue card.
4. Issue a physical card to yourself.
5. Do not activate the card.
6. Go to Reports.
7. Click Filters.
8. Click Card.

Expected Result:

In Step 7, Card field should not be present in Filters when the only physical card is not activated.

Actual Result:

In Step 7, Card field is present in Filters when the only physical card is not activated, which opens a blank page.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021883935258401101006
• Upwork Job ID: 1883935258401101006
• Last Price Increase: 2025-01-27
• Automatic offers:
• FitseTLT | Contributor | 105895388

Issue Owner

Current Issue Owner: @garrettmknight

[melvin-bot]

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

[FitseTLT]

🚨 Edited by proposal-police: This proposal was edited at 2025-01-24 15:28:51 UTC.

Proposal

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

Reports - Card filter page is empty when physical card is not activated

What is the root cause of that problem?

We are allowing the card filter if there are any cards here

App/src/pages/Search/AdvancedSearchFilters.tsx

Line 416 in fc199fa

const shouldDisplayCardFilter = shouldDisplayFilter(Object.keys(allCards).length, areCardsEnabled);

but inside card filter page we filter out cards that are not issued and activated via isCardHiddenFromSearch here

App/src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx

Line 168 in fc199fa

if (!representativeCard || !cardFeedArray.some((cardFeedItem) => isCard(cardFeedItem) && !isCardHiddenFromSearch(cardFeedItem))) {

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

We need to apply the isCardHiddenFromSearch filter to cards list to ensure that there are cards that are not hidden from search here

App/src/pages/Search/AdvancedSearchFilters.tsx

Line 416 in fc199fa

const shouldDisplayCardFilter = shouldDisplayFilter(Object.keys(allCards).length, areCardsEnabled);

    const shouldDisplayCardFilter = shouldDisplayFilter(Object.values(allCards).filter((card) => isCard(card) && !isCardHiddenFromSearch(card)).length, areCardsEnabled);

Optionally we can apply only isCardHiddenFromSearch filter or apply isCard filter only for workspace cards same as we do in SearchFiltersCardPage
Additionally we can apply the filtering for allCards here because we use it to get the title here because in SearchFiltersCardPage we filter out cards that are hidden for search though they might be part of the filter.

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)

[melvin-bot]

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

[melvin-bot]

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

[ahmedGaber93]

@FitseTLT's proposal using filter by isCardHiddenFromSearch LGTM!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

📣 @FitseTLT 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

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

• Fix - Reports - Card filter page is empty when physical card is not activated #55900

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

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

• @FitseTLT requires payment automatic offer (Contributor)
• @ahmedGaber93 requires payment through NewDot Manual Requests

[melvin-bot]

@ahmedGaber93 @garrettmknight @ahmedGaber93 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]

[melvin-bot]

@garrettmknight @francoisl @FitseTLT @ahmedGaber93 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[ikevin127]

Note

@garrettmknight The PR for this issue caused this deploy blocker / regression because this logic was faulty and the changes were not tested thoroughly.

[ahmedGaber93]

Hmm! I don't think #56519 is a regression from our PR.

Before our PR, the behavior was merging all cardList by spread operator which will use the object keys, so the root cause that explained in OP here #56600 should be existed before our PR

App/src/libs/CardUtils.ts

Line 103 in fc199fa

const feedCards: CardList = {...cardList};

our PR is just excluding items that hidden from search without touching the items that cause that issue #56519

Also, I am able to reproduce it after manually revert that change.

[ikevin127]

@Kicu Do you mind confirming whether changes from PR #55900 caused the deploy blocker you just addressed with PR #56600 ?

[ahmedGaber93]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other: Applause Internal Team

• [Contributor] 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: https://github.com/Expensify/App/pull/55281/files#r1953461029

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: N/A.

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

I think this a straight forward fix and no need for regression test here.

[Kicu]

@ikevin127 most likely yes, but a few days have passed and its not easy to re-reproduce this error now 😅
I think the crucial thing was the !isCard(card) check missing, because in theory we could've assigned an empty or null object to feedCards which I think was wrong.

[ikevin127]

Thanks for confirming! 👍

@garrettmknight Please take ☝️ this into account before issuing payments on this issue, this being the confirmation I asked for in #55719 (comment) to my previous statement made in #55719 (comment).

♻️ My reasoning for why the PR from this issue caused a regression is:

• the author added a block of code which was missing a crucial check (confirmed above)
• PR was merged seemingly fixing this issue, but because of the missing logic -> deploy blocker issue popped up (somehoew wasn't linked to this PR)
• Kiku rewrites the code block added by the PR from this issue in a follow-up PR, fixing the deploy blocker
• while completing the BZ Checklist on the deploy blocker issue, I discovered this issue and the problem code added by this PR

I don't understand how the author / reviewer can argue that the logic added by this issue's PR was flawless and that they should get full compensation despite the regression which clearly came from this issue's PR 🤔

[ahmedGaber93]

@Kicu @ikevin127 Thanks for discussing that, But I don't agree with you (the issue was found before our PR).

I think the crucial thing was the !isCard(card) check missing

App/src/libs/CardUtils.ts

Line 103 in fc199fa

const feedCards: CardList = {...cardList};

This is the code ^ before our PR, merging all cardList by spread operator
@Kicu Was !isCard(card) checked before our PR? Can you please confirm whether the logic before our PR ({...cardList}) have the same issue and should reproduce #56519? I think yes. Merging by spread operator will merge all values, and didn't check for anything.

---

@garrettmknight What happened is:

• before our PR the code have 2 issues
• our PR fix one issue that reported here (without touching the items that cause the other issue)
• and they fix the other issue that found before our PR

Thanks!

[Kicu]

hey, sorry I'm currently busy with 2 other projects within Expensify and I'm not able to come back to this code.
It is possible that I was not 100% correct in my previous message, I hope someone can verify if this bug was reproducible before.

[ahmedGaber93]

 I hope someone can verify if this bug was reproducible before.

I am able to reproduce it after manually reverting that change.

[ikevin127]

@ahmedGaber93 That's irrelevant, here's what happens:

• this is how the function looked before this issue's PR:

function mergeCardListWithWorkspaceFeeds(workspaceFeeds: Record<string, WorkspaceCardsList | undefined>, cardList = allCards) {
    const feedCards: CardList = {...cardList};
    Object.values(workspaceFeeds ?? {}).forEach((currentCardFeed) => {
        Object.values(currentCardFeed ?? {}).forEach((card) => {
            if (!isCard(card)) {
                return;
            }
            feedCards[card.cardID] = card;
        });
    });
    return feedCards;
}

• this is how the issue looked after this issue's PR:

function mergeCardListWithWorkspaceFeeds(workspaceFeeds: Record<string, WorkspaceCardsList | undefined>, cardList = allCards, shouldExcludeCardHiddenFromSearch = false) {
    const feedCards: CardList = {};
    Object.keys(cardList).forEach((cardKey) => {
        const card = cardList[cardKey];
        if (shouldExcludeCardHiddenFromSearch && isCardHiddenFromSearch(card)) {
            return;
        }
        feedCards[cardKey] = card;
    });

    Object.values(workspaceFeeds ?? {}).forEach((currentCardFeed) => {
        Object.values(currentCardFeed ?? {}).forEach((card) => {
            if (!isCard(card) || (shouldExcludeCardHiddenFromSearch && isCardHiddenFromSearch(card))) {
                return;
            }
            feedCards[card.cardID] = card;
        });
    });
    return feedCards;
}

Let's go with your logic:

• the crash was still happening before this issue's PR introduced the new code block (Object.keys...) ✅
• the PR introduced the new code block (Object.keys...) and now the crash is still happening, but sooner in the function -> in the code block the PR introduced (Object.keys...), specifically on the feedCards[cardKey] = card line ❌
• additionally, you can notice that the old logic was only handling the workspaceFeeds object, then using the spread cardList inside the loop, while after this PR changes, the cardList was mapped first at the top of the function (instead of old spread logic) -> which is what causes the crash earlier, after this PRs changes

This is what I mean when I say that it's irrelevant whether the older logic caused the crash as well, because the logic this PR added is causing the crash sooner in the function because of the missing !isCard(card) check from the new block added by this PR.

I am able to reproduce it after manually reverting that change.

Please let me know how you were able to reproduce the issue, exact steps / video proof would be good 👍

[ahmedGaber93]

the old logic was only handling the workspaceFeeds object, then using the spread cardList inside the loop, while after this PR changes, the cardList was mapped first at the top of the function (instead of old spread logic) -> which is what causes the crash earlier, after this PRs changes

This is not correct, the both logic handle cardList first.

App/src/libs/CardUtils.ts

Lines 102 to 103 in fc199fa

	function mergeCardListWithWorkspaceFeeds(workspaceFeeds: Record<string, WorkspaceCardsList | undefined>, cardList = allCards) {
	const feedCards: CardList = {...cardList};

[ahmedGaber93]

Let's put an end to this discussion and let the BZ or the internal developer decide.

[ikevin127]

This is not correct, the both logic handle cardList first.

That doesn't make sense, the old logic that you showed in permalink here was spreading the feedCards, while the logic added in the PR for this issue removed the spreading:

const feedCards: CardList = {}; 

and added the new Object.keys()... logic to populate the feedCards object as I've shown in the second code block in #55719 (comment).

I am able to reproduce it after manually reverting that change.

♻ Once again, please let me know how you were able to reproduce the issue, exact steps / video proof would be good 👍

Let's put an end to this discussion and let the BZ or the internal developer decide.

No problem with that, just wanted to make sure that BZ is informed about the regression source before issuing payments.

[ahmedGaber93]

♻ Once again, please let me know how you were able to reproduce the issue, exact steps / video proof would be good 👍

Using the same steps here #56519

20250211222304238.mp4

[garrettmknight]

Thanks for all the context here - I'm going to pause payment for the moment and dig into this Monday. Appreciate everyone keeping it cordial in the meantime.

[garrettmknight]

@ikevin127 I appreciate your thorough review on this one and I can see how halving payment could be a reasonable outcome to recomment. I'm going to leave payment full here for a few reasons:

• It looks like this PR was on prod when the blocker was found and QA couldn't repro on prod so this doesn't seem like the only thing causing that issue, even if it contributed.
• If the bug existed before and was replicated (and maybe even sped up), it'd get fixed separately anyway.

[garrettmknight]

Payment Summary:

• Contributor: @FitseTLT $250 via Upwork
• Contributor+: @ahmedGaber93 $250 via NewDot