Expensify
Prevent Self-Approval: Fix Unexpected Behavior for Historical Workspaces and Update Existing Approvers
cc @JmillsExpensify coming from our DM
While we have the ability to set "Prevent self approval" on a workspace. There are some ways in which this feature works unexpectedly in NewDot. Specifically, the following scenario is possible and should not be:
Historical workspace behavior
Problem
- User has historical workspaces with themselves already set as the approver.
- Self approval is then enabled.
- What should we do after that? They should not be able to approve their own expenses, but we need some way to "fix" the reports currently submitted to themselves.
Solution
- Create a modal that warns the user that any users who are currently approving their own reports will be removed as the manager.
- Set the approver back to the "default approver" as shown here:
Triggered auto assignment to @dannymcclain (Design), see these Stack Overflow questions for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @VictoriaExpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.
Specifically we need design help on:
Create a modal that warns the user that any users who are currently approving their own reports will be removed as the manager.
QA steps would be something like:
- As User A enable Rules on a workspace
- Invite User B
- In Workflows create a rule for User B that has them set as their own approver
- In Rules select the Prevent Self Approvals option
- Modal is triggered since now User B can't be their own approver. We will be setting their approver to the default approver if the modal is "confirmed".
Ok so I think we have two options here—we can go classic confirmation dialog or use one of our modals with the illustration.
Confirmation dialog
Illustrated modal
@Expensify/design any preference here? I feel like the regular confirmation dialog feels most appropriate for this, right?
[!NOTE] Need help with the copy here
Yeah, I tend to agree - just a simple, matter-of-fact dialog seems appropriate.
And actually, I just realized I did this one wrong. I think it should actually be like this:
Where we give you the warning and the choice. @shawnborton @dubielzyk-expensify, the question is, should this button be danger?
Hmm I could go either way honestly. What do you think? It doesn't feel quite serious enough to warrant a danger button but I can see why the logic would check out to do so if that's what we decide.
Yeah I feel the same way. Let's make @dubielzyk-expensify weigh in! :gavel:
Yeah, I'm with ya. Don't feel strongly. Danger button seems perhaps a bit too harsh for what it is. cc @jamesdeanexpensify for copy!?
I think we can go with the green button for this one—even if some workflows are reset to the default approver, no information or anything is actually being destroyed. Nothing is going to stop working. So I agree with Jon the the danger button feels too harsh.
Slight tweaks!
Prevent self-approvals? Any members currently approving their own expenses will be replaced with the default approver for this workspace (email/phone).
👋 I think I have a few questions about this. But maybe to start, it would be helpful if someone could list out the numbered steps of the proposed solution in full? Are the QA steps here the extent of the proposal, or is there more to it?
Thanks!
Also I have a question: How are we handling the case where someone enables this feature, they submit reports, and they are the default approver for the workspace? I would imagine we need to create a special approval workflow for that case, right? Asking because I ran across this case in a #migrate thread.
Any update on this one? Do we have clear next steps to move this along?
Btw, we need to find someone to pick this now that Marc is on leave.
Still need to find a volunteer. This is being highlighted as a hot pick in the weekly update.
I have a few questions regarding this:
What happens to open or submitted reports if the user was self-approving them?
- does toggling Prevent Self Approvals instantly re-route them to the default approver?
What if no valid manager can be set (e.g., the user is alone in the workspace/no other manager exists OR the user is the default workspace owner)?
- should we block enabling Prevent Self Approvals entirely?
cc @JmillsExpensify @trjExpensify
does toggling Prevent Self Approvals instantly re-route them to the default approver?
Good question. I think the "ideal" is that we'd re-route them, though that's also not very consistent with how we handle changes platform-wide. So as a result, I think the most consistent thing is to prevent self-approvals moving forward.
should we block enabling Prevent Self Approvals entirely?
Yes, I think we should. We're similarly locking features on this same page when someone tries to enable something that isn't possible (or that will error out).
should we block enabling Prevent Self Approvals entirely?
Yes, I think we should. We're similarly locking features on this same page when someone tries to enable something that isn't possible (or that will error out).
@JmillsExpensify
I've tried locking the setting if the user that wanted to prevent self approvals, was the workspace owner himself. However I decided to not purse that option. Why? Because it'd mean everyone would need to have a specific additional workflow where the workspace owner submits to another admin. Let me know if that's what we want though! I'll adjust accordingly.
Took the discussion to Slack quickly to make sure we're on the same page.
@lakchote I think we resolved this discussion/question in Slack, is that right?
@lakchote I think we resolved this discussion/question in Slack, is that right?
Yes, we resolved that discussion in Slack. However, @trjExpensify suggested here that we do not lock the option as we don't currently do it in OldDot. Should we get rid of this or not?
Currently, another problem we have is that the next steps are not calculated properly on NewDot, when prevent self approvals is enabled. On the self approver side, it's computed correctly. However, on the policy's owner side, it says we're waiting on the previous self approver to approve.
I feel like locking is a front-end paradigm, and we've experienced an issue in practice where locking would have prevented this issue and related support volume. So I'm in favor of moving forward with it.
Currently, another problem we have is that the next steps are not calculated properly on NewDot, when prevent self approvals is enabled. On the self approver side, it's computed correctly. However, on the policy's owner side, it says we're waiting on the previous self approver to approve.
My opinion on this one is that it's a separate problem and we are working on solving it as part of the next steps in Auth project.
I feel like locking is a front-end paradigm, and we've experienced an issue in practice where locking would have prevented this issue and prevented a support volume. So I'm for moving forward with it.
Should we prevent it from being enabled on OldDot as well then, so the two settings are aligned? Otherwise you can enable this on OldDot and it'll enable it on NewDot when it should otherwise be "locked" with one member? 🤔
I feel like locking is a front-end paradigm, and we've experienced an issue in practice where locking would have prevented this issue and prevented a support volume. So I'm for moving forward with it.
Should we prevent it from being enabled on OldDot as well then, so the two settings are aligned? Otherwise you can enable this on OldDot and it'll enable it on NewDot when it should otherwise be "locked" with one member? 🤔
I can raise another PR to do this, and make things consistent.