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 2022-02-08] [HOLD for payment 2022-01-25] Add the Transfer Balance Page #3922

Closed
5 tasks done
JmillsExpensify opened this issue Jul 8, 2021 · 89 comments
Closed
5 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@JmillsExpensify
Copy link

JmillsExpensify commented Jul 8, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

This is a new E.cash initiative. See expected result.

Expected Result:

image

  1. Create a new file /PaymentPages/TransferBalancePage.js

  2. Add a route for the transfer balance page: ROUTES.SETTINGS_TRANSFER_BALANCE = "settings/payments/transfer-balance"

  3. Add a "Transfer Balance" button to PaymentsPage.js that links to the new transfer page

    • Icon:
      • We will need to add the transfer icon to the assets/images folder, and then export it in src/components/Icon/Expensicons.js.
      • We will then import it in PaymentsPage.js and pass that icon as a param
    • OnPress:
      • Navigate to the transfer balance page
    • PrimaryText: “Transfer Balance”
    • shouldShowRightArrow: True
    • The button will only be shown if there is a balance above $0
  4. Add the <currentBalance> component to the top

  5. Add 2 <menuItem> components for instant/slow transfer

    • Instant:
      • Icon: LightningBolt (new)
      • PrimaryText: Instant
      • SecondaryText: 1.5% fee ($0.25 minimum)
      • onPress: () => this.setState({transferType: Instant})
    • ACH:
      • Icon: Bank
      • PrimaryText: 1-3 Business Days
      • SecondaryText: No Fee
      • onPress: () =. this.setState({transferType: ACH})
    • When the state changes, the item that was clicked will show a different border and icon color
  6. Create the "Which Account" section

    • This should be a <MenuItem>
    • For an example of the rendering take a look at PaymentMethodList.js
    • The initial account should be the account defined in the onyx prop USER_WALLET.linkedBankAccount
    • The onPress for the MenuItem will navigate to ChooseTransferAccountPage
  7. Create a new page ChooseTransferAccountPage.js (screenshot 3)

    • The route for this page will be ROUTES.SETTINGS_PAYMENTS_CHOOSE_TRANSFER_ACCOUNT = “settings/payments/choose-transfer-Account”
    • The page will display a <PaymentMethodList>
      • When a method is selected, we will route back to TransferBalancePage with the account type and ID
      • The selected method should be displayed in the choose account section
  8. Create the "Transfer" button

    • This should be a <PressableOpacity>
    • The onPress should:
      • Call PaymentMethods.TransferWalletBalance() (described below)
      • Display a <FullScreenLoadingIndicator>
      • When the API command is complete
        • Hide the loading indicator
        • Display a success modal (The far right screenshot) that on submit will reroute to PaymentsPage.js
        • This will be created by using the default react native component, similar to confirmModal.js
  9. Create a new method in PaymentMethods.js called TransferWalletBalance()

    • This should call a new method in API.js called TransferWalletBalance()
      • This will call the API command TransferWalletBalance (no parameters necessary)
    • Once the API call comes back positive this method should set USER_WALLET.balance = 0 in Onyx

Progress

Per this comment we are breaking this down to smaller pieces.

  • Do a simple PR to scaffold out some of the little changes first and add all the assets, consts, pages, etc (PR)

    • Add consts
    • Add Routes
    • Add blank pages for the new routes we are adding
    • Translations
    • API.js change
    • Move payment methods list building logic into PaymentUtils without modifying the original logic and use in PaymentMethodList.
  • Create the TransferBalancePage and have the account select feature disabled for now - we can test with just the default account and see that transfers can be initiated, errors handled, etc. (PR)

  • Create a new component that wraps AddPaymentMethodMenu (without modifying this component too much) and call it “AddBalanceTransferAccountButton”

  • Create a version of the PaymentMethodList that is selectable. It should not duplicate the logic, but wrap PaymentMethodList.

  • Build out the ChooseTransferBalanceAccount page and hook it up to TransferBalancePage so we can select an account.

Platform:

All platforms

Version Number: N/A
Logs: N/A
Notes/Photos/Videos: Please ask any questions in this issue.
Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/169395

View all open jobs on Upwork

@JmillsExpensify JmillsExpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 8, 2021
@MelvinBot
Copy link

Triggered auto assignment to @davidcardoza (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 8, 2021
@JmillsExpensify JmillsExpensify changed the title [P2P in E.cash] Add the Transfer Balance Page Add the Transfer Balance Page Jul 8, 2021
@JmillsExpensify
Copy link
Author

If you're coming from our Upwork posting (https://www.upwork.com/jobs/~01081fb3ff015abc6c), a heads up that we will prioritizing contributors that we have been actively working with. Thank you!

@MelvinBot
Copy link

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

@JmillsExpensify JmillsExpensify added Engineering External Added to denote the issue can be worked on by a contributor labels Jul 8, 2021
@MelvinBot MelvinBot added the Daily KSv2 label Jul 8, 2021
@Burhanuddin971
Copy link

I've submitted the proposal on Upwork. Please do check

@parasharrajat
Copy link
Member

Proposal

  1. We have to wait until Update Payments Page to show wallet balance, empty state, and more! #3890 is merged for some necessary functionality.
  2. I will create a new page /PaymentPages/TransferBalancePage.js and its routes to ROUTES.js
    ROUTES.SETTINGS_TRANSFER_BALANCE = "settings/payments/transfer-balance"
  3. on the Payments page, I will add the "Transfer Balance" menuItem as specified above. Which navigates to the new New TransferBalancePage page.
  4. Borrow src/components/CurrentWalletBalance.js and use it to show the wallet balance.
  5. Show 2 MenuItem components for instant/slow transfer mode. The selected mode will be highlighted using the border color as shown in the Mockups.
  6. Create the "Which Account" section which will show the selected Account via ONYSKEYS.USER_WALLET.linkedBankAccount
  7. Clicking this menu item will navigate to another page for selecting the bank account.
  8. I will create a new page ChooseTransferAccountPage.js accesible at ROUTES.SETTINGS_PAYMENTS_CHOOSE_TRANSFER_ACCOUNT = “settings/payments/choose-transfer-Account”
  9. The page will display a . When a method is selected, we will navigate back to TransferBalancePage with the account type and ID params. Now we will update the selected Account based on the param passed in the which account section.
  10. I will use the Button component to create the Transfer button.
  11. Transfer button will call the API method TransferWalletBalance() defined in the PaymentMethods action file.
  12. I will create the TransferWalletBalance action which calls the API with the TransferWalletBalance command.
  13. After success, we show the ConfirmModal with the changed copy shown in the Mockups and set USER_WALLET.balance = 0 in Onyx.
  14. On Confirming we close the modal and take the user back to the LHN.
  15. I will show a growl on the failure of the API call.

Question:

  1. Do we have to show the loader on the full page? But I think as per the pattern we are following app-wide, We should show the loader on the button.
  2. Do we need to show the growl on the failure of API call.

@tgolen
Copy link
Contributor

tgolen commented Jul 8, 2021

Thanks @parasharrajat I think your proposal is sound and we'd love to have you work on this since it's a bit more of an advanced project.

Do we have to show the loader on the full page?

I agree with you that showing the loader on the button would be best for now.

Do we need to show the growl on the failure of API call.

It's probably not totally necessary, but also easy to do, and nice to at least have an indication, so let's go ahead and add it.

One thing that wasn't really covered in this issue or the proposal is if there is any consideration that needs to be made for offline mode. Can the API requests just be queued and let the user know they are currently offline and it will be completed once they are back online? Not sure how exactly we want to handle that. cc @stitesExpensify @JmillsExpensify

@parasharrajat
Copy link
Member

if there is any consideration that needs to be made for offline mode. Can the API requests just be queued

@tgolen This topic requires some planning? We have already discussed this for IOU Request Screens. But there was no conclusion. There seems to be an issue on hold for that and @Julesssss knows better about that.

We should adopt a consistent plan for this. Either we architect these screens that support offline or we show a message and block access to it. And same should be followed across features like this.

@tgolen
Copy link
Contributor

tgolen commented Jul 8, 2021

Yeah, we are trying to get better at making the "offline consideration" part of our original planning and design process to get better at this as we move forward. If we can at least do something basic for now, I think that would be good, then once we have a more holistic UX for offline mode, we can come back and improve this.

@parasharrajat
Copy link
Member

Sure. Please let me know How do you want me to set it up. I think queueing this request won't be that hard but we may need to consider syncing the data when online to reflect actual state.

@tgolen
Copy link
Contributor

tgolen commented Jul 8, 2021

Yeah, I know we have a mechanism for queuing the requests, but I'm not sure about having the UI reflect the state when it comes back online. @marcaaron do you have any ideas or thoughts on how we approach this in other areas?

@marcaaron
Copy link
Contributor

if there is any consideration that needs to be made for offline mode. Can the API requests just be queued and let the user know they are currently offline and it will be completed once they are back online? Not sure how exactly we want to handle that

If the situation we are trying to handle while we are offline is the act of transferring the balance I think a couple of options would be:

  1. Detect that we are offline (easy) and block the interaction (plus tell the user to wait until they are online and try again)
  2. Detect we are offline and say the transfer was queued and will initiate the next time they are active.

I don't think there is anything we'd need to do specifically to make 2 happen. Once the user returns online the API request to transfer the balance should be made.

I think queueing this request won't be that hard but we may need to consider syncing the data when online to reflect actual state.

What data specifically are we trying to sync? I would presume that if the API request has a completion handler attached to it that updates data in Onyx (e.g. balance, etc) then it should execute when the API request finishes. It doesn't matter if that happens right away or when we move from offline to online and a queued request completes.

@parasharrajat
Copy link
Member

parasharrajat commented Jul 8, 2021

Thanks, @marcaaron. Do we have to persist with that request? in that case the handler will be lost.
And, Do we have to block a second request if one is in the queue?

@marcaaron
Copy link
Contributor

Do we have to persist with that request? in that case the handler will be lost.

Curious to hear more opinions on this, but it sounds out of scope for this issue and/or something that would have to be discussed more. The current network layer only supports this for requests that trigger Pusher event responses. So, we could handle it similar to report comments and send the response for any persisted request back to the user via Pusher instead of handling the direct response (but that requires an internal change).

And, Do we have to block a second request if one is in the queue?

I think ideally yes, although in the case of a balance transfer probably a duplicate request would just fail since we'd already have a transfer initiated, but I don't know the specifics behind how that is working in the BE (but doesn't seem like it would be a blocker).

Perhaps in the future we will have easily "persisted requests" that are totally serializable (with generic front end handlers) and de-dupable. But I think we have not really had a reason to do it so far and I'm not sure this is the best time either?

@parasharrajat
Copy link
Member

parasharrajat commented Jul 9, 2021

Sounds good. I will just queue the request for a retry and block further requests until the first one is completed.

@marcaaron
Copy link
Contributor

block further requests until the first one is completed

To clarify, I am not suggesting we do this. I don't think we need to worry about de-duplication or persistence right now. But would like to hear what @stitesExpensify thinks since he might know more about how this is working in the back end.

@stitesExpensify
Copy link
Contributor

I don't think we need to block further requests, it should be fine to do it multiple times

@parasharrajat
Copy link
Member

Requesting icons and High-resolution images for mockups. Thanks.

@MelvinBot MelvinBot removed the Overdue label Dec 28, 2021
@JmillsExpensify
Copy link
Author

@stitesExpensify Awesome! Should we go ahead and update the remaining todos within the OP as well?

@stitesExpensify
Copy link
Contributor

Sure, that makes sense to me! I'll update it.

@parasharrajat
Copy link
Member

Hopefully last PR #7200 for

Create a new component that wraps AddPaymentMethodMenu (without modifying this component too much) and call it “AddBalanceTransferAccountButton”

Create a version of the PaymentMethodList that is selectable. It should not duplicate the logic, but wrap PaymentMethodList.

Build out the ChooseTransferBalanceAccount page and hook it up to TransferBalancePage so we can select an account.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Jan 18, 2022
@botify botify changed the title Add the Transfer Balance Page [HOLD for payment 2022-01-25] Add the Transfer Balance Page Jan 18, 2022
@botify
Copy link

botify commented Jan 18, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.30-3 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 2022-01-25. 🎊

@MelvinBot MelvinBot removed the Overdue label Jan 18, 2022
@parasharrajat
Copy link
Member

parasharrajat commented Jan 25, 2022

Update: All PRs for the issue is done. But there were two small issues were reported by Shawn related to UI. I will send another small PR to cover that.

@stitesExpensify
Copy link
Contributor

Great, looking forward to it!

@parasharrajat
Copy link
Member

parasharrajat commented Jan 25, 2022

Submitted the PR #7373. Sorry for the delay in this task. There were many bottlenecks but I am happy it is done now.

Apart from it, I wanted to discuss the task compensation. First, the job is closed on Upwork. so a new job might be needed. The previous job was priced at $375. But that value does not satisfy the work done for this task.

There are a couple of reasons I feel that the budget should be increased.

  1. It was a big task looking at the number of changes for a single task.
  2. There were small changes in the requirements. I had to refactor the code for those changes. As the development span over a couple of months, there were many changes to data structure and variables which required refactoring many times.
  3. First PR was closed and I was requested to break it down into separate PRs which was time-consuming for me. (But it was a great experince. I realized there were a couple of issues that I noticed during this change)
  4. There were delays during the First PR review Feature: Transfer balance #4177.
  5. I created a total of 5 PRs for this issue.
  6. It was very hard to test it and I had to wait or use QA accounts to test it. Getting all the necessary information via questions and taking others' help was time-consuming.

Apart from it, the first PR went through N6-Hold and Company Offsite Hold.

@stitesExpensify
Copy link
Contributor

Thanks for all of the work @parasharrajat !
@JmillsExpensify I definitely agree that the compensation should be increased 👍

@marcaaron
Copy link
Contributor

+1 to increase for this. This one needed some extra love and attention, scope increased a bit, and took a fairly long time to complete at no fault of yours. Thanks for your patience with us and flexibility to do things the correct way @parasharrajat.

@parasharrajat
Copy link
Member

Thanks for your guidance as well. I never give up a task but there was a moment when I thought It will never complete.

@JmillsExpensify
Copy link
Author

JmillsExpensify commented Jan 26, 2022

Awesome, happy to process the payment and increase. I'm defaulting to doubling the job, though if that's not right then someone on this issue can chime in (or reach out) and I can process the right amount.

@parasharrajat
Copy link
Member

parasharrajat commented Jan 26, 2022

Apart from it, the first PR went through N6-Hold and Company Offsite Hold.

@JmillsExpensify Doubling works for me as far as the holding bonuses are counted separately.

@parasharrajat
Copy link
Member

parasharrajat commented Jan 26, 2022

@JmillsExpensify I saw that you released a bonus amount on the old contract. But I canceled that contract a month ago for some reason and no payment was released to me. Thus, I have to refund that bonus back to you to manage my finances. Sorry for the inconvenience. Please let me know if you have any questions otherwise, I will refund that bonus.

Could you please create a new fresh job and I would like to apply and receive payment through that.

This is the breakdown of the payment which I am aware of.

  1. Old Job price 375.
  2. Pay increase +375.
  3. N6-hold + 250.
  4. Company Offsite Hold + 250.

@JmillsExpensify
Copy link
Author

Ok thanks! Yes, that makes sense. I created a fresh job here: https://www.upwork.com/jobs/~0164b1f1886c372bf6

@Wp32793

This comment has been minimized.

@JmillsExpensify
Copy link
Author

I processed the payment, so I'm going to close the issue. Don't hesitate to reach out here or via Slack if I can help with anything else!

@botify botify added Weekly KSv2 and removed Weekly KSv2 labels Feb 1, 2022
@botify botify changed the title [HOLD for payment 2022-01-25] Add the Transfer Balance Page [HOLD for payment 2022-02-08] [HOLD for payment 2022-01-25] Add the Transfer Balance Page Feb 1, 2022
@botify
Copy link

botify commented Feb 1, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.33-3 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 2022-02-08. 🎊

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 Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests