Removing payment method actions model when payment method is removed from another device

[abdulrahuman5196]

@PauloGasparSv @0xmiroslav

Details

Removing payment method actions model when payment method is removed from another device

Fixed Issues

$ #14524
PROPOSAL: #14524 (comment)

Tests

1. Go to payment methods list page
2. Click on a payment method, so that 'Make Default' or 'Delete' modal comes up.
3. Now the delete the payment method from another device
4. Make sure the modal is dismissed as soon as the payment method is deleted on the original device

• Verify that no errors appear in the JS console

Offline tests

1. Go to payment methods list page
2. Click on a payment method, so that 'Make Default' or 'Delete' modal comes up.
3. Now the delete the payment method from another device
4. Make sure the modal is dismissed as soon as the payment method is deleted on the original device when it is online

QA Steps

1. Go to payment methods list page
2. Click on a payment method, so that 'Make Default' or 'Delete' modal comes up.
3. Now the delete the payment method from another device
4. Make sure the modal is dismissed as soon as the payment method is deleted on the original device when it is online

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If a new component is created I verified that:
  • A similar component doesn't exist in the codebase
  • All props are defined accurately and each prop has a /** comment above it */
  • The file is named correctly
  • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • The only data being stored in the state is data necessary for rendering and nothing else
  • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • All JSX used for rendering exists in the render method
  • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

All payment methods where deleted from another device

Web

web.mov

Mobile Web - Chrome

mweb.chrome.mp4

Mobile Web - Safari

mweb.safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mp4

[melvin-bot]

@PauloGasparSv @0xmiroslav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[abdulrahuman5196]

@0xmiroslav Gentle ping to review the PR

[0xmiros]

reviewing today

[0xmiros]

Can we also reset state on closing default/delete menu?

[0xmiros]

@abdulrahuman5196 sorry for delay but I was testing various edge cases.
Please pull from latest main

[abdulrahuman5196]

Can we also reset state on closing default/delete menu?
@0xmiroslav This is already done in the PR. Will update the PR for the 'add comments' review alone

[0xmiros]

This was missing originally but let's set default state value of selectedPaymentMethodType in constructor. '' should be fine.

[abdulrahuman5196]

Hi, @0xmiroslav Added the comments and the default selectedPaymentMethodType in constructor as requested.

[abdulrahuman5196]

Re-Requesting @0xmiroslav 's review somehow removed @PauloGasparSv review. I am not able to add back. Kindly review the PR.

[0xmiros]

                <PasswordPopover
                    isVisible={this.state.shouldShowPasswordPrompt}
                    onClose={this.hidePasswordPrompt}
                    anchorPosition={{
                        top: this.state.anchorPositionTop,
                        right: this.state.anchorPositionRight,
                    }}
                    onSubmit={(password) => {
-                       this.hidePasswordPrompt();
+                       this.setState({shouldShowPasswordPrompt: false});
                        this.makeDefaultPaymentMethod(password);
                    }}
                    submitButtonText={this.state.passwordButtonText}
                />

We cannot call this.hidePasswordPrompt(); on submit because cleared selected method is referenced again when make default.

Instead of above code diff, I prefer introducing flag parameter on hideDefaultDeleteMenu and hidePasswordPrompt.
Something like this:
hideDefaultDeleteMenu(shouldClearSelectedData), hidePasswordPrompt(shouldClearSelectedData)

[0xmiros]

Also, I don't think we need this workaround anymore

App/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js

Lines 251 to 253 in f8d7a3a

	// Due to iOS modal freeze issue, password modal freezes the app when closed.
	// LayoutAnimation undoes the running animation.
	LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));

@PauloGasparSv this is out of scope but I suggest to fix iOS freeze issue (#10509) on this PR.
After above code (introduced in #8173) removal, this issue is gone.

[abdulrahuman5196]

                <PasswordPopover
                    isVisible={this.state.shouldShowPasswordPrompt}
                    onClose={this.hidePasswordPrompt}
                    anchorPosition={{
                        top: this.state.anchorPositionTop,
                        right: this.state.anchorPositionRight,
                    }}
                    onSubmit={(password) => {
-                       this.hidePasswordPrompt();
+                       this.setState({shouldShowPasswordPrompt: false});
                        this.makeDefaultPaymentMethod(password);
                    }}
                    submitButtonText={this.state.passwordButtonText}
                />

We cannot call this.hidePasswordPrompt(); on submit because cleared selected method is referenced again when make default.

Instead of above code diff, I prefer introducing flag parameter on hideDefaultDeleteMenu and hidePasswordPrompt. Something like this: hideDefaultDeleteMenu(shouldClearSelectedData), hidePasswordPrompt(shouldClearSelectedData)

@0xmiroslav Nice callout. I have made the change to add a flag to reset selected method data and also cleared the selected method data after make default is called.

[abdulrahuman5196]

Also, I don't think we need this workaround anymore

App/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js

Lines 251 to 253 in f8d7a3a

	// Due to iOS modal freeze issue, password modal freezes the app when closed.
	// LayoutAnimation undoes the running animation.
	LayoutAnimation.configureNext(LayoutAnimation.create(50, LayoutAnimation.Types.easeInEaseOut, LayoutAnimation.Properties.opacity));

@PauloGasparSv this is out of scope but I suggest to fix iOS freeze issue (#10509) on this PR. After above code (introduced in #8173) removal, this issue is gone.

@0xmiroslav I not sure of this though. If we want to remove the code, i can remove it as a separate PR. Because if this change causes some regression we would end up revert this whole PR.

[0xmiros]

ok freeze issue can still be separate, though it's annoying while testing iOS app.

@abdulrahuman5196 We can replace all usages of this.setState({shouldShowDefaultDeleteMenu: false}); with hideDefaultDeleteMenu. Same applies to hidePasswordPrompt

[0xmiros]

@also, make sure to test various cases on your side and confirm expected results so it doesn't cause any regressions:
After selecting any payment method,

• just close default/delete modal
• tap Make default payment method on default/delete modal and close password modal
• tap Make default payment method on default/delete modal, enter password and click Make default payment method on password modal
• tap Delete on default/delete modal and cancel or close delete confirm modal
• tap Delete on default/delete modal and tap Delete on delete confirm modal

Also, try to delete selected payment method from another device on every modal show.

[abdulrahuman5196]

ok freeze issue can still be separate, though it's annoying while testing iOS app.

@abdulrahuman5196 We can replace all usages of this.setState({shouldShowDefaultDeleteMenu: false}); with hideDefaultDeleteMenu. Same applies to hidePasswordPrompt

@0xmiroslav
I can think of this as an code improvement change on existing code and not related to this particular fix.
Anyways i made the change and pushed it to this PR. But I would suggest to refrain from making not related changes to avoid regression from those changes in this fix, which would end up reverting this PR.
Kindly let me know if i am wrong.

[abdulrahuman5196]

@0xmiroslav

This also happens on production even when delete from same device so I think we should do nothing here.

Yes. I agree. This seems to be a different issue than what this PR is concerned.

[0xmiros]

@abdulrahuman5196 have you noticed that when add fake paypal, it deletes already existing payment methods on another device?

[abdulrahuman5196]

@abdulrahuman5196 have you noticed that when add fake paypal, it deletes already existing payment methods on another device?

@0xmiroslav I haven't seen this issue. I always use some random text as paypal id. But it gets added obviously.

[abdulrahuman5196]

Hi, @0xmiroslav I could see you had completed the reviewers checklist. Thank you for that. If we are good, could you kindly approve the PR?
Or if anything else is required, could you kindly let me know so that i can provide the required information.

[0xmiros]

We haven't tested yet with debit card so cannot approve yet, though I believe it will work. I asked @PauloGasparSv for help testing. We'll keep you posted.

[abdulrahuman5196]

Thank you for quick response @0xmiroslav

[PauloGasparSv]

Had to shift a focus a bit and will have to finish the review tomorrow, sorry guys! Will do this first thing in the morning.
The changes are looking great to me, just have to finish testing in all platforms

[abdulrahuman5196]

@PauloGasparSv Thank you.
Made both the changes requested. Kindly let me know if anything else is required.

[abdulrahuman5196]

@PauloGasparSv Kindly let me know if anything else is required from my end. Would be happy to sort it out sooner.

[PauloGasparSv]

LGTM! :D

Tested the debit card (check the desktop test) and also deleting a Workspace created VBA from the Workspace Page (check desktop and web tests)

@0xmiroslav do you have any more inputs on this or can we merge it? : )

Web

Web.Workspace.mov

Mobile Web - Chrome

Android.Web.mov

Mobile Web - Safari

ioS.Web.mov

Desktop

Desktop.mov

iOS

https://user-images.githubusercontent.com/6564265/217911574-2ba4ea8e-8bd9-48d2-94e0-7e23c72c7681.mov

Android

Android.mov

---

BTW, while testing I found the following unrelated bug:

Bug

StrangeBug.mov

We had an internal discussion and I'll create an internal issue for it (because it requires setting up the Expensify Card and a Domain)

[0xmiros]

@PauloGasparSv thanks for full testing. Desktop and web videos look good to me.
As long as it's unrelated bug (not caused by this PR), I think we can merge

[PauloGasparSv]

Merging then!

[PauloGasparSv]

Strange, first merge attempt failed with a message about signed commits.
Refreshed and tried again and the merge worked.

[OSBotify]

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

[OSBotify]

🚀 Deployed to staging by https://github.com/PauloGasparSv in version: 1.2.69-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

[OSBotify]

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.69-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅