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 2022-12-15] [$500] iOS - When hard-press username to open tooltip and navigate to same user detail tooltip remains open - reported by dhairyasenjaliya #9973

Closed
mvtglobally opened this issue Jul 19, 2022 · 63 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 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@mvtglobally
Copy link

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 a chat
  2. Press and hold to any username until copy clipboard appears
  3. Press on to that same username while clipboard open
  4. User detail page opens with clipboard remains open in same position

Expected Result:

clipboard should closed since we navigate to user detail

Actual Result:

clipboard appear in same position

Workaround:

unknown

Platform:

Where is this issue occurring?

  • iOS

Version Number: 1.1.85-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

iOS.clipboard.bug.mp4

Expensify/Expensify Issue URL:
Issue reported by: @dhairyasenjaliya
Slack conversation:

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Jul 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2022

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

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 20, 2022

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

@mateocole mateocole added Weekly KSv2 and removed Daily KSv2 labels Jul 20, 2022
@cead22 cead22 removed their assignment Jul 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 25, 2022

Triggered auto assignment to @sketchydroide (Demolition), see https://stackoverflow.com/c/expensify/questions/8099 for more details.

@melvin-bot melvin-bot bot added the Overdue label Jul 28, 2022
@sketchydroide
Copy link
Contributor

I will look into it next week

@melvin-bot melvin-bot bot removed the Overdue label Jul 29, 2022
@melvin-bot melvin-bot bot added the Overdue label Aug 8, 2022
@sketchydroide
Copy link
Contributor

will do it this week

@melvin-bot melvin-bot bot removed the Overdue label Aug 8, 2022
@mateocole
Copy link

feel free to assign back to me once this is looked into

@mateocole mateocole removed their assignment Aug 14, 2022
@melvin-bot melvin-bot bot added the Overdue label Aug 22, 2022
@sketchydroide
Copy link
Contributor

I've got deployer duties this week, and with clear daily releases I don't have time for this GH, hopefully next week 🤞🏼

@melvin-bot melvin-bot bot removed the Overdue label Aug 24, 2022
@melvin-bot melvin-bot bot added the Overdue label Sep 2, 2022
@sketchydroide
Copy link
Contributor

Will try to look at this in this week

@melvin-bot melvin-bot bot removed the Overdue label Sep 6, 2022
@melvin-bot melvin-bot bot added the Overdue label Sep 15, 2022
@sketchydroide
Copy link
Contributor

on my top list of things to look at now

@eVoloshchak
Copy link
Contributor

eVoloshchak commented Nov 30, 2022

PR is open

@aimane-chnaif
Copy link
Contributor

Since I am assigned as reviewer in PR, I just looked into this issue and adding my proposal here:

I don't think it's a good idea to disable selection on non touchable devices.

demo.mov

Since this happens only on mobile, we can add condition to selectable prop similar to here:

selectable={!canUseTouchScreen() || !props.isSmallScreenWidth}

        case 'TEXT':
            return (
                <Tooltip text={props.tooltipText}>
                    <Text
-                       selectable
+                       selectable={!canUseTouchScreen()} 
                        numberOfLines={props.isSingleLine ? 1 : undefined}
                        style={[styles.chatItemMessageHeaderSender]}
                    >
                        {Str.htmlDecode(props.fragment.text)}
                    </Text>
                </Tooltip>
            );

NOTE: excluding || !props.isSmallScreenWidth condition for iPad

@mananjadhav
Copy link
Collaborator

@aimane-chnaif Thanks for the comment, but do we have a specific need for selectable to be true? We're anyway relying on the context menu for our copy needs. Hence I think we should be good with current accepted solution.

@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Nov 30, 2022

@mananjadhav did you see my video above?
On any other popular chat platforms (i.e. slack desktop/web), username is selectable

@mananjadhav
Copy link
Collaborator

I did. I'll get some feedback from @sketchydroide @arielgreen on this one. I don't mind having canUseTouchScreen as well.

@sketchydroide do we want to disable selectable only for apps? and not web?

@JmillsExpensify JmillsExpensify added the Reviewing Has a PR in review label Nov 30, 2022
@mananjadhav
Copy link
Collaborator

@JmillsExpensify if you would also want to chime in?

@mananjadhav
Copy link
Collaborator

@sketchydroide did you get a chance to look at the previous comments?

@sketchydroide
Copy link
Contributor

I don't see a specific reason for the need to be selectable.
We can copy specific messages and we can copy the info from the user info page.

@JmillsExpensify let me know if you disagree because there is some level of UX involved and I don't know if this affects WAQ

But I think going with @eVoloshchak solution works

@sketchydroide
Copy link
Contributor

Also we want to have the same behaviour in all platforms so I think we should keep the same for both web and mobile

@JmillsExpensify
Copy link

Yeah I think that last point is the most critical, and why we don't need this to be selectable.

@sketchydroide
Copy link
Contributor

cool I think we are sticking with @eVoloshchak solution, thanks for stating your concerns @aimane-chnaif :)

@mananjadhav
Copy link
Collaborator

Was just about to comment on the thread that, the PR is already C+ reviewed.

@sketchydroide
Copy link
Contributor

yeah was just going over it right now :D

@sketchydroide
Copy link
Contributor

the solution has been merged

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 8, 2022
@melvin-bot melvin-bot bot changed the title [$500] iOS - When hard-press username to open tooltip and navigate to same user detail tooltip remains open - reported by dhairyasenjaliya [HOLD for payment 2022-12-15] [$500] iOS - When hard-press username to open tooltip and navigate to same user detail tooltip remains open - reported by dhairyasenjaliya Dec 8, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 8, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.36-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 2022-12-15. 🎊

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

melvin-bot bot commented Dec 8, 2022

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 / @eVoloshchak / @sketchydroide] The PR that introduced the bug has been identified. Link to the PR: N/A, upstream issue
  • [@mananjadhav / @eVoloshchak / @sketchydroide] 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, upstream issue
  • [@mananjadhav / @eVoloshchak / @sketchydroide] 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: N/A, upstream issue
  • [@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: N/A, upstream issue

@mananjadhav
Copy link
Collaborator

I couldn't track any PR that got the bug. This is anyway a native tooltip showing up in the UI, so I don't think we have any regression PR here. @sketchydroide what do you think?


@arielgreen @sketchydroide @JmillsExpensify This issue is very old and had been old for quite a few weeks, and by the time it opened, it should've been doubled. The reason I am asking is, how do we set the amount for this one? Should it be 500$ as per title or 1K$ as per the new bug bounty? Also this might be eligible for 50% bonus.

@sketchydroide
Copy link
Contributor

Yeah I don't see how this could have been a regression, it was just the way the tooltip behaved, this would be an upstream problem from the beggining.
The fix was to not use the tooltip. And use another soltuion instead.
As for the prices I have no idea, I think @JmillsExpensify would be the best person to decide here.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 14, 2022
@arielgreen
Copy link
Contributor

Thanks for raising this @mananjadhav. I've reviewed the issue, and the timeline.

This was originally held up internally. The timeline once it was posted is as follows:
11/7 - job posted to Upwork
11/16 - price increased
11/27 - proposal approved
11/29 - @mananjadhav noticed the comment didn't trigger and @eVoloshchak accepted, so we start the bonus clock.
11/30 - PR raised
12/2 - PR approved by @sketchydroide

This is eligible for the 50% bonus since it was merged within 3 bus days. I also think it's fair in this case to bump up the price to $1k since it should have been done prior to proposal being accepted.

Reporting bonus: $250 to @dhairyasenjaliya
Issue bounty: $1000+50% = $1500 to @eVoloshchak
C+ = $1500 to @mananjadhav

Issuing payment now!

@arielgreen
Copy link
Contributor

Offers sent @dhairyasenjaliya @eVoloshchak @mananjadhav, please accept

@mananjadhav
Copy link
Collaborator

Thanks @arielgreen. Accepted.

@arielgreen
Copy link
Contributor

Excellent, all three payments have been issued.

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 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests