[HOLD for payment 2023-06-07] [$1000] Login - No message that the account was "Closed Successfully" below the email field

kbecciv commented 1 year ago

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:

Go to URL https://staging.new.expensify.com/
Login to ND
Navigate to Settings > Security
Click/tap on "Close Account"

Expected Result:

User are logged out and that there's a message in the login screen that the account was "Closed Successfully" below the email field.

Actual Result:

User are logged out, but that there's no a message in the login screen that the account was "Closed Successfully" below the email field.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

Version Number: 1.3.11.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/3447225

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/008c5a1d-88a8-4461-abbe-2154dd05c64c

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01db54583762870fd2
Upwork Job ID: 1656431594058833920
Last Price Increase: 2023-05-17

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @stephanieelliott (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

kbecciv commented 1 year ago

QA team is Failed the step 9 on TestRail

hungvu193 commented 1 year ago

Proposal

I'm able to reproduce this issue.

Please re-state the problem that we are trying to solve in this issue.

Login - No message that the account was "Closed Successfully" below the email field

What is the root cause of that problem?

When we closed the account, we also logged user out and when logging out we also cleared the Onyx storage. https://github.com/Expensify/App/blob/45c3212061983394e0919839248512874c2af83f/src/libs/actions/SignInRedirect.js#L31-L54 Because of that, the information from ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM was also cleared, which caused the issue.

What changes do you think we should make in order to solve the problem?

We shouldn't clear ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM when user logged out, because we still needed to use it. So we need to add this key to the keysToPreserve array inside clearStorageAndRedirect function.

 const keysToPreserve = [];
    keysToPreserve.push(ONYXKEYS.NVP_PREFERRED_LOCALE);
    keysToPreserve.push(ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM);
    keysToPreserve.push(ONYXKEYS.ACTIVE_CLIENTS);
    keysToPreserve.push(ONYXKEYS.DEVICE_ID);

Also with browser that enable the auto-fill function, the message isn't showed because it will trigger text change when user logged out, and because we also cleared the "Close account successfully" message on text change, we could consider remove this piece of code inside onTextInput of LoginForm:

        // Clear the "Account successfully closed" message when the user starts typing
        // if (this.props.closeAccount.success) {
        //     console.log('this.props.closeAccount.success', this.props.closeAccount.success);
        //     CloseAccount.setDefaultData();
        // }

What alternative solutions did you explore? (Optional)

None

stephanieelliott commented 1 year ago

Was able to repro on MacOS / Safari, didn't find any similar issues, agree this is unexpected behavior.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01db54583762870fd2

melvin-bot[bot] commented 1 year ago

Current assignee @stephanieelliott is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

httol commented 1 year ago

Reproducible in staging?: Yes Reproducible in production?: No Seems the problems only exsit in staging

dukenv0307 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

User are logged out, but that there's no message in the login screen that the account was "Closed Successfully" below the email field.

What is the root cause of that problem?

There're 2 problems:

The Onyx value closeAccount that's used to show the account closed message here https://github.com/Expensify/App/blob/a7dc7c717729afd54db436eaa5e9216a55da18c1/src/pages/signin/LoginForm.js#L193, was removed after the user closes the account. Because when the user closes account the user is logged out immediately, and we're clearing most Onyx values when logging out.
When the text input in the LoginForm is autofilled by the browser, it will trigger text change which will clear the account closed message as can be seen here: https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/pages/signin/LoginForm.js#L117

What changes do you think we should make in order to solve the problem?

We should preserve the ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM key when logging out, we can add keysToPreserve.push(ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM); below this line https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/libs/actions/SignInRedirect.js#L35
When text change, we need to check if it's autofilled by browser, if yes then we'll not clear the close account message. We only clear close account successful message when text changes due to user input.

The approach to check if input is autofilled is already used here https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/components/TextInput/BaseTextInput.js#L134 in the code base.

We can introduce a new method somewhere common for reuse

isInputAutofilled(inputRef) {
        if (!inputRef.matches) {
            return false;
        }

        return inputRef.matches(':-webkit-autofill') || inputRef.matches(':autofill');
    }

Then change this condition https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/pages/signin/LoginForm.js#L116

to

if (this.props.closeAccount.success && !isInputAutofilled(this.input)) {

What alternative solutions did you explore? (Optional)

NA

tienifr commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

After close the account, "Closed Successfully" message is not shown

What is the root cause of that problem?

We should preserve the ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM key when logging out, we can add keysToPreserve.push(ONYXKEYS.FORMS.CLOSE_ACCOUNT_FORM); below this line

@dukenv0307's proposal won't work because we don't call clearStorageAndRedirect function to clear Onyx data

After confirm close account, we call CloseAccount API. Here is the response from pusher

We have 3 actions: clear onyx data, merge preferredLocale and merge closeAccount

In https://github.com/Expensify/react-native-onyx/blob/0fbe0a4d077f96c4bc30b0b14359f634b271b03b/lib/Onyx.js#L1212

We perform all actions simultaneously, so we can meet race condition (clear other data -> merge closeAccount -> clear closeAccount,...)

What changes do you think we should make in order to solve the problem?

When call Onyx.update we should perform clear action first if there is, then perform other actions (merge, set, ...)

To avoid the case uses fill the input with autofilled of Chrome: After close the account, the email input will be filled (autofilled of Chrome) -> the success message will be removed because the input is changed

https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/pages/signin/LoginForm.js#L116-L119

We can create a new function (isInputAutofilled) to check if the input is autofilled or not

input.matches(':-webkit-autofill') || input.matches(':autofill')

and only clear data if this.props.closeAccount.success && !isInputAutofilled

Result

https://github.com/Expensify/App/assets/113963320/1888d080-f157-44b1-9319-98e5a672c13e

puneetlath commented 1 year ago

@eVoloshchak thoughts on the proposals?

eVoloshchak commented 1 year ago

@hungvu193, @dukenv0307, I don't see clearStorageAndRedirect being called when closing an account. Could you double-check this, please?

dukenv0307 commented 1 year ago

That's right, It is my mistake. Agree with @tienifr's proposal

eVoloshchak commented 1 year ago

@tienifr's proposal looks good

We perform all actions simultaneously, so we can meet race condition (clear other data -> merge closeAccount -> clear closeAccount,...) When call Onyx.update we should perform clear action first if there is, then perform other actions (merge, set, ...)

I wonder if the order of actions passed from the back end matters. In this case, it makes no difference (since the clear action is already the first item). But if the order of actions matters and there is a case where the backend would return something like

1. 'merge'
2. 'clear'
3. 'set'

Should we disregard the order and still perform the clear action first? I'm almost sure this doesn't matter since currently we're performing all of the actions simultaneously. @puneetlath, could you chime in on this please?

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

hungvu193 commented 1 year ago

@hungvu193, @dukenv0307, I don't see clearStorageAndRedirect being called when closing an account. Could you double-check this, please?

Damn I swear it worked before 😂 , I was going to record a video to capture the result but I forgot. Anyway I didn't see clearStorageAndRedirect is being called now. My proposal isn't valid.

puneetlath commented 1 year ago

Very interesting.

When call Onyx.update we should perform clear action first if there is, then perform other actions (merge, set, ...)

How would we do this?

tienifr commented 1 year ago

https://github.com/Expensify/react-native-onyx/blob/1d496b001f327c509131597faf260a81c002be77/lib/Onyx.js#L1194-L1215

I think we should change these lines to

    const promises = [];
    const clearPromise = Promise.resolve();

    _.each(data, ({onyxMethod, key, value}) => {
        switch (onyxMethod) {
            case METHOD.SET:
                promises.push(()=>set(key, value));
                break;
            case METHOD.MERGE:
                promises.push(()=>merge(key, value));
                break;
            case METHOD.MERGE_COLLECTION:
                promises.push(()=>mergeCollection(key, value));
                break;
            case METHOD.CLEAR:
                clearPromise = clear
                break;
            default:
                break;
        }
    });

      return clearPromise.then(()=>Promise.all(_.map(promises,p=>p())));

@puneetlath @eVoloshchak please let me know what you think. Thanks

puneetlath commented 1 year ago

@tienifr we were discussing in this thread, but I'm still a little unclear on the root cause. In theory when we do promises.push(clear()); that is actually immediately calling the clear method. And then return Promise.all(promises); just returns when all the promises that have already been called are completed. Is that a correct understanding?

If so, in theory, the clear/merge/merge are being called in order right? Do they happen synchronously, one after the other? Or do they happen at the same time?

tienifr commented 1 year ago

In theory when we do promises.push(clear()); that is actually immediately calling the clear method. And then return Promise.all(promises); just returns when all the promises that have already been called are completed

That's correct, the clear/merge/merge are being called in order, but they do not happen in the right order.

clear is called first
wait for getAllKeys to be done https://github.com/Expensify/react-native-onyx/blob/1d496b001f327c509131597faf260a81c002be77/lib/Onyx.js#L1051
while getAllKeys is not completed, start to call merge() (merge closeAccount data)
noti closeAccount with success message to subscribers

https://github.com/Expensify/react-native-onyx/blob/1d496b001f327c509131597faf260a81c002be77/lib/Onyx.js#L871

wait for Storage.setItem to be done

https://github.com/Expensify/react-native-onyx/blob/1d496b001f327c509131597faf260a81c002be77/lib/Onyx.js#L874

getAllKeys (from step 2) is completed
noti closeAccount without success message to subscribers (because it's cleared)

https://github.com/Expensify/react-native-onyx/blob/1d496b001f327c509131597faf260a81c002be77/lib/Onyx.js#L1097

...

That why the closeAccount data doesn't have success message.

@puneetlath let me know if it's still unclear

puneetlath commented 1 year ago

Ok got it, that does make sense. However, I still don't totally follow how this will solve it:

return clearPromise.then(()=>Promise.all(_.map(promises,p=>p())));

Because any merge/set would have already been called before the clear, right?

tienifr commented 1 year ago

@puneetlath

change: promises.push(merge(key, value))

to promises.push(()=>merge(key, value));

If we do that, the merge/set won't be called till we perform Promise.all(_.map(promises,p=>p()))

puneetlath commented 1 year ago

Ah got it. In that case, sounds good to me.

melvin-bot[bot] commented 1 year ago

📣 @tienifr You have been assigned to this job by @puneetlath! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

tienifr commented 1 year ago

@puneetlath @eVoloshchak The PR: https://github.com/Expensify/react-native-onyx/pull/250 is ready for review

melvin-bot[bot] commented 1 year ago

@puneetlath @eVoloshchak @stephanieelliott @tienifr this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

stephanieelliott commented 1 year ago

The PR in Expensify/react-native-onyx was merged 👏

tienifr commented 1 year ago

@eVoloshchak @puneetlath The PR on App is ready for review: https://github.com/Expensify/App/pull/19579

stephanieelliott commented 1 year ago

PR deployed to staging but it looks like there was a regression, being discussed in Slack here

melvin-bot[bot] commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.20-5 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/19579

If no regressions arise, payment will be issued on 2023-06-07. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

[x] External issue reporter
[ ] Contributor that fixed the issue @tienifr
[ ] Contributor+ that helped on the issue and/or PR @eVoloshchak

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot[bot] commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[ ] [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@eVoloshchak] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[ ] [@eVoloshchak] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[ ] [@eVoloshchak] Determine if we should create a regression test for this bug.
[ ] [@eVoloshchak] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@puneetlath / @stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

stephanieelliott commented 1 year ago

Hey @eVoloshchak can you please complete the BugZero checklist above?

eVoloshchak commented 1 year ago

The PR that introduced the bug has been identified. Link to the PR: N/A, this was a limitation of react-native-onyx and it was implemented initially
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: n/a
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: This is a simple visual bug (text label not appearing) with a complex cause. I don't think there are any additional steps that could be taken, this could have been caught during review.

eVoloshchak commented 1 year ago

Regression Test Proposal

Navigate to Settings > Security
Click/tap on "Close Account", you're navigated to login screen
Verify that there's a message that the account was closed successfully below the email field

Do we agree 👍 or 👎

stephanieelliott commented 1 year ago

LGTM! @eVoloshchak and @tienifr are paid, but leaving this open til I can create the GH for the regression test.

puneetlath commented 1 year ago

@stephanieelliott can this be closed yet?

stephanieelliott commented 1 year ago

Yep! Regression test here: https://github.com/Expensify/Expensify/issues/291294

Expensify / App