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

[HOLD for payment 2024-08-01] [$125] [Payment card / Subscription] Make payment card only display last four digits #45309

Closed
blimpich opened this issue Jul 11, 2024 · 31 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@blimpich
Copy link
Contributor

blimpich commented Jul 11, 2024

Problem

Right now when you add a payment card to the new subscription page, it shows the 16 characters instead of just four:

Image

It should show just the last four:

Image

Solution

Lets fix this by only showing the last four digits in the subscription page.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a9042029c460e7a0
  • Upwork Job ID: 1811455490190961117
  • Last Price Increase: 2024-07-11
Issue OwnerCurrent Issue Owner: @CortneyOfstad / @isabelastisser
@blimpich blimpich self-assigned this Jul 11, 2024
@blimpich blimpich added Engineering Bug Something is broken. Auto assigns a BugZero manager. labels Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Jul 11, 2024
@blimpich blimpich added External Added to denote the issue can be worked on by a contributor and removed Daily KSv2 labels Jul 11, 2024
@melvin-bot melvin-bot bot changed the title [Payment card / Subscription] Make payment card only display last four digits [$250] [Payment card / Subscription] Make payment card only display last four digits Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Jul 11, 2024
@blimpich blimpich changed the title [$250] [Payment card / Subscription] Make payment card only display last four digits [$125] [Payment card / Subscription] Make payment card only display last four digits Jul 11, 2024
Copy link

melvin-bot bot commented Jul 11, 2024

Upwork job price has been updated to $125

@blimpich
Copy link
Contributor Author

Lowering price because this is pretty straightforward string manipulation.

@blimpich
Copy link
Contributor Author

Anyone who tests this should be able to simulate adding a test card by populating the fundList onyx key based off the types we define in the frontend, or you can add a real credit card, though know that we currently don't have a way of deleting credit cards added to the app.

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jul 11, 2024

Proposal

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

[Payment card / Subscription] Make payment card only display last four digits

What is the root cause of that problem?

Value is not sliced.

<Text style={styles.textStrong}>{translate('subscription.cardSection.cardEnding', {cardNumber: defaultCard?.accountData?.cardNumber})}</Text>

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

Slice the value to obtain last four digits. defaultCard?.accountData?.cardNumber.slice(-4)

fundlist can be populated for testing by using object below.

{
Card: {accountData: {addressName: 'Max', cardYear: 2029, cardMonth: 9, currency: 'USD', cardNumber: '1234567887654321', additionalData: {isBillingCard: true}}},
}

What alternative solutions did you explore? (Optional)

Result

Monosnap (83) New Expensify 2024-07-11 23-33-29

@kabeer95
Copy link

Feature Request: Mask Payment Card Numbers on Expensify

Summary:

To enhance security and compliance, we propose masking payment card numbers on Expensify, displaying only the last four digits. This feature will provide an additional layer of protection for sensitive payment information.

Current State:

Currently, Expensify displays the full payment card number, which may pose a security risk if unauthorized access is gained.

Proposed Solution:

Implement a feature to mask payment card numbers, displaying only the last four digits. For example:

Original card number: 424242XXXXXX4242
Masked card number: ************4242
Benefits:

Enhanced Security: Masking payment card numbers reduces the risk of unauthorized access to sensitive information.
Compliance: This feature aligns with industry standards for payment card security, such as PCI-DSS.
User Trust: By protecting payment information, Expensify demonstrates a commitment to user security and trust.
Implementation:

To implement this feature, we recommend the following:

Update the payment card number display to show only the last four digits.
Store the full payment card number securely, using industry-standard encryption and access controls.
Ensure that the masked card number is displayed consistently throughout the Expensify platform.
Estimated Development Time:

We estimate 20 hours to implement this feature and mask payment card numbers on Expensify.

Priority:

We consider this feature a high priority, as it directly impacts the security and trust of Expensify users.

Please let us know if you would like to discuss this feature further or proceed with implementation

@tienifr
Copy link
Contributor

tienifr commented Jul 13, 2024

Proposal

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

  • Make payment card only display last four digits

What is the root cause of that problem?

  • In CardSection, we display the card number with its fully 16 digits in two components like the image below:

Screenshot 2024-07-13 at 12 26 51

In card title:

<Text style={styles.textStrong}>{translate('subscription.cardSection.cardEnding', {cardNumber: defaultCard?.accountData?.cardNumber})}</Text>

In card billing status:

<SubscriptionBillingBanner
title={billingStatus.title}
subtitle={billingStatus.subtitle}
isError={billingStatus.isError}
icon={billingStatus.icon}
rightIcon={billingStatus.rightIcon}
onRightIconPress={handleBillingBannerClose}
rightIconAccessibilityLabel={translate('common.close')}
/>
);
}
return (
<>
<Section
title={translate('subscription.cardSection.title')}
subtitle={sectionSubtitle}
isCentralPane
titleStyles={styles.textStrong}
subtitleMuted
banner={BillingBanner}
>
<View style={[styles.mt8, styles.mb3, styles.flexRow]}>
{!isEmptyObject(defaultCard?.accountData) && (
<View style={[styles.flexRow, styles.flex1, styles.gap3]}>
<Icon
src={Expensicons.CreditCard}
additionalStyles={styles.subscriptionAddedCardIcon}
fill={theme.icon}
medium
/>
<View style={styles.flex1}>
<Text style={styles.textStrong}>{translate('subscription.cardSection.cardEnding', {cardNumber: defaultCard?.accountData?.cardNumber})}</Text>
<Text style={styles.mutedNormalTextLabel}>
{translate('subscription.cardSection.cardInfo', {
name: defaultCard?.accountData?.addressName,
expiration: `${cardMonth} ${defaultCard?.accountData?.cardYear}`,
currency: defaultCard?.accountData?.currency,
})}
</Text>
</View>
<CardSectionActions />
</View>
)}
</View>
{isEmptyObject(defaultCard?.accountData) && <CardSectionDataEmpty />}
{billingStatus?.isRetryAvailable !== undefined && (
<Button
text={translate('subscription.cardSection.retryPaymentButton')}
isDisabled={isOffline || !billingStatus?.isRetryAvailable}
isLoading={subscriptionRetryBillingStatusPending}
onPress={handleRetryPayment}
style={[styles.w100, styles.mt5]}
large
/>
)}
{!!account?.hasPurchases && (
<MenuItem
shouldShowRightIcon
icon={Expensicons.History}
wrapperStyle={styles.sectionMenuItemTopDescription}
title={translate('subscription.cardSection.viewPaymentHistory')}
titleStyle={styles.textStrong}
style={styles.mt5}
onPress={() => Navigation.navigate(ROUTES.SEARCH.getRoute(CONST.SEARCH.TAB.ALL))}
hoverAndPressStyle={styles.hoveredComponentBG}
/>
)}
{!!(subscriptionPlan && account?.isEligibleForRefund) && (
<MenuItem
shouldShowRightIcon
icon={Expensicons.Bill}
wrapperStyle={styles.sectionMenuItemTopDescription}
title={translate('subscription.cardSection.requestRefund')}
titleStyle={styles.textStrong}
disabled={isOffline}
onPress={() => setIsRequestRefundModalVisible(true)}
/>
)}
</Section>
{account?.isEligibleForRefund && (
<ConfirmModal
title={translate('subscription.cardSection.requestRefund')}
isVisible={isRequestRefundModalVisible}
onConfirm={requestRefund}
onCancel={() => setIsRequestRefundModalVisible(false)}
prompt={
<>
<Text style={styles.mb4}>{translate('subscription.cardSection.requestRefundModal.phrase1')}</Text>
<Text>{translate('subscription.cardSection.requestRefundModal.phrase2')}</Text>
</>
}
confirmText={translate('subscription.cardSection.requestRefundModal.confirm')}
cancelText={translate('common.cancel')}
danger
/>
)}
</>
);
}
CardSection.displayName = 'CardSection';
export default CardSection;

with the subtitle is from:
subtitle: translate('subscription.billingBanner.billingDisputePending.subtitle', {amountOwed, cardEnding}),

subtitle: translate('subscription.billingBanner.cardAuthenticationRequired.subtitle', {cardEnding}),

with cardEnding is from:
const cardEnding = accountData?.cardNumber ?? '';

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

    const cardEnding = (accountData?.cardNumber ?? '')?.slice(-4);

and

<Text style={styles.textStrong}>{translate('subscription.cardSection.cardEnding', {cardNumber: defaultCard?.accountData?.cardNumber})}</Text>

to:

                                <Text style={styles.textStrong}>{getPaymentMethodDescription(defaultCard?.accountType, defaultCard?.accountData)}</Text>

What alternative solutions did you explore? (Optional)

Result:

image

@sobitneupane
Copy link
Contributor

Thanks for the proposal everyone.

Proposal from @tienifr looks good to me.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Jul 15, 2024

Current assignee @blimpich is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 15, 2024
@tienifr
Copy link
Contributor

tienifr commented Jul 15, 2024

@blimpich Based on comment, to test this issue, I just need to:

simulate adding a test card by populating the fundList onyx key based off the types we define in the frontend

without:

add a real credit card

right?

@blimpich
Copy link
Contributor Author

Yes that should be fine 👍

@melvin-bot melvin-bot bot added Reviewing Has a PR in review and removed Daily KSv2 labels Jul 17, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Jul 25, 2024
@melvin-bot melvin-bot bot changed the title [$125] [Payment card / Subscription] Make payment card only display last four digits [HOLD for payment 2024-08-01] [$125] [Payment card / Subscription] Make payment card only display last four digits Jul 25, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 25, 2024
Copy link

melvin-bot bot commented Jul 25, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jul 25, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.11-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-08-01. 🎊

For reference, here are some details about the assignees on this issue:

  • @sobitneupane requires payment through NewDot Manual Requests
  • @tienifr requires payment through NewDot Manual Requests

Copy link

melvin-bot bot commented Jul 25, 2024

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:

  • [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
  • [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@sobitneupane] Determine if we should create a regression test for this bug.
  • [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@CortneyOfstad / @isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@isabelastisser
Copy link
Contributor

I'm back and taking this back. Thanks, @CortneyOfstad !

@isabelastisser
Copy link
Contributor

Not overdue.

@melvin-bot melvin-bot bot removed the Overdue label Jul 31, 2024
@isabelastisser
Copy link
Contributor

Hey @sobitneupane, please complete the BZ list above. Thanks!

Copy link

melvin-bot bot commented Aug 1, 2024

Payment Summary

Upwork Job

BugZero Checklist (@isabelastisser)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1811455490190961117/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@sobitneupane
Copy link
Contributor

sobitneupane commented Aug 5, 2024

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:

  • [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#44072

  • [@sobitneupane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

#44072 (comment)

  • [@sobitneupane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

Not required.

  • [@sobitneupane] Determine if we should create a regression test for this bug.

Yes.

  • [@sobitneupane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

#45309 (comment)

@melvin-bot melvin-bot bot removed the Overdue label Aug 5, 2024
@sobitneupane
Copy link
Contributor

Regression Test Proposal:

  • Go to /settings/subscription.
  • Add a new card if there is no.
  • Verify card title displays only last four digits of your card.

Do we agree 👍 or 👎

@isabelastisser
Copy link
Contributor

@blimpich, can you please review the regression test above? Thanks!

@isabelastisser
Copy link
Contributor

Payment summary:

C+ review @sobitneupane owed $250 via NewDot
Contributor @tienifr owed $250 via NewDot

@isabelastisser
Copy link
Contributor

All set.

@garrettmknight
Copy link
Contributor

$250 approved for @sobitneupane based on this summary.

@garrettmknight
Copy link
Contributor

$250 approved for @tienifr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Status: Done
Development

No branches or pull requests

8 participants