[HOLD for payment 2023-06-02] [$1000] Attachment modal shortcut bugs on native

[luacmartins]

Coming from this comment, we'll address:

Issue 3 - Attachment modal

iOS: Add attachment modal does not navigate based on arrow keys
iOS | Android: Left & Right arrow keys does not work (seems to be duplicate of the issue above)

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01da468373ed9db4e0
• Upwork Job ID: 1653407518037057536
• Last Price Increase: 2023-05-02

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[luacmartins]

@azimgd please comment on this issue so I can assign you

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

📣 @rushatgabhane You have been assigned to this job by @luacmartins!
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 📖

[twisterdotcom]

Ah, I love the ones where I'm just a money man. Keep me posted!

[azimgd]

👍 @luacmartins

[MelvinBot]

📣 @azimgd You have been assigned to this job by @twisterdotcom!
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 📖

[twisterdotcom]

Assigned ya!

[azimgd]

I was able to reproduce "Add attachment modal does not navigate based on arrow keys".

@rushatgabhane Could you connect a hardware keyboard and try to reproduce both, on production Android please.
@luacmartins What should "Left & Right arrow keys" actually do?

[luacmartins]

@azimgd left & right should cycle through the attachment list, same as pressing the visual arrows on the screen.

[azimgd]

Oh, I don't think that's a bug. It's a feature request, newdot doesn't support that on web.

Only up and down shortcuts are supported right now.
https://github.com/Expensify/App/blob/main/src/components/ArrowKeyFocusManager.js#L39

[twisterdotcom]

I'm confused @azimgd - I wonder why somebody would use up/down keys to move left or right through the attachments.

[rushatgabhane]

@twisterdotcom that's a new feature and not a bug.
Do we have a P/S for it?

I think new keyboard shortcuts should be handled more holistically. We need to plan and decide all the shortcuts to add.
vs adding them one at a time.

[rushatgabhane]

Oh wait, @azimgd has already done some work on adding left/right keyboard shortcut.

[twisterdotcom]

I'm confused about where we're at with this now. We agreed here this needs to be done: https://expensify.slack.com/archives/C01GTK53T8Q/p1683208083045299.

Are we just waiting on the PR to be ready?

[twisterdotcom]

Latest update is from 4 hours ago in the PR: #18407 (comment)

[melvin-bot]

@azimgd, @twisterdotcom, @luacmartins, @rushatgabhane Whoops! This issue is 2 days overdue. Let's get this updated quick!

[twisterdotcom]

Deployed to staging 10 hours ago: #18407 (comment)

[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.18-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Attachment shortcuts support on native devices #18407

If no regressions arise, payment will be issued on 2023-06-02. 🎊

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

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:

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

[rushatgabhane]

Checklist not applicable because it's a new feature. But we should create a new regression test

1. Connect hardware keyboard to mobile
2. Click search
3. Arrow up / Arrow down should scroll through the list
4. Enter should open the selected chat
5. Open a chat with multiple attachments and click on any recent one
6. Arrow left / Arrow Right should scroll through attachments

[rushatgabhane]

Applied to upwork job using account "Satish Gabhane"

[twisterdotcom]

@rushatgabhane which Upwork job? I'm only seeing this one which is closed: https://www.upwork.com/ab/applicants/1653543158120652800/job-details

[azimgd]

@twisterdotcom what's the correct upwork url?

[twisterdotcom]

The automated one was this one, but it looks like that was closed for some reason: https://www.upwork.com/jobs/~01da468373ed9db4e0.

Ideally, I'd have found the one @rushatgabhane applied to, but whatever, I've just sent you both an offer to a new job referencing this issue.

Am I right in thinking there's no bonus on this one? We haven't posted a comment like this automatically: #18300 (comment)

[twisterdotcom]

Paid out.