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

Block chronos@, receipts@ and concierge@ from Split Bill and Request Money #4413

Closed
5 tasks done
Santhosh-Sellavel opened this issue Aug 4, 2021 · 33 comments
Closed
5 tasks done
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2

Comments

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Aug 4, 2021

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. New Request/split bill
  2. Enter the amount
  3. In search enter the full email as "concierge@expensify.com" || "Chronos@expensify.com"
  4. From search you can find Concierge and Chronos in search results.

#4413 (comment)
We should also block 'receipts@expensify.com'.

Note: Chronus is shown in results only for the first time. if entered in lower case it will not show in results refer to attachments

From the second time, Chronus will show in Recents will not be able to reproduce.
There is an already issue for that raised here #4411

Expected Result:

We should not be able to request from Concierge or Chronos

Actual Result:

Concierge

Request-money.from.concierge.mov

Chronus:

Screenshot 2021-08-05 at 4 34 25 AM

Screenshot 2021-08-05 at 4 34 39 AM

Workaround:

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

Platform:

Where is this issue occurring?

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

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

@Santhosh-Sellavel Santhosh-Sellavel added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 4, 2021
@MelvinBot
Copy link

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

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 4, 2021
@Santhosh-Sellavel
Copy link
Collaborator Author

Proposal

In OptionUtilsList add new method isExcludedIOUUsers we can change it to
Screenshot 2021-08-05 at 5 36 28 AM

And uses from both IOUParticipantsRequest.js & IOUParticipantsSplit.js as below,
Screenshot 2021-08-05 at 5 36 44 AM

@sonialiap sonialiap removed their assignment Aug 7, 2021
@MelvinBot
Copy link

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

@sonialiap
Copy link
Contributor

We should probably also block receipts@expensify.com

@Santhosh-Sellavel
Copy link
Collaborator Author

@pecanoro proposed changes are available in drafted PR.

@sonialiap excluded receipts@expensify.com

@MelvinBot
Copy link

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

@pecanoro pecanoro removed their assignment Aug 12, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 12, 2021
@pecanoro pecanoro added Improvement Item broken or needs improvement. External Added to denote the issue can be worked on by a contributor labels Aug 12, 2021
@MelvinBot
Copy link

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

@pecanoro
Copy link
Contributor

pecanoro commented Aug 12, 2021

Anyways, regardless of the front end, maybe we should open an issue to block this in the back-end as well. I will open an issue for it in our other repos.

@Santhosh-Sellavel
Copy link
Collaborator Author

@pecanoro Yes, that would be even safer.

@Santhosh-Sellavel
Copy link
Collaborator Author

Santhosh-Sellavel commented Aug 12, 2021

Updated Proposal

Following are the changes, we need to make to restrict this in client-side search.

  1. Adding receipts@expensify.com to the CONST File.

Screenshot 2021-08-12 at 10 43 19 PM

  1. Function to check whether user is excluded from IOU

Screenshot 2021-08-12 at 10 43 43 PM

  1. Using the function in respective IOU confirmation pages,

Screenshot 2021-08-12 at 10 43 58 PM

Screenshot 2021-08-12 at 10 44 11 PM

@pecanoro If you are okay with the proposal, I'll wrap my PR work.
Thanks!

@stephanieelliott
Copy link
Contributor

Posted to Upwork: https://www.upwork.com/jobs/~013379d6a3621d716e

@MelvinBot MelvinBot removed the Overdue label Aug 13, 2021
@MelvinBot MelvinBot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 16, 2021
@marcaaron
Copy link
Contributor

@stephanieelliott Let's hire @Santhosh-Sellavel please, thanks!

@stephanieelliott
Copy link
Contributor

Hey @Santhosh-Sellavel can you post a proposal to the job in Upwork so we can hire you? https://www.upwork.com/jobs/~013379d6a3621d716e

@Santhosh-Sellavel
Copy link
Collaborator Author

Applied @stephanieelliott

@stephanieelliott
Copy link
Contributor

Great, gotcha @Santhosh-Sellavel!

@stephanieelliott stephanieelliott removed their assignment Aug 18, 2021
@stephanieelliott stephanieelliott 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 Aug 18, 2021
@MelvinBot
Copy link

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

@stephanieelliott
Copy link
Contributor

I'm taking an extended OOO, re-applied the External label to get another CM to take this the rest of the way, thanks @mallenexpensify!

@Christinadobrzyn
Copy link
Contributor

I see the PR related to this issue is here. Still being reviewed.

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Aug 20, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 20, 2021
@marcaaron
Copy link
Contributor

Hit a minor snag on this issue since there is another PR that adds a handful of other "expensify emails" and it may change the solution somewhat as there are other emails that should also be blocked.

It also looks like we should already be preventing emails like chronos@expensify.com in OptionsListUtils. I tested this on main and the logic works fine. So, overall I think the original solution presented here needs to change.

if (excludeConcierge) {
loginOptionsToExclude.push({login: CONST.EMAIL.CONCIERGE});
}
if (excludeChronos) {
loginOptionsToExclude.push({login: CONST.EMAIL.CHRONOS});
}
if (excludeReceipts) {
loginOptionsToExclude.push({login: CONST.EMAIL.RECEIPTS});
}

@pecanoro just to double check we probably do not want to be able to create IOUs with any of these EXPENSIFY_EMAILS ?

If so, then it is probably best to:

  1. Make sure the exclude logic added in Hide automated accounts from Recent sections of Split Money/Request Money #4648 is actually working (it seems to work fine - but @Santhosh-Sellavel noticed it does not work if we use improper case like Chronos@expensify.com)
  2. Also create an option to do all the emails excludeExpensifyEmails instead of a flag for each like we are doing here

excludeConcierge = false,
excludeChronos = false,
excludeReceipts = false,

@Jag96 what do you think of this solution? I'm unsure if we still want to keep the individual flags for excluding a specific participant with so many to exclude. I think we can just get rid of them as the only place where these emails are excluded are in the IOU selectors?

@Jag96
Copy link
Contributor

Jag96 commented Aug 24, 2021

If we're always going to apply the same logic for all of those accounts, then I think it's fine to use excludeExpensifyEmails instead of having individual values. I think this is the only place we have one value that isn't the same as the others, so we'd have to handle that case right?

@marcaaron
Copy link
Contributor

Ah nice catch thanks @Jag96 I missed that we were passing true there. I think then we can just remove the more specific handling for concierge@ and chronos@ (seems their defaults were false anyway if not specified).

Sorry for the back and forth here @Santhosh-Sellavel but just want to make sure we have a solid path forward and aren't introducing a solution we don't need. I know it's not the original proposal we discussed, but are you OK to make these changes?

@parasharrajat
Copy link
Member

Maybe an option allowedExpensifyEmails as array could be alternative.

@marcaaron
Copy link
Contributor

marcaaron commented Aug 24, 2021

Maybe an option allowedExpensifyEmails as array could be alternative.

Possibly, but I think in most cases we want to allow them. It's less common (so far) to want to exclude them (we only want to exclude all emails in 2 places and a single email in 1 place).

We'd have to do something like

allowedExpensifyEmails: _.without(EXPENSIFY_EMAILS, 'receipts@expensify.com')

which is less intuitive than

In IOU components...

excludeExpensifyLogins: true,

and

In NewChatPage...

excludeReceipts: true,

That said, it does seem kind of specific to have excludeReceipts for a single usage when we could just as easily pass a block list of emails that should be excluded. Those examples would change to:

loginsToExclude: EXPENSIFY_EMAILS,
loginsToExclude: [CONST.EMAIL.RECEIPTS],

@Santhosh-Sellavel
Copy link
Collaborator Author

Santhosh-Sellavel commented Aug 25, 2021

Sure @marcaaron, I'll make the changes. And raise a new PR!

@Santhosh-Sellavel
Copy link
Collaborator Author

@marcaaron whatever we discussed here is captured in this draft PR #4821, also there are some additional emails as well.

@marcaaron
Copy link
Contributor

Ok well there is no sense in having two separate issues handling the same thing. @AndrewGable @deetergp it would be good if there was some way to prevent contributors from working on the same issues in the future. Seeing as @Santhosh-Sellavel opened the issue first I suggest that we close this.

@mallenexpensify I think we should just pay @Santhosh-Sellavel and close this out.

@Santhosh-Sellavel
Copy link
Collaborator Author

@mallenexpensify Since this issue already closed. Kindly end the contract when you have time without any negative feedbacks. Thanks!

@mallenexpensify
Copy link
Contributor

Cancelled in Upwork, left positive feedback for you @Santhosh-Sellavel

image

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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