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-05-22] [$1000] Copy to clipboard has inconsistent behavior in the (Reimburse expenses and contact method page) VS (the profile's details page) #17751

Closed
1 of 6 tasks
kavimuru opened this issue Apr 20, 2023 · 62 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

@kavimuru
Copy link

kavimuru commented Apr 20, 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. Go to mweb chrome
  2. Go to any workspaces
  3. Go to reimburse expenses and try to click the copy icon besides email multiple times. Notice that the copy icon appears in between. But follow the same process by going to any chat and by clicking on any user's profile page
  4. Click on the copy icon multiple times and notice that in the user's email copy icon, only green check marks appears. Thus, it is inconsistent with the reimburse page where clicking on multiple times on copy icon doesn't display green mark continuously.
  5. Even the contact method page behaves the same like Reimburse page

Expected Result:

Copying to clipboard should behave in the same manner on Reimburse expenses and contact method page like that of the profile's details page

Actual Result:

Copying to clipboard behaves differently in reimburse expenses and contact method page unlike the profile's details page

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.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

copy.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681978408152299

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a8489353b4a203b3
  • Upwork Job ID: 1651171903300116480
  • Last Price Increase: 2023-05-03
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 20, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

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

@flaviadefaria
Copy link
Contributor

Discussing this in Slack before applying the label.

@melvin-bot melvin-bot bot removed the Overdue label Apr 24, 2023
@MelvinBot
Copy link

@flaviadefaria Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Apr 26, 2023
@flaviadefaria flaviadefaria added the External Added to denote the issue can be worked on by a contributor label Apr 26, 2023
@melvin-bot melvin-bot bot changed the title Copy to clipboard has inconsistent behavior in the (Reimburse expenses and contact method page) VS (the profile's details page) [$1000] Copy to clipboard has inconsistent behavior in the (Reimburse expenses and contact method page) VS (the profile's details page) Apr 26, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Current assignee @flaviadefaria is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

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

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

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

@flaviadefaria
Copy link
Contributor

Added the external label.

@melvin-bot melvin-bot bot removed the Overdue label Apr 26, 2023
@situchan
Copy link
Contributor

situchan commented Apr 26, 2023

Proposal

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

Copy to clipboard has inconsistent behavior between Contacts page and Profile detail page

What is the root cause of that problem?

In Contacts page, CopyTextToClipboard component is used. Here, we clear checkmark after 2000ms timeout.
But timeout is not cleared when icon is clicked so clearing checkmark still applies.

In Profile detail page, ContextMenuItem component is used which uses withDelayToggleButtonState HOC. Here, we clear checkmark after 1800ms timeout and timeout is cleared whenever click icon again. So clearing checkmark timer starts again.

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

Extend withDelayToggleButtonState HOC to support functionality of preventing restart interval before 1800ms timeout.
So add new param like canResetBeforeDelay to toggleDelayButtonState() function with default value to true.
If true, it's the current behavior as is.
If false, don't call clearTimeout and early return.
And to prevent redundant check, set this.resetButtonStateCompleteTimer to null on setTimeout callback.

  • In ContextMenuItem, call props.toggleDelayButtonState with 2nd param set to false
  • In CopyTextToClipboard, deprecate showCheckmark state value, and wrap with withDelayToggleButtonState to get isDelayButtonStateComplete prop. And instead of old setTimeout code, call props.toggleDelayButtonState(true, false)

Improvement: for consistency, we can deprecate 2nd param of toggleDelayButtonState in withDelayToggleButtonState and remove clearTimeout logic to prevent restarting timer whenever clicked.

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Apr 26, 2023

Proposal

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

Copy to clipboard is in-consistent between Reimburse page and profile detail page.

What is the root cause of that problem?

In Reimburse page we use CopyTextToClipboard component which set 2000ms timeout but doesn't reset the timeout once user clicks again. So the old timeouts reset the icon while continously pressing.
https://github.com/Expensify/App/blob/main/src/components/CopyTextToClipboard.js#L47

In profile detail page we are using CommunicationLink Component which uses withDelayToggleButtonState component at last to handle the timeout logics. This uses 1800ms timeout and also handles clearing of timeout after user presses again.
https://github.com/Expensify/App/blob/main/src/components/withDelayToggleButtonState.js#L35

This is why we have inconsistency.

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

  1. We can remove custom timeout handling in CopyTextToClipboard and start using withDelayToggleButtonState in CopyTextToClipboard as well. Suggesting to use withDelayToggleButtonState since it seems to work good and handle more cases and it seems right for the purpose. This way we can avoid duplicate code and the overhead of maintaining consistency. Similar implementation of ContextMenuItem using withDelayToggleButtonState https://github.com/Expensify/App/blob/main/src/components/ContextMenuItem.js
  2. In withDelayToggleButtonState toggleDelayButtonState function, if 'isDelayButtonStateComplete' state value is already true, do an early exit, since we don't want to affect existing timeouts. This way all existing components using withDelayToggleButtonState will have the common behaviour in terms of delayed button actions.
    https://github.com/Expensify/App/blob/main/src/components/withDelayToggleButtonState.js#L36

For 2) action item the Current components usage is:

  1. Details page copy to clipboard(CommunicationLink)
  2. Attachment download button(HeaderWithCloseButton)
  3. Download button in wallet statement page(Not sure if its active page though)

We can unify all delayable actions to work like -> Take action "copy to clipboard" or "download" -> action blocked -> Wait 1800ms -> action unblocked -> take action -> action blocked -> wait 1800ms -> action unblocked

What alternative solutions did you explore? (Optional)

We can also create helper classes to have the common timeout handling code and icon showing logic and make the components use it.

We can also reset the timer in CopyTextToClipboard component when user clicks again.

We can remove the reseting of timer logic in withDelayToggleButtonState component and make it in sync with the CopyTextToClipboard logic.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Apr 26, 2023

@flaviadefaria I'm unassigning due to low bandwidth, can you assign a new C+ here, by reapplying external here, thanks!

cc: @danieldoglas

@Santhosh-Sellavel Santhosh-Sellavel removed their assignment Apr 26, 2023
@danieldoglas danieldoglas added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Apr 26, 2023
@MelvinBot
Copy link

Current assignee @flaviadefaria is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

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

@flaviadefaria flaviadefaria added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels May 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 19, 2023

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 19, 2023
@melvin-bot

This comment was marked as duplicate.

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Daily KSv2 labels May 21, 2023
@arielgreen
Copy link
Contributor

@priya-zha @sobitneupane @situchan please apply to the Upwork posting so that I can issue payment

@melvin-bot melvin-bot bot removed the Overdue label May 24, 2023
@priya-zha
Copy link

@arielgreen Submitted the proposal.Thanks

@sobitneupane
Copy link
Contributor

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:

It is an inconsistency issue not really a bug. So, I don't think we can point a specific PR or issue as a culprit.

@sobitneupane
Copy link
Contributor

Regression Test Proposal:

  1. Login any account
  2. Go to Settings > Workspaces > any workspace > Reimburse expenses
  3. Click copy icon many times quickly
  4. Verify that copy icon changed to copied and keep copied icon with no impact on pressing for sometime (1800ms).
  5. Verify that after sometime (1800ms), it resets to copy icon and able to press again
  6. Press icon again and verify Steps 4. and 5.
  7. Go to any chat and click user profile avatar in any chat message
  8. Repeat Steps 3-6

Do we agree 👍 or 👎

@arielgreen
Copy link
Contributor

Calculating payments:

3 business days = 50% bonus

$500 to @priya-zha
$1000 + 50% = $1500 to @situchan
$1000 + 50% = $1500 to @sobitneupane

@arielgreen
Copy link
Contributor

Offers sent, ready for payment once accepted.

@arielgreen
Copy link
Contributor

I am OOO tomorrow, so reassigning

@arielgreen arielgreen added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels May 26, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 26, 2023

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

@melvin-bot

This comment was marked as duplicate.

@arielgreen
Copy link
Contributor

Also reassigning Flavia and I for credit.

@laurenreidexpensify
Copy link
Contributor

Have issued all payments in Upwork. Please reopen if I've missed anything. thanks

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