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-03-17] Feature Request: Data is getting written to Onyx after logout - reported by @kidroca #7434

Closed
mvtglobally opened this issue Jan 27, 2022 · 44 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@mvtglobally
Copy link

mvtglobally commented Jan 27, 2022

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. Be logged out
  2. Be on a slow network (see "Slow 3g" on the opened dev tools)
  3. Log to an account with some existing conversations
  4. Quickly log out the second you see LHN loaded (You can verify in dev tools that requests are still pending)
  5. Now log in with another account
  6. Observe issues - LHN has conversations from the previous account, a lot of unread messages

Expected Result:

We can abort requests to save bandwidth and prevent other issues

Actual Result:

Some times we know there are requests bound to fail or requests for whom we're no longer interested in the results.

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.31-0
Reproducible in staging?:
Reproducible in production?:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

New.Expensify.-.Google.Chrome.2022-01-25.21-05-00.mp4

Upwork URL: https://www.upwork.com/jobs/~01bcb9c814f48af12c
Issue reported by: @kidroca
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1642188186471800

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Jan 27, 2022
@MelvinBot
Copy link

Triggered auto assignment to @Christinadobrzyn (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 Jan 27, 2022
@MelvinBot
Copy link

Triggered auto assignment to @alex-mechler (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@alex-mechler
Copy link
Contributor

Tagging a couple people who were involved in the discussions around this and sending it external

cc @iwiznia @marcaaron

@alex-mechler alex-mechler added the External Added to denote the issue can be worked on by a contributor label Jan 28, 2022
@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

Yeah, this is happening.

@dylanexpensify
Copy link
Contributor

Working on getting posted!

@MelvinBot MelvinBot removed the Overdue label Jan 31, 2022
@dylanexpensify
Copy link
Contributor

@MelvinBot
Copy link

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

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 31, 2022
@MelvinBot
Copy link

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

@rushatgabhane
Copy link
Member

rushatgabhane commented Jan 31, 2022

Excited to review proposals on this one!

@parasharrajat
Copy link
Member

A solution could be:

  1. Make xhr method return cancellable promise.

    function xhr(command, data, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) {
    via
    export default function makeCancellablePromise(promise) {
    .

  2. Track all the cancel callbacks from this call.

    HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure)

  3. Add a method in Network.js to call all of that cancel callbacks and add that to

    function cleanupSession() {
    .

Method 2
  1. Add a new Flag to the Network.js something like blockinFlightRequests: Boolean.
  2. Create a callback to set that flag to true and call that callback to
    function cleanupSession() {
    ..
  3. Update
    HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure)
    call so that it checks that flag and don't execute the handlers for API call.

    App/src/libs/Network.js

    Lines 228 to 235 in 2cdb7c3

    HttpUtils.xhr(queuedRequest.command, finalParameters, queuedRequest.type, queuedRequest.shouldUseSecure)
    .then(response => onResponse(queuedRequest, response))
    .catch((error) => {
    // When the request did not reach its destination add it back the queue to be retried
    const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry');
    if (shouldRetry) {
    networkRequestQueue.push(queuedRequest);
    return;

Pseudocode

            .then(response => blockinFlightRequests? undefined: onResponse(queuedRequest, response))
            .catch((error) => {
 				if(blockinFlightRequests) {
					return;
				}
                // When the request did not reach its destination add it back the queue to be retried
                const shouldRetry = lodashGet(queuedRequest, 'data.shouldRetry');
                if (shouldRetry) {
                    networkRequestQueue.push(queuedRequest);
                    return;
                }

                onError(queuedRequest, error);
            });

But it imposes a challenge for us When should we set the blockinFlightRequests flag back to false. I don't know have any idea for now.

@tgolen
Copy link
Contributor

tgolen commented Mar 4, 2022

Not overdue

@MelvinBot MelvinBot removed the Overdue label Mar 4, 2022
@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Mar 10, 2022
@botify
Copy link

botify commented Mar 10, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.41-6 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-03-17. 🎊

@botify botify changed the title Feature Request: Data is getting written to Onyx after logout - reported by @kidroca [HOLD for payment 2022-03-17] Feature Request: Data is getting written to Onyx after logout - reported by @kidroca Mar 10, 2022
@dylanexpensify dylanexpensify removed their assignment Mar 11, 2022
@dylanexpensify dylanexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Mar 11, 2022
@MelvinBot
Copy link

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

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Mar 11, 2022
@dylanexpensify
Copy link
Contributor

Reassigning as I'm OOO next week! Thanks Tom for handling the payment if no regressions! 🙇

@dylanexpensify dylanexpensify added Weekly KSv2 and removed Daily KSv2 labels Mar 11, 2022
@botify botify removed the Weekly KSv2 label Mar 17, 2022
@MelvinBot MelvinBot added the Daily KSv2 label Mar 17, 2022
@trjExpensify
Copy link
Contributor

@kidroca - paid. @rushatgabhane waiting for you to accept the offer for C+.

@rushatgabhane
Copy link
Member

@trjExpensify accepted, thanks!

@trjExpensify
Copy link
Contributor

Cool, settled!

@kidroca
Copy link
Contributor

kidroca commented Mar 18, 2022

@trjExpensify
I thought I was getting paid for reporting and fixing the issues
It seems I was only paid for fixing

@trjExpensify
Copy link
Contributor

You thought correctly! Sorry about that, I just settled the contract that was in place.

So I sent you a bonus for an additional $250. Got an error message in Upwork saying there was a problem with the account. Did it again and now it looks like I've paid you an extra $250. Can you double check that? 😅

@trjExpensify trjExpensify reopened this Mar 18, 2022
@kidroca
Copy link
Contributor

kidroca commented Mar 21, 2022

So I sent you a bonus for an additional $250. Got an error message in Upwork saying there was a problem with the account. Did it again and now it looks like I've paid you an extra $250. Can you double check that? 😅

Just checked, indeed I've been paid twice for reporting
I've issued a refund for $250 reverting one of the bonus payments

@trjExpensify
Copy link
Contributor

Perfect, just got the email for the refund. Going to close this one out again, thanks!

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

No branches or pull requests