fix: Make Reject / Hold Messaging More Explicit#81806
fix: Make Reject / Hold Messaging More Explicit#81806srikarparsi merged 27 commits intoExpensify:mainfrom
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ab8d5139c
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f1aca27eb
ℹ️ 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".
|
@TaduJR Please don’t skip the AI review comments. For each suggestion, please react with 👍 or 👎. If you choose 👎, kindly explain why. |
|
@TaduJR Do you have any updates on the PR? Have you had a chance to review all the AI suggestions? |
…ject-Hold-Messaging-More-Explicit
…ject-Hold-Messaging-More-Explicit
…ject-Hold-Messaging-More-Explicit # Conflicts: # src/pages/Search/SearchHoldReasonPage.tsx
|
@TaduJR We have Prettier check / Prettier check (pull_request)Failing after 43s |
…ject-Hold-Messaging-More-Explicit
|
@TaduJR We still have an error: |
It was coming from main |
…ject-Hold-Messaging-More-Explicit
|
Hi @srikarparsi What latest here? Thanks! |
|
@MelvinBot can you please review this PR |
Code ReviewI've reviewed the changes across all 15 files. Overall this PR is well-structured and the approach is sound. All CI checks are passing. What this PR does
Review findingsLogic correctness - The submitter detection logic is sound in both contexts:
Minor observation - In Tests - Good coverage in Previous review threads - The Search.ts PERF-11 changes were reverted per No blocking issues found. The PR looks ready. |
|
🚧 @srikarparsi 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/srikarparsi in version: 9.3.32-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.32-3 🚀
|
Explanation of Change
The hold and reject reason form copy was generic and didn't clearly communicate the intent of each action. This change makes the messaging role-aware.
Hold: Since both submitters and approvers can hold expenses, the form now shows different copy depending on who is holding:
Reject: Only approvers can reject, so the copy is updated directly:
The role is determined using the existing
isCurrentUserSubmitter()utility, following the same pattern already used by the educational modals inMoneyRequestHeader.tsxwhich show different modals to submitters vs approvers.Fixed Issues
$ #81216
PROPOSAL: #81216 (comment)
Tests
Hold - Approver flow:
Hold - Submitter flow:
Reject - Approver flow:
Bulk hold from Search - Approver:
Bulk hold from Search - Submitter:
1:1 request - Payer hold:
1:1 request - Requester hold:
Bulk hold from Search - Mixed (submitter + approver):
Offline tests
Same as tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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.https://expensify.slack.com/archives/C01GTK53T8Q/p1771440128761189
Screenshots/Videos
MacOS: Chrome / Safari