[Payment due @Krishna2323] Add support for SmartScan hotel nightly rate category violations - #97596
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 ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@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] |
|
@akinwale I can review if required. |
@Krishna2323 Please go ahead. Thanks. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b55cd376b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
… days for night counting
garrettmknight
left a comment
There was a problem hiding this comment.
Steps look good to me.
|
|
||
| // A SmartScanned multi-day reservation is measured against its average nightly rate rather than its total | ||
| const reservationNights = TransactionUtils.getReservationNights(updatedTransaction); | ||
| const amountForLimitCheck = reservationNights > 0 ? expenseAmount / reservationNights : expenseAmount; |
There was a problem hiding this comment.
Does the backend floor/round/ceil the per-night average, and at what precision?
There was a problem hiding this comment.
There is no rounding in the backend. The divided value is compared directly with the actual limit for the violation check.
| canCalculateAmountViolations && !isInvoiceTransaction && TransactionUtils.hasReservationList(updatedTransaction) && isSameCurrency && expenseAmount > -updatedTransaction.amount; | ||
| const shouldCategoryShowOverLimitViolation = | ||
| canCalculateAmountViolations && !isInvoiceTransaction && typeof categoryOverLimit === 'number' && expenseAmount > categoryOverLimit && isControlPolicy; | ||
| canCalculateAmountViolations && !isInvoiceTransaction && typeof categoryOverLimit === 'number' && amountForLimitCheck > categoryOverLimit && isControlPolicy; |
There was a problem hiding this comment.
Per-night averaging also applies to the workspace-level Max expense amount rule (shouldShowOverLimitViolation uses amountForLimitCheck), but only the category copy mentions it. I think we should add the same sentence to maxExpenseAmountDescription?
There was a problem hiding this comment.
The explicit requirement was to update just the category copy.
- Admin sets a "Flag amounts over" amount on the Hotel (or Car Rental) category and selects "Individual expense" as the limit type.
- If SmartScan detects reservationStartDate and reservationEndDate on a receipt in that category, we calculate the per-night average (amount / nights) and use that instead of the total expense amount when checking against the threshold.
- If no reservation dates are detected, the limit type behaves as it does today - flagging based on the full expense amount.
- We'll update the explainer text under "Individual expense" to note that multi-day reservations are evaluated using the per-night average rather than the total.
@sakluger can provide some more insight for this.
There was a problem hiding this comment.
That's a really good question. I hadn't considered that this would apply to the workspace limit, I originally intended for it to only apply to the category limits. Let me check with the team.
There was a problem hiding this comment.
We should only apply the nightly rate to category limits, not the overall workspace max expense amount. We would expect the global max amount to always trigger for any expenses over that amount.
|
@Krishna2323 @akinwale @sakluger added a comment in Slack regarding this.
I’d expect the category-level max expense amount to override the general max whenever a specific limit is set for that category. Otherwise, there’s not much value in defining a category-specific maximum if it only takes effect when it’s lower than the general limit. Especially if this matches how the product currently behaves, I’d keep it as is. |
|
I agree with @flaviadefaria. We shouldn't change the logic for when to use a category limit vs the overall limit. I assume that today, any defined category limit overrides the workspace limit. |
|
Cool, so I think we're good to move forward. @akinwale @Krishna2323, what are the next steps here? |
|
@akinwale, could you please resolve the conflicts and make the requested changes? |
| * Returns the number of nights covered by a SmartScanned reservation receipt, or 0 when the | ||
| * transaction has no usable reservation range. | ||
| */ | ||
| function getReservationNights(transaction: OnyxEntry<Transaction>): number { |
There was a problem hiding this comment.
Would this be better suited in TravelUtils libs?
There was a problem hiding this comment.
Not necessarily. It's more useful for determining if a particular transaction would be processed on a per-night amount basis for checking the transaction violations.
There was a problem hiding this comment.
Why these changes in actions files?
There was a problem hiding this comment.
This must be from an incorrect or incomplete merge. I'll restore the file from the main branch.
|
@akinwale friendly bump |
|
@MonilBhavsar I have merged with the latest main. All tests are now passing. |
MonilBhavsar
left a comment
There was a problem hiding this comment.
@akinwale , there are still changes in .github/actions/javascript/isDeployChecklistLocked/index.js and tests are failing. Please check
a61fea7 to
92ccc9e
Compare
|
@MonilBhavsar Restored the file from main. The only failing GH check I see is |
|
It would be really great if we could merge this by tomorrow at the latest. |
There was a problem hiding this comment.
Unlike every other locale, expenseSubtitle is unchanged here. Is it expected?
There was a problem hiding this comment.
I have regenerated the translations for the changed paths.
|
|
||
| // A SmartScanned multi-day reservation is measured against its average nightly rate rather than its total | ||
| const reservationNights = TransactionUtils.getReservationNights(updatedTransaction); | ||
| const amountForLimitCheck = reservationNights > 0 ? expenseAmount / reservationNights : expenseAmount; |
There was a problem hiding this comment.
Could this per-night calculation also affect Category total limits? For example, a $500 five-night hotel expense with a $300 daily category limit is checked as $100/night and would not be flagged. Should we restrict this behavior to the Individual expense limit type?
There was a problem hiding this comment.
Yes, we only want to apply the per-night limit to the "Individual expense" category limit type, not the "Category total" limit type. See https://github.com/Expensify/Expensify/issues/632262#issuecomment-4997562779.
|
I have a call on Monday with the customer who requested this. What are the chances that we can get it merged tomorrow or Monday? |
|
@MonilBhavsar @akinwale are we on track for merging this today? |
@MonilBhavsar I have updated the nightly limit check to @flaviadefaria Hopefully. |
|
🚧 MonilBhavsar has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.4.77-0 🚀
|
|
Yes — help site changes are required, and I've opened a draft PR for them: #101008 WhyThis PR changed how a workspace category's Flag amounts over cap is measured, and the help site described that setting as nothing more than "Set a spending cap for this category." Two things are now undocumented:
What I changed
Scope boundaries I read out of the diff, and how I verified the labelsFrom
I also documented the new violation string — Nightly rate over [amount]/person category limit — from Every label in the new copy was read off the live app on web rather than the source: the Category rules section header, the Flag amounts over row and page, the Amount field, the Type row, the Individual expense / Category total options and their verbatim subtitles, and Max expense amount under Expenses on the Rules page. Screenshots are in the docs PR body.
@akinwale, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR view run · no recording available |
|
@akinwale @Krishna2323 @garrettmknight Is applause.expensifail.com domain enabled for Advanced Duplicate Detection or do we have to provide accounts for the same? |
|
@akinwale @MonilBhavsar @Krishna2323 can you check the comment above please? |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.77-4 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Payment issue created: #101133 |
Explanation of Change
When a workspace category's Flag amounts over limit (limit type: Individual expense) is applied to an expense whose SmartScanned receipt has hotel reservation dates, the limit is now compared against the average nightly rate (total ÷ nights) rather than the full expense amount.
Expenses without reservation dates are unaffected, and the receipt-required and itemized-receipt-required thresholds continue to use the full amount. The overLimit and overCategoryLimit violations show new "Nightly rate over…" copy when the limit was applied per night, distinguished by the nights field the backend now returns.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/632262
PROPOSAL: https://github.com/Expensify/Expensify/issues/632262#issuecomment-5110547879
Tests
Prerequisites
$200)$300, Type:Individual expense).Test: Nightly rate under Category limit
Travel.Test: Nightly rate over Category limit
Travel.Offline tests
QA Steps
Same as test steps.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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