-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable Submit/Approve buttons when self approving is disabled and user is trying to self approve #37348
Conversation
…ove your own reports
@alitoshmatov 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] |
I marked the PR as ready for review but it's still a working in progress (mostly done at this point) as I wanted to go over a few things. We're dealing with two separate issues here (when self-approval is disabled):
As for 1., it's is straight forward and it's done. As per my proposal I added a isAllowedToSubmitDraftExpenseReport and then disabled the Submit button here, here and here. Now as for 2., I wanted to clarify a few assumptions that I made:
The PR is pretty much done based on the assumptions above. Let me know your thoughts on this and I'll do any final modifications needed and push the final version. |
I think we should just handle disabling approve button |
So should we just disable the submit AND approve buttons but keep the pay buttons intact? |
@kevinksullivan could we have your input on this please? Basically, in the context of self-approval, do we want to hide the pay buttons in the dropdown if the |
@lakchote I noticed that the Pay buttons are no longer displayed on staging when for reports that are not yet approved. Here's the PR that made the change. I'd say we no longer need to worry about this and we can focus again on disabling the Submit and Approve buttons accordingly (as @alitoshmatov had already suggested). I'll make the necessary changes to the PR to simplify it and remove all changes that deal with hiding payment buttons. As for disabling the Approve button, are we considering this part of this issue, or out of scope? If we should treat it as out of scope, then my comments/questions below won't really matter. If part of the issue, then: I'll update the logic to disable the
@alitoshmatov when you said this, did you mean you agree that we could get rid of |
I agree with you on this. |
I'd say yes @barros001. In BE, there's already checks that prevent self-approval actually. |
…ic data for self-approving
@alitoshmatov Ok, here's the final version of the PR. When disabling the "Approve" button, we have the either disable the entire SettlementButton or only the Approve item inside of the dropdown menu. On my tests the Approve button always showed by itself but following the code it is possible that the Approve button would show along with the Pay options. IMO it would not make sense to allow for Payment but not Approval but for the sake of not making assumptions that could break things down the road, I introduced a new "disabled" option to the DropDownButton component (which honestly can come handy in the future). Here's how the Approve would look like if Pay buttons are also present: Other than that, I made changes to the |
I’ll take a look soon and report back. I did notice that toggling self approval on OD stopped automatically updating the button on ND via pusher but it would after navigating or refreshing. It did after I merged main so something happened there. I also noticed that the new variable that replaced isPreventSelfApprovalEnabled (sorry, on my phone and can’t lookup the new name) got renamed so could also be related. Anyways I’ll find out what’s happening.
|
@alitoshmatov you're right, it's an issue with Line 167 in fa6bcc0
This is the last place using the deprecated |
@barros001 Are we good to proceed? |
@alitoshmatov yes. I was just waiting to hear from you if you think we should kill the deprecated usage I mentioned on the other comment. I'll merge main and solve conflicts shortly. |
Yes as far as I understand removing it does not affect our solution. Let's just not use it in our solution and leave everything else the same related to |
@alitoshmatov main merged and conflicts resolved. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeapprove-android.movAndroid: mWeb ChromeiOS: Nativeapprove-ios.mp4iOS: mWeb Safariapprove-safari.mp4MacOS: Chrome / Safariapprove-web.movMacOS: Desktopapprove-desktop.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think we can finally merge this PR
LGTM. Thank you both @barros001 and @alitoshmatov for your involvement in this. |
@lakchote looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
✋ 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/lakchote in version: 1.4.56-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.4.56-8 🚀
|
Details
Fixed Issues
$ #36425
PROPOSAL: #36425 (comment)
Tests
Precondition:
Admin is in the Collect workspace.
"Prevent Self-Approval" is enabled.
Offline tests
Precondition:
Admin is in the Collect workspace.
"Prevent Self-Approval" is enabled.
QA Steps
Precondition:
Admin is in the Collect workspace.
"Prevent Self-Approval" is enabled.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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.webm
Android: mWeb Chrome
android-web.webm
iOS: Native
Uploading ios.mp4…
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov