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 2023-10-13] [$500] [Distance] - App does not refocus on address field on delete and cancel #27182

Closed
6 tasks
kbecciv opened this issue Sep 11, 2023 · 45 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Sep 11, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open the app
  2. Click plus and open request money
  3. Click on distance
  4. Click on Add stop
  5. Open newly created stop
  6. Click on 3 dots and click on delete waypoint
  7. Click cancel in confirmation popup
  8. Observe that address field is not focused

Expected Result:

App should focus back on address field on delete and cancel as we do for normal report messages

Actual Result:

App does not focus back on address field on delete and cancel

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.67.2
Reproducible in staging?: y
Reproducible in production?: y
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

no.focus.back.on.delete.cancel.address.field.mp4
Recording.4396.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1694111463191149

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0115c7c503e332eb0f
  • Upwork Job ID: 1701291764562169856
  • Last Price Increase: 2023-09-11
  • Automatic offers:
    • Krishna2323 | Contributor | 26724773
@kbecciv kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 11, 2023
@melvin-bot melvin-bot bot changed the title [Distance] - App does not refocus on address field on delete and cancel [$500] [Distance] - App does not refocus on address field on delete and cancel Sep 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0115c7c503e332eb0f

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Sep 11, 2023

Proposal

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

App does not refocus on address field on delete and cancel.

What is the root cause of that problem?

We are not refocusing on input when modal is closed.

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

We need to focus on the input using input ref when waypoint delete modal is closed by passing onModalHide prop to ConfirmModal.

onModalHide={() => {
    InteractionManager.runAfterInteractions(() => {
        if (!textInput.current) return;
        textInput.current.focus();
    });
}}

<ConfirmModal
title={translate('distance.deleteWaypoint')}
isVisible={isDeleteStopModalOpen}
onConfirm={deleteStopAndHideModal}
onCancel={() => setIsDeleteStopModalOpen(false)}
prompt={translate('distance.deleteWaypointConfirmation')}
confirmText={translate('common.delete')}
cancelText={translate('common.cancel')}
danger

Result

@yh-0218
Copy link
Contributor

yh-0218 commented Sep 11, 2023

Proposal

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

App does not refocus on address field on delete and cancel.

What is the root cause of that problem?

We are not focusing on input when modal is closed.

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

We can call this function when close modal.

    const onModalHide = useCallback(() => {
        setIsDeleteStopModalOpen(false)
        if (textInput === null || textInput.current === null) {
            return;
        }
        const timeout = _.delay(() => {
            textInput.current.focus();
        }, 500);
        return () => clearTimeout(timeout);
    }, [textInput]);

<ConfirmModal
title={translate('distance.deleteWaypoint')}
isVisible={isDeleteStopModalOpen}
onConfirm={deleteStopAndHideModal}
onCancel={() => setIsDeleteStopModalOpen(false)}
prompt={translate('distance.deleteWaypointConfirmation')}
confirmText={translate('common.delete')}
cancelText={translate('common.cancel')}
danger

What alternative solutions did you explore? (Optional)

@greg-schroeder
Copy link
Contributor

greg-schroeder commented Sep 12, 2023

Looks like we were double assigned @CortneyOfstad (because of this, which im gonna fix) - looks like you were assigned first, though, so I'm going to remove my assignment :)

@greg-schroeder greg-schroeder removed their assignment Sep 12, 2023
@CortneyOfstad
Copy link
Contributor

@mananjadhav Any thoughts on the proposals above? TIA!

@mananjadhav
Copy link
Collaborator

@Krishna2323's proposal looks like will work.

🎀 👀 🎀 C+ reviewed.

@melvin-bot
Copy link

melvin-bot bot commented Sep 17, 2023

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

@amyevans
Copy link
Contributor

Hmm that didn't trigger the automation, I'm going to try again

@amyevans amyevans assigned Krishna2323 and unassigned Krishna2323 Sep 18, 2023
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 18, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($500)

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

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

Offer link
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 📖

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($500)

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

📣 @dhanashree-sawant 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@CortneyOfstad
Copy link
Contributor

CortneyOfstad commented Oct 4, 2023

No worries! Thanks for the clarification @amyevans!

As for the payment, that has to be delayed based on the PR being reverted so going to adjust the title.

@CortneyOfstad CortneyOfstad changed the title [HOLD for payment 2023-10-02] [$500] [Distance] - App does not refocus on address field on delete and cancel [$500] [Distance] - App does not refocus on address field on delete and cancel Oct 4, 2023
@CortneyOfstad CortneyOfstad removed Reviewing Has a PR in review Awaiting Payment Auto-added when associated PR is deployed to production labels Oct 4, 2023
@Beamanator
Copy link
Contributor

Seems the PR for this got reverted here: #28677

Should we add this focus issue to this focus issue tracker? #15992

Also it seems like this bug was created when testing the revert PR: #28921, I assume the revert didn't cause the bug, but maybe we can fix both focus bugs at the same time?

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 6, 2023
@melvin-bot melvin-bot bot changed the title [$500] [Distance] - App does not refocus on address field on delete and cancel [HOLD for payment 2023-10-13] [$500] [Distance] - App does not refocus on address field on delete and cancel Oct 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 6, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.78-4 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 2023-10-13. 🎊

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 – @dhanashree-sawant paid $25 (50%) in Upwork
  • Contributor that fixed the issue — @Krishna2323 paid $250 (50%) in Upwork
  • Contributor+ that helped on the issue and/or PR — @mananjadhav to be paid $250 (50%) in NewDot
  • Additional payment — @tienifr paid $50 in Upwork for helping to find that it was expected behavior, not a bug

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

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
Copy link

melvin-bot bot commented Oct 6, 2023

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] The PR that introduced the bug has been identified. Link to the PR:
  • [@mananjadhav] 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:
  • [@mananjadhav] 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:
  • [@mananjadhav] Determine if we should create a regression test for this bug.
  • [@mananjadhav] 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.
  • [@CortneyOfstad] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 12, 2023
@Beamanator
Copy link
Contributor

Quick bump, can we fix #28921 as part of this one, or should it be separate?

@amyevans
Copy link
Contributor

@Beamanator sorry, I totally missed your question the first time. The summary of this GH is that we've introduced no changes to the app and the GH will be closed as expected behavior. I see you've added them both to the tracking issue, thanks! I think that's the best course of action - we can come back to these GHs at a later date and approach more holistically, because honestly it's not efficient trying to address them each separately with a different group of individuals.

@CortneyOfstad
Copy link
Contributor

CortneyOfstad commented Oct 13, 2023

After an internal discussion with the team, it was decided that this is expected behavior, so this GH will be closed.

However, due to the work by @mananjadhav and @Krishna2323 as C+ and contributor on working on the PR, it was decided to offer a 50% payment.

@dhanashree-sawant also offering you 50% for the reporting — this has been paid in Upwork 👍

@mananjadhav if you would like to request that in NewDot, I will get a payment summary posted below.

@Krishna2323 I've paid you via Upwork, so you are all set 👍

Once all payments are completed (just waiting on NewDot), we will close this 👍

@mananjadhav
Copy link
Collaborator

Thanks @CortneyOfstad, yes would need payment summary.

@tienifr
Copy link
Contributor

tienifr commented Oct 13, 2023

@CortneyOfstad I was the one who spotted the expected behavior and raised here: #27182 (comment). Would love to get compensated for that as well.

@melvin-bot melvin-bot bot added the Overdue label Oct 16, 2023
@CortneyOfstad
Copy link
Contributor

@amyevans Does that sound fair to you for compensating @tienifr as well? Just want a second pair of eyes — thanks!

@melvin-bot melvin-bot bot removed the Overdue label Oct 16, 2023
@amyevans
Copy link
Contributor

👍

@CortneyOfstad
Copy link
Contributor

Alright @tienifr, after an internal discussion it was decided to offer $50 as a "bug reporter" for this issue in regards to expected behavior.

If possible, can you send over your Upwork profile so I can make sure that I am sending the proposal to the correct individual? Thanks!

@tienifr
Copy link
Contributor

tienifr commented Oct 17, 2023

@CortneyOfstad
Copy link
Contributor

Perfect! Just sent the proposal @tienifr 👍

@CortneyOfstad
Copy link
Contributor

@tienifr you have been paid 👍

Payment Summary

External issue reporter – @dhanashree-sawant paid $25 (50%) in Upwork
Contributor that fixed the issue — @Krishna2323 paid $250 (50%) in Upwork
Contributor+ that helped on the issue and/or PR — @mananjadhav to be paid $250 (50%) in NewDot
Additional payment — @tienifr paid $50 in Upwork for helping to find that it was expected behavior, not a bug

@JmillsExpensify
Copy link

@250 payment approved for @mananjadhav based on summary above.

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 Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests