-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Feature/38774 expensify persona #41343
Changes from 21 commits
e343aa6
9a58f4f
6da74e4
aaa3cd4
282490a
f50eaaa
0410d8f
179575f
9cba84a
ba3aa46
5c2e6d2
36842b8
a6b6593
d858c04
56cd07f
8365690
717fe74
b765fe2
e921c92
eab9f0b
99b7398
0205df8
08a87cd
2015e43
5dc7f6c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,4 +5,6 @@ import type {Account} from '@src/types/onyx'; | |
const isValidateCodeFormSubmitting = (account: OnyxEntry<Account>) => | ||
!!account?.isLoading && account.loadingForm === (account.requiresTwoFactorAuth ? CONST.FORMS.VALIDATE_TFA_CODE_FORM : CONST.FORMS.VALIDATE_CODE_FORM); | ||
|
||
export default {isValidateCodeFormSubmitting}; | ||
const isAccountIDOddNumber = (accountID: number) => accountID % 2 === 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we please move this to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair, I think we were not looking at this as a beta, but I can see where you are coming from. @koko57 do you think you could move this around to permissions file? |
||
|
||
export default {isValidateCodeFormSubmitting, isAccountIDOddNumber}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5274,8 +5274,6 @@ function shouldReportBeInOptionList({ | |
report?.reportName === undefined || | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
report?.isHidden || | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
participantAccountIDs.includes(CONST.ACCOUNT_ID.NOTIFICATIONS) || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was added here #32361 but now we can show it at least in the LHN. You should still not bae able to start a new chat with the [email protected] account There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even when a user accountID is even, we show Expensify persona. Screen.Recording.2024-06-10.at.15.01.09.movDo we want to show chat with Expensify persona only for the first time? Or do we want to always show it (even after user re-login the account)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mountiny yeah, but even before my latest changes we had this account displayed when starting a new chat
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the logic is that in the onboarding flow, some will get the tasks from Concierge and some from the Expensify persona. It should only show the Expensify persona for those who have the onboarding flow added to the persona chat. Not for the others. I now realize this might be tricky cc @francoisl
I think that is a bug and it should not show up in any of those lists There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [email protected] is visible even if I revert the latest changes, so I need to add a new filter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mountiny should this persona chat be always visible, or it should disappear at some point? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Users should not be able to create group chats with the system account, or invite it to existing rooms, workspaces, etc. For example, the "Add to group" button shouldn't appear here for the Expensify row (maybe we can look into that in a follow-up PR if that requires a lot of changes)
Yeah I don't think it's something we had considered originally. To be clear though, do you mean that EITHER Concierge or the system chat would remain sticky in the LHN when you're in "Most Recent" mode? Concierge is pinned by default (I unpinned it here to show that it remains in the LHN), but yes I think I agree that if the onboarding tasks are created in the Concierge chat, then it just adds extra noise in the LHN to also show the system DM, so it would make sense to hide it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree. I dont think the system chat should be pinned really though so it would only show in the Most Recent as in focus mode it would not be unread There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mountiny @francoisl ok, so I added a logic to display Persona only for users onboarded this way. I agree with @francoisl - filtering [email protected] should be done in a follow-up, as it's something that is occurring on staging |
||
(participantAccountIDs.length === 0 && | ||
!isChatThread(report) && | ||
!isPublicRoom(report) && | ||
|
@@ -5364,6 +5362,17 @@ function shouldReportBeInOptionList({ | |
return true; | ||
} | ||
|
||
/** | ||
* Returns the system report from the list of reports. | ||
*/ | ||
function getSystemChat(): OnyxEntry<Report> { | ||
if (!allReports) { | ||
return null; | ||
} | ||
|
||
return Object.values(allReports ?? {}).find((report) => report?.chatType === CONST.REPORT.CHAT_TYPE.SYSTEM) ?? null; | ||
} | ||
|
||
/** | ||
* Attempts to find a report in onyx with the provided list of participants. Does not include threads, task, expense, room, and policy expense chat. | ||
*/ | ||
|
@@ -6992,6 +7001,7 @@ export { | |
getRoomWelcomeMessage, | ||
getRootParentReport, | ||
getRouteFromLink, | ||
getSystemChat, | ||
getTaskAssigneeChatOnyxData, | ||
getTransactionDetails, | ||
getTransactionReportName, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB - this could be made a bit more generic as
NumberUtils.isOdd(value: number)