[HOLD for payment 2023-02-02] [$1000] Add payment message comes only after sometime when the bank is removed.

[kavimuru]

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

---

Action Performed:

1. Go to settings -> payment
2. Make sure you have only one payment method enabled
3. Delete the payment method

Expected Result

Add payment message should come immediately

Actual Result

Add payment message comes after a delay

Workaround:

unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.54-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
Notes/Photos/Videos:

Recording.1277.mp4

payment.method.delay.mov

Expensify/Expensify Issue URL:
Issue reported by: @abdulrahuman5196
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673543185153959

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c49a234eb88604e9
• Upwork Job ID: 1616099123063414784
• Last Price Increase: 2023-01-19

[maddylewis]

it sounds like this proposed solution could work as a fix for this - #14046 (comment)

[melvin-bot]

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

[melvin-bot]

Current assignee @maddylewis is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

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

[abdulrahuman5196]

Copy pasting the original solution from #14046 (comment)

Root Cause
In PaymentMethodList.js we have set the add payment method message to be displayed as 'ListEmptyComponent' which will only display when we provide empty data to FlatList. But in this case when we delete the payment method, we still have the payment method data with "pendingAction": "delete" value as noted below which will only go away after we get response from server(also request still in network queue as well). So we still pass a data with payment entry to FlatList and flatList doesn't show the add payment menthod message.

{
"title": "Plaid Checking",
"description": "Account ending in 0000",
"methodID": 1854845,
"key": "bankAccount-1854845",
"accountType": "bankAccount",
"isDefault": false,
"pendingAction": "delete",
"iconSize": 40,
"iconFill": null,
"wrapperStyle": null
}

Note: leaving extra data as not required.

Why is the single payment data entry with pendingAction not shown?
We send a single payment method data to Flatlist which uses OfflineWithFeedback component for each row which hides its children when it has pendingAction of 'delete' - https://github.com/Expensify/App/blob/main/src/components/OfflineWithFeedback.js#L90. So visually the row doesn't have anything.

--- a/src/pages/settings/Payments/PaymentMethodList.js
+++ b/src/pages/settings/Payments/PaymentMethodList.js
@@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
 import {FlatList} from 'react-native';
 import lodashGet from 'lodash/get';
 import {withOnyx} from 'react-native-onyx';
+import {withNetwork} from '../../../components/OnyxProvider';
 import styles from '../../../styles/styles';
 import * as StyleUtils from '../../../styles/StyleUtils';
 import MenuItem from '../../../components/MenuItem';
@@ -122,6 +123,10 @@ class PaymentMethodList extends Component {
             combinedPaymentMethods = _.filter(combinedPaymentMethods, paymentMethod => paymentMethod.accountType === this.props.filterType);
         }
 
+        if (!this.props.network.isOffline) {
+            combinedPaymentMethods = _.filter(combinedPaymentMethods, paymentMethod => paymentMethod.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE);
+        }
+
         combinedPaymentMethods = _.map(combinedPaymentMethods, paymentMethod => ({
             ...paymentMethod,
             onPress: e => this.props.onPress(e, paymentMethod.accountType, paymentMethod.accountData, paymentMethod.isDefault, paymentMethod.methodID),
@@ -254,6 +259,7 @@ PaymentMethodList.defaultProps = defaultProps;
 
 export default compose(
     withLocalize,
+    withNetwork(),
     withOnyx({
         bankAccountList: {
             key: ONYXKEYS.BANK_ACCOUNT_LIST,

This UI filter prevents any pending action to be sent to the flatlist which solves the issue. We can also add errors check and other pending actions check in the filter if required.

[s77rt]

Proposal

diff --git a/src/pages/settings/Payments/PaymentMethodList.js b/src/pages/settings/Payments/PaymentMethodList.js
index b1be756e2a..92c9c04f62 100644
--- a/src/pages/settings/Payments/PaymentMethodList.js
+++ b/src/pages/settings/Payments/PaymentMethodList.js
@@ -11,6 +11,7 @@ import Button from '../../../components/Button';
 import Text from '../../../components/Text';
 import compose from '../../../libs/compose';
 import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
+import {withNetwork} from '../../../components/OnyxProvider';
 import ONYXKEYS from '../../../ONYXKEYS';
 import CONST from '../../../CONST';
 import * as Expensicons from '../../../components/Icon/Expensicons';
@@ -116,7 +117,11 @@ class PaymentMethodList extends Component {
     getFilteredPaymentMethods() {
         // Hide any billing cards that are not P2P debit cards for now because you cannot make them your default method, or delete them
         const filteredCardList = _.filter(this.props.cardList, card => card.accountData.additionalData.isP2PDebitCard);
-        let combinedPaymentMethods = PaymentUtils.formatPaymentMethods(this.props.bankAccountList, filteredCardList, this.props.payPalMeData);
+        let combinedPaymentMethods = _.filter(PaymentUtils.formatPaymentMethods(this.props.bankAccountList, filteredCardList, this.props.payPalMeData), paymentMethod => (
+            this.props.network.isOffline
+            || paymentMethod.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
+            || !_.isEmpty(paymentMethod.errors)
+        ));
 
         if (!_.isEmpty(this.props.filterType)) {
             combinedPaymentMethods = _.filter(combinedPaymentMethods, paymentMethod => paymentMethod.accountType === this.props.filterType);
@@ -254,6 +259,7 @@ PaymentMethodList.defaultProps = defaultProps;
 
 export default compose(
     withLocalize,
+    withNetwork(),
     withOnyx({
         bankAccountList: {
             key: ONYXKEYS.BANK_ACCOUNT_LIST,

RCA

When you remove a payment method, it's not removed right away, it's only made invisible until we get a response from the server (pusher) we remove it and thus the delay.

The fix is to filter out items that are being deleted (not actually deleted but in the process of deletion).

The filter must be exactly as in the proposal (3 conditions) because we use <OfflineWithFeedback /> underneath and those conditions are derived from here:

App/src/components/OfflineWithFeedback.js

Line 90 in 304d759

const hideChildren = !props.network.isOffline && props.pendingAction === 'delete' && !hasErrors;

---

This issue has occurred twice already and this is the third time it's being reported. #13615 #14142

[thienlnam]

Still reproducible on main, looks like we already have a couple good proposals here for review cc @eVoloshchak

Screen.Recording.2023-01-19.at.10.51.27.AM.mov

[thienlnam]

Actually, @jasperhuangg would you consider this something that should have been resolved in #14046? Or should we have it taken care of in this issue

[abdulrahuman5196]

@thienlnam
Just to note: I was one for proposal at #14046 (comment) (But my proposal was not implemented though)and reporter of this issue. The core issue behind #14046 is “Add payment message never shows up when switching from offline to online as it was not removed from onyx” and current issue is "Add payment message only shows up after sometime when user is online itself(after successful API call) because we don't have a UI filter"

[thienlnam]

Yup, I'm aware of that - but this flow should have been tested while getting fixed in the previous issue and could just be considered a regression of the other issue.

I do see that you made comment of this in your other proposal, and so I agree that we can take this as a separate issue.

[thienlnam]

While both proposals are similar, I'm going to move forward with @s77rt 's proposal as it prevents another edge case where we have an error deleting the member, but still need it to show up while the user is offline so they can clear the error

Was pointed out that the first proposal had the same consideration

[abdulrahuman5196]

@thienlnam I also initially called out the errors check in my proposal
#14262 (comment)

This UI filter prevents any pending action to be sent to the flatlist which solves the issue. We can also add errors check and other pending actions check in the filter if required.

Even called out the same in original proposal on #14046

[thienlnam]

Ah my bad missed that comment, please make sure to add it in your code changes next time - sorry @s77rt

[abdulrahuman5196]

@thienlnam PR is ready for the fix - #14483
@eVoloshchak

[melvin-bot]

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

[melvin-bot]

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

• Adding UI filter to exclude delete pending items to be sent to FlatList when online #14483

If no regressions arise, payment will be issued on 2023-02-02. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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:

• [@eVoloshchak / @thienlnam] The PR that introduced the bug has been identified. Link to the PR:
• [@eVoloshchak / @thienlnam] 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:
• [@eVoloshchak / @thienlnam] 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:
• [@maddylewis] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://expensify.testrail.io/index.php?/cases/view/1971178&group_by=cases:section_id&group_id=229064&group_order=asc&display_deleted_cases=0

[abdulrahuman5196]

Applied

[maddylewis]

paid!

[maddylewis]

buddy check for regression test here - https://expensify.slack.com/archives/C01SKUP7QR0/p1675798332736439

[maddylewis]

@eVoloshchak and @thienlnam - lmk if we're good to check off your action items in this comment - #14262 (comment)

in the meantime, ill get a regression test added asap.

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: Add Red Brick Road Offline Handling to Payments List #10333

• 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: Add Red Brick Road Offline Handling to Payments List #10333 (comment)

• 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:

I the third item is the same as in #13615 (comment)

For this, I don't really think there was a regression or we could've caught this earlier. This behavior was introduced back when we originally added workspaces into newDot. We have solid documentation on ensuring actions are done optimistically, so I think that should be sufficient in this case.