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.

Sign up for GitHub

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

Fix/35134: Auto select category and tag #35862

Merged
merged 18 commits into from
Mar 7, 2024
Merged

Fix/35134: Auto select category and tag #35862

Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
6db0aa7
fix auto select category and tag
dukenv0307 Feb 6, 2024
61f9d38
fix remove hardcode
dukenv0307 Feb 6, 2024
142587c
fix typo
dukenv0307 Feb 6, 2024
5a3120b
fix add comment
dukenv0307 Feb 7, 2024
d5833a6
fix merge main
dukenv0307 Feb 17, 2024
2842e16
fix merge main
dukenv0307 Feb 17, 2024
6241f85
fix handle multiple tags
dukenv0307 Feb 19, 2024
7c5fa7f
fix remove hardcode
dukenv0307 Feb 19, 2024
7729a29
fix remove console log
dukenv0307 Feb 19, 2024
7820222
fix merge main
dukenv0307 Feb 19, 2024
b7f38fc
fix required field
dukenv0307 Feb 22, 2024
4142436
fix merge main
dukenv0307 Feb 27, 2024
3085c9b
fix issue when updating tag in OD
dukenv0307 Feb 27, 2024
014531d
fix merge main
dukenv0307 Mar 3, 2024
2be9fa8
fix use required filed in both single tag and multiple tags
dukenv0307 Mar 3, 2024
8e43ae2
fix create isTagListRequired
dukenv0307 Mar 5, 2024
71bf809
Merge branch 'main' into fix/35134-auto-select-the-category-and-tag
dukenv0307 Mar 5, 2024
0854392
fix lint after merge main
dukenv0307 Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/components/MoneyTemporaryForRefactorRequestConfirmationList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {useIsFocused} from '@react-navigation/native';
import {format} from 'date-fns';
import Str from 'expensify-common/lib/str';
import {isUndefined} from 'lodash';
import lodashGet from 'lodash/get';
import PropTypes from 'prop-types';
import React, {useCallback, useEffect, useMemo, useReducer, useRef, useState} from 'react';
Expand Down Expand Up @@ -490,6 +491,31 @@ function MoneyTemporaryForRefactorRequestConfirmationList({
IOU.setMoneyRequestMerchant(transaction.transactionID, distanceMerchant, true);
}, [isDistanceRequestWithPendingRoute, hasRoute, distance, unit, rate, currency, translate, toLocaleDigit, isDistanceRequest, transaction]);

// Auto select the category if there is only one enabled category and it is required
useEffect(() => {
const enabledCategories = _.filter(policyCategories, (category) => category.enabled);
if (iouCategory || !shouldShowCategories || enabledCategories.length !== 1 || !isCategoryRequired) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using isCategoryRequired here means we'll get inconsistent behaviour vs tags as isCategoryRequired relies on canUseViolations being true, whereas the checks for the tags are not reliant on that. Do you think it's better to just use lodashGet(policy, 'requiresCategory', false) directly here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjcoffee With tags, we not only have isTagRequired which is canUseViolations && lodashGet(policy, 'requiresTag', false) but also have the required field for each tag in multiple level tags. So which required field should we use in this case? I am using the first one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dukenv0307 Ah good point! For multi-level tags I think we need to use the required field on each tag. For single-level tags however, we need to use isTagRequired as on oldDot this option is only available when multi-level tags are turned off (People must tag expenses):

image

Regarding my previous comment, I think it probably makes sense to just add canUseViolations to the tags to keep things consistent. Tags and Categories don't display the Required text unless canUseViolations is true so it makes sense for us to tie in with that I think!

return;
}
IOU.setMoneyRequestCategory(transaction.transactionID, enabledCategories[0].name);
}, [iouCategory, shouldShowCategories, policyCategories, transaction, isCategoryRequired]);

// Auto select the tag if there is only one enabled tag and it is required
useEffect(() => {
let updatedTagsString = TransactionUtils.getTag(transaction);
policyTagLists.forEach((tagList, index) => {
const enabledTags = _.filter(tagList.tags, (tag) => tag.enabled);
const isTagListRequired = isUndefined(tagList.required) ? false : tagList.required && canUseViolations;
if (!isTagListRequired || enabledTags.length !== 1 || TransactionUtils.getTag(transaction, index)) {
return;
}
updatedTagsString = IOUUtils.insertTagIntoTransactionTagsString(updatedTagsString, enabledTags[0] ? enabledTags[0].name : '', index);
});
if (updatedTagsString !== TransactionUtils.getTag(transaction) && updatedTagsString) {
IOU.setMoneyRequestTag(transaction.transactionID, updatedTagsString);
}
}, [policyTagLists, transaction, policyTags, isTagRequired, canUseViolations]);

/**
* @param {Object} option
*/
Expand Down
Loading