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-09-27] [HOLD for payment 2023-09-06] [$1000] Mention list doesnt automatically popup when typing @ #25649

Closed
6 tasks done
kavimuru opened this issue Aug 22, 2023 · 121 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Aug 22, 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. Login to staging new dot
  2. Go to any chat
  3. Type @ and observe that the mention list doesnt automatically pop-up but does when another key is stroked or even when going back to chat and opening chat

Expected Result:

Mention list should automatically pop-up when typing @

Actual Result:

To pop-up mention key needs another key stroke either name suggestion, or going back to chat and coming back or typing space and back space

Actual Result:

Describe what actually happened

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

2023_08_22_06_17_44.mp4

Expensify/Expensify Issue URL:
Issue reported by: @SofoniasN
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692674547623649

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cb30763071efa133
  • Upwork Job ID: 1695188077897199616
  • Last Price Increase: 2023-08-25
  • Automatic offers:
    • situchan | Contributor | 26373026
    • sofoniasN | Reporter | 26373029
    • WikusKriek | Contributor | 26649676
@kavimuru kavimuru added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 22, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 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

@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 2023

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

@hungvu193
Copy link
Contributor

hungvu193 commented Aug 22, 2023

Feel like regression from #24512

This line introduce it:

if (shouldBlockMentionCalc.current || selection.end < 1) {

We should remove selection.end < 1

@mvtglobally
Copy link

#24512 related?

@WikusKriek
Copy link
Contributor

Hi, it seems like some merge conflicts occurred. The job of this.state.section.end < 1 was to close the list on native. So it can either be removed if it is not needed anymore or replaced with selectionEnd < 1. I am busy testing this.

@techievivek
Copy link
Contributor

@WikusKriek Can you please push a PR to fix this regression and we can cherry pick it to staging, thanks.

@WikusKriek
Copy link
Contributor

WikusKriek commented Aug 22, 2023

While this fix of selectionEnd < 1 does solve the issue on other platforms, #24512 still seems to be a problem. So on iOS native typing '@' does not open the suggestion list. @techievivek what is your thoughts on this?

@techievivek
Copy link
Contributor

@WikusKriek

While this fix of selectionEnd < 1 does solve the issue on other platforms

You meant to say that it is still broken on native ios?

@WikusKriek
Copy link
Contributor

@techievivek yes.

@techievivek
Copy link
Contributor

I suggest we revert the main pull request (#24512) and focus on finding a better solution for the original issue. Our goal is to ensure that all features work seamlessly across all platforms.

@WikusKriek
Copy link
Contributor

WikusKriek commented Aug 22, 2023

@techievivek So the issue existed before my PR. The problem was that the '@' did not launch suggestions list on iOS, created the PR specificaaly for this iOS '@' did not launch suggestions issue it tested well on all platforms. Some major changes to the file happened in parallel to my changes and now the issue still exists.

@WikusKriek
Copy link
Contributor

I suspect reverting (#24512) will not change anything.

@WikusKriek
Copy link
Contributor

WikusKriek commented Aug 22, 2023

I suggest merging this #25660 and then we can have another look at #24512 to see how we can solve the iOS issue again. @parasharrajat and @Beamanator might have something to add here.

@techievivek
Copy link
Contributor

Thanks, @WikusKriek , for providing additional context. I agree that we should proceed with #25660. Once it's merged and CP'ed, we can revisit the original issue and find a solution that works for iOS native.

@techievivek
Copy link
Contributor

Can you please fill the Author checklist in the PR?

@WikusKriek
Copy link
Contributor

Can you please fill the Author checklist in the PR?

Done

@techievivek
Copy link
Contributor

techievivek commented Sep 20, 2023

@situchan coming from here #25649 (comment)

Actually, I think we fixed "2 issues" here and made double efforts.

Can you share the 2 PRs that you mentioned above? @WikusKriek is already being paid separately for this issue #24265, right?

@WikusKriek
Copy link
Contributor

@WikusKriek is already being paid separately for this issue #24265, right?

Yes #24265 is its own issue(#25649 (comment)) and I got paid for that seperately. But I still did 2 PRs here #25660 and #27327.

@situchan
Copy link
Contributor

Can you share the 2 PRs that you mentioned above?

@techievivek

@techievivek
Copy link
Contributor

Thanks for the links. Just to confirm, the fix for iOS was not included in #25660, right? According to the description in the PR, Suggestion list will display when opening chat and typing @ on all platforms except iOS native.. This can also be confirmed from the screen recordings for iOS. So, I would treat this as a single fix for all the platforms.

@techievivek
Copy link
Contributor

Let me know if I am missing anything here.

@situchan
Copy link
Contributor

situchan commented Sep 20, 2023

#25660 fixed all platforms (including iOS - you can confirm from reviewer checklist) but composer refactor caused regression of iOS only, not affected other platforms.
So we fixed iOS issue again in #27327

@melvin-bot melvin-bot bot added the Overdue label Sep 28, 2023
@adelekennedy
Copy link

adelekennedy commented Sep 29, 2023

Can I make sure I understand where we are here?
@situchan and @WikusKriek you'd like for us to include the speed bonus for this issues as there were actually two PRs or, are you asking to be paid out for two issues?

@melvin-bot melvin-bot bot removed the Overdue label Sep 29, 2023
@situchan
Copy link
Contributor

Waiting for confirmation from @techievivek on #25649 (comment)

@WikusKriek
Copy link
Contributor

@adelekennedy @techievivek just to confirm this is what we are asking, we fixed two issues in two different PR's one with a speed bonus one without. Both solutions fixed the problem on all platforms.

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

@adelekennedy The summary over here #25649 (comment) looks correct. @WikusKriek has also linked the PRs that were created in the process #25649 (comment).

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

adelekennedy commented Oct 16, 2023

How much is due in that case 😅 I agree the speed bonus is due but we don't have guidance on paying anything additional:

Payouts due:

Issue Reporter: $250 @SofoniasN
Contributor: $1500 @WikusKriek
Contributor+: $1500 @situchan

Eligible for 50% #urgency bonus? Y

Upwork job is here.

@SofoniasN
Copy link

Hey @adelekennedy this issue was created before the price change policy and i believe is eligible for the original reporter bonus payment. TIA

@adelekennedy
Copy link

@SofoniasN amended my comment above

@situchan
Copy link
Contributor

situchan commented Oct 17, 2023

@techievivek can you please double confirm amount (1.5k or 2k) with @adelekennedy? Thanks

@adelekennedy
Copy link

@techievivek bump on this please

@melvin-bot melvin-bot bot added the Overdue label Nov 2, 2023
@SofoniasN
Copy link

@adelekennedy slight bump

@WikusKriek
Copy link
Contributor

@adelekennedy, I think the Agreed upon amount is 2K based on this #25649 (comment), maybe you can just double the bounty and pay out the base amount?

@melvin-bot melvin-bot bot removed the Overdue label Nov 2, 2023
@techievivek
Copy link
Contributor

@adelekennedy

How much is due in that case 😅 I agree the speed bonus is due but we don't have guidance on paying anything additional:

It would be 2k.
First PR: 1k for fix + 500 for the bonus.
Second PR: 500 for the fix.

@SofoniasN
Copy link

@adelekennedy bumping again!

@adelekennedy
Copy link

back from ooo

Payouts due:

Issue Reporter: $250 @SofoniasN - Upwork
Contributor: $2000 @WikusKriek - Upwork
Contributor+: $2000 @situchan - Upwork

Eligible for 50% #urgency bonus? Y

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. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests