Conversation
…ibility For Classic-created Control policies, isAttendeeTrackingEnabled is undefined since the field didn't exist historically. This should default to true (enabled) to maintain backwards compatibility, but was incorrectly defaulting to false in most locations. Changes: - Update all 7 ?? false occurrences to ?? true - Add ?? true at raw call sites passing to AttendeeUtils functions - Fix raw truthiness checks in ExpenseReportListItem, CategorySettingsPage - Add special case in isPolicyFeatureEnabled for IS_ATTENDEE_TRACKING_ENABLED Co-authored-by: Francois Laithier <francoisl@users.noreply.github.com>
|
@MelvinBot how about we add a function isAttendeeTrackingEnabled(policy: OnyxEntry<Policy>): boolean {
return isControlPolicy(policy) && policy?.isAttendeeTrackingEnabled ?? true;
}function in |
Co-authored-by: francoisl <francoisl@users.noreply.github.com> Co-authored-by: Francois Laithier <francoisl@users.noreply.github.com>
|
Great idea! I've added an function isAttendeeTrackingEnabled(policy: OnyxEntry<Policy>): boolean {
return (isControlPolicy(policy) && policy?.isAttendeeTrackingEnabled) ?? true;
}This combines the Control policy check with the The one exception is Pushed in 829f18e. |
Co-authored-by: Francois Laithier <francoisl@users.noreply.github.com>
|
Fixed the failing Prettier check: ran prettier on 3 files with formatting diffs (import line wrapping in |
The isAttendeeTrackingEnabled function requires isControlPolicy() to be true (policy type CORPORATE). The tests were using TEAM type policies, so attendee tracking was always returning false regardless of the isAttendeeTrackingEnabled property value. Co-authored-by: Francois Laithier <francoisl@users.noreply.github.com>
|
Fixed the failing |
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.
|
francoisl
left a comment
francoisl
left a comment
There was a problem hiding this comment.
@MelvinBot please add the test I mentioned, and pull main into the branch as it's getting quite far behind.
| @@ -257,7 +257,7 @@ describe('useAdvancedSearchFilters', () => { | |||
|
|
|||
| describe('attendee filter visibility', () => { | |||
There was a problem hiding this comment.
@MelvinBot let's add a test to confirm that we show the attendee filter when the isAttendeeTrackingEnabled prop is absent
There was a problem hiding this comment.
Added a test that creates a Corporate policy without isAttendeeTrackingEnabled set (property explicitly deleted) and asserts the attendee filter is visible. All 18 tests pass.
…d is absent Co-authored-by: Francois Laithier <francoisl@users.noreply.github.com>
…rackingEnabledDefault
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@aimane-chnaif 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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84075f531b
ℹ️ 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".
| * on Control policies when the property is undefined. | ||
| */ | ||
| function isAttendeeTrackingEnabled(policy: OnyxEntry<Policy>): boolean { | ||
| return (isControlPolicy(policy) && policy?.isAttendeeTrackingEnabled) ?? true; |
There was a problem hiding this comment.
This change makes Classic control policies resolve isAttendeeTrackingEnabled to true when the field is missing, but the toggle action still snapshots the original value with !!policy?.isAttendeeTrackingEnabled in setPolicyAttendeeTrackingEnabled(). For a legacy policy (undefined flag), toggling OFF and then hitting an API failure will roll back to false instead of the effective default true, leaving the setting incorrectly disabled until a full refetch. Please ensure rollback uses the same resolver (or preserves undefined) as this new defaulting logic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| return; | ||
| } | ||
| if (key === CONST.SEARCH.SYNTAX_FILTER_KEYS.ATTENDEE && !(policyDerived?.isAttendeeTrackingEnabled ?? false)) { | ||
| if (key === CONST.SEARCH.SYNTAX_FILTER_KEYS.ATTENDEE && !(policyDerived?.isAttendeeTrackingEnabled ?? true)) { |
There was a problem hiding this comment.
Changing this fallback to ?? true causes the ATTENDEE filter to be shown whenever policyDerived is temporarily undefined (e.g., initial Onyx load), unlike other feature-gated filters that default to hidden. That can surface an attendee filter in contexts where policy capabilities are not yet known and produce incorrect/flickering filter UI; the Classic compatibility behavior is already handled by isPolicyFeatureEnabled() once policy data is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hm maybe. You could also argue the opposite that we're not showing the attendee selector when we should. Not going to block on this.
|
Not sure why MelvinBot decided to assign 2 C+s here, @aimane-chnaif hopefully you haven't started reviewing yet. I'll remove you since you were assigned second. |
|
@suneox are you able to start reviewing this today you think? |
Yes, I lost focus on this one and I will take a look at this one in few hours |
|
Based on my analysis of latest Affected Areas
Test Matrix
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppCleanShot.2026-04-07.at.03.04.23.1.mp4Android: mWeb ChromeCleanShot.2026-04-07.at.03.06.47.2.mp4iOS: HybridAppCleanShot.2026-04-07.at.03.07.43.mp4iOS: mWeb SafariCleanShot.2026-04-07.at.03.07.43.3.mp4MacOS: Chrome / SafariCleanShot.2026-04-07.at.02.43.38.4.mp4CleanShot.2026-04-07.at.02.32.15.3.mp4CleanShot.2026-04-07.at.02.30.58.2.mp4CleanShot.2026-04-07.at.02.18.49.1.mp4 |
suneox
left a comment
suneox
left a comment
There was a problem hiding this comment.
The current change set LGTM, and the functionality still works as expected.
melvin-bot
bot
changed the title
|
🎯 @suneox, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
| * on Control policies when the property is undefined. | ||
| */ | ||
| function isAttendeeTrackingEnabled(policy: OnyxEntry<Policy>): boolean { | ||
| return (isControlPolicy(policy) && policy?.isAttendeeTrackingEnabled) ?? true; |
There was a problem hiding this comment.
| return; | ||
| } | ||
| if (key === CONST.SEARCH.SYNTAX_FILTER_KEYS.ATTENDEE && !(policyDerived?.isAttendeeTrackingEnabled ?? false)) { | ||
| if (key === CONST.SEARCH.SYNTAX_FILTER_KEYS.ATTENDEE && !(policyDerived?.isAttendeeTrackingEnabled ?? true)) { |
There was a problem hiding this comment.
Hm maybe. You could also argue the opposite that we're not showing the attendee selector when we should. Not going to block on this.
|
🚧 @francoisl 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/francoisl in version: 9.3.58-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. The change corrects the internal default value of
None of these articles describe the default on/off state of the Attendee Tracking toggle, so the default value change creates no inaccuracy in the docs. |
4 participants
Explanation of Change
For backwards compatibility with Expensify Classic,
isAttendeeTrackingEnabledshould default totruewhen the property isundefinedon a policy. Classic-created Control policies don't have this field set, but attendee tracking was historically always enabled. The incorrect?? falsedefault caused two bugs:Changes:
?? falsenullish coalescing fallbacks to?? trueacross the codebase?? trueat call sites that passpolicy?.isAttendeeTrackingEnabledtoAttendeeUtilsfunctions (MoneyRequestConfirmationList, MoneyRequestView, CategorySettingsPage) — the function parameter defaults inAttendeeUtils.tsare intentionally not changed, since attendee tracking is restricted to Control policies and callers should explicitly pass the resolved valueundefinedasfalse(ExpenseReportListItem guard, CategorySettingsPage pending action check)isPolicyFeatureEnabled()forIS_ATTENDEE_TRACKING_ENABLEDto returntruewhen the field isundefined, ensuring theuseAdvancedSearchFiltersselector and any other consumer of this utility correctly defaultsThis is safe because violations only fire when a category has
areAttendeesRequired=true(requires explicit admin action), so policies without required attendees categories will see no behavior change.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/617341
Tests
isAttendeeTrackingEnabledis not set on the policy object)Offline tests
N/A — this change only affects default values for a policy property. Offline behavior is unchanged since the fallback logic is applied at render time from cached policy data.
QA Steps
isAttendeeTrackingEnabledNVP is not set)PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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: Native
N/A — no UI changes, only default value logic
Android: mWeb Chrome
N/A — no UI changes, only default value logic
iOS: Native
N/A — no UI changes, only default value logic
iOS: mWeb Safari
N/A — no UI changes, only default value logic
MacOS: Chrome / Safari
N/A — no UI changes, only default value logic