Implement confirmation message when toggling off approvals#72132
Conversation
Codecov Report❌ Patch coverage is
... and 49 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@shawnborton @Ollyws One of you needs to 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] |
|
Screenshots look good to me, let's get it into final review. Will also spin up a test build. |
|
🚧 @shawnborton 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, Desktop, and Web. Happy testing! 🧪🧪
|
|
Hmm I just tested this and I didn't get any kind of confirmation modal for a simple approval case on a Collect policy. It does seem to work for Control policies though where I have multiple approval workflows created. What is the expected behavior here? CleanShot.2025-10-09.at.16.31.52.mp4 |
@shawnborton yes, this is expected behaviour as OP in issue |
|
Ah okay, ignore me then. |
|
But cc @Expensify/design and @Expensify/product-pr for a second look at that, to make sure we're all aligned. I kinda think it would be more simple to just always throw the dialog when the user disables? |
I feel the same way. I can see how the most simple case doesn't really get deleted, but still, it'd be cleaner IMO to just always show it. |
|
I'm fine going that route, or doing nothing. I don't feel particularly strongly. Reason being, if you just have a default rule and you disable/enable, then nothing is "lost" and everything is like it was before. |
I think if you set someone other than the owner it would be lost if we're resetting workflows upon disabling. So maybe that's a reason to do it as it's a possibility. |
|
@nkdengineer can we change this so that we always show the confirmation modal as a default when the user disables the workflows? Decision was made here: https://expensify.slack.com/archives/C05LX9D6E07/p1760114132904579?thread_ts=1758231638.519859&cid=C05LX9D6E07 |
|
@carlosmiceli i updated cc @Ollyws |
Reviewer Checklist
Screenshots/Videos |
|
@nkdengineer Could you take a look at those conflicts, thanks. |
|
@Ollyws i updated |
|
Tests are failing. |
|
@carlosmiceli all good now |
|
All yours @shawnborton ! |
|
All good on my end - I normally don't merge these since I'm just a designer :) feel free to merge if you are good with it! |
|
✋ 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/carlosmiceli in version: 9.2.32-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.32-6 🚀
|






Explanation of Change
Fixed Issues
$ #71336
#72087
PROPOSAL: #71336 (comment)
Tests
Offline tests
Same as tests
QA Steps
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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop