-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[HOLD for payment 2025-01-17] [$250] Copilot not correctly displayed in report action #53882
Comments
Triggered auto assignment to @anmurali ( |
Edited by proposal-police: This proposal was edited at 2024-12-10 20:49:32 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.What is the root cause of that problem?wrong Account IDs being passed here.
Because icon.id also would contain delegator account id if action is performed by a delegator from here. App/src/pages/home/report/ReportActionItemSingle.tsx Lines 111 to 114 in fca45c9
App/src/pages/home/report/ReportActionItemSingle.tsx Lines 154 to 159 in fca45c9
What changes do you think we should make in order to solve the problem?pass
We would also need to add delegateAccountID prop to component, and render on behalf of delegate... tooltip wherever needed. Specifics could be discussed during PR. What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?Mostly doesn't need any test for such small ui change, but it could be added in case What alternative solutions did you explore? (Optional)Alternatively modify icon object to contain both actorAccountID and delegateAccountID, and modify to read those IDs from icon prop, this will solve both single icon and multiple icon issue. |
Edited by proposal-police: This proposal was edited at 2024-12-18 10:17:17 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.User C on behalf of User C approved the report as action message. What is the root cause of that problem?By default, the
App/src/pages/home/report/ReportActionItemSingle.tsx Lines 111 to 115 in fca45c9
App/src/pages/home/report/ReportActionItemSingle.tsx Lines 218 to 220 in fca45c9
What changes do you think we should make in order to solve the problem?
App/src/pages/home/report/ReportActionItemSingle.tsx Lines 289 to 291 in fca45c9
App/src/pages/home/report/ReportActionItemSingle.tsx Lines 218 to 220 in fca45c9
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?What alternative solutions did you explore? (Optional)If we want to display the fallback avatar and the delegate email, here we can remove App/src/pages/home/report/ReportActionItemSingle.tsx Lines 111 to 115 in fca45c9
Then here, we will not display the tooltip for this issue because we don't have personal detail of delegate account
Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
@anmurali Eep! 4 days overdue now. Issues have feelings too... |
Job added to Upwork: https://www.upwork.com/jobs/~021869141076979307076 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @dukenv0307 ( |
We have 2 proplems here:
The tooltip shows the wrong value: A (as copilot for A) @ChavdaSachin We shouldn't use I like @nkdengineer's proposal to switch to use
It shows the actor as A -> That's incorrect (the correct actor is C) BE doesn't return personalDetail of C. I'm not sure it's the bug since user B doesn't know about user C, we need to confirm the expectation in this case. Overall, let's go with @nkdengineer's proposal to fix the first issue. 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@dangrous Can you please take a look at this point? Thanks |
dang, I was hoping someone would come up with a clever FE solution, haha. We're aware of the personalDetails issue but haven't been able to figure out a solution to it yet unfortunately. I'll bump that discussion and see if we can get more action. (internal link here) But, we can fix the tooltip in the meantime. @nkdengineer's proposal for that makes sense to me! |
📣 @nkdengineer 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
Just to note - the BE issue would also solve some similar - but rare - issues, like not having the personalDetails of a user with whom the report was previously shared but isn't shared anymore. Good/bad news about that, it's a broader problem, not just for copilot. Sooooo trying to figure out what to do. |
I think we're aligned on a way forward in a broad sense, but need to figure out the best way to implement. This may need a mini design doc or at least a little bit more process, so it could be a bit of time. |
@dangrous, @anmurali, @dukenv0307, @nkdengineer Huh... This is 4 days overdue. Who can take care of this? |
Oops yes, dropped on this one. The consensus in Slack was that we should make a mini design doc for this issue since the likely solution breaks 1:1:1 and is a little more complicated that initially thought. It will also account for other situations in which we wouldn't have personalDetails stored in the report for users. I will try to get started on that process this week, though it might be a bit of time. |
Follow up - given that, I think we might want to split this issue, since it's sort of two bugs anyway:
@anmurali does that make sense to you? I can make the new issue and then we can pay/close this one. |
Sounds good to me. |
Great. I made https://github.com/Expensify/Expensify/issues/463521 to cover the internal remaining aspect of this and this App issue to track the actual bug we noticed for this. We're good to finish out this issue now. Thanks! |
@dangrous, @anmurali, @dukenv0307, @nkdengineer Huh... This is 4 days overdue. Who can take care of this? |
@dangrous - do I pay $250 to @dukenv0307 and @nkdengineer ? |
yep that's perfect! |
@dangrous, @anmurali, @dukenv0307, @nkdengineer Huh... This is 4 days overdue. Who can take care of this? |
@dangrous, @anmurali, @dukenv0307, @nkdengineer 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
@dangrous, @anmurali, @dukenv0307, @nkdengineer 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it! |
bump @anmurali for payment when you have a moment - thank you! |
@dangrous, @anmurali, @dukenv0307, @nkdengineer 12 days overdue. Walking. Toward. The. Light... |
Contributor: @nkdengineer paid $250 via Upwork Do we want a regresssion test for this? I feel like we might (it might also be created later if BE work gets gone) |
BugZero Checklist:
Bug classificationSource of bug:
Where bug was reported:
Who reported the bug:
Regression Test ProposalTest:
Do we agree 👍 or 👎 |
I'd modify step 4 to say "Ensure it says [Copilot account] (as copilot for [original account]) in the tooltip" just to be extra clear but otherwise looks great! |
Thx @dukenv0307 and @dangrous test case created with updated step 4 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 9.0.73-6
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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: @heyjennahay
Slack conversation (hyperlinked to channel name): expensify_expense
Action Performed:
Expected Result:
User C on the behalf of User A approved the report as action message
Actual Result:
User C on behalf of User C approved the report as action message.
While reproducing observed this: On hovering over a username, the tooltip displays the text:
"User C (as copilot for User C)"
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @dukenv0307The text was updated successfully, but these errors were encountered: