Repository navigation
[Payment due @TaduJR] Prevent over-limit approvers who already approve in the workflow - #105257
Conversation
This comment has been minimized.
This comment has been minimized.
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. |
|
Do you need review here @Gonals |
Yes, please |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Good for product
Reviewer Checklist
Screenshots/VideosiOS: HybridAppiOS: mWeb Safari |
|
@TaduJR, while it is ok to get AI-help during reviews, we already have AI reviewers. The idea is to have a human reviewer beyond it. After all, the human is responsible for the review. I can't imagine you can confirm whether those translations are more or less accurate than the previous ones :/ |
Yes We do have Melvin, and ofc the transition are checked via ChatGPT. You know we do use to verify using GPT and there were even a checkbox that you should verify translations that why I used AI for the translation But beyond the translation the code comments are real bugs and nits I caught during review and even reproduced the parent body comments. |
1 addressed. 2 is fine, not a problem (no workflows currently set with that case The overall block is on purpose after discussing it with the team and figuring out what our customers have |
|
Resolved most of the comments, since I'm only addressing point 1. |
|
@MelvinBot review |
|
I found no blocking issues. The save check and both picker filters agree. The beta gate leaves legacy workflows unchanged. CI is green except the independent-approval check. I have two non-blocking notes:
|
|
Thanks @Gonals, great work.
I reproduced point 2 with a real workflow: one saved with the current app, where B's additional approver C is also an approver, can't be saved with this PR, not even for a members-only edit. The error only shows after Save, and beta admins can still create these workflows until this ships. Since this is intended, maybe you could add a QA step for this case? Sorry about the JaimeGPT translation suggestions, I'll leave those to the Polyglot Parrot in the future. The code side looks good to me. |
What I mean is that there are no real users with this setup. It is not a common setup and we released the beta yesterday, so this is not a real problem |
|
@marcochavezf 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] |
|
🎯 @TaduJR, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 marcochavezf 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/marcochavezf in version: 9.5.27-0 🚀
|
|
No help site update needed. Every behavior change here applies only when the |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.27-2 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Payment issue created: #105663 |



Explanation of Change
Usually, if an approver is in the approval workflow already, we don't show it as an option in other steps. This is not true for over-limit approvers. Let's make it true.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/696896
PROPOSAL:
Tests
Setup: a Control workspace with Approvals turned on, members A, B, C, D and E, and the
multipleApproversbeta enabled.Go to Workflows and tap Add approval workflow. In Expenses from, choose D. Choose A as the approver and tap Skip on the approval limit page.
Add B as the second approver. On B's approval limit page, enter 50 in Report amount and tap Additional approver.
Verify that A and B are not in the list and E is.
Choose E and tap Next. Add C as the third approver, tap Skip on its approval limit page, then tap Add workflow.
Verify that the workflow is saved and B shows "Reports above $50.00 forward to E".
Offline tests
Same
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