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

[HOLD for payment 2024-11-20] [$250] Search filters - Category list shows default categories when no workspace is created #50564

Closed
6 tasks done
lanitochka17 opened this issue Oct 10, 2024 · 45 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Oct 10, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.47-1
Reproducible in staging?: Y
Reproducible in production?: Y
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): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Log in with a new account (no workspace)
  3. Go to Search
  4. Click Filters
  5. Click Categories

Expected Result:

Category list should be empty because no workspace is created

Actual Result:

Category list shows default categories when no workspace is created

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6630371_1728540215030.20241010_140142.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021844424503641996992
  • Upwork Job ID: 1844424503641996992
  • Last Price Increase: 2024-10-31
Issue OwnerCurrent Issue Owner: @garrettmknight
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 10, 2024
Copy link

melvin-bot bot commented Oct 10, 2024

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.

@lanitochka17
Copy link
Author

@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@lanitochka17
Copy link
Author

We think that this bug might be related to #wave-control

@NJ-2020
Copy link
Contributor

NJ-2020 commented Oct 10, 2024

Edited by proposal-police: This proposal was edited at 2024-10-16 06:05:34 UTC.

Proposal

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

Search filters - Category list shows default categories when no workspace is created

What is the root cause of that problem?

Right here we don't filter out the personal policy for allPolicyIDCategories

const [allPolicyIDCategories] = useOnyx(ONYXKEYS.COLLECTION.POLICY_CATEGORIES);
const singlePolicyCategories = allPolicyIDCategories?.[`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${policyID}`];
const categoryItems = useMemo(() => {
if (!singlePolicyCategories) {
const uniqueCategoryNames = new Set<string>();
Object.values(allPolicyIDCategories ?? {}).map((policyCategories) => Object.values(policyCategories ?? {}).forEach((category) => uniqueCategoryNames.add(category.name)));
return Array.from(uniqueCategoryNames).map((categoryName) => ({name: categoryName, value: categoryName}));
}
return Object.values(singlePolicyCategories ?? {}).map((category) => ({name: category.name, value: category.name}));
}, [allPolicyIDCategories, singlePolicyCategories]);

And inside AdvancedSearchFilters.tsx we don't hide the categories, tags, and taxes if the list is empty

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

We can create 2 new hooks inside PolicyUtils to get the lists of tags and categories, for taxes we already have right here:

const taxRates = getAllTaxRates();

and we will hide the categories, tags and taxes inside AdvancedSearchFilters page if the list is empty

function getAllTags(): Record<string, string> {
    const tags: Record<string, string> = {};
    Object.values(allTags ?? {})?.forEach((policy) => {
        if (!policy?.Tag.tags) {
            return;
        }
        Object.entries(policy?.Tag.tags).forEach(([tagKey, tag]) => {
            if (!tags[tag.name]) {
                tags[tag.name] = tagKey;
                return;
            }
        });
    });
    return tags;
}

function getAllCategories() {
    const uniqueCategoryNames = new Set<string>();
    const personalPolicyID = getPersonalPolicy()?.id;
    Object.entries(allCategories ?? {})?.forEach(([policyKey, policyCategories]) => {
        if (isEmptyObject(policyCategories) || policyKey === `${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${personalPolicyID}`) {
            return;
        }
        Object.values(policyCategories ?? {}).forEach((category) => uniqueCategoryNames.add(category.name));
    });
    return Array.from(uniqueCategoryNames).map((categoryName) => ({name: categoryName, value: categoryName}));
}

Then inside AdvancedSearchFilters

const tags = getAllTags();
const categories = getAllCategories();

...
else if (key === CONST.SEARCH.SYNTAX_FILTER_KEYS.CATEGORY) {
    if (isEmptyObject(categories)) {
        return undefined;
    }
    filterTitle = baseFilterConfig[key].getTitle(searchAdvancedFilters, key, translate);
} else if (key === CONST.SEARCH.SYNTAX_FILTER_KEYS.TAG) {
    if (isEmptyObject(tags)) {
        return undefined;
    }
    filterTitle = baseFilterConfig[key].getTitle(searchAdvancedFilters, key, translate);
}  else if (key === CONST.SEARCH.SYNTAX_FILTER_KEYS.TAX_RATE) {
    if (isEmptyObject(taxRates)) {
        return undefined;
    }
    filterTitle = baseFilterConfig[key].getTitle(searchAdvancedFilters, taxRates);
}

For cards we already hide right here if the lists are empty

const [cardList = {}] = useOnyx(ONYXKEYS.CARD_LIST);

} else if (key === CONST.SEARCH.SYNTAX_FILTER_KEYS.CARD_ID) {
if (Object.keys(cardList).length === 0) {
return undefined;
}
filterTitle = baseFilterConfig[key].getTitle(searchAdvancedFilters, cardList);

But i think for cards we should update the if condition from above code to:

if (isEmptyObject(cardList)) {
    return undefined;
}

And Inside the SearchFiltersCategoryPage we can use the new hook we just created getAllCategories if the current policy categories is empty which also will filter out the personal policy
And we should update from this:

const categoryItems = useMemo(() => {
const items = [{name: translate('search.noCategory'), value: CONST.SEARCH.EMPTY_VALUE as string}];
if (!singlePolicyCategories) {
const uniqueCategoryNames = new Set<string>();
Object.values(allPolicyIDCategories ?? {}).map((policyCategories) => Object.values(policyCategories ?? {}).forEach((category) => uniqueCategoryNames.add(category.name)));
items.push(...Array.from(uniqueCategoryNames).map((categoryName) => ({name: categoryName, value: categoryName})));
} else {
items.push(...Object.values(singlePolicyCategories ?? {}).map((category) => ({name: category.name, value: category.name})));
}
return items;
}, [allPolicyIDCategories, singlePolicyCategories, translate]);

to

const categoryItems = useMemo(() => {
    if (!singlePolicyCategories) {
        return PolicyUtils.getAllCategories();
    }
    return Object.values(singlePolicyCategories ?? {}).map((category) => ({name: category.name, value: category.name}));
}, [singlePolicyCategories]);

Here's the test branch

Result

What alternative solutions did you explore? (Optional)

@truph01
Copy link
Contributor

truph01 commented Oct 10, 2024

Edited by proposal-police: This proposal was edited at 2024-10-11 23:28:14 UTC.

Proposal

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

Category list shows default categories when no workspace is created

What is the root cause of that problem?

  • In the logic to get the categories from policies:

Object.values(allPolicyIDCategories ?? {}).map((policyCategories) => Object.values(policyCategories ?? {}).forEach((category) => uniqueCategoryNames.add(category.name)));

we don't filter out the personal policy, which also has categories.

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

  • In this, we should check if the policy is personal policy, if so, don't get categories from it:
            const personalPolicyID = PolicyUtils.getPersonalPolicy()?.id;
            const allPolicyIDCategoriesWithoutPersonalPolicy = Object.keys(allPolicyIDCategories).map((key)=>{
                if(key === `policyCategories_${personalPolicyID}`){
                    return {}
                }
                return allPolicyIDCategories?.[key]
            })
          
            allPolicyIDCategoriesWithoutPersonalPolicy.map((policyCategories) => Object.values(policyCategories ?? {}).forEach((category) => uniqueCategoryNames.add(category.name)));

What alternative solutions did you explore? (Optional)

  • Beside the main solution, if we want to hide the category option in AdvancedSearchFilters if there is no category option, we can:
  1. Create a util hook to get the categories, tags, cards, ... data:
const useFilterOption = () => {
    const [searchAdvancedFiltersForm] = useOnyx(ONYXKEYS.FORMS.SEARCH_ADVANCED_FILTERS_FORM);
    const selectedCategoriesItems = searchAdvancedFiltersForm?.category?.map((category) => ({name: category, value: category}));
    const policyID = searchAdvancedFiltersForm?.policyID ?? '-1';
    const [allPolicyIDCategories] = useOnyx(ONYXKEYS.COLLECTION.POLICY_CATEGORIES);
    const singlePolicyCategories = allPolicyIDCategories?.[`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${policyID}`];

    const categories = useMemo(() => {
        if (!singlePolicyCategories) {
            const uniqueCategoryNames = new Set<string>();
            Object.values(allPolicyIDCategories ?? {}).map((policyCategories) => Object.values(policyCategories ?? {}).forEach((category) => uniqueCategoryNames.add(category.name)));
            return Array.from(uniqueCategoryNames).map((categoryName) => ({name: categoryName, value: categoryName}));
        }
        return Object.values(singlePolicyCategories ?? {}).map((category) => ({name: category.name, value: category.name}));
    }, [allPolicyIDCategories, singlePolicyCategories]);

    const [allPoliciesTagsLists] = useOnyx(ONYXKEYS.COLLECTION.POLICY_TAGS);
    const singlePolicyTagsList: PolicyTagLists | undefined = allPoliciesTagsLists?.[`${ONYXKEYS.COLLECTION.POLICY_TAGS}${policyID}`];

    const tags = useMemo(() => {
        if (!singlePolicyTagsList) {
            const uniqueTagNames = new Set<string>();
            const tagListsUnpacked = Object.values(allPoliciesTagsLists ?? {}).filter((item) => !!item) as PolicyTagLists[];
            tagListsUnpacked
                .map((policyTagLists) => {
                    return getTagNamesFromTagsLists(policyTagLists);
                })
                .flat()
                .forEach((tag) => uniqueTagNames.add(tag));
            return Array.from(uniqueTagNames).map((tagName) => ({name: tagName, value: tagName}));
        }
        return getTagNamesFromTagsLists(singlePolicyTagsList).map((name) => ({name, value: name}));
    }, [allPoliciesTagsLists, singlePolicyTagsList]);

    const [cards] = useOnyx(ONYXKEYS.CARD_LIST);
    return {categories, tags, cards};
};
  1. In AdvancedSearchFilters use:
    const {cards, categories, tags} = useFilterOption();

then for the key category, tag, or card, check if its options length is greater than 0, if not, return undefined in:

const filters = typeFiltersKeys[currentType].map((key) => {

@garrettmknight garrettmknight added the External Added to denote the issue can be worked on by a contributor label Oct 10, 2024
@melvin-bot melvin-bot bot changed the title Search filters - Category list shows default categories when no workspace is created [$250] Search filters - Category list shows default categories when no workspace is created Oct 10, 2024
Copy link

melvin-bot bot commented Oct 10, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 10, 2024
Copy link

melvin-bot bot commented Oct 10, 2024

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

@Shahidullah-Muffakir
Copy link
Contributor

Shahidullah-Muffakir commented Oct 11, 2024

Edited by proposal-police: This proposal was edited at 2024-10-14 20:00:32 UTC.

Proposal

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

  1. Currently, we show Default Categories in search filters even if no Workspaces are created by the user. These categories should be hidden when no Workspaces exist.
  2. Based on this comment, we should hide the Categories, Tags, Taxes, and Cards filters in the AdvancedSearchFilters page when their corresponding lists are empty.

What is the root cause of that problem?

We haven't added checks in the AdvancedSearchFilters component to hide filters when their respective lists (Categories, Tags, Taxes, Cards) are empty.

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

Test branch

To fix the issue:

  1. We need access to the lists for Categories, Tags, Taxes, and Cards in the AdvancedSearchFilters component.
  2. Tags are already available here and Cards here.
  3. To hide the Tags filter, we just need to check the number of tags:
const [allPoliciesTagsLists] = useOnyx(ONYXKEYS.COLLECTION.POLICY_TAGS);
`const hasTags = Object.values(allPoliciesTagsLists).some(policy => policy.Tag.tags.length > 0);
  1. For the Categories filter, if the user hasn’t created any Workspace, we will hide it. This will also solve the issue mentioned in the original post:
const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);
if (Object.keys(policies).length === 1) { // Each user has one personal workspace, so we can ignore that.
  1. The hiding of Cards is already implemented here, no need to add any code for it.

If any list (Categories, Tags, Taxes, Cards) is empty, we will return undefined to hide the corresponding filter.

Note: if we want to hide the default Categories in SearchFiltersCategoryPage if user is trying to access it using the url:
https://staging.new.expensify.com/search/filters/category

we can add the same check, in SearchFiltersCategoryPage.tsx , component to hide the default category list.

const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY);
if (Object.keys(policies).length === 1)
Screen.Recording.2024-10-15.at.2.09.09.AM.mov

@289Adam289
Copy link
Contributor

@luacmartins is this an unexpected behavior?
I am currently working on Search v2 and I looked on proposals and if this is a bug @truph01 proposal should work.

@luacmartins
Copy link
Contributor

Yes, we shouldn't show tags in this case. Additionally, we should hide the categories, tags, taxes and cards filter from the AdvancedSearchFilters page if the list is empty

@truph01
Copy link
Contributor

truph01 commented Oct 11, 2024

Proposal updated

@fedirjh
Copy link
Contributor

fedirjh commented Oct 14, 2024

cc @NJ-2020 and @Shahidullah-Muffakir Just in case you missed this comment #50564 (comment), We should hide the filter from AdvancedSearchFilters.

@Shahidullah-Muffakir
Copy link
Contributor

@fedirjh , Thank you, will update my proposal.

@Shahidullah-Muffakir
Copy link
Contributor

@fedirjh , Proposal updated, If needed I can make a test branch. Thank you.

@Shahidullah-Muffakir
Copy link
Contributor

Here is the test branch, Thank you.

@NJ-2020
Copy link
Contributor

NJ-2020 commented Oct 16, 2024

Proposal Updated

@NJ-2020
Copy link
Contributor

NJ-2020 commented Oct 16, 2024

@fedirjh Done, Thanks

Actually i want to upload the video result but seems not working i think maybe there's an issue when trying to upload the file

Copy link

melvin-bot bot commented Oct 17, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Oct 17, 2024
@Shahidullah-Muffakir
Copy link
Contributor

i think maybe there's an issue when trying to upload the file

@NJ-2020 I think it could be because of the size of the file, I always compress the video before attaching it.

Copy link

melvin-bot bot commented Oct 18, 2024

@garrettmknight, @fedirjh Whoops! This issue is 2 days overdue. Let's get this updated quick!

@dylanexpensify dylanexpensify moved this to Release 3: Fall 2024 (Nov) in [#whatsnext] #expense Oct 18, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Nov 6, 2024
@luacmartins luacmartins self-assigned this Nov 6, 2024
@garrettmknight garrettmknight moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Nov 11, 2024
Copy link

melvin-bot bot commented Nov 12, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 13, 2024
@melvin-bot melvin-bot bot changed the title [$250] Search filters - Category list shows default categories when no workspace is created [HOLD for payment 2024-11-20] [$250] Search filters - Category list shows default categories when no workspace is created Nov 13, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 13, 2024
Copy link

melvin-bot bot commented Nov 13, 2024

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

Copy link

melvin-bot bot commented Nov 13, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.60-3 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 2024-11-20. 🎊

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

  • @Kicu does not require payment (Contractor)
  • @fedirjh requires payment through NewDot Manual Requests

Copy link

melvin-bot bot commented Nov 13, 2024

@fedirjh @garrettmknight @fedirjh 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 garrettmknight moved this from Bugs and Follow Up Issues to Hold for Payment in [#whatsnext] #expense Nov 14, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 19, 2024
Copy link

melvin-bot bot commented Nov 20, 2024

Payment Summary

Upwork Job

  • Contributor: @Kicu is from an agency-contributor and not due payment
  • Reviewer: @fedirjh owed $250 via NewDot

BugZero Checklist (@garrettmknight)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1844424503641996992/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2024
@garrettmknight
Copy link
Contributor

@fedirjh please complete the checklist when you get a chance.

@melvin-bot melvin-bot bot removed the Overdue label Nov 21, 2024
@fedirjh
Copy link
Contributor

fedirjh commented Nov 22, 2024

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
  • 2b. Reported on staging (deploy blocker)
  • 2c. Reported on both staging and production
  • 2d. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [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: N/A This is a new feature rather than a bug fix.

  • [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.

  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

  1. Open Search
  2. Open Advanced filters RHP
  3. Verify that there is no tag filter displayed if user has no tags
  4. Verify that there is no category filter displayed if user has no categories
  5. Verify that there is no tax filter displayed if user has no taxes

Do we agree 👍 or 👎

@melvin-bot melvin-bot bot added the Overdue label Nov 25, 2024
Copy link

melvin-bot bot commented Nov 25, 2024

@garrettmknight, @Kicu, @luacmartins, @fedirjh Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@garrettmknight
Copy link
Contributor

@fedirjh thanks - submit the payment request when you're ready!

@github-project-automation github-project-automation bot moved this from Hold for Payment to Done in [#whatsnext] #expense Nov 25, 2024
@melvin-bot melvin-bot bot removed the Overdue label Nov 25, 2024
@JmillsExpensify
Copy link

Payment summary: @fedirjh owed $250 via NewDot

@garrettmknight
Copy link
Contributor

$250 approved for @fedirjh

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Status: Done
Development

No branches or pull requests

10 participants