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 gray-out and name issue in offline category update #51927

Merged
merged 13 commits into from
Dec 4, 2024
Merged

fix gray-out and name issue in offline category update #51927

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
36a58e3
fix gray-out and name issue in offline category update
huult Nov 2, 2024
724046b
Merge remote-tracking branch 'upstream/main' into 50563-fix-offline-c…
huult Nov 3, 2024
d582cc3
remove unused code
huult Nov 5, 2024
09a7c37
Add RENAME_WORKSPACE_CATEGORY to ignore last update ID for fixing fir…
huult Nov 6, 2024
a929426
Merge remote-tracking branch 'upstream/main' into 50563-fix-offline-c…
huult Nov 14, 2024
d4ce223
remove renaming ws category
huult Nov 21, 2024
1e406c7
Merge remote-tracking branch 'upstream/main' into 50563-fix-offline-c…
huult Nov 21, 2024
91e7a4c
Merge remote-tracking branch 'upstream/main' into 50563-fix-offline-c…
huult Nov 25, 2024
1b89a1f
Remove unused imports
huult Nov 25, 2024
22b0f36
Merge remote-tracking branch 'upstream/main' into 50563-fix-offline-c…
huult Nov 27, 2024
38d8ba7
Update MCC group for failure data
huult Nov 27, 2024
d9f7184
Merge remote-tracking branch 'upstream/main' into 50563-fix-offline-c…
huult Nov 29, 2024
425e743
Update MCC group for failed data
huult Nov 29, 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
19 changes: 17 additions & 2 deletions src/libs/CategoryUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {LocaleContextProps} from '@components/LocaleContextProvider';
import CONST from '@src/CONST';
import type {Policy, TaxRate, TaxRatesWithDefault} from '@src/types/onyx';
import type {ApprovalRule, ExpenseRule} from '@src/types/onyx/Policy';
import type {ApprovalRule, ExpenseRule, MccGroup} from '@src/types/onyx/Policy';
import * as CurrencyUtils from './CurrencyUtils';

function formatDefaultTaxRateText(translate: LocaleContextProps['translate'], taxID: string, taxRate: TaxRate, policyTaxRates?: TaxRatesWithDefault) {
Expand Down Expand Up @@ -68,4 +68,19 @@ function getCategoryDefaultTaxRate(expenseRules: ExpenseRule[], categoryName: st
return categoryDefaultTaxRate;
}

export {formatDefaultTaxRateText, formatRequireReceiptsOverText, getCategoryApproverRule, getCategoryExpenseRule, getCategoryDefaultTaxRate};
function updateCategoryInMccGroup(mccGroups: Record<string, MccGroup>, oldCategoryName: string, newCategoryName: string, shouldClearPendingAction?: boolean) {
if (oldCategoryName === newCategoryName) {
return mccGroups;
}

const updatedGroups: Record<string, MccGroup> = {};

for (const [key, group] of Object.entries(mccGroups || {})) {
updatedGroups[key] =
group.category === oldCategoryName ? {...group, category: newCategoryName, pendingAction: shouldClearPendingAction ? null : CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE} : group;
}

return updatedGroups;
}

export {formatDefaultTaxRateText, formatRequireReceiptsOverText, getCategoryApproverRule, getCategoryExpenseRule, getCategoryDefaultTaxRate, updateCategoryInMccGroup};
1 change: 1 addition & 0 deletions src/libs/Middleware/SaveResponseInOnyx.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const requestsToIgnoreLastUpdateID: string[] = [
SIDE_EFFECT_REQUEST_COMMANDS.RECONNECT_APP,
WRITE_COMMANDS.CLOSE_ACCOUNT,
WRITE_COMMANDS.DELETE_MONEY_REQUEST,
WRITE_COMMANDS.RENAME_WORKSPACE_CATEGORY,
Copy link
Contributor

Choose a reason for hiding this comment

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

all these requests are updating the app to the most current state

Is this really the case for WRITE_COMMANDS.RENAME_WORKSPACE_CATEGORY request?

Copy link
Contributor Author

@huult huult Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I added it to fix the issue of updating the category. After that, when I go back and go to the settings, I see that the category is grayed out even though it was changed

VIDEO
Screen.Recording.2024-11-06.at.14.01.14.mp4

The RCA: lastUpdateIDFromClient is less than the previousUpdateID received, so we set this response, which did not clear the pendingAction. This issue occurred, and it only happens the first time.

This issue same RCA with #50780 #49329 and #44114

I raised this issue in Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1730457202958539?thread_ts=1730392335.081509&cid=C01GTK53T8Q

I think this issue is out of the scope of this ticket and has a different RCA. However, if we don't fix it, we will need to perform a regression test. Can you advise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eVoloshchak Could you review my comment above when you have time? Thank you

Copy link
Contributor

Choose a reason for hiding this comment

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

The RCA: lastUpdateIDFromClient is less than the previousUpdateID received

@huult, it seems to me that this should be resolved on the backend ideally. Do you agree with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eVoloshchak Yes, if they are able to fix it, we should update from the BE side. This issue is not related to this ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eVoloshchak what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we skip this case? It is not related to this issue and has a different RCA. We will report a new ticket to fix it.

Yes, that's exactly what needs to be done. This should be fixed on the back end, it's not related to this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eVoloshchak I think we need to confirm with the internal team to avoid regression when testing this case, or we can add this command to the ignore list. Which option do you choose or suggest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eVoloshchak Could you review this pull request so it can be completed? It has been open for 3 weeks and involves a small change, so your prompt review would be appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eVoloshchak I have removed this command from the ignore list.

SIDE_EFFECT_REQUEST_COMMANDS.GET_MISSING_ONYX_MESSAGES,
];

Expand Down
15 changes: 14 additions & 1 deletion src/libs/actions/Policy/Category.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import * as ReportUtils from '@libs/ReportUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Policy, PolicyCategories, PolicyCategory, RecentlyUsedCategories, Report} from '@src/types/onyx';
import type {ApprovalRule, CustomUnit, ExpenseRule} from '@src/types/onyx/Policy';
import type {ApprovalRule, CustomUnit, ExpenseRule, MccGroup} from '@src/types/onyx/Policy';
import type {PolicyCategoryExpenseLimitType} from '@src/types/onyx/PolicyCategory';
import type {OnyxData} from '@src/types/onyx/Request';

Expand Down Expand Up @@ -542,8 +542,12 @@ function renamePolicyCategory(policyID: string, policyCategory: {oldName: string
const policyCategoryExpenseRule = CategoryUtils.getCategoryExpenseRule(policy?.rules?.expenseRules ?? [], policyCategory.oldName);
const approvalRules = policy?.rules?.approvalRules ?? [];
const expenseRules = policy?.rules?.expenseRules ?? [];
const mccGroup = policy?.mccGroup ?? {};
const updatedApprovalRules: ApprovalRule[] = lodashCloneDeep(approvalRules);
const updatedExpenseRules: ExpenseRule[] = lodashCloneDeep(expenseRules);
const clonedMccGroup: Record<string, MccGroup> = lodashCloneDeep(mccGroup);
const updatedMccGroup = CategoryUtils.updateCategoryInMccGroup(clonedMccGroup, policyCategory.oldName, policyCategory.newName);
const updatedMccGroupWithClearedPendingAction = CategoryUtils.updateCategoryInMccGroup(clonedMccGroup, policyCategory.oldName, policyCategory.newName, true);

if (policyCategoryExpenseRule) {
const ruleIndex = updatedExpenseRules.findIndex((rule) => rule.id === policyCategoryExpenseRule.id);
Expand Down Expand Up @@ -596,10 +600,18 @@ function renamePolicyCategory(policyID: string, policyCategory: {oldName: string
approvalRules: updatedApprovalRules,
expenseRules: updatedExpenseRules,
},
mccGroup: updatedMccGroup,
},
},
],
successData: [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
mccGroup: updatedMccGroupWithClearedPendingAction,
},
},
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY_CATEGORIES}${policyID}`,
Expand Down Expand Up @@ -639,6 +651,7 @@ function renamePolicyCategory(policyID: string, policyCategory: {oldName: string
rules: {
approvalRules,
},
mccGroup: updatedMccGroupWithClearedPendingAction,
huult marked this conversation as resolved.
Show resolved Hide resolved
},
},
],
Expand Down
1 change: 1 addition & 0 deletions src/types/onyx/Policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1901,4 +1901,5 @@ export type {
ApprovalRule,
ExpenseRule,
NetSuiteConnectionConfig,
MccGroup,
};
Loading