Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Due for payment 2025-03-17] [$400] Cards - In company cards settings page, on refresh entered details are not shown #56746

Open
2 of 8 tasks
IuliiaHerets opened this issue Feb 12, 2025 · 41 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Feb 12, 2025

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: V9.0.97-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both
Issue reported by: Applause Internal Team
Device used: Redmi note 10s android 13
App Component: Workspace Settings

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Go to workspace settings - enable company cards
  3. Tap company cards
  4. Add a American express card selecting American express corporate cards
  5. Tap settings
  6. Change the card feed name and save
  7. Toggle on "allow deleting transactions"
  8. Refresh the page
  9. Now card feed name is empty
  10. Tap on card feed name
  11. Note in preview saved feed name is shown
  12. Note "allow deleting transactions" is toggle off
  13. Navigate back to company cards page
  14. Tap settings
  15. Now note feed name is displayed and "allow deleting transactions" toggle is on.

Expected Result:

In company cards settings page, on refresh entered details are not shown similarly it must not be displayed in preview and while revisiting the page.

Actual Result:

In company cards settings page, on refresh entered details are not shown but displayed in preview and while revisiting the 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

Bug6740346_1739345791710.Screenrecorder-2025-02-12-12-55-05-817.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021890532491432510986
  • Upwork Job ID: 1890532491432510986
  • Last Price Increase: 2025-02-24
  • Automatic offers:
    • brunovjk | Reviewer | 106260425
    • nkdengineer | Contributor | 106260426
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @mallenexpensify
@IuliiaHerets IuliiaHerets added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Feb 12, 2025
Copy link

melvin-bot bot commented Feb 12, 2025

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.

@IuliiaHerets IuliiaHerets changed the title mWeb - Cards - In company cards settings page, on refresh entered details are not shown Cards - In company cards settings page, on refresh entered details are not shown Feb 12, 2025
@twilight2294
Copy link
Contributor

twilight2294 commented Feb 12, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-12 10:12:09 UTC.

Proposal

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

In company cards settings page, on refresh entered details are not shown

What is the root cause of that problem?

We do not have cardFeeds as a dependency while extracting selectedFeed, intially selectedFeed is undefined for sometime and it gets the onyx value after sometime, but the useMemo in selectedFeed only runs once:

const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

Which causes the selectedFeed to the undefined and we do not get the values

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

We need to add cardFeeds as a dependency to the useMemo this will make sure that we get the values for the feed whenever the values for SHARED_NVP_PRIVATE_DOMAIN_MEMBER are fetched.

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

This is just a dependency addition and i don't think that we need a test for this, the values are not fetched on the update in onyx, so i think we can skip test for this issue

What alternative solutions did you explore? (Optional)

Copy link
Contributor

⚠️ @twilight2294 Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

@bernhardoj
Copy link
Contributor

Proposal

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

Company card settings page shows empty name after refresh.

What is the root cause of that problem?

The card data is intentionally memoized once only.

const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

However, after refresh, the onyx data is still loading, so the data is undefined.

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

Add loading state to the memo deps.

const [lastSelectedFeed, lastSelectedFeedResult] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
const isLoading = isLoadingOnyxValue(lastSelectedFeedResult);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), [isLoading]);

We can show loading indicator when it's loading if needed.

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

N/A

@melvin-bot melvin-bot bot added the Overdue label Feb 14, 2025
@mallenexpensify mallenexpensify added the External Added to denote the issue can be worked on by a contributor label Feb 14, 2025
@melvin-bot melvin-bot bot changed the title Cards - In company cards settings page, on refresh entered details are not shown [$250] Cards - In company cards settings page, on refresh entered details are not shown Feb 14, 2025
Copy link

melvin-bot bot commented Feb 14, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 14, 2025
Copy link

melvin-bot bot commented Feb 14, 2025

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

@melvin-bot melvin-bot bot removed the Overdue label Feb 14, 2025
@mallenexpensify
Copy link
Contributor

@brunovjk 👀 plz on the proposals above. Thx

@brunovjk
Copy link
Contributor

Sure! Im on it :D

@nkdengineer
Copy link
Contributor

nkdengineer commented Feb 15, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-18 02:31:42 UTC.

Proposal

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

In company cards settings page, on refresh entered details are not shown but displayed in preview and while revisiting the page.

What is the root cause of that problem?

After we refresh the page, both cardFeeds and lastSelectedFeed are still undefined while the onboarding data is loading. Then selectedFeed is undefined because this useMemo has no dependency. The reason we add this useMemo is here

const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

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

Add the loading status of cardFeeds and lastSelectedFeed data as dependency of this useMemo

const [cardFeeds, cardFeedsMetaData] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed, lastSelectedFeedMetaData] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
const isLoadingLastSelectedFeedData = isLoadingOnyxValue(lastSelectedFeedMetaData);
const isLoadingCardFeedsData = isLoadingOnyxValue(cardFeedsMetaData);

// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), [isLoadingLastSelectedFeedData, isLoadingCardFeedsData]);

const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

The reason we need to add both of them is when we reset cache or login again, lastSelectedFeed will be undefined and then we need to get the defaultFeed from cardFeeds data. So if we don't add isLoadingCardFeedsData as dependency, in this case, the selectedFeed is also undefined.

const defaultFeed = Object.keys(getCompanyFeeds(cardFeeds, true)).at(0) as CompanyCardFeed | undefined;

OPTIONAL: We can show the loading page while the feed data is still loading

To fix the deeplink case, I think we need to use useEffect here instead of useFocusEffect

useFocusEffect(fetchCompanyCards);

and also add cardFeeds.isLoading as a dependency here

const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), [isLoadingLastSelectedFeedData, isLoadingCardFeedsData, cardFeeds?.isLoading]);

const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

We can add only the isLoadingLastSelectedFeedData as the dependency but we need to update the last selected feed when we navigate to the company card setting page here.

onPress={() => {
    updateSelectedFeed(selectedFeed, policyID);
    Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_SETTINGS.getRoute(policyID));
}}

onPress={() => Navigation.navigate(ROUTES.WORKSPACE_COMPANY_CARDS_SETTINGS.getRoute(policyID))}

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

None

What alternative solutions did you explore? (Optional)

We can add cardFeeds and lastSelectedFeed as dependencies here, which will also fix the deep link case

const [cardFeeds] = useOnyx(`${ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER}${workspaceAccountID}`);
const [lastSelectedFeed] = useOnyx(`${ONYXKEYS.COLLECTION.LAST_SELECTED_FEED}${policyID}`);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps -- we want to run the hook only once to escape unexpected feed change
const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

And to fix the minor issue here, we can delay the API call instead of delaying the navigation logic because the problem of the minor issue is lastSelectedFeed is changed while we dismiss this page

const deleteCompanyCardFeed = () => {
        setDeleteCompanyCardConfirmModalVisible(false);
      Navigation.goBack();
      if (selectedFeed) {
          const {cardList, ...cards} = cardsList ?? {};
          const cardIDs = Object.keys(cards);
          const feedToOpen = (Object.keys(companyFeeds) as CompanyCardFeed[])
              .filter((feed) => feed !== selectedFeed && companyFeeds[feed]?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
              .at(0);
          InteractionManager.runAfterInteractions(() => {
              deleteWorkspaceCompanyCardFeed(policyID, workspaceAccountID, selectedFeed, cardIDs, feedToOpen);
          });
      }
  };

const deleteCompanyCardFeed = () => {
if (selectedFeed) {
const {cardList, ...cards} = cardsList ?? {};
const cardIDs = Object.keys(cards);
const feedToOpen = (Object.keys(companyFeeds) as CompanyCardFeed[])
.filter((feed) => feed !== selectedFeed && companyFeeds[feed]?.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
.at(0);
deleteWorkspaceCompanyCardFeed(policyID, workspaceAccountID, selectedFeed, cardIDs, feedToOpen);
}
setDeleteCompanyCardConfirmModalVisible(false);
Navigation.setNavigationActionToMicrotaskQueue(Navigation.goBack);

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@huult
Copy link
Contributor

huult commented Feb 15, 2025

Proposal

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

In company cards settings page, on refresh entered details are not shown

What is the root cause of that problem?

cardFeeds and lastSelectedFeed are retrieved from useOnyx, but they are initially undefined until the data is fully loaded.
The useMemo hook runs only once due to an empty dependency array ([]), so it does not update when cardFeeds and lastSelectedFeed change.
This causes selectedFeed to remain undefined, leading to unexpected behavior.

const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

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

To resolve this issue, we just remove useMemo

const selectedFeed = useMemo(() => getSelectedFeed(lastSelectedFeed, cardFeeds), []);

 const selectedFeed = getSelectedFeed(lastSelectedFeed, cardFeeds);

Ran the pipeline to test ESLint and saw that it passed, so we don’t add useMemo here."

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

None

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@brunovjk
Copy link
Contributor

Reviewing proposals.

@mallenexpensify mallenexpensify added Internal Requires API changes or must be handled by Expensify staff External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Internal Requires API changes or must be handled by Expensify staff labels Feb 17, 2025
Copy link

melvin-bot bot commented Feb 17, 2025

Current assignee @brunovjk is eligible for the External assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Feb 21, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@brunovjk
Copy link
Contributor

brunovjk commented Feb 21, 2025

Thanks @nkdengineer, I believe we can go with your proposal then, what do you think @danieldoglas? thanks a lot.

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 21, 2025

Current assignee @danieldoglas is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added the Overdue label Feb 24, 2025
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 24, 2025
Copy link

melvin-bot bot commented Feb 24, 2025

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

Offer link
Upwork job

Copy link

melvin-bot bot commented Feb 24, 2025

📣 @nkdengineer 🎉 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 melvin-bot bot removed the Overdue label Feb 24, 2025
@danieldoglas
Copy link
Contributor

Assigned nkdengineer. Adjusting the issue price.

@danieldoglas danieldoglas changed the title [$250] Cards - In company cards settings page, on refresh entered details are not shown [$400] Cards - In company cards settings page, on refresh entered details are not shown Feb 24, 2025
Copy link

melvin-bot bot commented Feb 24, 2025

Upwork job price has been updated to $400

@nkdengineer
Copy link
Contributor

@VickyStash We're using useFocusEffect here so if we reset cache and open the company card setting page by deeplink, the API isn't called and we don't have card feed data. Can we use useEffect instead? Also with useEffect, it also fixes the bug the loading shows every time we close the RHP.

useFocusEffect(fetchCompanyCards);

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 26, 2025
@VickyStash
Copy link
Contributor

@nkdengineer I see that there were some updates lately with removing useFocusEffect in favour of useEffect in workspace related pages, maybe it's worth to take a look.
Just make sure that everything works/loads as expected.

@brunovjk
Copy link
Contributor

You let me know when it's ready for review @nkdengineer? Thanks.

@nkdengineer
Copy link
Contributor

@brunovjk It's ready for review.

@brunovjk
Copy link
Contributor

Great, thanks. I'll finish reviewing another PR and get back here tomorrow at the latest.

@brunovjk
Copy link
Contributor

brunovjk commented Mar 5, 2025

Update: Reviewing

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 10, 2025
@melvin-bot melvin-bot bot changed the title [$400] Cards - In company cards settings page, on refresh entered details are not shown [Due for payment 2025-03-17] [$400] Cards - In company cards settings page, on refresh entered details are not shown Mar 10, 2025
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Mar 10, 2025
Copy link

melvin-bot bot commented Mar 10, 2025

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Mar 10, 2025

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.10-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2025-03-17. 🎊

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

Copy link

melvin-bot bot commented Mar 10, 2025

@brunovjk @mallenexpensify @brunovjk 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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
Status: LOW
Development

No branches or pull requests

9 participants