Repository navigation
[Merge ATS] Add approval pages - #101105
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 bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@DylanDylann 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] |
|
Found 2 bugs: After unsetting the Advanced mode final approver, the other policy API will set it back
final.mp4Can't set the first approver to Recruiting coordinator; the API says it's invalid invalidcoordinator.mp4 |
| approvalModeWarningTitle: 'Change approval mode?', | ||
| approvalModeWarningPrompt: (providerName: string, helpSiteURL: string) => | ||
| `Are you sure you would like to change the approval mode for this workspace? Learn more about the different ${providerName}-enabled workflow modes in our <a href="${helpSiteURL}">help site</a>.`, | ||
| approvalModeWarningConfirm: 'Change approval mode', |
There was a problem hiding this comment.
@yuwenmemon please review the copy and run the translation script if it looks good.
There was a problem hiding this comment.
For the English copy, I think @Expensify/design can also help review it
updateMergeApprovalMode only includes finalApprover in the request when it's not undefined:
Fix: send an explicit empty string when the mode has the field but it was cleared, instead of omitting the key. Pushed a fix for this.
The backend only accepts the literal value coordinator for the approver field — not recruitingCoordinator.
App's CONST.MERGE.ATS_APPROVER_FIELD.RECRUITING_COORDINATOR = 'recruitingCoordinator' (src/CONST/index.ts:3365). Backend only ever accepted 'coordinator' |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Core design doc issue
|
@ishpaul777 thanks! Fixed it. |
|
@DylanDylann Fixed z.mp4 |
Can you invite me to a test workspace I can try it out on? |
|
@dubielzyk-expensify invited you as an admin |
|
🚧 dubielzyk-expensify has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-09-23.at.16.03.50.movAndroid: mWeb ChromeScreen.Recording.2026-09-23.at.15.56.59.moviOS: HybridAppScreen.Recording.2026-09-23.at.16.04.18.moviOS: mWeb SafariScreen.Recording.2026-09-23.at.15.55.48.movMacOS: Chrome / SafariScreen.Recording.2026-09-23.at.15.53.27.mov |
That makes sense to me, I am down with that idea 👍 |
| if (connectionName === CONST.POLICY.CONNECTIONS.NAME.MERGE_ATS) { | ||
| parameters.approverField = approverField; | ||
| parameters.finalApprover = finalApprover; | ||
| } |
There was a problem hiding this comment.
Let's define additional parameters more clearly for each case, only setting them when needed, to avoid setting parameters to undefined/null values. This helps prevent unexpected bugs
There was a problem hiding this comment.
If the value is undefined, then the param is not passed to the API.
|
The rest looks good to me. @bernhardoj Let me know when you resolve the above issues. Thanks |
|
🚧 yuwenmemon has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/yuwenmemon in version: 9.4.93-0 🚀
|
|
Hi @bernhardoj. QA team faced with #101105 (comment) during testing this PR bandicam.2026-09-24.17-52-47-795.mp4 |
|
@IuliiaHerets hmm, I can't reproduce it. Do you connect to Greenhouse through the Onyx operation? I think the API needs a real connection. |
|
@bernhardoj No, QA team used a real connection |
|
Hmm, then there might be an issue with the connection cc: @yuwenmemon can you check it? |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.4.93-3 🚀
Bundle Size Analysis (Sentry): |
|
I checked the staging logs. The Greenhouse connection on workspace EF68DD18E7F90185 was disconnected at 14:51:51 UTC, shortly before this recording began. The UI was still showing a stale Greenhouse card, but the backend no longer had a merge_ats connection. Therefore, the Basic approval request at 14:53:19 UTC failed with InsufficientPrivileges. |


Explanation of Change
Fixed Issues
$ #99513
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: have a workspace with recruiting enabled, Greenhouse connected and setup completed
Basic approval • [approver_display_name]Advanced approval • [first_approver] -> [approver_display_name]Custom approvalPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
web.mp4