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-05-26] [$500] On Request/send money, The contact preview is clickable and Cursor is set to pointer over it - reported by @Tushu17 #8034

Closed
mvtglobally opened this issue Mar 8, 2022 · 51 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Mar 8, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to Chat > Select Actions
  2. Send/Request money from chat Actions
  3. Enter amount > Next

Expected Result:

Contact preview shouldn't be clickable as it doesn't perform any functions.

Actual Result:

Contact preview is clickable and showing cursor as a pointer

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.40-1
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

screen-rec.1.1.mp4
2.New.Expensify.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Tushu17
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1645806249670219

View all open jobs on GitHub

@MelvinBot
Copy link

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

@K4tsuki
Copy link
Contributor

K4tsuki commented Mar 8, 2022

@mvtglobally, the reporter is @Tushu17.

@mvtglobally mvtglobally changed the title On Request/send money, The contact preview is clickable and Cursor is set to pointer over it - reported by @K4tsuki On Request/send money, The contact preview is clickable and Cursor is set to pointer over it - reported by @Tushu17 Mar 8, 2022
@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 8, 2022

This will be fixed here - #7702 (comment)
Issue can be closed once 7702 is merged

@MelvinBot
Copy link

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

@rushatgabhane
Copy link
Member

pr on hold

@Justicea83
Copy link
Contributor

this issue is being worked on in another PR

@MelvinBot MelvinBot removed the Overdue label Mar 14, 2022
@MelvinBot
Copy link

@Justicea83 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@rushatgabhane
Copy link
Member

PR is on hold (because low priority)

@Justicea83
Copy link
Contributor

Waiting for PR to be merged

@MelvinBot MelvinBot removed the Overdue label Mar 18, 2022
@MelvinBot
Copy link

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

@Justicea83
Copy link
Contributor

Being worked on in another PR

@MelvinBot MelvinBot removed the Overdue label Mar 22, 2022
@Justicea83 Justicea83 removed their assignment Mar 24, 2022
@MelvinBot
Copy link

Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@flodnv
Copy link
Contributor

flodnv commented Mar 29, 2022

@Justicea83 @rushatgabhane Then let's assign @roryabraham who is working on the PR.

@MelvinBot MelvinBot removed the Overdue label Mar 29, 2022
@flodnv flodnv added Weekly KSv2 and removed Daily KSv2 labels Mar 29, 2022
@rushatgabhane
Copy link
Member

rushatgabhane commented Apr 10, 2022

@roryabraham my issue with isIOUAttachedToExistingChatReport is that the variable name implies that it should be true for 1:1 chats. But it's actually false, which could lead to confusion.

How about isSplitInitiatedFromGroupOrRoom?

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Apr 11, 2022

But it's actually false, which could lead to confusion.

@rushatgabhane
It should be false only when IOU flow is initiated from the FAB ➕ button, have you checked how it becomes false?

Also, we are not just renaming here actually, we should make changes to pass value correctly. Earlier we were passing true if it was from the group chat.

@mallenexpensify mallenexpensify changed the title [$250] On Request/send money, The contact preview is clickable and Cursor is set to pointer over it - reported by @Tushu17 [$500] On Request/send money, The contact preview is clickable and Cursor is set to pointer over it - reported by @Tushu17 Apr 15, 2022
@mallenexpensify
Copy link
Contributor

Doubled price to $500
https://www.upwork.com/jobs/~0188dac8c590e2d011

@rushatgabhane
Copy link
Member

rushatgabhane commented Apr 17, 2022

we are not just renaming here actually, we should make changes to pass value correctly

Ohhh okay. I'm not a fan of that idea, why not just rename? Won't it be easier while achieving the same thing?
isGroupSplit -> isSplitInitiatedFromGroupOrRoom

So it'll look like this canModifyParticipants = hasMultipleParticipants && isSplitInitiatedFromGroupOrRoom

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Apr 17, 2022

But I think this (#8034 (comment)) is a simple & much cleaner solution.

I'll leave it to @roryabraham to make the final call.

@roryabraham
Copy link
Contributor

I'm honestly fine with either. I think the two solutions are similar enough that it doesn't really matter. As long as we don't have both isGroupSplit and hasMultipleParticipants, then I'm 👍 for either plan.

I think I might prefer isIOUAttachedToExistingChat because I worry about setting an example for variable names like isXInitiatedFromY – I could see how that would encourage bad patterns where a function takes a flag and behaves differently based on where it's called from, which makes functions harder to reason about

@rushatgabhane
Copy link
Member

@Santhosh-Sellavel feel free to fix this issue. (I don't have the bandwidth)

@Santhosh-Sellavel
Copy link
Collaborator

@roryabraham
I can't pick this one until Friday. So I'll raise PR on Friday. If that's okay please removeHelp Wanted Label, or let's leave it open to the pool. Thanks!

@roryabraham
Copy link
Contributor

This doesn't seem very urgent, so Friday is fine imo.

@roryabraham roryabraham removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 18, 2022
@Santhosh-Sellavel
Copy link
Collaborator

@roryabraham Draft is already up, need to update PR Details.
I will update within 1 or 2 days. Sorry for the delay.

@Santhosh-Sellavel
Copy link
Collaborator

PR is Up now.

@mallenexpensify
Copy link
Contributor

Looks like the PR is awaiting @roryabraham 's review
#8760

@mallenexpensify
Copy link
Contributor

PR is being worked on
#8760

@mallenexpensify
Copy link
Contributor

PR is on stagin

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 19, 2022
@melvin-bot melvin-bot bot changed the title [$500] On Request/send money, The contact preview is clickable and Cursor is set to pointer over it - reported by @Tushu17 [HOLD for payment 2022-05-26] [$500] On Request/send money, The contact preview is clickable and Cursor is set to pointer over it - reported by @Tushu17 May 19, 2022
@melvin-bot
Copy link

melvin-bot bot commented May 19, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.63-2 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-05-26. 🎊

@Santhosh-Sellavel
Copy link
Collaborator

Doubled price to $500 https://www.upwork.com/jobs/~0188dac8c590e2d011

@mallenexpensify This job is no longer available can you set up a job for me!

@mallenexpensify
Copy link
Contributor

Hired you @Santhosh-Sellavel , please accept.
https://www.upwork.com/jobs/~0131d47defbb078996

@Tushu17 , can you please apply too? (note... the price might show $500, you're due $250 for reporting, I'm partially testing the ability to pay less than the job price)

@Santhosh-Sellavel
Copy link
Collaborator

Thanks @mallenexpensify!

@Tushu17
Copy link
Contributor

Tushu17 commented May 24, 2022

@mallenexpensify Okay, Applied. Thanks

@mallenexpensify
Copy link
Contributor

Paid @Santhosh-Sellavel $500 for the fix
Hired @Tushu17 for $250 for reporting the issue, please let me know when you've accepted the offer

@Tushu17
Copy link
Contributor

Tushu17 commented May 24, 2022

@mallenexpensify Okay, Offer accepted. Thank you.

@mallenexpensify
Copy link
Contributor

Oh... just seeing payment isn't due til May 26th. Since I already paid @Santhosh-Sellavel for the fix, going to pay out @Tushu17 and close this. Comment on the issue if it needs to be reopened, no comments on the PR for five days so we're likely good.

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

No branches or pull requests

10 participants