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

[$250] Hide "Reveal Details" button when logged in as Copilot #57618

Open
8 tasks done
lanitochka17 opened this issue Feb 28, 2025 · 40 comments
Open
8 tasks done

[$250] Hide "Reveal Details" button when logged in as Copilot #57618

lanitochka17 opened this issue Feb 28, 2025 · 40 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Feb 28, 2025

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: 9.1.7-1
Reproducible in staging?: Y
Reproducible in production?: Y
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: N/A
Email or phone of affected tester (no customers): applausetester+sj9032@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

  • User A has assigned a virtual card to themselves
  • User B (copilot) has limited access to User A account
  • The following steps are done by User B
  1. Go to staging.new.expensify.com
  2. Go to Account settings
  3. Open account switcher
  4. Select User A (with limited access)
  5. Go to Wallet
  6. Click on the virtual card
  7. Note that "Reveal Details" button is visible and clickable

Expected Result:

"Reveal Details" button is hidden from view

Actual Result:

"Reveal Details" button is clickable and prompts a magic code to be sent with inconsistent UI

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

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

Screenshots/Videos

Add any screenshot/video evidence
Bug6757593_1740754288053.20250228_224146.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021895518523085098853
  • Upwork Job ID: 1895518523085098853
  • Last Price Increase: 2025-03-07
  • Automatic offers:
    • ChavdaSachin | Contributor | 106463438
    • twilight2294 | Contributor | 106465091
Issue OwnerCurrent Issue Owner: @fedirjh
@lanitochka17 lanitochka17 added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Feb 28, 2025
Copy link

melvin-bot bot commented Feb 28, 2025

Triggered auto assignment to @greg-schroeder (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.

@greg-schroeder greg-schroeder added the External Added to denote the issue can be worked on by a contributor label Feb 28, 2025
@melvin-bot melvin-bot bot changed the title Wallet - Missing email in "Please enter the magic code sent to" when revealing card details as copilot [$250] Wallet - Missing email in "Please enter the magic code sent to" when revealing card details as copilot Feb 28, 2025
Copy link

melvin-bot bot commented Feb 28, 2025

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

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

melvin-bot bot commented Feb 28, 2025

Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)

@gijoe0295
Copy link
Contributor

gijoe0295 commented Feb 28, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-02-28 18:01:20 UTC.

Proposal

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

Missing email in "Please enter the magic code sent to" when revealing card details as copilot

What is the root cause of that problem?

When acting as a delegate, the account data is missing the primaryLogin:

descriptionPrimary={translate('cardPage.enterMagicCode', {contactMethod: primaryLogin})}

Image

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

Use the util here to fallback to session.email:

App/src/libs/UserUtils.ts

Lines 260 to 262 in 9672b4a

function getContactMethod(): string {
return account?.primaryLogin ?? session?.email ?? '';
}

We can optionally apply this same fix to other pages using account.primaryLogin for the enterMagicCode copies.

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

None - UI bug

What alternative solutions did you explore? (Optional)

None

Copy link
Contributor

⚠️ @gijoe0295 Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

@twilight2294

This comment has been minimized.

@truph01
Copy link
Contributor

truph01 commented Mar 1, 2025

Proposal

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

  • "Please enter the magic code sent to" message is missing the email

What is the root cause of that problem?

const primaryLogin = account?.primaryLogin ?? '';

In case of copilot, the account.primaryLogin is undefined.

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

  • We should prevent users from revealing card details if they are acting as a copilot and their role is limited, as these details are sensitive. To achieve this, we can update the following code in

const openValidateCodeModal = (revealedCardID: number) => {

to:

 const {isDelegateAccessRestricted, delegatorEmail} = useDelegateUserDetails();
    const isActingAsDelegate = !!account?.delegatedAccess?.delegate || false;
    const [isNoDelegateAccessMenuVisible, setIsNoDelegateAccessMenuVisible] = useState(false);
    const openValidateCodeModal = (revealedCardID: number) => {
        if (isDelegateAccessRestricted) {
            setIsNoDelegateAccessMenuVisible(true);
            return;
        }
        setCurrentCardID(revealedCardID);
        setIsValidateCodeActionModalVisible(true);
    };

Additionally, we need to add a modal to handle restricted access. This modal should be placed at

<DelegateNoAccessModal
                isNoDelegateAccessMenuVisible={isNoDelegateAccessMenuVisible}
                onClose={() => setIsNoDelegateAccessMenuVisible(false)}
            />
  • Next, to display the correct email in the validation code modal when the copilot has full access, we can update:

const primaryLogin = account?.primaryLogin ?? '';

as follows:

    const primaryLogin = account?.primaryLogin ?? delegatorEmail ?? '';
  • Finally, we should consider using if (isActingAsDelegate) { instead of if (isDelegateAccessRestricted) {. This ensures that card details are always hidden for copilots, regardless of their role.

  • Note: We can update the title of the restrict modal to something like Block Copilots from attempting to access virtual card details instead of using the default restrict title in this case.

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

  • We can render the card page, then simulate the click action on the "Reveal details" button and then verify that the restricted access modal is shown.

What alternative solutions did you explore? (Optional)

@truph01
Copy link
Contributor

truph01 commented Mar 1, 2025

@greg-schroeder I believe we should prevent copilots from revealing card details for security and privacy reasons, especially if their role is limited. What are your thoughts?

@twilight2294
Copy link
Contributor

twilight2294 commented Mar 1, 2025

Proposal

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

Missing email in "Please enter the magic code sent to" when revealing card details as copilot

What is the root cause of that problem?

The primaryLogin is fetched from the current accounts primary login value, when someone co-pilots in the account we do not have this field set as now we are acting as a delegate, this causes the bug in the OP, this is the RCA:

const primaryLogin = account?.primaryLogin ?? '';

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

I tested the steps in the OP and observed that the magic code is sent to the email ID of the original account holder. So for our case it won't make any sense to show the input code modal or blocking modal as the code is never sent to the co-piloted account.

So I propose to hide the Reveal Details button if there is a co-pilot logged in the user account as we cannot get the magic code to their account:

    const isSignedInAsdelegate = !!account?.delegatedAccess?.delegate || false;

  <MenuItemWithTopDescription
      description={translate('cardPage.virtualCardNumber')}
      title={maskCard('')}
      interactive={false}
      titleStyle={styles.walletCardNumber}
      shouldShowRightComponent
      rightComponent={
          isSignedInAsdelegate ? (
              <Button
                  text={translate('cardPage.cardDetails.revealDetails')}
                  onPress={() => openValidateCodeModal(card.cardID)}
                  isDisabled={isCardDetailsLoading[card.cardID] || isOffline}
                  isLoading={isCardDetailsLoading[card.cardID]}
              />
          ) : undefined
      }
  />

This will make sure that whether the delegate has full or limited access we never show then the button as it is not of any use

Minor style adjustments can be made during PR phase

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

We write up a UI test for this bug, we should update the onyx data to show that the current user has been logged in as a copilot, then render the virtual expensify card page, and confirm that we do not have the reveal details button

What alternative solutions did you explore? (Optional)

  • we can use the util getContactMethod() to get the login
  • We should use the hook currentUserPersonalDetails to find the details of the user and then assign it to primary login:
    const primaryLogin = formatPhoneNumber(currentUserPersonalDetails?.login ?? '');
  • We need to format the input incase the login is a phone number

@twilight2294
Copy link
Contributor

twilight2294 commented Mar 1, 2025

@fedirjh @greg-schroeder I think we shouldn't show the button at all because the magic code goes to the original account and not to the account the co-pilot is on regardless of full or limited, my proposal outlines the same

@melvin-bot melvin-bot bot added the Overdue label Mar 3, 2025
@greg-schroeder
Copy link
Contributor

Okay, confirmed we don't allow Full Access Copilots to view virtual card details via delegated access; I think that's for obvious reasons, and we shouldn't do that for NewDot either.

@greg-schroeder
Copy link
Contributor

greg-schroeder commented Mar 3, 2025

In that case, I think this issue should be adjusted to say: "Block Copilots from attempting to access virtual card details"

@truph01
Copy link
Contributor

truph01 commented Mar 3, 2025

@greg-schroeder In this PR, a new DelegateNoAccessModal was introduced. As mentioned in the PR details:

This PR aims to restrict copilots from performing certain actions on behalf of the owner.
As A Part of Solution Primarily Show DelegetNoAccessModal when copilot tries to perform any restricted action

This means that instead of hiding buttons from the copilot, we simply display the restriction modal when they attempt to interact with them.

Following the same logic, in this case, we should also show the restriction modal when the copilot clicks on the reveal button.

@greg-schroeder
Copy link
Contributor

Confirming with the team the best approach: https://expensify.slack.com/archives/C03U7DCU4/p1741013290450699

Stand by

@greg-schroeder
Copy link
Contributor

greg-schroeder commented Mar 3, 2025

All right; team consensus is to mirror OldDot behavior and simply hide the "Show Details" button; this shouldn't be accessible by Copilots in the first place. While this PR did introduce the DelegateNoAccessModal, it has a pretty broad scope and covers many different actions. We want to keep the behavior the same across platforms when we can.

I updated the issue title and OP

@greg-schroeder greg-schroeder changed the title [$250] Wallet - Missing email in "Please enter the magic code sent to" when revealing card details as copilot [$250] Hide "Reveal Details" button when logged in as Copilot Mar 3, 2025
@greg-schroeder
Copy link
Contributor

I think this might actually be more of a #retain issue rather than #quality given the change in scope

@greg-schroeder
Copy link
Contributor

Next up is proposal adjustments most likely, then review by @fedirjh

@melvin-bot melvin-bot bot removed the Overdue label Mar 6, 2025
Copy link

melvin-bot bot commented Mar 7, 2025

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

@fedirjh
Copy link
Contributor

fedirjh commented Mar 7, 2025

@twilight2294's proposal looks good to me.

@ChavdaSachin
Copy link
Contributor

@fedirjh I am unsure why my proposal did not look good enough. But My proposal mostly is similar to the selected proposal and includes an extra case which should be handled here.

And I request even though if you don't change your mind and want to continue with the currently selected proposal, then please ask contributor to handle the case 2 I mentioned in my proposal as well to avoid an extra issue.

Thank you.

@twilight2294
Copy link
Contributor

@fedirjh can you also put the 🎀 👀 🎀 C+ reviewed label here please, internal engineer didn't get assigned here

@melvin-bot melvin-bot bot added the Overdue label Mar 10, 2025
@greg-schroeder
Copy link
Contributor

Friendly bump @fedirjh on the above so we can assign someone to confirm contributor assignment <3

@melvin-bot melvin-bot bot removed the Overdue label Mar 10, 2025
@fedirjh
Copy link
Contributor

fedirjh commented Mar 10, 2025

@ChavdaSachin Thanks for your proposal and for highlighting the extra case! The selected proposal is complete, includes a testing plan, and addresses the main issue. Since your proposal is nearly identical, I proceeded with the selected one. The extra case you mentioned should be handled in review. Appreciate your input!

@fedirjh
Copy link
Contributor

fedirjh commented Mar 10, 2025

@twilight2294's proposal looks good to me. Let's cover the extra case mentioned by @ChavdaSachin.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Mar 10, 2025

Triggered auto assignment to @iwiznia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 10, 2025
Copy link

melvin-bot bot commented Mar 10, 2025

📣 @ChavdaSachin 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@iwiznia
Copy link
Contributor

iwiznia commented Mar 10, 2025

Proposal looks good, is there other places where we ask for a validate code that we need to update too besides this one?

@twilight2294
Copy link
Contributor

@iwiznia can you assign this to me? the C+ summary is for me 😅

@iwiznia
Copy link
Contributor

iwiznia commented Mar 10, 2025

Oh crap, sorry, yes I totally assigned the wrong person

@iwiznia iwiznia assigned twilight2294 and unassigned fedirjh Mar 10, 2025
Copy link

melvin-bot bot commented Mar 10, 2025

📣 @twilight2294 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@iwiznia
Copy link
Contributor

iwiznia commented Mar 10, 2025

@ChavdaSachin
Copy link
Contributor

@ChavdaSachin Thanks for your proposal and for highlighting the extra case! The selected proposal is complete, includes a testing plan, and addresses the main issue. Since your proposal is nearly identical, I proceeded with the selected one. The extra case you mentioned should be handled in review. Appreciate your input!

@fedirjh I think such simple UI changes does not need any tests.
You could see when I implemented most of the copilot restrictions here we haven't build any test.
And this issue could be said as missed cases for the same PR

@greg-schroeder
Copy link
Contributor

Hey @twilight2294 let me know once you have a draft PR up so we can link it to the issue, thanks!

@twilight2294
Copy link
Contributor

@fedirjh PR ready for review

@twilight2294
Copy link
Contributor

Hey @twilight2294 let me know once you have a draft PR up so we can link it to the issue, thanks!

@greg-schroeder PR is here:

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: HIGH
Development

No branches or pull requests

8 participants