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

Feature/38774 expensify persona #41343

Merged
merged 25 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e343aa6
new persona added
burczu Apr 22, 2024
9a58f4f
using expensify persona only for odd account ids
burczu Apr 22, 2024
6da74e4
handling new system report type
burczu Apr 26, 2024
aaa3cd4
navigating to system chat at the end of the onboarding process added
burczu Apr 26, 2024
282490a
addional todo comment added
burczu Apr 26, 2024
f50eaaa
fix: resolve conflicts
koko57 Apr 30, 2024
0410d8f
fix: minor fix
koko57 Apr 30, 2024
179575f
fix: remove comment
koko57 Apr 30, 2024
9cba84a
fix: remove unnecessary condition
koko57 Apr 30, 2024
ba3aa46
fix: remove console log
koko57 Apr 30, 2024
5c2e6d2
fix: remove unnecessary change
koko57 May 2, 2024
36842b8
fix: apply requested changes
koko57 May 2, 2024
a6b6593
fix: lint
koko57 May 2, 2024
d858c04
Merge branch 'main' into feature/38774-expensify-persona-agata
burczu May 6, 2024
56cd07f
fix: resolve conflicts
koko57 Jun 4, 2024
8365690
fix: apply requested changes
koko57 Jun 4, 2024
717fe74
fix: lint
koko57 Jun 4, 2024
b765fe2
Merge branch 'main' into feature/38774-expensify-persona
koko57 Jun 10, 2024
e921c92
fix: show Persona chat in LHN
koko57 Jun 10, 2024
eab9f0b
fix: remove unnecessary comment
koko57 Jun 10, 2024
99b7398
fix: show only Expensify name for persona chat
koko57 Jun 10, 2024
0205df8
Merge branch 'main' into feature/38774-expensify-persona
koko57 Jun 11, 2024
08a87cd
fix: show persona chat only for onboarded by persona users
koko57 Jun 11, 2024
2015e43
Merge branch 'main' into feature/38774-expensify-persona
koko57 Jun 11, 2024
5dc7f6c
fix: lint and typecheck
koko57 Jun 11, 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
3 changes: 2 additions & 1 deletion src/components/LHNOptionsList/OptionRowLHN.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti
!!optionItem.isThread ||
!!optionItem.isMoneyRequestReport ||
!!optionItem.isInvoiceReport ||
ReportUtils.isGroupChat(report)
ReportUtils.isGroupChat(report) ||
ReportUtils.isSystemChat(report)
}
/>
{isStatusVisible && (
Expand Down
1 change: 1 addition & 0 deletions src/libs/API/parameters/CompleteGuidedSetupParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type {OnboardingPurposeType} from '@src/CONST';
type CompleteGuidedSetupParams = {
firstName: string;
lastName: string;
actorAccountID: number;
guidedSetupData: string;
engagementChoice: OnboardingPurposeType;
};
Expand Down
4 changes: 3 additions & 1 deletion src/libs/AccountUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please move this to the permissions file so that it is treated just like any of the other betas?

Copy link
Contributor

Choose a reason for hiding this comment

The 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};
14 changes: 12 additions & 2 deletions src/libs/ReportUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mountiny Replying - I've just fixed it. But I need to confirm - is there any other situation that we want to hide this kind of chat (including persona as a participant) from LHN? If there is any I need to think of changing this confition

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 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

Copy link
Contributor

@sobitneupane sobitneupane Jun 10, 2024

Choose a reason for hiding this comment

The 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.mov

Do 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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Screenshot 2024-06-10 at 11 26 22

Do we want to show chat with Expensify persona for the first time only? Do we want to always show it (even after user logs in to already active account)?

  • and I also wonder what is expected behavior here? Should we show the Expensify persona chat for all the users?

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 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

yeah, but even before my latest changes we had this account displayed when starting a new chat

I think that is a bug and it should not show up in any of those lists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-06-10 at 17 10 04

[email protected] is visible even if I revert the latest changes, so I need to add a new filter.
For displaying the Expensify persona only for the users onboarded by persona - I still need to figure this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

is there any other situation that we want to hide this kind of chat

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)

image

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

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.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Screenshot 2024-06-11 at 09 37 34
Screenshot 2024-06-11 at 09 37 13

(participantAccountIDs.length === 0 &&
!isChatThread(report) &&
!isPublicRoom(report) &&
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -6992,6 +7001,7 @@ export {
getRoomWelcomeMessage,
getRootParentReport,
getRouteFromLink,
getSystemChat,
getTaskAssigneeChatOnyxData,
getTransactionDetails,
getTransactionReportName,
Expand Down
18 changes: 17 additions & 1 deletion src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Onyx from 'react-native-onyx';
import type {PartialDeep, ValueOf} from 'type-fest';
import type {Emoji} from '@assets/emojis/types';
import type {FileObject} from '@components/AttachmentModal';
import AccountUtils from '@libs/AccountUtils';
import * as ActiveClientManager from '@libs/ActiveClientManager';
import * as API from '@libs/API';
import type {
Expand Down Expand Up @@ -2008,6 +2009,17 @@ function navigateToConciergeChat(shouldDismissModal = false, checkIfCurrentPageA
}
}

/**
* Navigates to the 1:1 system chat
*/
function navigateToSystemChat() {
const systemChatReport = ReportUtils.getSystemChat();

if (systemChatReport && systemChatReport.reportID) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(systemChatReport.reportID));
}
}

/** Add a policy report (workspace room) optimistically and navigate to it. */
function addPolicyReport(policyReport: ReportUtils.OptimisticChatReport) {
const createdReportAction = ReportUtils.buildOptimisticCreatedReportAction(CONST.POLICY.OWNER_EMAIL_FAKE);
Expand Down Expand Up @@ -3115,7 +3127,9 @@ function completeOnboarding(
},
adminsChatReportID?: string,
) {
const targetEmail = CONST.EMAIL.CONCIERGE;
const isAccountIDOdd = AccountUtils.isAccountIDOddNumber(currentUserAccountID ?? 0);
const targetEmail = isAccountIDOdd ? CONST.EMAIL.NOTIFICATIONS : CONST.EMAIL.CONCIERGE;

const actorAccountID = PersonalDetailsUtils.getAccountIDsByLogins([targetEmail])[0];
koko57 marked this conversation as resolved.
Show resolved Hide resolved
const targetChatReport = ReportUtils.getChatByParticipants([actorAccountID, currentUserAccountID]);
const {reportID: targetChatReportID = '', policyID: targetChatPolicyID = ''} = targetChatReport ?? {};
Expand Down Expand Up @@ -3404,6 +3418,7 @@ function completeOnboarding(
engagementChoice,
firstName,
lastName,
actorAccountID,
guidedSetupData: JSON.stringify(guidedSetupData),
};

Expand Down Expand Up @@ -3683,6 +3698,7 @@ export {
saveReportActionDraft,
deleteReportComment,
navigateToConciergeChat,
navigateToSystemChat,
addPolicyReport,
deleteReport,
navigateToConciergeChatAndDeleteReport,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {FormOnyxValues} from '@components/Form/types';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
import KeyboardAvoidingView from '@components/KeyboardAvoidingView';
import OfflineIndicator from '@components/OfflineIndicator';
import {useSession} from '@components/OnyxProvider';
import Text from '@components/Text';
import TextInput from '@components/TextInput';
import withCurrentUserPersonalDetails from '@components/withCurrentUserPersonalDetails';
Expand All @@ -16,6 +17,7 @@ import useLocalize from '@hooks/useLocalize';
import useOnboardingLayout from '@hooks/useOnboardingLayout';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import AccountUtils from '@libs/AccountUtils';
import * as ErrorUtils from '@libs/ErrorUtils';
import Navigation from '@libs/Navigation/Navigation';
import * as ValidationUtils from '@libs/ValidationUtils';
Expand All @@ -36,6 +38,7 @@ function BaseOnboardingPersonalDetails({currentUserPersonalDetails, shouldUseNat
const {shouldUseNarrowLayout} = useOnboardingLayout();
const {inputCallbackRef} = useAutoFocusInput();
const [shouldValidateOnChange, setShouldValidateOnChange] = useState(false);
const {accountID} = useSession();

useDisableModalDismissOnEscape();

Expand Down Expand Up @@ -69,6 +72,8 @@ function BaseOnboardingPersonalDetails({currentUserPersonalDetails, shouldUseNat
// Otherwise stay on the chats screen.
if (isSmallScreenWidth) {
Navigation.navigate(ROUTES.HOME);
} else if (AccountUtils.isAccountIDOddNumber(accountID ?? 0)) {
Report.navigateToSystemChat();
} else {
Report.navigateToConciergeChat();
}
Expand All @@ -79,7 +84,7 @@ function BaseOnboardingPersonalDetails({currentUserPersonalDetails, shouldUseNat
Navigation.navigate(ROUTES.WELCOME_VIDEO_ROOT);
}, variables.welcomeVideoDelay);
},
[isSmallScreenWidth, onboardingPurposeSelected, onboardingAdminsChatReportID],
[isSmallScreenWidth, onboardingPurposeSelected, onboardingAdminsChatReportID, accountID],
);

const validate = (values: FormOnyxValues<'onboardingPersonalDetailsForm'>) => {
Expand Down
Loading