Repository navigation
Show Per Diem Download CSV option before rates are added - #104252
Conversation
…n grab the import template Co-authored-by: Eskalifer1 <[email protected]>
|
I created this PR from an upstream branch because I don't have push access to your fork. To take ownership of this branch and push updates, run: Then you can close this PR and open a new one from your fork. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8af1e0fe71
ℹ️ 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".
| if (hasVisibleSubRates || canWritePerDiem) { | ||
| menuItems.push({ |
There was a problem hiding this comment.
Gate the empty-template action on backend support
When an admin has no Per Diem rates, this new condition exposes the download action, but downloadPerDiemCSV() calls ExportPerDiemCSV, which currently rejects that exact case with “No rates found to be exported.” Consequently, the newly enabled action always ends in the download-failed modal and cannot provide the promised import template. This change should be coordinated with the backend support or otherwise generate/provide a valid empty template.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot What do you think about that?
Seems like we are wating on BE PR to be deployed: #104175 (comment)
Should we make any action here?
There was a problem hiding this comment.
No code change is needed. The Codex finding is valid, but the backend PR already fixes it, so the frontend should not work around it.
I recommend these steps:
- Keep this PR in draft, or mark it on hold, until the backend PR deploys. If it merges first, admins on empty workspaces get the "Download failed" modal.
- After the backend PR deploys, retest on a workspace with no rates. Clicking "Download CSV" should download a header-only CSV.
- Fill in the Tests and QA sections, then mark the PR ready for review.
Building the template in the frontend would duplicate the backend's CSV format and could drift from it, so I don't recommend it.
|
The failing checks are unrelated to this PR, so I didn't push a fix.
Next step: Once someone fixes EvidenceThe Jest failures are two |
|
I rechecked the checks and still didn't push a fix. The failures come from
Next step: After |
|
I checked again and didn't push a fix. Every failing check still comes from
Merging |
DetailsExplanation of ChangeThe "Download CSV" option in the Per Diem page's "More" menu was only shown once the workspace had at least one rate. Admins use that download to get the import template with the right headers, so they need it before adding rates (Classic already allows this). This PR shows "Download CSV" when the workspace has rates or the user can edit Per Diem ( Fixed Issues$ #104175 Tests
Offline tests
QA StepsSame as tests
PR Author Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / Safari@MelvinBot please set the PR body to the content in the details section above |
|
I didn't push a fix. Every failing check still comes from
A fix for |
|
🤖 Done. I replaced the PR body with the content inside your The new body drops the earlier note that the backend currently returns "No rates found to be exported." for workspaces with no rates. Add it back if you want reviewers to expect the "Download failed" modal. |
|
@MelvinBot merge main |
…dCsvBeforeRates Co-authored-by: Eskalifer1 <[email protected]>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@MelvinBot Let's add precondition to Tests that we should use staging API(not production) |
|
I added this precondition to the top of the
I left |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp104252-android-native.movAndroid: mWeb Chrome104252-android-web.moviOS: HybridApp104252-ios-native.moviOS: mWeb Safari104252-ios-web.movMacOS: Chrome / Safari104252-web.mov |
|
@codex review |
|
@MelvinBot Mark this PR as ready for review |
|
I marked this PR as ready for review. |
|
@Eskalifer1 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] |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 JS00001 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.5.16-0 🚀
|
|
No help site update is needed. This PR only makes Download CSV appear for admins before any rates exist, and no help article says the option requires existing rates. DetailsThe closest article is Configure Per Diem in a workspace. It says "Download CSV – Export your current Per Diem rate list." That sentence is still accurate. |
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.5.18-0 🚀
|
|
No help site update needed. This PR only makes Download CSV appear for admins before any Per Diem rates exist, and no sentence in Configure-Per-Diem-in-a-workspace.md says the option is hidden until rates are added, so the article is still accurate. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.18-1 🚀
Bundle Size Analysis (Sentry): |





Explanation of Change
The "Download CSV" option in the Per Diem page's "More" menu was only shown once the workspace had at least one rate. Admins use that download to get the import template with the right headers, so they need it before adding rates (Classic already allows this).
This PR shows "Download CSV" when the workspace has rates or the user can edit Per Diem (
canWritePerDiem). Read-only members keep the old behavior, since an empty template isn't useful to someone who can't import.Fixed Issues
$ #104175
PROPOSAL: #104175 (comment)
Tests
Precondition: Use the staging API (not production).
Per diem(Workspace should not have per diem rates - just enabled section)Offline tests
Per diem(Workspace should not have per diem rates - just enabled section)QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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