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

[QBO] [$250] Workspace settings – “Members must tag all spend” is enabled when all tags are disabled #41309

Closed
4 of 6 tasks
izarutskaya opened this issue Apr 30, 2024 · 36 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 Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@izarutskaya
Copy link

izarutskaya commented Apr 30, 2024

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: v1.4.68-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4524619
Email or phone of affected tester (no customers): applausetester+jp_e_category@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Log in as Collect WS Admin
  3. Go to WS settings
  4. Disable all categories and open Settings
  5. Verify that “Members must categorize all spend” is locked
  6. Disable all tags and open Settings
  7. Verify that “Members must tag all spend” is enabled.

Expected Result:

Set up as required is locked when all tags and categories are disabled.

Actual Result:

Inconsistency between tags and categories settings. “Members must tag all spend” is enabled when all tags are disabled, but in categories setting “Members must categorize all spend” is locked when all categories are disabled.

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

Bug6466258_1714458601941.Members_must_tag_all_spend.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01074744206851fea7
  • Upwork Job ID: 1787964144555667456
  • Last Price Increase: 2024-06-05
Issue OwnerCurrent Issue Owner: @rushatgabhane
@izarutskaya izarutskaya added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 30, 2024
Copy link

melvin-bot bot commented Apr 30, 2024

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

@izarutskaya
Copy link
Author

We think this issue might be related to the #collect project.

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Apr 30, 2024

Proposal

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

Changes in “Members must tag all spend” and “Members must categorise all spend”.

What is the root cause of that problem?

New change based on this comment.

In WorkspaceCategoriesSettingsPage, ToggleSettingOptionRow is used which has the disabled condition which also checks !hasEnabledOptions.

<ToggleSettingOptionRow
title={translate('workspace.categories.requiresCategory')}
subtitle={isConnectedToAccounting ? `${translate('workspace.categories.needCategoryForExportToIntegration')} ${translate('workspace.accounting.qbo')}` : ''}
isActive={policy?.requiresCategory ?? false}
onToggle={updateWorkspaceRequiresCategory}
pendingAction={policy?.pendingFields?.requiresCategory}
disabled={!policy?.areCategoriesEnabled || !hasEnabledOptions || isConnectedToAccounting}
wrapperStyle={[styles.mt2, styles.mh4]}
errors={policy?.errorFields?.requiresCategory ?? undefined}
onCloseError={() => Policy.clearPolicyErrorField(policy?.id ?? '', 'requiresCategory')}
/>

Below is the code in WorkspaceTagsSettingsPage

<Text style={[styles.textNormal, styles.colorMuted]}>{translate('workspace.tags.requiresTag')}</Text>
<Switch
isOn={policy?.requiresTag ?? false}
accessibilityLabel={translate('workspace.tags.requiresTag')}
onToggle={updateWorkspaceRequiresTag}
/>

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

Update isActive condition in WorkspaceCategoriesSettingsPage:

isActive={(policy?.requiresCategory ?? false) || isConnectedToAccounting}

disabled condition:

disabled={!policy?.areCategoriesEnabled || isConnectedToAccounting}

Instead of this, add the below block to use ToggleSettingOptionRow:

<View style={styles.flexGrow1}>
    <ToggleSettingOptionRow
        title={translate('workspace.tags.requiresTag')}
        isActive={policy?.requiresTag ?? false}
        accessibilityLabel={translate('workspace.tags.requiresTag')}
        onToggle={updateWorkspaceRequiresTag}
        pendingAction={policy?.pendingFields?.requiresTag}
        disabled={!policy?.areTagsEnabled}
        wrapperStyle={[styles.mt2, styles.mh4, styles.mb4]}
        errors={policy?.errorFields?.requiresTag ?? undefined}
        onCloseError={() => Policy.clearPolicyErrorField(policy?.id ?? '', 'requiresTag')}
    />
    <OfflineWithFeedback
        errors={policyTags?.[policyTagName]?.errors}
        pendingAction={policyTags?.[policyTagName]?.pendingAction}
        errorRowStyles={styles.mh5}
    >
        <MenuItemWithTopDescription
            title={policyTagName}
            description={translate(`workspace.tags.customTagName`)}
            onPress={() => Navigation.navigate(ROUTES.WORKSPACE_EDIT_TAGS.getRoute(route.params.policyID))}
            shouldShowRightIcon
        />
    </OfflineWithFeedback>
</View>

If we want to have a check on whethere at least one tag is enabled, then we can defined hasEnabledOptions like this:

const hasEnabledOptions = OptionsListUtils.hasEnabledOptions(PolicyUtils.getTagLists(policyTags)?.[0]?.tags ?? {});

And then pass it to disabled:

disabled={!policy?.areTagsEnabled || !hasEnabledOptions}

Similarly, if we want, this logic can be kept in categories settings also (it is already available there).

@neonbhai
Copy link
Contributor

Proposal

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

"Members must tag all spend” is enabled when all tags are disabled"

What is the root cause of that problem?

We don't disable the tag switch if there are no enabled options:

<Switch
isOn={policy?.requiresTag ?? false}
accessibilityLabel={translate('workspace.tags.requiresTag')}
onToggle={updateWorkspaceRequiresTag}
/>

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

We will add a check for enabled options here:

<Switch
    ...
    disabled={!policy?.areTagsEnabled || !hasEnabledOptions}
/>

@ShridharGoel
Copy link
Contributor

Proposal

Updated

@melvin-bot melvin-bot bot added the Overdue label May 2, 2024
Copy link

melvin-bot bot commented May 3, 2024

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

Copy link

melvin-bot bot commented May 7, 2024

@johncschuster Still overdue 6 days?! Let's take care of this!

@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label May 7, 2024
@melvin-bot melvin-bot bot changed the title Workspace settings – “Members must tag all spend” is enabled when all tags are disabled [$250] Workspace settings – “Members must tag all spend” is enabled when all tags are disabled May 7, 2024
Copy link

melvin-bot bot commented May 7, 2024

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

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

melvin-bot bot commented May 7, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label May 7, 2024
@trjExpensify
Copy link
Contributor

Hm, the bug here seems to be that we've toggled and locked required categories when the workspace is not connected to an accounting integration.

@hayata-suenaga, do you agree the below is the expected behaviour?

Categories

  • Not connected to an accounting solution: Toggled off, not locked
  • Connected to an accounting solution: Toggled on, locked

Tags

  • Not connected to an accounting solution: Toggled off, not locked
  • Connected to an accounting solution: Toggled off, not locked

Required categories are enforced with an accounting integration because we have to have a category added to the expense to be able to export the expense to a corresponding account.
Required tags are totally up to the discretion of the admin, they aren't mandatory coding requirements.

@trjExpensify trjExpensify changed the title [$250] Workspace settings – “Members must tag all spend” is enabled when all tags are disabled [QBO] [$250] Workspace settings – “Members must tag all spend” is enabled when all tags are disabled May 8, 2024
@ShridharGoel
Copy link
Contributor

ShridharGoel commented May 8, 2024

@trjExpensify Would these be independent of whether there's at least one enabled tag/category or not?

@trjExpensify
Copy link
Contributor

Yeah, enabling/disabling imported category/tag values don't inform the "members must categories/tag all spend" toggle.

@ShridharGoel
Copy link
Contributor

Proposal

Updated

@hayata-suenaga
Copy link
Contributor

isActive={(policy?.requiresCategory ?? false) || isConnectedToAccounting}

@ShridharGoel, this is not necessary as requiresCategory is set to true when the workspace is connected to an accounting solution

@hayata-suenaga
Copy link
Contributor

@hayata-suenaga, do you agree the below is the expected behaviour?

yes the requirements seem to be correct 😄

Hm, the bug here seems to be that we've toggled and locked required categories when the workspace is not connected to an accounting integration.

I wonder this is actually a bug 🤔 The switch for requiring categorization should be off when there is no active category to be used. Otherwise, submitters cannot submit expense reports (they are required to categorize expenses but there is no categories to select from).

I think the bug description that @neonbhai provided seems to be correct. We're not disabling requiring tags when all tags are non-active.

@trjExpensify, I think the issue in the context of the QBO project is that the user can disable call categories even when connected to QBO.

So I believe there are two issues here:

  • Requiring tags is not locked and disabled when no tags are active
  • The admin can disable all categories even when there is accounting connection

Do you agree that these are the issues we should address?

@ShridharGoel
Copy link
Contributor

I think the bug description that @neonbhai provided seems to be correct.

That description is the same as the OP title. But based on this comment, I think we want to keep the toggling independent of enabled tag/category because of which I updated my proposal.

@trjExpensify
Copy link
Contributor

The switch for requiring categorization should be off when there is no active category to be used. Otherwise, submitters cannot submit expense reports (they are required to categorize expenses but there is no categories to select from).

They can submit an expense still I believe, it just shows a violation that categories are required and they haven't added one. But cool, I'm fine with disabling the required toggle if all values have been disabled. So I think we can just address this one:

Requiring tags is not locked and disabled when no tags are active

The admin can disable all categories even when there is accounting connection

I think this is okay to allow them to do, otherwise it's super difficult to achieve the below which is very common for a QBO admin setting up the workspace:

  • 100s of their chart of accounts get imported from QBO by default
  • They only have a handful of expense accounts they want to code expenses to and thus make visible to the members to select when creating their expenses
  • So they bulk select all > disable
  • then individually select the 10 expense accounts > enable

If we prevented them from being able to bulk select > disable all of their category values, they'd have to individually click through 100s of accounts to turn them off.

@melvin-bot melvin-bot bot added the Weekly KSv2 label May 10, 2024
@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented May 10, 2024

nice! then let's address this issue here

Requiring tags is not locked and disabled when no tags are active

Because the change is small, I opened a PR myself based on @neonbhai's proposal. @rushatgabhane, please review this PR when you have time. 🙇

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@hayata-suenaga
Copy link
Contributor

@mvtglobally, I was able to reproduce this on staging. Could you test it again, please? 😄

Screen.Recording.2024-05-13.at.1.08.36.PM.mov

@kbecciv
Copy link

kbecciv commented May 13, 2024

Issue is reproducible on build 1.4.73.1

Recording.475.mp4

@rushatgabhane
Copy link
Member

issue still reproducible in PR

@trjExpensify
Copy link
Contributor

Oh, the PR linked isn't fixing it?

@rushatgabhane
Copy link
Member

yes unless I'm doing something wrong - #42004 (comment)

@hayata-suenaga
Copy link
Contributor

@rushatgabhane, I commented on the PR 😄

@hayata-suenaga hayata-suenaga added Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels May 29, 2024
@hayata-suenaga
Copy link
Contributor

@rushatgabhane for C+ for this PR

Copy link

melvin-bot bot commented May 29, 2024

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

@johncschuster
Copy link
Contributor

Payment Summary:

Contributor+: @rushatgabhane $250 - paid via Manual Request

@neonbhai
Copy link
Contributor

@johncschuster hi, I may be eligible for compensation here as the PR was based on the proposal as mentioned here. Please take a look 🙇

@JmillsExpensify
Copy link

$250 approved for @rushatgabhane

Copy link

melvin-bot bot commented Jun 3, 2024

@johncschuster, @rushatgabhane Eep! 4 days overdue now. Issues have feelings too...

@rushatgabhane
Copy link
Member

not overdue

@melvin-bot melvin-bot bot added Daily KSv2 and removed Daily KSv2 labels Jun 4, 2024
@hayata-suenaga
Copy link
Contributor

just waiting for payment?

Copy link

melvin-bot bot commented Jun 5, 2024

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

@hayata-suenaga
Copy link
Contributor

are we waiting for payment here or has the payment been issued? 😄

@rushatgabhane
Copy link
Member

all done, issue can be closed

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 Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
Archived in project
Development

No branches or pull requests

10 participants