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

fix: app does not open admin room after onboarding #55654

Merged
merged 16 commits into from
Feb 4, 2025
Merged
3 changes: 1 addition & 2 deletions src/libs/navigateAfterOnboarding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,14 @@ const navigateAfterOnboarding = (
onboardingPolicyID?: string,
activeWorkspaceID?: string,
onboardingAdminsChatReportID?: string,
shouldOpenAdminRoom?: boolean,
) => {
Navigation.dismissModal();

// When hasCompletedGuidedSetupFlow is true, OnboardingModalNavigator in AuthScreen is removed from the navigation stack.
// On small screens, this removal redirects navigation to HOME. Dismissing the modal doesn't work properly,
// so we need to specifically navigate to the last accessed report.
if (!isSmallScreenWidth) {
if (shouldOpenAdminRoom && onboardingAdminsChatReportID) {
if (onboardingAdminsChatReportID) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(onboardingAdminsChatReportID));
}
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ function BaseOnboardingAccounting({shouldUseNativeStyles}: BaseOnboardingAccount
setOnboardingAdminsChatReportID();
setOnboardingPolicyID();
});
navigateAfterOnboarding(isSmallScreenWidth, canUseDefaultRooms, onboardingPolicyID, activeWorkspaceID, onboardingAdminsChatReportID, true);
navigateAfterOnboarding(isSmallScreenWidth, canUseDefaultRooms, onboardingPolicyID, activeWorkspaceID, onboardingAdminsChatReportID);
}}
pressOnEnter
/>
Expand Down
22 changes: 16 additions & 6 deletions tests/unit/navigateAfterOnboardingTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ describe('navigateAfterOnboarding', () => {
jest.clearAllMocks();
});

it('should navigate to the admin room report if shouldOpenAdminRoom and has onboardingAdminsChatReportID', () => {
it('should navigate to the admin room report if onboardingAdminsChatReportID is provided', () => {
const navigate = jest.spyOn(Navigation, 'navigate');

navigateAfterOnboarding(false, true, undefined, undefined, ONBOARDING_ADMINS_CHAT_REPORT_ID, true);
navigateAfterOnboarding(false, true, undefined, undefined, ONBOARDING_ADMINS_CHAT_REPORT_ID);
expect(navigate).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(ONBOARDING_ADMINS_CHAT_REPORT_ID));
});

it('should not navigate if shouldOpenAdminRoom is false', () => {
navigateAfterOnboarding(false, true, undefined, undefined, ONBOARDING_ADMINS_CHAT_REPORT_ID, false);
it('should not navigate if onboardingAdminsChatReportID is not provided', () => {
navigateAfterOnboarding(false, true, undefined, undefined);
expect(Navigation.navigate).not.toHaveBeenCalled();
});

Expand All @@ -67,7 +67,7 @@ describe('navigateAfterOnboarding', () => {
mockFindLastAccessedReport.mockReturnValue(lastAccessedReport);
mockShouldOpenOnAdminRoom.mockReturnValue(false);

navigateAfterOnboarding(true, true, ONBOARDING_POLICY_ID, ACTIVE_WORKSPACE_ID, undefined, false);
navigateAfterOnboarding(true, true, ONBOARDING_POLICY_ID, ACTIVE_WORKSPACE_ID, ONBOARDING_ADMINS_CHAT_REPORT_ID);
expect(navigate).not.toHaveBeenCalled();
});

Expand All @@ -76,7 +76,17 @@ describe('navigateAfterOnboarding', () => {
mockFindLastAccessedReport.mockReturnValue(lastAccessedReport);
mockShouldOpenOnAdminRoom.mockReturnValue(false);

navigateAfterOnboarding(true, true, ONBOARDING_POLICY_ID, ACTIVE_WORKSPACE_ID, undefined, false);
navigateAfterOnboarding(true, true, ONBOARDING_POLICY_ID, ACTIVE_WORKSPACE_ID, ONBOARDING_ADMINS_CHAT_REPORT_ID);
expect(Navigation.navigate).not.toHaveBeenCalled();
});

it('should navigate to last accessed report if shouldOpenOnAdminRoom is true on small screens', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be "should navigate to last accessed report on small screens"

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should navigate to last accessed report if shouldOpenOnAdminRoom is true on small screens', () => {
it('should navigate to last accessed report on small screens', () => {

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, we don't need to care shouldOpenOnAdminRoom and onboardingAdminsChatReportID param

Copy link
Contributor Author

@mkzie2 mkzie2 Feb 5, 2025

Choose a reason for hiding this comment

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

This is the shouldOpenOnAdminRoom function I mentioned. It returns true when the URL contains openOnAdminRoom param:

export default function shouldOpenOnAdminRoom() {

Not the shouldOpenAdminRoom param we just removed above. So no need to update this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, got it

const navigate = jest.spyOn(Navigation, 'navigate');
const lastAccessedReport = {reportID: REPORT_ID};
mockFindLastAccessedReport.mockReturnValue(lastAccessedReport);
mockShouldOpenOnAdminRoom.mockReturnValue(true);

navigateAfterOnboarding(true, true, ONBOARDING_POLICY_ID, ACTIVE_WORKSPACE_ID, ONBOARDING_ADMINS_CHAT_REPORT_ID);
expect(navigate).toHaveBeenCalledWith(ROUTES.REPORT_WITH_ID.getRoute(REPORT_ID));
});
});