-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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-09-17] [$250] Add Offline and Error Patterns to Remaining Integrations #45406
Comments
Triggered auto assignment to @puneetlath ( |
|
Triggered auto assignment to Design team member for new feature review - @shawnborton ( |
Job added to Upwork: https://www.upwork.com/jobs/~014b0b4b88631ddf1d |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 ( |
Hi, I'm Marcin from Software Mansion and I'd like to work on this issue! |
📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
Add Offline and Error Patterns to Remaining IntegrationsProblem StatementOur current integrations lack essential offline and error handling patterns, which results in a suboptimal user experience when encountering errors or losing connectivity. Root CauseThe absence of robust offline and error handling stems from a rushed implementation to meet aggressive deadlines. Priority was given to functionality over resilience, leaving our system vulnerable to disruptions caused by network failures or errors. Changes to Solve the ProblemAdd Offline Pattern // Offline pattern
if (!navigator.onLine) {
// Store data locally and sync when connectivity is restored
localStorage.setItem('data', JSON.stringify(data));
displayOfflineMessage();
} else {
// Sync data with server
syncDataWithServer();
}
function displayOfflineMessage() {
// Display offline message to user
alert('You are offline. Your changes will be synced when connectivity is restored.');
} Add Error PatternIntroduce an error pattern to centrally manage and communicate errors to users, facilitating retries for failed actions. javascript
BenefitsImproved User Experience: Enhancing our system with offline and error patterns ensures users experience greater reliability and continuity, even when faced with connectivity issues or errors. |
@kabeer95 Thanks for your proposal. Unfortunately this issue was already assigned to Marcin from Software Mansion based on #45406 (comment). Additionally, in the future - in order for reviewers to be able to consider your proposal you need to precisely follow the proposal template when posting a proposal. |
@ikevin127 okay. |
Posting my update because I'll be OOO till Monday I started working on fixing offline and error patterns in Xero integration and created a draft PR. You can check the web video in the PR description but I haven't worked on offline handling yet. The best (and fastest) way will be splitting this work into a few PRs (one per integration). It should help with testing. |
I also found and fixed some bugs (e.g. showing offline state for all export subpages when only one is updated). It will require thorough review but @hungvu193 is up-to-date (he reviewed my Sage Intacct offline and error PR) so we should be able to merge things fast 🚀 |
ℹ️ Notice that @shubham1206agra will take over the NetSuite PR #46667 as they have credentials for the integration and I don't - the passing over was discussed on Slack. Please remove me from the PR as reviewer and assign them! Also @hungvu193 took over the SageIntaact PR #46294 for the same reason. This PR is already merged and payment is coming up on 13th. @stephanieelliott Both C+ reviewers mentioned above will require payment as part of this issue. I'm continuing with the review of the other PRs / follow-ups (QBO and Xero). |
What's the difference between:
I thought we already handled that one 🤔 Also @hungvu193, mind reviewing this one as well since you got the credential for Sage and reviewed the other PR ? |
Yeah, I can take that one 😄 . Hey @war-in @289Adam289, can you answer the above question? |
Hello 👋 after merging the first PR, the strategy of handling error and offline behaviours changed a bit, so we needed to update the Sage Intacct PR (details in this thread on #wave8-collect-admins) 🤓 |
I see, I'll review today 😄 |
Ok, so I want to make sure i have this right -- these are the PRs and reviewers due payment I have on this issue @hungvu193 @ikevin127 can you look this over and confirm I have the relevant PRs and correct person due payment?
|
I reviewed 2 SageIntacct PRs, but no payment needed for me, I will be paid centrally later when SageIntacct project is completed |
@stephanieelliott The 2 Sage Intaact merged PRs were both reviewed by @hungvu193 which as they mentioned will not be due payment per every PR. The Xero PR I reviewed first was already paid out to me on 8/5. So far we're all good with payments, there will be other PRs reviewed by me as part of this issue where I'll need payment once the regression period is over. |
@stephanieelliott NetSuite will be handled by me actually. |
I will require payment as reviewer of PR: |
Summarizing payment for #46898 Contributor @war-in - no payment due (agency) |
Status check on the remaining PRs on this issue:
|
The NetSuite one was taken by @shubham1206agra as C+ reviewer since only they have the credentials required to test NetSuite integration. The 2nd one is correct, currently waiting on CME for final approve and merge, hopefully soon 😁 |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
Both on staging! |
Notes for payments
|
Just noting that the regression happened due to changes in BE to accommodate another change not related to this issue. So I don't think normal regression applies here. |
Summarizing payment: PR #46667
Upwork job is here: https://www.upwork.com/jobs/~021837018600586595691 PR #46511
Upwork job is here: https://www.upwork.com/jobs/~014b0b4b88631ddf1d |
@stephanieelliott Offer accepted |
All paid! Thanks |
With #44739 we added offline and error patterns to the Sage Intacct connection menu.
One observation from discussion in Slack was that this pattern is not necessarily implemented for the other integrations (QBO, Xero, and NetSuite) and we'd like to maintain parity between all integrations menus.
Solution
Implement the same offline and error patterns implemented for Intacct in the other integrations
cc @war-in @JmillsExpensify @shawnborton @trjExpensify
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @stephanieelliottThe text was updated successfully, but these errors were encountered: