Closed kbecciv closed 10 months ago
Triggered auto assignment to @JmillsExpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Job added to Upwork: https://www.upwork.com/jobs/~01d1fdbe3efd12338f
Platforms
in OP are β
)Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External
)
Chat - Wrong sub avatar for split bill
The problem is inside the ReportActionItemSingle
component when the condition below is false
.
const isWorkspaceActor = ReportUtils.isPolicyExpenseChat(props.report) && (!actorAccountID || displayAllActors);
To retrieve the Workspace's icon (AKA "secondaryAvatar") for this kind of Report chat ("policyExpenseChat"), is used the method getIcons
from the ReportUtils
as you can see in the image below:
The method getIcons
returns a tuple with the icons whose positions depend on the Report type.
When the Report type is expense
then the position Zero will have the member icon, and the position One will have the workspace icon, otherwise the position Zero will have the workspace icon, and the position One member icon.
function getIcons(report, personalDetails, defaultIcon = null, defaultName = '', defaultAccountID = -1, policy = undefined) {
...
if (isPolicyExpenseChat(report) || isExpenseReport(report)) {
const workspaceIcon = getWorkspaceIcon(report, policy);
const memberIcon = {
source: UserUtils.getAvatar(lodashGet(personalDetails, [report.ownerAccountID, 'avatar']), report.ownerAccountID),
id: report.ownerAccountID,
type: CONST.ICON_TYPE_AVATAR,
name: lodashGet(personalDetails, [report.ownerAccountID, 'displayName'], ''),
fallbackIcon: lodashGet(personalDetails, [report.ownerAccountID, 'fallbackIcon']),
};
return isExpenseReport(report) ? [memberIcon, workspaceIcon] : [workspaceIcon, memberIcon];
}
...
}
In the ReportActionItemSingle
component, to know which position has the secondaryAvatar
is checked if the isOwnPolicyExpenseChat
property in the Report has the value true
, if yes, it gets the position Zero, otherwise, the position One. (as you can see in the first image)
The problem is that the ReportActionItemSingle
component uses the position One where there is the memberIcon
object because the isOwnPolicyExpenseChat
property in the Report has the value false
.
To get the correct position in the tuple, we should add a condition to use the position Zero if the isOwnPolicyExpenseChat
property in the Report has the value true
(current logic) or if this Report is a policy Expense chat.
I tried to understand why the getIcons
methods should return a tuple with different values for the index related to the Report's type, but there are many locals that use this method and change this piece of code to return always the same values in the same position without tests is a change that I thought could bring other issues.
Fix evidence:
https://github.com/Expensify/App/assets/698363/252b856d-e53e-4d38-8924-6f2e793d9e7f
Wrong sub avatar for split bill
The root cause of the problem is that the secondaryAvatar
variable is the memberIcon
instead of being the workspaceIcon
after the spilt action occurs.
After the spilt action is created, the isExpenseReport
function outputs false
when the first report action, the money request, is passed through it. Since, report.action.isOwnPolicyExpenseChat
is false
for user A, it will select the second icon item in the output array from getIcons
function which is the memberIcon
.
Since we need to get the workspaceIcon
instead of the memberIcon
from the icons array, I believe that adding other condition for checking if the report is the not an expense report.
Therefore, changing the above line of code to something similar to the one below should allow the workspaceIcon
can be selected for secondaryAvatar
.
secondaryAvatar = ReportUtils.getIcons(props.report, {})[props.action.isOwnPolicyExpenseChat || !ReportUtils.isExpenseReport(props.report) ? 0 : 1];
@cubuspl42 thoughts on the proposals so far?
@Tony-MK Isn't your proposal essentially equivalent to the earlier one?
@cubuspl42 Thanks for reviewing my proposal. I believe my proposal has the same RCA as the one before. The difference is the solution. The added condition in my proposal checks if the report is not an expense report. Unlike the previous proposal, I suggest using the negation of the output from the ReportUtils.isExpenseReport
function. While, the previous proposal checks if the Report is a policy Expense chat.
@wlegolas Do you maybe have a Git branch with the code you used to test your idea?
Hi @cubuspl42 yes I already have. That video I created using my branch, but I haven't pushed this branch yet.
Would you share it?
Sure @cubuspl42, here is the link with the changes I did to fix this issue.
https://github.com/Expensify/App/compare/main...wlegolas:ExpensifyApp:bugfix/issue-29614
If you have any questions or suggestions, please let me know.
Hi @cubuspl42 I updated my branch with a Unit Test to validate this issue.
You can see all changes in this link: https://github.com/Expensify/App/compare/main...wlegolas:ExpensifyApp:bugfix/issue-29614
@wlegolas
Your root cause analysis sounds solid and I appreciate the efforts to add tests!
We can move forward with the proposal by @wlegolas
C+ reviewed π π π
Triggered auto assignment to @techievivek, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
π£ @cubuspl42 π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @wlegolas 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 π
π£ @dhanashree-sawant π An offer has been automatically sent to your Upwork account for the Reporter role π Thanks for contributing to the Expensify app!
Hi @cubuspl42 thank you for your feedback about my proposal and the tests.
I'll post today the PR with these changes and I'll let you know when the PR is available for review.
Hi @cubuspl42 I opened the PR: https://github.com/Expensify/App/pull/30068
If you have any questions or suggestions, feel free to ask me.
PR review in progress
Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:
On to the next one π
Hi @JmillsExpensify.
I didn't receive the offer for this job in my Upwork account, could you help me, please?
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.91-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2023-11-02. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
Not overdue, @cubuspl42 can you please fill out the above checklist π
@JmillsExpensify It looks like @wlegolas hasn't received an upwork offer https://github.com/Expensify/App/issues/29614#issuecomment-1781927656, can you please take a look into it.
Thanks for closing the loop on that regression test. I'll get it added.
Payment issue:
Hi @JmillsExpensify
I asked in this comment https://github.com/Expensify/App/issues/29614#issuecomment-1781927656 that I haven't received an Upwork offer.
When I started this fix, I followed the instructions in this comment https://github.com/Expensify/App/issues/29614#issuecomment-1772323420 and applied to the Upwork job, but I don't know just applying to this job is enough.
Could you please help me understand if there is something else that I or you need to do?
@JmillsExpensify It looks like @wlegolas hasn't received an offer yet. Can you please take a look and help them with the next step, thanks.
Hi @JmillsExpensify if I need to do something from my said, please let me know.
I followed the instructions in this comment https://github.com/Expensify/App/issues/29614#issuecomment-1772323420 applying to this job in Upwork, I don't know if can help you.
@wlegolas, @JmillsExpensify, @cubuspl42, @techievivek Eep! 4 days overdue now. Issues have feelings too...
Gentle bump @JmillsExpensify ^
Sorry missed the notifications. Jumping in to resolve payments now!
Contributors with contracts paid out. @wlegolas if you're online at the moment, please accept the offer in Upwork and I'll make sure it's processed. Thank you!
Hi @JmillsExpensify I accepted the offer.
Thank you so much.
All paid out and the regression test has been created. Closing this one out!
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.3.84.0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697216946635739
Action Performed:
Expected Result:
App should display workspace profile in split message as sub avatar
Actual Result:
App displays default profile image avatar of same user in split message
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Android: Native
https://github.com/Expensify/App/assets/93399543/db9f25c6-8288-4e32-9671-9f93164893f7Android: mWeb Chrome
https://github.com/Expensify/App/assets/93399543/b6165d08-2240-40cc-ad43-24f6a5cf86fbiOS: Native
https://github.com/Expensify/App/assets/93399543/92368c32-53f6-4b8a-a401-347c389bf26eiOS: mWeb Safari
https://github.com/Expensify/App/assets/93399543/0f709996-b8f5-4949-bb0d-a734b7b53c50MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/93399543/644ebd7a-02a2-478e-bfb2-f65ab343948c https://github.com/Expensify/App/assets/93399543/dd2440a3-b625-44fe-a8e5-476709e0b790 https://github.com/Expensify/App/assets/93399543/26dc0c6a-4d4f-4e3f-a8a6-099b32e14c24MacOS: Desktop
https://github.com/Expensify/App/assets/93399543/867d9f12-274a-4625-85d6-c0b376833b18View all open jobs on GitHub
Upwork Automation - Do Not Edit