Open vincdargento opened 3 weeks ago
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.
π¨ Edited by proposal-police: This proposal was edited at 2025-01-24 15:28:51 UTC.
Reports - Card filter page is empty when physical card is not activated
We are allowing the card filter if there are any cards here
https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/pages/Search/AdvancedSearchFilters.tsx#L416
but inside card filter page we filter out cards that are not issued and activated via isCardHiddenFromSearch
here
https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/pages/Search/SearchAdvancedFiltersPage/SearchFiltersCardPage.tsx#L168
We need to apply the isCardHiddenFromSearch filter to cards list to ensure that there are cards that are not hidden from search here https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/pages/Search/AdvancedSearchFilters.tsx#L416
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.
N/A
Job added to Upwork: https://www.upwork.com/jobs/~021883935258401101006
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ahmedGaber93 (External
)
@FitseTLT's proposal using filter by isCardHiddenFromSearch
LGTM!
π π π C+ reviewed
Triggered auto assignment to @francoisl, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
π£ @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 π
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.94-25 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2025-02-13. :confetti_ball:
For reference, here are some details about the assignees on this issue:
@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]
@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!
[!note] @garrettmknight The PR for this issue caused this deploy blocker / regression because this logic was faulty and the changes were not tested thoroughly.
Hmm! I don't think https://github.com/Expensify/App/issues/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 https://github.com/Expensify/App/pull/56600 should be existed before our PR
our PR is just excluding items that hidden from search without touching the items that cause that issue https://github.com/Expensify/App/issues/56519
Also, I am able to reproduce it after manually revert that change.
@Kicu Do you mind confirming whether changes from PR https://github.com/Expensify/App/pull/55900 caused the deploy blocker you just addressed with PR https://github.com/Expensify/App/pull/56600 ?
[x] [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
[x] [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.
[x] [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.
@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.
Thanks for confirming! π
@garrettmknight Please take βοΈ this into account before issuing payments on this issue, this being the confirmation I asked for in https://github.com/Expensify/App/issues/55719#issuecomment-2652152934 to my previous statement made in https://github.com/Expensify/App/issues/55719#issuecomment-2651926081.
β»οΈ My reasoning for why the PR from this issue caused a regression is:
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 π€
@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
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 https://github.com/Expensify/App/issues/56519? I think yes. Merging by spread operator will merge all values, and didn't check for anything.
@garrettmknight What happened is:
Thanks!
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.
Β I hope someone can verify if this bug was reproducible before.
I am able to reproduce it after manually reverting that change.
@ahmedGaber93 That's irrelevant, here's what happens:
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:
feedCards[cardKey] = card
line β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 changesThis 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 π
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.
https://github.com/Expensify/App/blob/fc199faf777dc7f1710b3d5cb4ab7abf6106309c/src/libs/CardUtils.ts#L102-L103
Let's put an end to this discussion and let the BZ or the internal developer decide.
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 https://github.com/Expensify/App/issues/55719#issuecomment-2657833002.
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.
β» 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 https://github.com/Expensify/App/issues/56519
https://github.com/user-attachments/assets/f9b57e15-163e-4d56-ace8-d2783b731206
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.
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:
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:
Screenshots/Videos
https://github.com/user-attachments/assets/37adce39-2645-446d-b639-7111b83de981
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @garrettmknight