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.

Sign up for GitHub

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

hide expensify from new chat page #50937

Merged
merged 7 commits into from
Nov 6, 2024
Merged

hide expensify from new chat page #50937

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5866325
hide expensify from new chat page
Nodebrute Oct 16, 2024
039f0d6
fix type error
Nodebrute Oct 16, 2024
0fd9a41
fix lint error
Nodebrute Oct 16, 2024
0c50840
Merge branch 'Expensify:main' into hideexpensify
Nodebrute Oct 23, 2024
1a6950b
remove is group chat
Nodebrute Oct 23, 2024
694ecd7
Merge branch 'Expensify:main' into hideexpensify
Nodebrute Oct 29, 2024
6916c06
remove unused var
Nodebrute Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libs/OptionsListUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1994,7 +1994,7 @@ function getOptions(
allPersonalDetailsOptions = lodashOrderBy(allPersonalDetailsOptions, [(personalDetail) => personalDetail.text?.toLowerCase()], 'asc');
}

const optionsToExclude: Option[] = [];
const optionsToExclude: Option[] = [{login: CONST.EMAIL.NOTIFICATIONS}];

// If we're including selected options from the search results, we only want to exclude them if the search input is empty
// This is because on certain pages, we show the selected options at the top when the search input is empty
Expand Down
4 changes: 2 additions & 2 deletions src/pages/NewChatPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type NewChatPageProps = {
isGroupChat?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nodebrute, please remove isGroupChat prop as its been not used anywhere in the component.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nodebrute bump

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Krishna2323 I’ve removed all instances where the isGroupChat prop was used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me 🙌🏻. @ahmedGaber93 we can move forward here.

};

const excludedGroupEmails = CONST.EXPENSIFY_EMAILS.filter((value) => value !== CONST.EMAIL.CONCIERGE);
const excludedGroupEmails: string[] = CONST.EXPENSIFY_EMAILS.filter((value) => value !== CONST.EMAIL.CONCIERGE);

function useOptions({isGroupChat}: NewChatPageProps) {
const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState('');
Expand Down Expand Up @@ -229,7 +229,7 @@ function NewChatPage({isGroupChat}: NewChatPageProps) {

const itemRightSideComponent = useCallback(
(item: ListItem & OptionsListUtils.Option, isFocused?: boolean) => {
if (!!item.isSelfDM || (item.accountID && CONST.NON_ADDABLE_ACCOUNT_IDS.includes(item.accountID))) {
if (!!item.isSelfDM || (item.login && excludedGroupEmails.includes(item.login))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add more context about using login instead of accountID? Is there any comparison between them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the excludedGroupEmails contains the logins of all Expensify accounts. That's why I'm using login here. Previously, we were using NON_ADDABLE_ACCOUNT_IDS, which had the account IDs for CHRONOS and NOTIFICATIONS. However, excludedGroupEmails also includes the logins for these two accounts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More context about using login instead of accountID is accountID can be optimisticAccountID in userToInvite util API respond with the full user data

App/src/libs/OptionsListUtils.ts

Lines 1661 to 1664 in 652d2ff

[optimisticAccountID]: {
accountID: optimisticAccountID,
login: searchValue,
},

And this can cause this issue

  1. Login with new account (to confirm there is no local data)
  2. Click fab > start chat
  3. Search for chronos@expensify.com for the first time

You will see add to group is displayed temporary, and you can add it to group util API respond with the full user data.

issue.mp4

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to remove NON_ADDABLE_ACCOUNT_IDS because it is not used in anywhere now

return null;
}
/**
Expand Down
Loading