Repository navigation
Fix Describe rule bugs - #104055
Fix Describe rule bugs #104055
Conversation
|
|
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@Krishna2323 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@Krishna2323 Steps 6 and 8 can only be tested after the backend PRs are deployed. But you can review the rest |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
| const generatedCategories = values.categories?.filter(isSpendRuleCategory); | ||
| setDraftSpendRule(generatedCategories ? {...values, categories: generatedCategories} : values); | ||
| const generatedCardIDs = | ||
| isAllCards && expensifyCardSettings ? getEligibleSpendRuleCards(cardsList, expensifyCardSettings).map((card) => String(card.cardID)) : spendRuleForm?.cardIDs; |
There was a problem hiding this comment.
When card settings haven't loaded yet (e.g. a deep link), an "all cards" answer quietly keeps the current selection instead. Do we think it's worth disabling Describe until expensifyCardSettings is loaded? Or the whole modal?
There was a problem hiding this comment.
i don't think it is worth disabling.
the admin can choose the cards manually too
| setDraftMerchantRule({...values, ruleType: CONST.POLICY.EXPENSE_DEFAULT_RULE_TYPE.MERCHANT}); | ||
| const applyGeneratedRule = ({tax, ...values}: GeneratedRuleValues) => { | ||
| if (!isScopedToCategory) { | ||
| setDraftMerchantRule({...values, ruleType: CONST.POLICY.EXPENSE_DEFAULT_RULE_TYPE.MERCHANT}); |
There was a problem hiding this comment.
tax is destructured out of the signature but only the category branch puts it back, so merchant rules lose it. Line 354 reads form?.tax for merchant rules. Does an expenseDefaults answer ever carry a tax?
There was a problem hiding this comment.
merchant rule doesn't have tax
There was a problem hiding this comment.
Merchant rules do have a tax field. Line 590 renders the tax row whenever hasUsableTaxRates(policy), line 639's filter keeps it, and line 354 reads form?.tax for merchant rules. Do you mean the backend never sends tax on an expenseDefaults answer?
There was a problem hiding this comment.
The backend doesn't send tax for an expenseDefaults rule. It only sends it for categoryTaxRate.
#103928 @rushatgabhane this still fails: Monosnap.screencast.2026-10-20.02-03-46.mp4 |
|
#103859 On android, the modal doesn't appear: modal_doesn.t_appear.mp4 |
|
this is not a valid bug. the instruction says "require itemized receipt", for which a receipt is required too. |
|
@rushatgabhane Yeah correct, but it should throw error in that case? Currently it looks like the request was successful. |
|
no why should it err? the intention of this feature is to do best effort for the user's request and transform it to a valid rule.
fixed, thanks! |
|
🚧 JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.5.15-1 🚀
|
|
No help site update is needed: every user-facing change in this PR is in the Describe rule flow, which only shows when the |
Explanation of Change
Fixes the Describe bugs from QA, listed below.
Also adds Describe to the category tax rate editor.
Backend PRs:
Fixed Issues
$ #103859
$ #103872
$ #103930
$ #103928
$ #103863
$ #103869
PROPOSAL: #92743 (comment)
Tests
Steps 6 and 8 can only be tested after the backend PRs are deployed.Offline tests
Same as tests. Describe needs a connection, but the modal still opens empty while offline.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-10-15.at.02.39.39.mov
Screen.Recording.2026-10-15.at.02.30.21.mov
Screen.Recording.2026-10-15.at.02.29.42.mov
Screen.Recording.2026-10-15.at.02.29.01.mov
Screen.Recording.2026-10-15.at.02.28.19.mov
Screen.Recording.2026-10-15.at.02.27.16.mov