Show only Custom range (not Custom date) in Consolidated Travel Billing export - #97461
Show only Custom range (not Custom date) in Consolidated Travel Billing export#97461rlinoz wants to merge 1 commit into
Conversation
…ng export The export requires a closed date range, but the picker offered a single-select Custom date (On/After/Before). A lone After or Before could never satisfy the completion check, producing a 'Please select a date range to export' error. Add shouldShowCustomDate (default true) to DatePresetFilterBase/DateFilterBase and set it false on the travel invoicing export so only presets + Custom range are offered. Remove the now-unreachable After/Before validation paths.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@carlosmiceli 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| description={customDateDescription} | ||
| onPress={selectCustomDateMode} | ||
| /> | ||
| {shouldShowCustomDate && ( |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
The new shouldShowCustomDate boolean prop is a configuration flag whose sole purpose is to conditionally render the "Custom date" MenuItem ({shouldShowCustomDate && (<MenuItem ... />)}). This is the composition-over-configuration anti-pattern (Case 1/Case 2): a shouldShow* flag that toggles element visibility is a behavioral flag, and every new consumer cements the config-driven API. Adding more visibility toggles to this component grows its surface area and coupling over time.
Prefer expressing this as composition so consumers include/omit the element rather than toggling it via a prop. For example, let the caller decide whether the custom-date option is present by composing it in, instead of the component branching on a flag:
// Consumer that wants the option composes it in; consumer that doesn't simply omits it.
// The component renders its children/slots and never needs a shouldShowCustomDate branch.If a compositional refactor is out of scope here, at minimum keep this flag from proliferating — the presence/absence of the custom-date option should be driven by composition rather than an ever-growing set of shouldShow* booleans.
Reviewed at: 68b53d6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I think this is not worth a refactor, let me know if you disagree.
2 participants
Explanation of Change
The Consolidated Travel Billing export requires a closed date range (a concrete start and end date the backend can bound the statement to). The date picker, however, offered a single-select Custom date option (On / After / Before), where After and Before are mutually exclusive.
This changed recently when we added the "Custom range" option, so this PR adds a flag that hides the "Custom date"
Fixed Issues
$
PROPOSAL:
Tests
This month,Last month, andCustom range— there is noCustom dateoption.Custom range, pick afromand atodate, and save.Export to PDFand verify the export succeeds with noPlease select a date range to exporterror.Export to CSV.This month) instead of a custom range and verify export still succeeds.Offline tests
Same as Tests.
QA Steps
Same as Tests.
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