Closed lakchote closed 1 week ago
🚧 @lakchote has triggered a test build. You can view the workflow run here.
@dannymcclain @alitoshmatov 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]
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/55740/index.html | https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/55740/index.html | |
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/55740/NewExpensify.dmg | https://55740.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
@trjExpensify @JmillsExpensify @dannymcclain you can test it out here 👆
Do we need to add a test to the QA steps to cover the existing submitted report case? I take it we want to handle this like any other approval workflow change after a report has already been submitted?
Existing submitted report
Approve
button on the submitted reportDo we need to add a test to the QA steps to cover the existing submitted report case? I take it we want to handle this like any other approval workflow change after a report has already been submitted?
Yes, good idea. I've added it to the QA steps.
🚧 @lakchote has triggered a test build. You can view the workflow run here.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/55740/index.html | ❌ FAILED ❌ | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/55740/NewExpensify.dmg | https://55740.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
🚧 @lakchote has triggered a test build. You can view the workflow run here.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/55740/index.html | https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/55740/index.html | |
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/55740/NewExpensify.dmg | https://55740.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
Added a QA step for the case where there is only the workspace owner in the workspace.
@trjExpensify @JmillsExpensify @dannymcclain feel free to test out the feature with the links just above
cc @Expensify/design as well ☝️
What happens if you have this enabled, but then you try to set up a new approval workflow where someone approves to themselves? Is that possible, and if so, do we throw an error?
What happens if you have this enabled, but then you try to set up a new approval workflow where someone approves to themselves? Is that possible, and if so, do we throw an error?
It's not a possible option. Self approvers are not shown anymore once the feature is enabled:
https://github.com/user-attachments/assets/c7774b76-aaa3-4c8d-b8c1-69d3a4fbb2c1
Awesome, that answers my question!
I feel like this is looking good! @Expensify/design any other thoughts?
All good on my end 👍
No notes from me 👍
Great! Thank you Design team 😄
@alitoshmatov can you review the PR Please?
Also @dannymcclain could you please approve the PR?
@lakchote When all members are removed from workspace while prevent self approval
is enabled what should be expected result? Right now it is still enabled.
Also when testing 3rd case from QA Tests following is happening:
Prevent self approval
which warns about removing current workflow with self approvalPrevent self approval
is enabledNow user B can see Approve
button but it is disabled
User A has no Approve
button, and sees Waiting for User B to approve expense(s).
. Now this particular request is stuck in this status until Prevent self approval
is disabled
edit: This is also happening when default approver submits a request and then owner enabled Prevent self-approval
see 2nd video
1-video
https://github.com/user-attachments/assets/ef9258ae-4651-479b-b960-ee3b2fd0d311
2-video
https://github.com/user-attachments/assets/fead6c6f-98b5-414f-9dc6-f8f2aae4fe7d
@lakchote When all members are removed from workspace while
prevent self approval
is enabled what should be expected result? Right now it is still enabled.
Fixed in https://github.com/Expensify/App/pull/55740/commits/0c2e7e02b8bdddbb97b6a16a66aefa6756380613
Also when testing 3rd case from QA Tests following is happening:
- User B has self approval workflow where it approves their own requests
- Make request as user B
- Approve button is visible
- As user A(owner) enable
Prevent self approval
which warns about removing current workflow with self approval- User A Confirms and
Prevent self approval
is enabledNow user B can see
Approve
button but it is disabled User A has noApprove
button, and seesWaiting for User B to approve expense(s).
. Now this particular request is stuck in this status untilPrevent self approval
is disabled
Speaking of what @alitoshmatov reported, the workflow mentioned here @trjExpensify is not what is currently happening now with the "Prevent Self Approvals" feature. However, this is preexisting from this PR - this PR aimed to introduce the modal logic that is.
Here is what it'll look like for user A (policy owner) after enabling "Prevent self approvals":
As you can see the next steps are wrong...
Here is what it'll look like for user B (admin that was previously self-approving):
As you can see it has not the correct behavior you did expect (he can't approve the report to be automatically forwarded to policy's owner)?
It seems like there's a business logic/implementation problem, next steps aren't handled properly.
cc @BrtqKr since you've authored the Rules migration logic in this PR
Speaking of what @alitoshmatov reported, the workflow mentioned https://github.com/Expensify/App/pull/55740#issuecomment-2615642023 @trjExpensify is not what is currently happening now with the "Prevent Self Approvals" feature
Is that not how OldDot handles the same scenario for submitted reports when the prevent self approval setting is enabled?
Speaking of what @alitoshmatov reported, the workflow mentioned #55740 (comment) @trjExpensify is not what is currently happening now with the "Prevent Self Approvals" feature
Is that not how OldDot handles the same scenario for submitted reports when the prevent self approval setting is enabled?
Next steps will be solved in Auth as per Jason's comment here. The scope of this issue should limit itself to the modal that shows whenever trigger conditions for "Prevent Self Approvals" are there.
@lakchote Can you resolve the conflicts
The changes look good.
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. for onClick={this.submit}
the method this.submit
should be bound to this
in the constructor)this
are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this);
if this.submit
is never passed to a component event handler like onClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
is working as expected in all cases)Design
label and/or tagged @Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test
steps.@marioexpensify 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]
Nice!! I'll review this later today / early tomorrow. Thank you 😄
@MarioExpensify if you could review this today that'd be great!
@lakchote Yep, I'm on it right now. Can you please merge main? It seems there are conflicts 😞
The conflicts are going to be fun to handle. I'll report back when you can merge 😅
@MarioExpensify ready for review again!
:hand: 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/MarioExpensify in version: 9.0.96-0 🚀
platform | result |
---|---|
🤖 android 🤖 | success ✅ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
🤖🔄 android HybridApp 🤖🔄 | success ✅ |
🍎🔄 iOS HybridApp 🍎🔄 | success ✅ |
This PR is failing because of issue #56644, https://github.com/Expensify/App/issues/56645
This PR is failing because of issue #56644, #56645
I'll be handling these two issues.
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.96-1 🚀
platform | result |
---|---|
🤖 android 🤖 | true ❌ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
🤖🔄 android HybridApp 🤖🔄 | failure ❌ |
🍎🔄 iOS HybridApp 🍎🔄 | failure ❌ |
Explanation of Change
Implement the "Prevent Self-Approvals" mechanism in NewDot.
Video test
https://github.com/user-attachments/assets/b59b49c7-9685-421b-a9f3-ac0ffc3d95d7
Fixed Issues
$ https://github.com/Expensify/App/issues/53799
Tests
Prerequisites:
1. Basic Flow (No self approvers)
Expected result: The toggle should turn on without any modal or warnings (because no one is self-approving in this scenario).
2. Verify Modal Trigger When a Member is Their Own Approver
Expected result: a modal appears because User B is self-approving. This modal says “Any members currently approving their own expenses will be removed and replaced with the default approver for this workspace ({defaultWorkspaceOwnerEmail}).".
{defaultWorkspaceOwnerEmail}
should be replaced by the default workspace approver/owner.Expected results: The workflow where user B was self approving should have been removed. The workflow that remains should be the default one where all workspace members submits to the workspace approver.
3. Workspace with several members become a workspace with only one member
Prerequisite: having a workspace with
Prevent self approvals
enabledPrevent self approvals
got disabledPrevent self approvals
got disabled4. Check we can't enable the feature if there's only one user (the workspace owner)
5. Check Spanish Translations
“Any members currently approving their own expenses will be removed and replaced with the default approver for this workspace ({defaultWorkspaceOwnerEmail})."
Offline tests
Settings > Troubleshoot > Force offline
QA Steps
Same as in tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
https://github.com/user-attachments/assets/b59b49c7-9685-421b-a9f3-ac0ffc3d95d7MacOS: Desktop