Closed izarutskaya closed 6 months ago
Triggered auto assignment to @slafortune (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
We think this issue might be related to the #collect project.
Wrong header message is shown when admin is submitting their own request
When we submit a report we display the message in header using the following logic: https://github.com/Expensify/App/blob/ef949e0cdfb150d869a8bc89ec7db31ac3f04e52/src/libs/NextStepUtils.ts#L192
But here we have a problem that the first optimisticNextStep
is as follows:
https://github.com/Expensify/App/blob/ef949e0cdfb150d869a8bc89ec7db31ac3f04e52/src/libs/NextStepUtils.ts#L196-L219
Now as seen in the video, we first display this action in the header
and later we fetch the system action from the BE
In this we also check whether the current submitter
is the policy admin
or not:
https://github.com/Expensify/App/blob/ef949e0cdfb150d869a8bc89ec7db31ac3f04e52/src/libs/NextStepUtils.ts#L224-L246
But here we miss out the condition where the submitter can be a admin
of a policy
but not the owner of the policy.
This will set the action message which is consistent with the BE
code
Now we need to match the optimistic messages for policy owner
as well as policy admin
.
Update the (isOwner)
condition to if (isOwner || !isManager)
This will match the case where the submitter is the policy admin
but is not the policy owner
, regular employee won't save this optimistic message now as he is the manager of the report and the above condition will return false
Job added to Upwork: https://www.upwork.com/jobs/~01cd32e3a37f04cf21
Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External
)
Hi @GandalfGwaihir have you tried to run your solution? I think these modifications will not change anything in the behavior.
ummm is it so, let me try testing it just to be double sure
In the meantime can you point out where exactly you think it would fail?
I believe the code is identical to what we currently have; it has just been rewritten. (talking about the message
array)
Updated my proposal, now it covers every case of the submit-approve
process and the text is consistent with the BE
message, also added result video.
"Waiting for you to approve ..." message appears as the next steps for the admin after admin clicks on submit and the message doesn't update to the right one unless admin goes back to workspace chat and returns back to report conversation.
In here we're hardcoding you
in all cases, while there could be cases that the current user (Admin) is viewing an Expense of an Employee, and Admin should see message like [Submitter] is waiting for [Approver] to approve these these expenses.
To clarify, there're 2 types of next step messages based on who is currently viewing the next step message:
In this case the message will look like
Waiting for [Approver] to approve these expenses.
Which is covered by this optimistic next step logic
In this case the message will look like
[Submitter] is waiting for [Approver] to approve these these expenses.
This is covered by this optimistic next step logic
The bug here is in the 2nd case, where the [Approver]
is hardcoded as you
, which is wrong in case the viewing user is not the approver.
Change this to
text: managerDisplayName,
The managerDisplayName
will show you
or manager login
based on whether the current user is the approver (reference), this is already used the same in the 1st case above
We should double check other optimistic next step messages to make sure they don't have similar bugs.
If we want the 1st scenario to show not only in case the submitter is current user, but also the case where another person (which is not the approver) views the report, we can update this to
if (!isSelfApproval) {
We'll also need to make sure submitsTo
is set optimistically when creating a workspace (or related scenarios), to make sure issues like explained in here or here doesn't happen
@GandalfGwaihir @nkdengineer
Would you help me figure out the differences between your proposed solutions? (I'm talking about the updated version of @GandalfGwaihir's proposal)
@GandalfGwaihir
Your proposal is well-written in general π
π π π
Triggered auto assignment to @rlinoz, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@rlinoz
@GandalfGwaihir noticed that this optimistic message...
...is not in sync with the backend. If I understood this correctly, the backend doesn't seem to support it. @GandalfGwaihir suggests completely removing it from the frontend.
Do you know whether this is the right course, or maybe it's the backend which should be adjusted?
Thanks @cubuspl42 :)
Some more context for @rlinoz :
The first message was introduced for the case when the current submitter is the approver also on the FE
:
But that case is already covered in the second condition where we check for managerDisplayName
https://github.com/Expensify/App/blob/cb71f33e529eac374b76478c9e6a2d9147beebf5/src/libs/NextStepUtils.ts#L231
And is the report is self approve
then we subtitute you
there, so that edge case is also covered:
https://github.com/Expensify/App/blob/cb71f33e529eac374b76478c9e6a2d9147beebf5/src/libs/NextStepUtils.ts#L81
I don't think we want to remove the first block because that is how we display to the approver himself.
I think we might just need to change the if to something like if (isOwner || !isManager)
, then we would show the same way the backend is sending.
Also I see that SubmitReport
is not sending the nextStep back, maybe it should?
I think we might just need to change the if to something like if (isOwner || !isManager), then we would show the same way the backend is sending.
Let me test this one
@rlinoz , tested you solution to include the !isManager condition it works well now, also tested when the report submitter is the admin, you were right in suggesting that we show that optimistic message to the admin
I have added alternative solution in my proposal to cover that case
If policy owner
submits employees report:
If policy admin who is not the owner of the policy
submits employees report:
Huum, @GandalfGwaihir are the images reversed?
The first image should be the approver case.
The first one is for the case where report is submitted by the policy owner
the second one is where the report is submitted by
a policy admin
who is not
the owner of the policy
Invite another admin and make that user an approver
In the OP steps, they want a admin who is not the owner of the policy to submit the report
Yeah, but who is the approver? Only the approver should get the is waiting for you to review...
.
If you didn't change anything it should be the policy owner, and the prints are correct now, but just making sure.
The Policy owner
is the approver in this case, we are using a collect policy
over here :)
Here are the settings from OD:
So here whogandalf
is the policy owner & admin
, whogandalf+approver
is the policy admin
and whogandalf+employee
is the policy employee
Cool, if @cubuspl42 is ok with this changes we can proceed π
Cool, if @cubuspl42 is ok with this changes we can proceed π
@rlinoz @cubuspl42 My initial proposal already contains this solution in the alternative solution part (well before @rlinoz suggested similar solution and @GandalfGwaihir updated the proposal). Please see the quoted part below:
If we want the 1st scenario to show not only in case the submitter is current user, but also the case where another person (which is not the approver) views the report, we can update this to
if (!isSelfApproval) {
According to this comment, that check is the correct way to go, if the user is not the approver, they will not see the is waiting for you to review
but will see Waiting for ... to review/approve expenses
instead π
@nkdengineer , your approach of using !isSelfApproval
will have a regression when we have control policy
in control policies
we have options to let different users submit their report to different policy members:
According to https://github.com/Expensify/App/issues/39199#issuecomment-2027598338, that check is the correct way to go, if the user is not the approver,
isSelfApproval
checks whom the report has been submitted to rather than who the approver of the report is :)
Here's the control policy case:
I think we should rely on the isManager
and isOwner
checks as those would be more reliable
isSelfApproval checks whom the report has been submitted to rather than who the approver of the report is :)
@GandalfGwaihir I don't think so, if you look at this, the approver that we'll be showing as the one to take action here, is the one in submitsTo
, not the managerId
your approach of using !isSelfApproval will have a regression
If you indeed found there's a regression, could you clarify the steps to reproduce, and a video of the same?
@cubuspl42 is OOO today, and I couldn't check if the isSelfApproval
solution yields the same result, will get to that tomorrow.
@cubuspl42 is OOO today, and I couldn't check if the isSelfApproval solution yields the same result, will get to that tomorrow.
@rlinoz I've tested many scenarios from different account roles and all are correct according to the expectation. Let me know if you want to clarify anything before moving forward.
I'm catching up! Thanks, Rodrigo, for all the input; I tagged you because I was under the impression that we'll need some backend insights.
Okey, so summing up:
@nkdengineer was the first one to suggest changing the condition of this block:
They suggested changing it to...
if (!isSelfApproval) {
// ...
}
After the discussion, @GandalfGwaihir agreed with Rodrigo to change the mentioned condition to...
if (isOwner || !isManager) {
// ...
}
As I understand it, these conditions might be equivalent in practice.
If you indeed found there's a regression, could you clarify the steps to reproduce, and a video of the same?
@GandalfGwaihir Would you agree to provide this info? I'd like to know whether there is an observably different case between these two conditions.
@GandalfGwaihir can you help us with the comment above?
looking at it now... will update this comment
Okay i had only one concern, when we create a policy offline, we don't set submits to optimistically i guess, so at that time isSelfApproval
would be false
right?
https://github.com/Expensify/App/blob/14ff9445d22bd92d0645abd456ac805cedc06c28/src/libs/NextStepUtils.ts#L76
And the other one is obviously for Control Policies
, there we can have multiple submitters and multiple approveres, also:
Please refer here for the control policy query
Okay i had only one concern, when we create a policy offline, we don't set submits to optimistically i guess, so at that time isSelfApproval would be false right?
@GandalfGwaihir The problem here is not with the usage of submitsTo
but the fact that we don't set submits to optimistically
(and it will also affect anywhere we use submitsTo
, not just this case). We'll set submitsTo
optimistically and it will work well.
And the other one is obviously for Control Policies, there we can have multiple submitters and multiple approveres, also:
@GandalfGwaihir Please help clarify clear reproducible steps if you found a regression, thanks!
@GandalfGwaihir Please help clarify clear reproducible steps if you found a regression, thanks!
Well we need to create control policy on OD and create multiple approvers for a request and set who approves to whom, No clear regression steps that i can think of but control policies
are more complex in terms of approval settings ;)
Well we need to create control policy on OD and create multiple approvers for a request and set who approves to whom, No clear regression steps that i can think of but control policies are more complex in terms of approval settings ;)
@GandalfGwaihir In this case the submitsTo
will also reflect the approver that will need to approve the report at each step, so !isSelfApproval
approach will still work well. Let us know if you indeed found a specific case that proves otherwise.
CC: @mountiny for vis, related to optimistic next steps.
I agree with @nkdengineer that the fact we do not set the submitsTo optimistically can cause some of these issues, I had similar conclussions when working on this PR https://github.com/Expensify/App/pull/39563/files (that mainly focuses on the offline case of creating a new workspace and then submitting a report there)
Okay, everyone, thanks for the polite discussion!
I appreciate your input, @GandalfGwaihir, but your updated proposal was mostly inspired by Rodrigo's words, while @nkdengineer suggested something likely equivalent (no limitation has been proved), but independently.
In my opinion, it's fair to let @nkdengineer handle this one
C+ reviewed π π π
Current assignee @rlinoz is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
Thanks all, assigning @nkdengineer
π£ @cubuspl42 π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @nkdengineer You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing π
Moving to weekly, but when can we expect a PR @nkdengineer ?
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.57-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4455456 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team
Action Performed:
Pre-requisites:
Expected Result:
"Waiting for [approver email] to approve ..." should show up right after admin clicks on the submit button.
Actual Result:
"Waiting for you to approve ..." message appears as the next steps for the admin after admin clicks on submit and the message doesn't update to the right one unless admin goes back to workspace chat and returns back to report conversation.
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
https://github.com/Expensify/App/assets/115492554/8f5d3ec0-4a05-48f5-8bf1-d8ba2ccea757
View all open jobs on GitHub
Upwork Automation - Do Not Edit