[HOLD for payment 2022-12-20] [$250] Closing split bill doesn't refocus on text-input(composer) if already focused - reported by @aneequeahmad

[mvtglobally]

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. Login in web app.
2. Click on any report in LHN (text-input will be focused).
3. Click on + icon on left end of text-input.
4. Click on split bill.
5. Click cross icon on split bill page.

Expected Result:

Text-input should be focused.

Actual Result:

Text-input is not focused

Workaround:

unknown

Platform:

Where is this issue occurring?

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

Version Number: 1.1.96-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2022-08-24.at.12.11.12.PM.mov

Expensify/Expensify Issue URL:
Issue reported by: @aneequeahmad
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1661325304904489

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @davidcardoza (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[davidcardoza]

This could definitely be fixed. Putting this in the engineering pool.

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

@davidcardoza, @arielgreen, @Luke9389 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Luke9389]

Odd, this one doesn't show up on my K2 at all... 🤔

[Luke9389]

@arielgreen is OOO today (she's heading back to the States). We can export this tomorrow.

[aimane-chnaif]

Proposal

RCA:
This happens because onModalHide (after closing popover) is called after react-navigation focus listener (after opening split bill page).
Therefore, Modal.setModalVisibility(false) in BaseModal is called after Modal.setModalVisibility(true) in AuthScreens.
As a result, isVisible is saved as false in Onyx even though modal page is currently open.

Solution1:
In PopoverMenu component, props.onItemSelected(item); should be called first. And then item.onSelected(); should be called after onModalHide callback.
This is already done in native side: https://github.com/Expensify/App/blob/main/src/components/PopoverMenu/index.native.js
We no longer need platform specific handling for this component.
So remove current index.js and rename index.native.js to index.js

Solution2:

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Lines 79 to 82 in 567db53

	const modalScreenListeners = {
	focus: () => {
	Modal.setModalVisibility(true);
	},

Apply setTimeout here so updated code will look like this:

const modalScreenListeners = {
    focus: () => {
        setTimeout(() => Modal.setModalVisibility(true));
    },

No need to set delay (0 as default). It's enough for focus() to be called after onModalHide() callback

I know setTimeout is not recommended solution for this project and I prefer Solution 1.

[melvin-bot]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[Luke9389]

Hmm. I wonder why this issue didn't get a C+ assigned

[thesahindia]

Hmm. I wonder why this issue didn't get a C+ assigned

Exported label is missing.

[Luke9389]

Ah right, we need to wait for it to be Exported by @arielgreen

[arielgreen]

https://www.upwork.com/jobs/~011227dc49a4e5b8d2

[melvin-bot]

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

[melvin-bot]

Current assignee @Luke9389 is eligible for the Exported assigner, not assigning anyone new.

[melvin-bot]

@mananjadhav, @davidcardoza, @arielgreen, @Luke9389, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[davidcardoza]

@Luke9389 bump

[Luke9389]

Thanks, was OOO yesterday

[arielgreen]

bumped Luke again here

[Luke9389]

We're still in the review process on that PR. You can see the conversation has been happening over the past few days (I was OOO yesterday). I think the problem with this issue was that we didn't use the Reviewing label once the PR was up, so the overdue messages got annoying.

[mananjadhav]

I've tested the PR again. All yours @Luke9389

[melvin-bot]

@mananjadhav, @davidcardoza, @arielgreen, @Luke9389, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

[davidcardoza]

@Luke9389 Anything I can help with here?

[mananjadhav]

@Luke9389 This is waiting on you. Can you help it move forward?

[Luke9389]

Hey sorry y'all. I've been stacked this week. 👀

[Luke9389]

Done!

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

[melvin-bot]

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

• fix "Closing split bill doesn't refocus on composer" on web/desktop #12031

If no regressions arise, payment will be issued on 2022-12-20. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter @aneequeahmad
• Contributor that fixed the issue @aimane-chnaif
• Contributor+ that helped on the issue and/or PR @mananjadhav

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:

• [@mananjadhav / @aimane-chnaif / @Luke9389] The PR that introduced the bug has been identified. Link to the PR: fix "Closing split bill doesn't refocus on composer" on web/desktop #12031
• [@mananjadhav / @aimane-chnaif / @Luke9389] 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: fix "Closing split bill doesn't refocus on composer" on web/desktop #12031 (comment)
• [@mananjadhav / @aimane-chnaif / @Luke9389] 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. N/A, this is captured in I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
• [@davidcardoza / @arielgreen] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://expensify.testrail.io/index.php?/cases/view/1971040&group_by=cases:section_id&group_order=asc&display_deleted_cases=0&group_id=229064

[mananjadhav]

@arielgreen this is due for payment. Also note for C+ payment that it had a regression.

[arielgreen]

Timeline:
10/20: Contributor assigned to issue
12/6: PR merged - 31 business days (calculation) - -50%
12/13: Regression -50% C+

So:
Reporting bonus: $250 to @aneequeahmad
Contributor pay - $250*.5 = $125 to @aimane-chnaif
C+ = $250*.5*.5 = $62.50 to @mananjadhav

Issuing payment now.

[aimane-chnaif]

@arielgreen I don't think timeline bonus/penalty is applied to this issue because I was assigned on Oc 19 and raised PR on Oct 20.
New rule started on Nov.

[arielgreen]

Oh! Good catch @aimane-chnaif -- my apologies. You are correct. I'll go ahead and bonus you and @mananjadhav the difference. Apologies and thanks for letting me know.

[mananjadhav]

Please ignore the ping! I saw the difference was applied!