-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Comments
Triggered auto assignment to @davidcardoza ( |
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! |
Triggered auto assignment to @tgolen ( |
I've submitted the proposal on Upwork. Please do check |
Proposal
Question:
|
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.
I agree with you that showing the loader on the button would be best for now.
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 |
@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. |
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. |
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. |
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? |
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:
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.
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. |
Thanks, @marcaaron. 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).
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? |
Sounds good. I will just queue the request for a retry and 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. |
I don't think we need to block further requests, it should be fine to do it multiple times |
Requesting icons and High-resolution images for mockups. Thanks. |
@stitesExpensify Awesome! Should we go ahead and update the remaining todos within the OP as well? |
Sure, that makes sense to me! I'll update it. |
Hopefully last PR #7200 for
|
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. 🎊 |
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. |
Great, looking forward to it! |
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.
Apart from it, the first PR went through N6-Hold and Company Offsite Hold. |
Thanks for all of the work @parasharrajat ! |
+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. |
Thanks for your guidance as well. I never give up a task but there was a moment when I thought It will never complete. |
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. |
@JmillsExpensify Doubling works for me as far as the holding bonuses are counted separately. |
@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.
|
Ok thanks! Yes, that makes sense. I created a fresh job here: https://www.upwork.com/jobs/~0164b1f1886c372bf6 |
This comment has been minimized.
This comment has been minimized.
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! |
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. 🎊 |
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:
Create a new file
/PaymentPages/TransferBalancePage.js
Add a route for the transfer balance page:
ROUTES.SETTINGS_TRANSFER_BALANCE = "settings/payments/transfer-balance"
Add a "Transfer Balance" button to
PaymentsPage.js
that links to the new transfer pagesrc/components/Icon/Expensicons.js.
Add the
<currentBalance>
component to the topAdd 2
<menuItem>
components for instant/slow transferCreate the "Which Account" section
<MenuItem>
PaymentMethodList.js
USER_WALLET.linkedBankAccount
ChooseTransferAccountPage
Create a new page
ChooseTransferAccountPage.js
(screenshot 3)ROUTES.SETTINGS_PAYMENTS_CHOOSE_TRANSFER_ACCOUNT = “settings/payments/choose-transfer-Account”
<PaymentMethodList>
TransferBalancePage
with the account type and IDCreate the "Transfer" button
<PressableOpacity>
onPress
should:<FullScreenLoadingIndicator>
confirmModal.js
Create a new method in
PaymentMethods.js
calledTransferWalletBalance()
API.js
calledTransferWalletBalance()
TransferWalletBalance
(no parameters necessary)USER_WALLET.balance = 0
in OnyxProgress
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)
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
The text was updated successfully, but these errors were encountered: