72031 fix required description by category#73725
Conversation
|
@ZhenjaHorbach 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] |
|
I’ve finished implementing the ticket ‘The Description field should display a “Required” label or indicator.’ Should the description field also show an error when submitting a transaction? I didn’t see this in the expected result, but I double-checked and tested it locally. Screen.Recording.2025-10-29.at.13.17.38.mov |
…description-by-category
Codecov Report❌ Patch coverage is
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@ZhenjaHorbach #73725 (comment) Please take a look at this comment when you get a chance |
#73882 It seems we have another ticket to handle this |
…description-by-category
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2025-11-03.15.40.51.movAndroid: mWeb Chrome2025-11-03.15.40.51.moviOS: HybridApp2025-11-03.15.44.11.moviOS: mWeb Safari2025-11-03.15.44.11.movMacOS: Chrome / Safari2025-11-03.15.54.14.movMacOS: Desktop2025-11-03.15.54.14.mov |
Technically, categories are also required |
|
@ZhenjaHorbach Thanks! This PR is ready for review now |
|
LGTM! |
neil-marcellini
left a comment
There was a problem hiding this comment.
Looks good, thank you.
| iouIsReimbursable, | ||
| onToggleReimbursable, | ||
| isReceiptEditable = false, | ||
| isDescriptionRequired = false, |
There was a problem hiding this comment.
NAB: Can we set up this flag within this components instead of taking a prop? Not a blocker because I assume that required adding a lot of additional useOnyx calls to this file.
| const isSavedRef = useRef(false); | ||
| useRestartOnReceiptFailure(transaction, reportID, iouType, action); | ||
|
|
||
| const {policyForMovingExpensesID} = usePolicyForMovingExpenses(); |
There was a problem hiding this comment.
What is this policy for moving expenses thing? Why don't we use the policy directly?
There was a problem hiding this comment.
NAB: Oh I see, it's kind of a fall back. Still, I would like to learn more about why this is needed.
| it('returns true when category has areCommentsRequired set to true regardless of enabled status', () => { | ||
| expect(isCategoryDescriptionRequired(mockPolicyCategories, 'Office', true)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
NAB: The name of this test makes it sound like areRulesEnabled should be false, but it's true.
|
✋ 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/neil-marcellini in version: 9.2.44-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.44-5 🚀
|
| if (isDescriptionRequired && !values.moneyRequestComment) { | ||
| addErrorMessage(errors, INPUT_IDS.MONEY_REQUEST_COMMENT, translate('common.error.fieldRequired')); | ||
| } |
There was a problem hiding this comment.
We should not block submission without required description to align with required category/tag cases. Only block when missing merchant. This was also C+ concern here #73725 (comment) that was not resolved.
See Slack discussion in this issue #88707 for details.
Explanation of Change
Fixed Issues
$ #72031
PROPOSAL: #72031 (comment)
Tests
Same QA step
Offline tests
QA Steps
Prerequisites:
A category is configured with:
The workspace has Categories enabled.
The workspace has Rules enabled.
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: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2025-10-29.at.12.39.22.mov
MacOS: Desktop
Screen.Recording.2025-10-29.at.12.44.10.mov