[HOLD for payment 2023-08-01] [$1000] Close account offline - Close account is enabled by ENTER and no close account text

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue found when executing PR #19328

Action Performed:

1. Go to staging.new..expensify.com
2. Go offline
3. Go toSettings > Security > Close account
4. Enter email address
5. Press ENTER on keyboard
6. Reconnect network

Expected Result:

In Step 5, Close account CTA is disabled and cannot be accessed by keyboard
In Step 6, after reconnecting network, close account message shows up in login screen

Actual Result:

In Step 5, Close account CTA is disabled but it can be accessed by keyboard
In Step 6, after reconnecting network, close account message does not show up in login screen

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.35.4

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6111998_bandicam_2023-06-30_21-01-37-689.mp4

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/~0137e6674165abf18a
• Upwork Job ID: 1676655158891503616
• Last Price Increase: 2023-07-12

[melvin-bot]

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

[melvin-bot]

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

[hungvu193]

Proposal

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

Close account offline - Close account is enabled by ENTER and no close account text

What is the root cause of that problem?

We always submit the form when we're at input or select and press Enter:

App/src/components/FormSubmit/index.js

Lines 30 to 33 in e23cd16

	if (tagName === 'INPUT' || tagName === 'SELECT') {
	this.props.onSubmit();
	return;
	}

In this issue, we only disabled the submit button, and user was still able to submit via Enter pressed.

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

We can handle ENTER key pressed similar the way we handle Submit button by adding a new props enabledWhenOffline or disablePressOnEnter inside our FormSubmit.

// FormSubmit
    submitForm(event) {
       // if user is offline and enabledWhenOffline or disablePressOnEnter is true then we do nothing.
        if (this.props.disablePressOnEnter || (isOffline && !this.props.enabledWhenOffline)) {
            return;
        }

What alternative solutions did you explore? (Optional)

N/A

[samh-nl]

Proposal

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

The account closure form can be submitted despite being offline.

What is the root cause of that problem?

No check is performed when submitting regarding the online/offline status.

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

Prevent the confirmation modal from being shown if the user is offline by querying network.isOffline.

App/src/pages/settings/Security/CloseAccountPage.js

Lines 60 to 63 in 0bbf3fc

	const showConfirmModal = (values) => {
	setConfirmModalVisibility(true);
	setReasonForLeaving(values.reasonForLeaving);
	};

What alternative solutions did you explore? (Optional)

[Julesssss]

Possibly introduced by this PR?

[sakluger]

Possibly introduced by #19328?

Yes, possibly! Otherwise, here is the original issue where we refactored the Close Account page to use a form: #11341.

I'll comment on the recent PR to see if it's a regression.

[parasharrajat]

It is not a regression from #19328. It was present before that PR. While we are at it, we should find the root cause and fix all the pages where such issues can happen. So first an Audit should be done in the App.

[hungvu193]

Agreed. @parasharrajat. Updated my proposal (#21951 (comment)) with new approach.

[samh-nl]

Thank you @parasharrajat for reviewing the proposals. I agree with your assessment that a general solution is necessary. Please see my revision below for a solution that leverages the existing enabledWhenOffline prop.

Proposal

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

The account closure form can be submitted despite being offline.

What is the root cause of that problem?

The FormSubmit component does not evaluate the prop enabledWhenOffline and the current online/offline status when submitting forms.

App/src/components/FormSubmit/index.js

Lines 21 to 45 in 3e02ef4

	submitForm(event) {
	// ENTER is pressed with modifier key or during text composition, do not submit the form
	if (event.shiftKey || event.key !== CONST.KEYBOARD_SHORTCUTS.ENTER.shortcutKey || isEnterWhileComposition(event)) {
	return;
	}
	const tagName = lodashGet(event, 'target.tagName', '');
	// ENTER is pressed on INPUT or SELECT element, call the submit callback.
	if (tagName === 'INPUT' || tagName === 'SELECT') {
	this.props.onSubmit();
	return;
	}
	// Pressing Enter on TEXTAREA element adds a new line. When `dataset.submitOnEnter` prop is passed, call the submit callback.
	if (tagName === 'TEXTAREA' && lodashGet(event, 'target.dataset.submitOnEnter', 'false') === 'true') {
	this.props.onSubmit();
	return;
	}
	// ENTER is pressed on checkbox element, call the submit callback.
	if (lodashGet(event, 'target.role') === 'checkbox') {
	this.props.onSubmit();
	}
	}

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

A return statement must be included in the submitForm method if props.enabledWhenOffline=false and network.isOffline=true.

What alternative solutions did you explore? (Optional)

Implementing this check in the wrapping Form component, however FormSubmit is referenced directly elsewhere (and can be in future usage) and so it must be implemented at the lowest level: inside FormSubmit.

[sakluger]

Alright, sounds like there's consensus that we need to audit the behavior across all pages and fix the root cause. Adding the external label.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0137e6674165abf18a

[melvin-bot]

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

[melvin-bot]

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

[hoangzinh]

Proposal

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

Close account offline - Close account is enabled by ENTER and no close account text

What is the root cause of that problem?

As other people pointed out above, we're wrapping Form component with FormSubmit, and in FormSubmit component, we have logics to submit form when Enter key is pressed

App/src/components/FormSubmit/index.js

Lines 29 to 33 in 8f22b48

	// ENTER is pressed on INPUT or SELECT element, call the submit callback.
	if (tagName === 'INPUT' || tagName === 'SELECT') {
	this.props.onSubmit();
	return;
	}

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

I'm not really agree on above proposals, with stop call onSubmit in the FormSubmit, because it can lead to an unexpected behavior that the Form won't trigger validations on pressing Enter key when Network is offline.

Thus, I think we should check network is available or the props.enabledWhenOffline is true inside the submit callback of Form component, after form validations. So we only disable form submit when network is offline, all other functions of form will work as usual.

[sakluger]

Waiting for proposals.

[melvin-bot]

📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Upwork job
Please accept the offer 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 📖

[hoangzinh]

@eVoloshchak @joelbettner Thanks for accepting my proposal. The PR is ready #23099. Please help me review it. Thanks ❤️

[sakluger]

PR was merged and deployed to staging last week, should deploy to prod today or tomorrow.

[melvin-bot]

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

[melvin-bot]

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

• Fix - close account confirmation modal is displayed by ENTER when network is offline #23099

If no regressions arise, payment will be issued on 2023-08-01. 🎊

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.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @eVoloshchak does not require payment (Eligable for Manual Requests)
• @hoangzinh requires payment

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]

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.
• [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[sakluger]

Summarizing payouts for this issue:

Contributor: @hoangzinh $1500 (hired on Upwork)
Contributor+: @eVoloshchak $1500 (payable via Manual Request after BZ checklist is completed)

Above payments include efficiency bonus 🎉
Upwork job: https://www.upwork.com/jobs/~0137e6674165abf18a

[sakluger]

Paid @hoangzinh and assigned to @JmillsExpensify and @anmurali for the Manual Request payment.

[sakluger]

Oh yeah - @eVoloshchak could you please finish the BZ checklist so that your payment can be approved? Thanks!

[JmillsExpensify]

Not seeing a request for this one yet in NewDot. Let me know if I'm missing something.

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: I can't pinpoint the exact PR that caused this. The first PR to use enabledWhenOffline prop was Policy employees merge refactor #10395, but it's completely unrelated to this and was adding this prop to use in a single specific case. I'd say this is something that wasn't implemented initially, not a regression per se
• 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: I don't think this is necessary, in general, keyboard navigation is out of scope, with the exception of this specific case (confirming something by pressing space/enter)
• Determine if we should create a regression test for this bug.
  Is it easy to test for this bug? Yes
  Is the bug related to an important user flow? Yes
  Is it an impactful bug? No
  While this specific case isn't impactful, the bug was present on all forms throughout the app, so we might want to add a regression test

[eVoloshchak]

Regression Test Proposal

• Go to Settings > Security > Close account
• Disconnect from network
• Enter your default contact method
• While contact method input is still focused, press ENTER on keyboard
• Verify that there is no close account confirmation modal shown
• Connect back to the network
• While contact method input is focused, press ENTER on keyboard
• Verify that close account confirmation modal is shown

Do we agree 👍 or 👎

[eVoloshchak]

Requested the payment via NewDot

[JmillsExpensify]

Reviewed details for @eVoloshchak. These details are accurate based on summary from Business Reviewer and are now approved for payment in NewDot.

[sakluger]

Created the regression test issue: https://github.com/Expensify/Expensify/issues/305598. We're all set here, thanks everyone!