Skip to content
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

[$75] Copilot - Delegated user is not visible in the list until relogin #53971

Open
1 of 8 tasks
vincdargento opened this issue Dec 11, 2024 · 60 comments
Open
1 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@vincdargento
Copy link

vincdargento commented Dec 11, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue was found while executing QA for PR #52980

Version Number: v9.0.74-2
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: #52980
Email or phone of affected tester (no customers): New Gmail account
Issue reported by: Applause Internal Team
Device used: MacOS Catalina 10.15.7
App Component: User Settings

Action Performed:

Precondition: sign up new gmaill account on desktop app. don't validate account.

  1. Web: go to Settings > Security
  2. Add copilot > enter email from precondition
  3. Desktop: go to Settings > click on account switcher

Expected Result:

List of accounts is opened. Delegated account's email is visible.

Actual Result:

List of accounts is opened. Delegated account's email is not visible. Blank row with a 'Full' label. User has to relogin to be see delegated account's email.

NOTE: despite email is blank, user still can switch account.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021876420696889822612
  • Upwork Job ID: 1876420696889822612
  • Last Price Increase: 2025-02-03
Issue OwnerCurrent Issue Owner: @tgolen
@vincdargento vincdargento added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 11, 2024
Copy link

melvin-bot bot commented Dec 11, 2024

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@truph01

This comment was marked as outdated.

@melvin-bot melvin-bot bot added the Overdue label Dec 16, 2024
Copy link

melvin-bot bot commented Dec 17, 2024

@adelekennedy Eep! 4 days overdue now. Issues have feelings too...

@adelekennedy
Copy link

struggling to reproduce - testing again

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Dec 18, 2024
Copy link

melvin-bot bot commented Dec 23, 2024

@adelekennedy Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Dec 25, 2024

@adelekennedy this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Dec 25, 2024

@adelekennedy Still overdue 6 days?! Let's take care of this!

Copy link

melvin-bot bot commented Dec 27, 2024

@adelekennedy Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

Copy link

melvin-bot bot commented Dec 31, 2024

@adelekennedy 12 days overdue now... This issue's end is nigh!

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Jan 3, 2025
Copy link

melvin-bot bot commented Jan 3, 2025

This issue has not been updated in over 14 days. @adelekennedy eroding to Weekly issue.

@melvin-bot melvin-bot bot removed the Overdue label Jan 3, 2025
@truph01
Copy link
Contributor

truph01 commented Jan 3, 2025

Proposal

Please re-state the problem that we are trying to solve in this issue.

  • List of accounts is opened. Delegated account's email is not visible. Blank row with a 'Full' label. User has to relogin to be see delegated account's email.

What is the root cause of that problem?

  • If the personalDetails data in:

text: personalDetails?.displayName ?? personalDetails?.login ?? '',

is undefined, it can result in the bug where the delegated user does not appear in the list.

  • There are several scenarios that could cause this bug. In this specific case, it occurs due to a Pusher issue, where the personal details data is not sent to the desktop device (refer to the steps described in the original post).

What changes do you think we should make in order to solve the problem?

  • To address this issue comprehensively, we need to resolve the underlying Pusher issue to ensure personalDetails data is properly sent to the desktop device.

  • Additionally, applying a frontend fix is essential to improve the user experience. By displaying fallback data instead of an empty row when personalDetails is undefined, we can prevent confusion and maintain a more consistent and intuitive UI. This layered approach ensures both the root cause and its immediate user-facing impact are effectively handled.

  • We already have extensive logic in the app to display fallback data when the target data is undefined. So, the similar can be applied in this case.

  • First, we need to add an additional prop, named email in:

additionalProps: Partial<Omit<PopoverMenuItem, 'icon' | 'iconType'>> = {},

and add the fallback data for the text and icon:

            text: personalDetails?.displayName ?? personalDetails?.login ?? additionalProps.email ?? '',
            icon: personalDetails?.avatar ?? Expensicons.FallbackAvatar ?? '',
  • In:

return createBaseMenuItem(personalDetails, error, {

pass the email prop to the 3rd param so it can be used in the above step.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

  • We can consider applying the fallback data to the description field as well:

description: Str.removeSMSDomain(personalDetails?.login ?? ''),

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jan 3, 2025
Copy link
Contributor

github-actions bot commented Jan 3, 2025

Yours could be considered a template-based comment, but there is a missing mandatory line. Here's the evaluation:

  1. Proposal line: Present
  2. Problem statement: Present
  3. Root cause: Present
  4. Solution changes: Present
  5. Alternative solutions: Present (optional), but this doesn't count towards mandatory lines.

You missed the line for scenarios in automated tests and its user content.
Thus, mandatory content is missing. I'll proceed with the response:

Your proposal will be dismissed because you did not follow the proposal template.

@melvin-bot melvin-bot bot added the Overdue label Jan 6, 2025
@adelekennedy adelekennedy added the External Added to denote the issue can be worked on by a contributor label Jan 7, 2025
@melvin-bot melvin-bot bot changed the title Copilot - Delegated user is not visible in the list until relogin [$250] Copilot - Delegated user is not visible in the list until relogin Jan 7, 2025
Copy link

melvin-bot bot commented Jan 7, 2025

Job added to Upwork: https://www.upwork.com/jobs/~021876420696889822612

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 7, 2025
@huult
Copy link
Contributor

huult commented Jan 23, 2025

@tgolen I tested your JSON. You are missed login and avatar.

Example should be:

{
            updates: [
                {
                    data: [
                        {
                            key: 'account',
                            onyxMethod: 'merge',
                            value: {
                                delegatedAccess: {
                                    delegates: [],
                                    delegators: [
                                        {
                                            email: '4@1.com',
                                            role: 'all',
                                        },
                                    ],
                                },
                            },
                        },
                        {
                            key: 'personalDetailsList',
                            onyxMethod: 'merge',
                            value: {
                                '9': {
                                    firstName: 'T T C D',
                                    lastName: 'T T C D',
                                    login: '4@1.com',
                                    avatar: 'https://d1wpcgnaa73g0y.cloudfront.net/26271df52c9c9db57fa0221feaef04d18a045f2b_128.jpeg',
                                },
                            },
                        },
                    ],
                    eventType: 'onyxApiUpdate',
                },
            ],
            lastUpdateID: pushJSONS?.lastUpdateID,
            previousUpdateID: pushJSONS?.previousUpdateID,
        }

function subscribeToPrivateUserChannelEvent(eventName: string, accountID: string, onEvent: (pushJSON: OnyxUpdatesFromServer) => void) {

function subscribeToPrivateUserChannelEvent(eventName: string, accountID: string, onEvent: (pushJSONS: OnyxUpdatesFromServer) => void) {
    const pusherChannelName = `${CONST.PUSHER.PRIVATE_USER_CHANNEL_PREFIX}${accountID}${CONFIG.PUSHER.SUFFIX}` as const;

    function logPusherEvent(pushJSONS: OnyxUpdatesFromServer) {
        const pushJSON = {
            updates: [
                {
                    data: [
                        {
                            key: 'account',
                            onyxMethod: 'merge',
                            value: {
                                delegatedAccess: {
                                    delegates: [],
                                    delegators: [
                                        {
                                            email: '3@1.com',
                                            role: 'all',
                                        },
                                    ],
                                },
                            },
                        },
                        {
                            key: 'personalDetailsList',
                            onyxMethod: 'merge',
                            value: {
                                '8': {
                                    firstName: 'T T C D',
                                    lastName: 'T T C D',
                                    login: '3@1.com',
                                    avatar: 'https://d1wpcgnaa73g0y.cloudfront.net/26271df52c9c9db57fa0221feaef04d18a045f2b_128.jpeg',
                                },
                            },
                        },
                    ],
                    eventType: 'onyxApiUpdate',
                },
            ],
            lastUpdateID: pushJSONS?.lastUpdateID,
            previousUpdateID: pushJSONS?.previousUpdateID,
        };
        Log.info(`[Report] Handled ${eventName} event sent by Pusher`, false, pushJSON);
    }

    function onPusherResubscribeToPrivateUserChannel() {
        NetworkConnection.triggerReconnectionCallbacks('Pusher re-subscribed to private user channel');
    }

    function onEventPush(pushJSONS: OnyxUpdatesFromServer) {
        const pushJSON = {
            updates: [
                {
                    data: [
                        {
                            key: 'account',
                            onyxMethod: 'merge',
                            value: {
                                delegatedAccess: {
                                    delegates: [],
                                    delegators: [
                                        {
                                            email: '3@1.com',
                                            role: 'all',
                                        },
                                    ],
                                },
                            },
                        },
                        {
                            key: 'personalDetailsList',
                            onyxMethod: 'merge',
                            value: {
                                '8': {
                                    firstName: 'T T C D',
                                    lastName: 'T T C D',
                                    login: '3@1.com',
                                    avatar: 'https://d1wpcgnaa73g0y.cloudfront.net/26271df52c9c9db57fa0221feaef04d18a045f2b_128.jpeg',
                                },
                            },
                        },
                    ],
                    eventType: 'onyxApiUpdate',
                },
            ],
            lastUpdateID: pushJSONS?.lastUpdateID,
            previousUpdateID: pushJSONS?.previousUpdateID,
        };
        logPusherEvent(pushJSON);
        onEvent(pushJSON);
    }

    function onSubscriptionFailed(error: Error) {
        Log.hmmm('Failed to subscribe to Pusher channel', {error, pusherChannelName, eventName});
    }
    Pusher.subscribe(pusherChannelName, eventName, onEventPush, onPusherResubscribeToPrivateUserChannel).catch(onSubscriptionFailed);
}
Screen.Recording.2025-01-23.at.09.08.58.mp4

@tgolen
Copy link
Contributor

tgolen commented Jan 23, 2025

Oh, Thanks! OK, I can add those two fields... but that brings up another question. What if the user has never uploaded a custom avatar or added their first and last name? Will the frontend fallback to just displaying their login (email or phone number) and their default avatar?

@tgolen
Copy link
Contributor

tgolen commented Jan 23, 2025

Nevermind on those other questions. I found the proper data I needed to return and it's working fine now:

Image

I've submitted an internal PR to implement this and I'll keep you updated on the progress.

@tgolen
Copy link
Contributor

tgolen commented Jan 23, 2025

Daily Update

  • As mentioned, I created an internal PR with a fix for this

Next Steps

  • I need to get the tests passing in that PR, and then have it reviewed and merged
  • Once that is deployed, we can re-evaluate this issue and see if we still want to make any frontend changes or not (I'm leaning towards not)

ETA

  • Backend change deployed by Tuesday, Jan. 28

@tgolen
Copy link
Contributor

tgolen commented Jan 24, 2025

Daily Update

  • I updated the internal PR today and got the tests working
  • It's now in full review

Next Steps

  • Wait for the PR to be merged and deployed

ETA

  • Same, Tuesday, Jan. 28

@melvin-bot melvin-bot bot added the Overdue label Jan 27, 2025
@tgolen
Copy link
Contributor

tgolen commented Jan 27, 2025

Daily Update

  • The internal PR is in final review and should be merged today

Next Steps

  • Same

ETA

  • Same

@melvin-bot melvin-bot bot removed the Overdue label Jan 27, 2025
Copy link

melvin-bot bot commented Jan 28, 2025

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@tgolen
Copy link
Contributor

tgolen commented Jan 28, 2025

Daily Update

  • The internal PR is still under review
  • @puneetlath spotted a small performance boost that I could apply
  • I made sure to get the new delegate query timed

Next Steps

ETA

  • Tuesday, Jan. 28

@tgolen
Copy link
Contributor

tgolen commented Jan 29, 2025

Daily Update

Next Steps

  • Wait for the PR to be deployed to production
  • I will update this GH again so that the bug can be re-evaluated and see if there is anything else necessary to do

ETA

  • Should be deployed tomorrow, Jan. 30

@tgolen
Copy link
Contributor

tgolen commented Jan 31, 2025

Daily Update

  • The internal PR was deployed to production

Next Steps

  • @huult @eVoloshchak can you please take a look at this issue and reevaluate the bug? Let's determine if anything else needs to happen on the frontend or not.

ETA

  • TBD

@huult
Copy link
Contributor

huult commented Jan 31, 2025

I'm testing

@melvin-bot melvin-bot bot added the Overdue label Feb 3, 2025
@huult
Copy link
Contributor

huult commented Feb 3, 2025

@tgolen I tested and saw that the avatar, name, and email are displaying as shown in the video below. I think we don’t need to make any updates on the frontend.

Screen.Recording.2025-02-03.at.15.24.41.mp4

@tgolen
Copy link
Contributor

tgolen commented Feb 3, 2025

OK, great. Thank you for verifying! I will go ahead and close this out then.

@tgolen tgolen closed this as completed Feb 3, 2025
@melvin-bot melvin-bot bot removed the Overdue label Feb 3, 2025
@github-project-automation github-project-automation bot moved this from Bugs and Follow Up Issues to Done in [#whatsnext] #expense Feb 3, 2025
@huult
Copy link
Contributor

huult commented Feb 3, 2025

@tgolen May I receive any payment here?

@tgolen
Copy link
Contributor

tgolen commented Feb 3, 2025

I don't think we typically pay out anything for internal issues, although I appreciate your help with this. If you were to receive payment, how much payment are you requesting?

@huult
Copy link
Contributor

huult commented Feb 3, 2025

@tgolen Thank you for your appreciation! I’m happy to help without any payment. However, if payment is possible, I would kindly accept any amount you consider appropriate.

@tgolen
Copy link
Contributor

tgolen commented Feb 3, 2025

OK, I'll approve this for a partial $75 payment. @adelekennedy Can you make that happen, please?

@tgolen tgolen changed the title [$250] Copilot - Delegated user is not visible in the list until relogin [$75] Copilot - Delegated user is not visible in the list until relogin Feb 3, 2025
Copy link

melvin-bot bot commented Feb 3, 2025

Upwork job price has been updated to $75

@huult
Copy link
Contributor

huult commented Feb 5, 2025

Friendly bump, @adelekennedy, for this comment.

@huult
Copy link
Contributor

huult commented Feb 11, 2025

Friendly bump, @adelekennedy

@adelekennedy adelekennedy reopened this Feb 11, 2025
@adelekennedy
Copy link

reopened for payment

https://www.upwork.com/jobs/~021889354259199458212

@huult please apply above!

@huult
Copy link
Contributor

huult commented Feb 11, 2025

@adelekennedy , Applied. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
Status: Done
Development

No branches or pull requests

7 participants