Open lanitochka17 opened 3 months ago
Triggered auto assignment to @roryabraham (DeployBlockerCash
), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
@roryabraham FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
We think that this bug might be related to #wave-collect - Release 1
reproduced locally on main
reproduced locally on the production branch as well
reproduced in production, demoting
Job added to Upwork: https://www.upwork.com/jobs/~017992a05a461e2089
Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External
)
Triggered auto assignment to @twisterdotcom (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
pro-tip: go offline before step 6 in the reproduction steps and the green dot that shouldn't be there is persistent
related function is ReportUtils.requiresAttentionFromCurrentUser
the reason the green dot is there is because the report has hasOutstandingChildRequest: true
I suspect this is a problem introduced by https://github.com/Expensify/App/pull/34866/files, IOU.needsToBeManuallySubmitted
will return true
for any report that is not a policyExpenseChat
. I think it's default should be false
, since IOU reports by default do not need to be manually submitted
confirmed that this can be fixed with a simple diff:
diff --git a/src/libs/actions/IOU.ts b/src/libs/actions/IOU.ts
index 761ae02ea4..b420eac4e0 100644
--- a/src/libs/actions/IOU.ts
+++ b/src/libs/actions/IOU.ts
@@ -444,7 +444,7 @@ function needsToBeManuallySubmitted(iouReport: OnyxTypes.Report) {
return isFromPaidPolicy && !policy.harvesting?.enabled;
}
- return true;
+ return false;
}
/**
removing help wanted, will just open the PR myself
PR ready for review: https://github.com/Expensify/App/pull/39470
this one was super simple, reducing to $250
Upwork job price has been updated to $250
@roryabraham @abdulrahuman5196 I don't think this is the right way to fix the issue, it will cause regression because for 1-1 IOU, this line will never be evaluated, and hasOutstandingChildRequest
not set to true
in case after requesting the money, the requester is the one owing money.
It's expected that the needsToBeManuallySubmitted
will return true
by default if it's not policy expense, it's so that we'll not early return here and will set hasOutstandingChildRequest
based on this
I agree the name is a bit confusing, we might want to rename it to be clear, but we can't change the default return to false
Green dot shows up in LHN briefly when requeting money from another user
In here, we're not checking if policy
is not a personal policy before checking isPolicyAdmin
. This leads to in 1-1 DM, the current user is always the admin of their own personal policy, thus it will early return here with hasOutstandingChildRequest
as true
.
In here, check that the policy is not a personal policy first.
if (policy?.type !== CONST.POLICY.TYPE.PERSONAL && PolicyUtils.isPolicyAdmin(policy)) {
Or can check iouReport
is a policy expense chat, like here
We can avoid passing personal policy to getOutstandingChildRequest
in the first place (or higher up in the buildOnyxDataForMoneyRequest
)
@nkdengineer I have not been able to reproduce any regression with my solution. If there is indeed a regression, can you please provide clear reproduction steps?
@roryabraham Please try these steps (after applying your solution):
Now, the GBR in User B's LHN will disappear, which is not correct because User B still owes User A money after step 5. If user B is online, the GBR will disappear momentarily then will appear again after the back-end request is successful, the step of going offline is so that the issue is more visible.
Meanwhile, in staging and prod now, the GBR will stay visible in User B's LHN after step 5. So the above behavior is a regression.
The reason why this is happening is explained on top of my proposal
thanks for the reproduction steps @nkdengineer, I can reproduce the regression you're reporting. I'm still not sure about your RCA though - I'm quite certain that needsToBeManuallySubmitted
should only be true for expense reports.
thanks again for pointing out that issue @nkdengineer - the reproduction steps were very helpful. I ended up fixing the problem in a different way.
thanks again for pointing out that issue @nkdengineer - the reproduction steps were very helpful. I ended up fixing the problem in a different way.
@roryabraham There're other regressions with that approach too, please note needsToBeManuallySubmitted
is also used here and here
I'm quite certain that needsToBeManuallySubmitted should only be true for expense reports.
@roryabraham I think the problem here is that the naming of needsToBeManuallySubmitted
is confusing.
The use case for that method is:
If this report is an expense report, and it has automatic submit, then hasOutstandingChildRequest
will always be false
(it's the intention of this condition)
That's why needsToBeManuallySubmitted
returns true
for personal request, so that the hasOutstandingChildRequest
will not be forced to be false
here, but will depend on other conditions.
We should instead rename to hasAutomaticSubmission
(which is an inverted version of needsToBeManuallySubmitted
)
And we'll use hasAutomaticSubmission(iouReport)
here
I'm still not sure about your RCA though
@roryabraham Did you try keeping the some logic we had to make admins see expense reports that will be automatically submitted with a green dot
(mentioned here)? You'll see the bug in the OP is not fixed right? That's because the RCA is there as I mentioned in my proposal
And the needsToBeManuallySubmitted
fix doesn't fix the root cause but it's more like a code polish.
There're other regressions with that approach too
Can you please provide reproduction steps for these other regressions?
This issue has not been updated in over 15 days. @twisterdotcom, @abdulrahuman5196, @roryabraham eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
Are we still waiting on a response to @roryabraham from @nkdengineer here?
Are we still waiting on a response to @roryabraham from @nkdengineer here?
Yes.
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.59-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
Green dot will not show up in LHN when requeting money from another user
Actual Result:
Green dot shows up in LHN briefly when requeting money from another user
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/09517cc8-19ac-4d1c-a621-af1f8c5ad93e
https://github.com/Expensify/App/assets/78819774/1a94b9cb-fc2d-4f61-98ca-5e10c76a3969
View all open jobs on GitHub
Upwork Automation - Do Not Edit