[NewFeature] Rework: Add ability to require attendees based on category selection#78315
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 |
bedf24c to
106c7da
Compare
|
|
106c7da to
1bc3f55
Compare
1bc3f55 to
23d97e3
Compare
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.
|
|
@jayeshmangwani 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] |
|
@ikevin127 Based on this code, Attendee Tracking defaults to true on Control policies when isAttendeeTrackingEnabled attribute is missing, so we may need to reuse that logic on our PR. App/src/pages/workspace/rules/IndividualExpenseRulesSection.tsx Lines 181 to 183 in 7c2f739 |
…7-categoryRequireAttendeesReopen
|
@jayeshmangwani Makes sense - updated ✅ |
|
|
…7-categoryRequireAttendeesReopen
@jayeshmangwani ✅ This was addressed by using data we have on FE side before actually opening the report (BE would return the data if this is done first), then rendering the error in Reports > Expenses based on that. I added the issue / test in PR description for verification purposes. ☝️ Noting that a FE fix was suggested here by an engineer, even though my take was that this could probably be handled on BE side when Reports > Expenses page is opened and search API endpoint is called. 🟢 Ready for review! |
|
✅ Merged main to resolve conflict. @JS00001 Before merging please refer to this comment to make sure we're aligned for a specific case when it comes to FE to BE expectations - thank you! |
@ikevin127 Is this saying that the BE should create policies with the value for attendee tracking required as true/enabled by default? |
|
Not quite - according to this code comment referenced here there are cases in which, as the comment says:
but this does not make sense to do on the FE side because:
I would assume that if a WS was created in OD (scenario 2), then user logged in on ND, the variable would already be toggled ON if it was previously toggled ON in OD. 👉 This is what I mean by 'this should be handled on BE side' - FE shouldn't be tasked with keeping track of this variable across OD / ND in case variables differ - they should be unified and already be ON if it was turned ON before. |
|
@ikevin127 And this issue is currently still causing the bugs from above? |
|
@JS00001 No - everything works well now since we removed that FE-only fallback to 🟢 This is ready for merge! |
…7-categoryRequireAttendeesReopen
|
I dont think we should add |
…7-categoryRequireAttendeesReopen
|
@JS00001 Changes applied - should be ready to merge! 🟢 Commit 👇 below was pushed for a typecheck issue - still ready to merge 🙌 |
|
✋ 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.2.95-0 🚀
|
|
This PR failing because of an issue |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.95-5 🚀
|
Explanation of Change
We’re expanding support for required category fields and coding. Specifically, we’ll add more category-based rules within each NewDot category. For example: “If the category is Meals & Entertainment, then require the Attendees field.”
Scope:
Description hintfield more discoverable: Specifically, we'll move Description hint field below Payroll code and above Delete categoryRequire fieldspush row: This new row contains options for bothRequire descriptionandRequire attendees, like soAttendees required for this category.👁️ See issue OP / screenshots for more details / UI representation.
Fixed Issues
$ #76155
PROPOSAL:
Tests
Preconditions:
I. Enable Attendees Required for Category
Require description/Require attendeesRequire attendees.II. Confirmation Page Validation
III. Existing Expense Violation Display
🧪 Tests from initial PR ❌ regressions
I. Expense - App crash after deleting category in the list #78076
II. Expense - "Multiple attendees required for this category" appears when splitting expense offline #78132
Preconditions:
III. Reports - "Multiple attendees required for this category" does not appear on expense row #78040
Preconditions:
Advertisingcategory.Require fields.Require attendees.Offline tests
I. Offline Toggle
II. Offline Expense Creation
III. Offline Category Change
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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: HybridApp
android-hybrid.mp4
Android: mWeb Chrome
android-mweb.mp4
iOS: HybridApp
ios-hybrid.mov
iOS: mWeb Safari
ios-mweb.mov
MacOS: Chrome / Safari
web.mov