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 + bonus Aug 20] Block ability to select Chronos from Recents list when splitting bill or requesting money #4411

Closed
mananjadhav opened this issue Aug 4, 2021 · 27 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@mananjadhav
Copy link
Collaborator

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


I am not quite sure of the role of Chronos. Can be closed if this is an incorrect assumption.

Action Performed:

  1. Go to the sidebar and click on '+'
  2. Click on the split bill or request money
  3. If you've got Chronos in recent chats, you'll see it loads up and you can request money from Chronos

Expected Result:

It shouldn't allow split/request money transactions with Chronos.

Actual Result:

chronos-recent-contacts-iou-requests

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

can-request-money-from-chronos

  • 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

@mananjadhav mananjadhav 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 @mateocole (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
@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Aug 4, 2021

If this is a valid issue then,

Proposed Solution

getNewChatOptions defined in OptionsListUtils.js is being used to get participants in NewChat as well as IOUParticipantsRequest. It has a flag to ignoreConceirgeEmails and we can extend it to ignore Chronos email too.

  1. In OptionsListUtils.js change the function type signature with one of the following options:
function getNewChatOptions( reports, personalDetails, searchValue = '', excludeConcierge, excludeChronos, betas) {

or

function getNewChatOptions( reports, personalDetails, searchValue = '', { excludeConcierge, excludeChronos }, betas) {

Accordingly update getOptions, where we can just add another flag, excludeChronos = false

and add the following check

	if (excludeChronos) {
        loginOptionsToExclude.push({login: CONST.EMAIL.CHRONOS});
    }
  1. Update references of getNewChatOptions in NewChatPage.js and/or IOUParticipantsRequest.js to accommodate the params change.

@Santhosh-Sellavel
Copy link
Collaborator

Hey, I too found the same issue just about half an hour ago. Wondering coincidence or hack? 🤔

@Santhosh-Sellavel
Copy link
Collaborator

Hey, I too found the same issue just about half an hour ago. Wondering coincidence or hack? 🤔

But for the concierge.

@Santhosh-Sellavel
Copy link
Collaborator

Okay, yours is a different issue, you are choosing Chronus from recent.

@mananjadhav mananjadhav changed the title [Unsure]: You can split with/request money from Chronos You can split with/request money from Chronos Aug 5, 2021
@MelvinBot
Copy link

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

@mateocole mateocole added the Improvement Item broken or needs improvement. label Aug 6, 2021
@aldo-expensify aldo-expensify added the External Added to denote the issue can be worked on by a contributor label Aug 6, 2021
@MelvinBot
Copy link

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

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Aug 7, 2021

Can you block from recentreceipts@expensify.com also? @mananjadhav #4413 (comment)

You had already noticed this.

@MelvinBot
Copy link

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

@mateocole mateocole removed their assignment Aug 10, 2021
@MelvinBot MelvinBot removed the Overdue label Aug 10, 2021
@stephanieelliott
Copy link
Contributor

Ok I took a look at #4413 and I'm not sure I understand the difference between these two issues. It sounds like @mananjadhav and @Santhosh-Sellavel understand these as two separate PRs, can you please share a little more detail on the difference here?

Once I am sure these are duplicates, I can move this to Upwork and get the proposal reviewed 😃.

@mananjadhav
Copy link
Collaborator Author

#4413, deals with the data from the API and the search. This one deals with the recent chats.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Aug 11, 2021

Simple @stephanieelliott,

Before creating any IOU Requests, just message Chronos anything.
Now try to create an IOU request/split, enter the amount. On Participant's page, in the recent section, you can find Chronos. So you can select from recent and give a request. And Concierge is already blocked from recent. @mananjadhav proposing the same for Chronos also.

But in #4413,
Create an IOU request/split, enter the amount. On Participant's page, Search for Chronos, Concierge using their emails, which are only excluded from Recents but are able to find through search results.

Hope you got cleared!

@Santhosh-Sellavel
Copy link
Collaborator

@stephanieelliott
Additionally, input from #4413 (comment), we need to exclude one more email receipts@expensify.com in both the recent section & search results.

Both look similar, it's really not.
In #4413 I am suggesting excluding emails from search.
In #4411 No need to search just pick Chronos from recent.

It's a coincidence we found these similar issues almost at the same time. IMO It's not a duplicate.

@stephanieelliott
Copy link
Contributor

Ah got it, thanks so much for the clarity @mananjadhav and @Santhosh-Sellavel!

Here is the job on Upwork: https://www.upwork.com/jobs/~01254c03b03bc32ed4

@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Aug 13, 2021

@Jag96 @stephanieelliott I've sent the proposal on Upwork.

@Jag96 Then are we saying we'll add that many parameters to the function call? I would recommend option 2, because you'll have one parameter, and if the list grows we won't have to add params, only adjust the flags in one object param. Will let you decide and then implement.

function getNewChatOptions( reports, personalDetails, searchValue = '', excludeConcierge, excludeChronos, excludeReceipts, betas) {

Also, apart from chronos, concierge and receipts which are the other automated accounts?

@Jag96
Copy link
Contributor

Jag96 commented Aug 13, 2021

I think option 2 is fine as long as we make sure JSDocs are added appropriately, so for example anywhere we'd have a check for excludedOptions.excludeConcierge, we'd make sure excludedOptions.excludeConcierge is in the function params, similar to what we do in api.js.

Right now I think the only three we want to check are concierge, chronos, and receipts, so that sounds ok to me. Let me know if that makes sense!

@mananjadhav
Copy link
Collaborator Author

Got it. Noted. Will handle it accordingly.

@stephanieelliott stephanieelliott changed the title You can split with/request money from Chronos Block ability to select Chronos from Recents list when splitting bill or requesting money Aug 13, 2021
@parasharrajat
Copy link
Member

parasharrajat commented Aug 13, 2021

@stephanieelliott #4413 sounds the same and look like two contributors are working on the same thing.

Maybe changing the title to show the difference is a good thing to do.

@stephanieelliott
Copy link
Contributor

I had already changed the titled to differentiate between the two... #4411 deals with choosing Chronos from recents list, #4413 deals with manually typing in chronos@ (and blocking it and other similar automated accounts). This was based on my understanding of the difference, feel free to edit either title to make it more clear @parasharrajat!

@MelvinBot MelvinBot removed the Overdue label Aug 16, 2021
@Jag96 Jag96 added the Reviewing Has a PR in review label Aug 17, 2021
@stephanieelliott stephanieelliott removed their assignment Aug 18, 2021
@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 18, 2021
@stephanieelliott stephanieelliott added Exported and removed Help Wanted Apply this label when an issue is open to proposals by contributors Exported labels Aug 18, 2021
@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 18, 2021
@stephanieelliott stephanieelliott removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 18, 2021
@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 @Christinadobrzyn!

@Jag96 Jag96 changed the title Block ability to select Chronos from Recents list when splitting bill or requesting money [HOLD for payment + bonus Aug 20] Block ability to select Chronos from Recents list when splitting bill or requesting money Aug 19, 2021
@Christinadobrzyn
Copy link
Contributor

The PR for this issue is here.

@Jag96 Jag96 added Weekly KSv2 and removed Daily KSv2 labels Aug 20, 2021
@Christinadobrzyn
Copy link
Contributor

Looks like Upwork job autoclosed - so reopened:

Internal posting here - https://www.upwork.com/ab/applicants/1428898063992438784/job-details
External posting here - https://www.upwork.com/jobs/~01cf17d46c45404346

Hired @mananjadhav - sorry Manan but can you let me know when you accept the hire request so I can pay you?

@mananjadhav
Copy link
Collaborator Author

@Christinadobrzyn I just texted you on Upwork. While I did accept the current offer, I can see the old job as well in the contract. Will you take care of closing the one of them?

@Christinadobrzyn
Copy link
Contributor

Ah thank you @mananjadhav I was able to pay you through the original job posting.

Closed the second job I created.

@mananjadhav
Copy link
Collaborator Author

Thanks @Christinadobrzyn

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. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants