Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.43k stars 2.8k forks source link

[$250] New users seeing Expensify (notifications@expensify.com) in Contacts #49673

Open m-natarajan opened 1 week ago

m-natarajan commented 1 week ago

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


Version Number: 9.0.40-0 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 Expensify/Expensify Issue URL: Issue reported by: @danielrvidal Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1727214973622039

Action Performed:

  1. Sign up for a new account
  2. Selected Manage my team as intention
  3. Go through onboarding
  4. Select Start chat

Expected Result:

Expensify DM not displayed in contacts as it is deprecated

Actual Result:

Expensify (notifications@expensify.com)is displayed in my contacts

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

image

image (1)

https://github.com/user-attachments/assets/830c4f06-85ca-4dbd-8dcc-6aeec02d3014

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021839750030898104642
  • Upwork Job ID: 1839750030898104642
  • Last Price Increase: 2024-10-04
Issue OwnerCurrent Issue Owner: @ahmedGaber93
melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Nodebrute commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-09-30 21:09:39 UTC.

Proposal

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

New users seeing Expensify (notifications@expensify.com) in Contacts

What is the root cause of that problem?

In here we are not excluding the CONST.EMAIL.NOTIFICATIONS

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

We should add CONST.EMAIL.NOTIFICATIONS in excluded emails here We can do something like the pseudo code

   const excludedEmails = isGroupChat 
    ? excludedGroupEmails
    : [CONST.EMAIL.NOTIFICATIONS];

and then we can pass excludedEmails here

A more clean way to do this will be by passing CONST.EMAIL.NOTIFICATIONS here

 isGroupChat ? excludedGroupEmails : [CONST.EMAIL.NOTIFICATIONS]

What alternative solutions did you explore? (Optional)

We can add here CONST.EMAIL.NOTIFICATIONS https://github.com/Expensify/App/blob/dc74ee347bf3dc134b5488d2a28911cd031104b3/src/libs/OptionsListUtils.ts#L1910

   const optionsToExclude: Option[] = [{login: CONST.EMAIL.NOTIFICATIONS}];
mkzie2 commented 1 week ago

Edited by proposal-police: This proposal was edited at 2023-10-03T22:41:00Z.

Proposal

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

Expensify notifications@expensify.com is displayed in my contacts.

What is the root cause of that problem?

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

We should add:

?.filter((o) => o.login !== CONST.EMAIL.NOTIFICATIONS)

to this data:

https://github.com/Expensify/App/blob/dc74ee347bf3dc134b5488d2a28911cd031104b3/src/libs/OptionsListUtils.ts#L1553

mkzie2 commented 1 week ago

Proposal updated

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to @joekaufmanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

joekaufmanexpensify commented 1 week ago

Reproduced.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

Krishna2323 commented 6 days ago

Edited by proposal-police: This proposal was edited at 2024-09-30 21:45:00 UTC.

Proposal


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

New users seeing Expensify (notifications@expensify.com) in Contacts

What is the root cause of that problem?

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/NewChatSelectorPage.tsx#L57-L63

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


What alternative solutions did you explore? (Optional)

Result

joekaufmanexpensify commented 4 days ago

pending review of proposals by @ahmedGaber93

ahmedGaber93 commented 4 days ago

Reviewing today

ahmedGaber93 commented 4 days ago

@Nodebrute thanks for the proposal.

const excludedEmails = isGroupChat ? [CONST.EMAIL.NOTIFICATIONS, ...excludedGroupEmails] : [CONST.EMAIL.NOTIFICATIONS];

excludedGroupEmails already have CONST.EMAIL.NOTIFICATIONS, no need to add it again in isGroupChat case

ahmedGaber93 commented 4 days ago

@mkzie2 thanks for the proposal.

We've already prevented users from chatting with this account and hidden it from search and the LHN in https://github.com/Expensify/App/issues/32340.

Yeah, but we allowed it again here

Nodebrute commented 4 days ago

@ahmedGaber93 Sorry, it's just pseudocode. The main solution was to add [CONST.EMAIL.NOTIFICATIONS] in excludedEmails. I have updated the code

Nodebrute commented 4 days ago

Proposal Updated Added a more clean way to do it. The logic is same as my main solution. I have just added a clean way of doing it.

ahmedGaber93 commented 4 days ago

@Krishna2323 thanks for the proposal. I don't think we need to prevent the user from starting chat with all excludedGroupEmails, I think we need only CONST.EMAIL.NOTIFICATIONS

Krishna2323 commented 4 days ago

@ahmedGaber93, okay but I think we can't create groups with expensify emails except for concierge. This was the behaviour before the refactor of the NewChatPage.

I have added alternative solution in my proposal to reflect that.

mkzie2 commented 4 days ago

@joekaufmanexpensify We only hide notification@expensify.com in new chat page, or all the search pages in the app (chat search, expense filter, participant selector,...)? cc @Expensify/design

ahmedGaber93 commented 3 days ago

I think notification@expensify.com now is hidden in all those places, expect new chat and search page. Here we will hide it on the new chat page, and I think no need to hide it from the search page because user should be able to find it to browse the old chat messages any time he need it.

Krishna2323 commented 3 days ago

@ahmedGaber93, what do you think about https://github.com/Expensify/App/issues/49673#issuecomment-2384210401?

joekaufmanexpensify commented 3 days ago

We deprecated the Expensify DM notification@expensify.com. If we still show it in the new chat and search page, will someone who previously has no history with it still see it? Or do you mean only those who had messages with this DM prior to when we deprecated it?

ahmedGaber93 commented 3 days ago

Currently, notification@expensify.com is displayed in search page for the new users, but if we decide to hide it from here, I think we can't limit displaying it for old users only, and it will disappear for all.

Screenshot 2024-10-01 at 6 07 48 AM

ahmedGaber93 commented 3 days ago

but I think we can't create groups with expensify emails except for concierge. This was the behaviour before the refactor of the NewChatPage.

@Krishna2323 I agree it is an issue, but it is a different root cause, and I think it should fix separately, unless @joekaufmanexpensify thinks otherwise.

joekaufmanexpensify commented 3 days ago

Confirmed we should hide it for everyone on every page where it might appear

Krishna2323 commented 3 days ago

I think we can't create groups with expensify emails except for concierge. This was the behaviour before the refactor of the NewChatPage.

@joekaufmanexpensify, what do you think about this? I think it's tightly related to this issue and should be fixed here. The solution is also not complex.

@ahmedGaber93, I have updated the Alternative 2 section in my proposal. It uses the same solution as the first proposal, the only difference is that my proposal also fixes the 'Add to Group' button for Expensify emails.

joekaufmanexpensify commented 1 day ago

@joekaufmanexpensify, what do you think about this? I think it's tightly related to this issue and should be fixed here. The solution is also not complex.

Is this saying that it's not currently possible for anyone using a non-expensify.com email address to create a group chat with an email address that has expensify.com in it?

Krishna2323 commented 1 day ago

Is this saying that it's not currently possible for anyone using a non-expensify.com email address to create a group chat with an email address that has expensify.com in it?

Yes, that was the app behaviour before the refactor of NewChatPage. I think we should confirm this once again. Could you please ask someone from the internal team about this? Or should I start a slack convo?

We still pass the expensify emails for excluding it from the list when creating a group chat but that isn't working after the refactor.

https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/NewChatPage.tsx#L40 https://github.com/Expensify/App/blob/3047c1baa21f47825f5c1ffd652184341dd22256/src/pages/NewChatPage.tsx#L60

ahmedGaber93 commented 1 day ago

The current code have a logic to prevent adding those emails to groups, but this code doesn't work fine after refactoring NewChatPage. I think this issue should fix separately, please let me know if it should be fixed here?

https://github.com/user-attachments/assets/935ffea3-e193-46b6-9c5a-f5da779c15f9

joekaufmanexpensify commented 13 hours ago

Got it. If we want to keep it, it'd be nice to fix it here too, if it's an easy fix. I am confirming internally and will circle back!

melvin-bot[bot] commented 12 hours ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸